| Message ID | 20221007075354.568752-1-patrick.rudolph@9elements.com |
|---|---|
| Headers | show |
| Series | Add support for Maxim MAX735x/MAX736x variants | expand |
On Fri, Oct 07, 2022 at 09:53:50AM +0200, Patrick Rudolph wrote: > Update the pca954x bindings to add support for the Maxim MAX735x/MAX736x > chips. The functionality will be provided by the exisintg pca954x driver. > > While on it make the interrupts support conditionally as not all of the > existing chips have interrupts. > > For chips that are powered off by default add an optional regulator > called vdd-supply. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > --- > .../bindings/i2c/i2c-mux-pca954x.yaml | 39 ++++++++++++++++--- > 1 file changed, 34 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml > index 9f1726d0356b..efad0a95806f 100644 > --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml > +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml > @@ -4,21 +4,25 @@ > $id: http://devicetree.org/schemas/i2c/i2c-mux-pca954x.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: NXP PCA954x I2C bus switch > +title: NXP PCA954x I2C and compatible bus switches > > maintainers: > - Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > description: > - The binding supports NXP PCA954x and PCA984x I2C mux/switch devices. > - > -allOf: > - - $ref: /schemas/i2c/i2c-mux.yaml# Why do you move the allOf statement to the bottom of the schema? > + The binding supports NXP PCA954x and PCA984x I2C mux/switch devices, > + and the Maxim MAX735x and MAX736x I2C mux/switch devices. What about combining the sentence: "The binding supports NXP PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices." ? Currently it does look a bit bulky. > > properties: > compatible: > oneOf: > - enum: > + - maxim,max7356 > + - maxim,max7357 > + - maxim,max7358 > + - maxim,max7367 > + - maxim,max7368 > + - maxim,max7369 > - nxp,pca9540 > - nxp,pca9542 > - nxp,pca9543 > @@ -59,10 +63,33 @@ properties: > description: if present, overrides i2c-mux-idle-disconnect > $ref: /schemas/mux/mux-controller.yaml#/properties/idle-state > > + vdd-supply: > + description: A voltage regulator supplying power to the chip. > + > required: > - compatible > - reg > > +allOf: > + - $ref: /schemas/i2c/i2c-mux.yaml# > + - if: > + not: > + properties: > + compatible: > + contains: > + enum: > + - maxim,max7367 > + - maxim,max7369 > + - nxp,pca9542 > + - nxp,pca9543 > + - nxp,pca9544 > + - nxp,pca9545 > + then: > + properties: > + interrupts: false > + "#interrupt-cells": false > + interrupt-controller: false I'd suggest to add an opposite definition. Evaluate the properties for the devices which expect them being evaluated instead of falsing their existence for the devices which don't support the interrupts. -Sergey > + > unevaluatedProperties: false > > examples: > @@ -79,6 +106,8 @@ examples: > #size-cells = <0>; > reg = <0x74>; > > + vdd-supply = <&p3v3>; > + > interrupt-parent = <&ipic>; > interrupts = <17 IRQ_TYPE_LEVEL_LOW>; > interrupt-controller; > -- > 2.37.3 >
On Fri, Oct 07, 2022 at 09:53:53AM +0200, Patrick Rudolph wrote: > Add a vdd regulator and enable it for boards that have the > mux powered off by default. > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Reviewed-by: Peter Rosin <peda@axentia.se> > --- > drivers/i2c/muxes/i2c-mux-pca954x.c | 34 ++++++++++++++++++++++++----- > 1 file changed, 29 insertions(+), 5 deletions(-) > > diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c > index 992976fa6798..857a4ec387be 100644 > --- a/drivers/i2c/muxes/i2c-mux-pca954x.c > +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c > @@ -49,6 +49,7 @@ > #include <linux/module.h> > #include <linux/pm.h> > #include <linux/property.h> > +#include <linux/regulator/consumer.h> > #include <linux/slab.h> > #include <linux/spinlock.h> > #include <dt-bindings/mux/mux.h> > @@ -133,6 +134,7 @@ struct pca954x { > struct irq_domain *irq; > unsigned int irq_mask; > raw_spinlock_t lock; > + struct regulator *supply; > }; > > /* Provide specs for the MAX735x, PCA954x and PCA984x types we know about */ > @@ -473,6 +475,9 @@ static void pca954x_cleanup(struct i2c_mux_core *muxc) > struct pca954x *data = i2c_mux_priv(muxc); > int c, irq; > > + if (!IS_ERR_OR_NULL(data->supply)) First of all AFAICS the data->supply pointer will never be null on the pca954x_cleanup() invocations in your current implementation. So IS_ERR() would be enough here. Second in the next comment I'll suggest to you to implement the optional regulator semantic, which implies initializing the data->supply pointer with NULL if the get-regulator function returns -ENODEV. That shall look easier than the IS_ERR() macro. So checking the data->supply pointer for being not-null would be enough here. > + regulator_disable(data->supply); > + > if (data->irq) { > for (c = 0; c < data->chip->nchans; c++) { > irq = irq_find_mapping(data->irq, c); > @@ -531,15 +536,32 @@ static int pca954x_probe(struct i2c_client *client, > pca954x_select_chan, pca954x_deselect_mux); > if (!muxc) > return -ENOMEM; > + unrelated change... > data = i2c_mux_priv(muxc); > > i2c_set_clientdata(client, muxc); > data->client = client; > > + data->supply = devm_regulator_get(dev, "vdd"); > + if (IS_ERR(data->supply)) { Judging by the DT-bindings the power-supply is supposed to be optional. Isn't it? AFAICS from the _regulator_get() semantic if no vdd-supply is specified and a regulator request method with the non-optional semantic is called an ugly warning will be printed to the system log. Most of the users of the driver don't have the power-supply specified for the device. You don't want to have their logs polluted with the false warning, do you? If so you should use the devm_regulator_get_optional() method here. If it returns the -ENODEV error just overwrite the data->supply with NULL. In case of any other error halt the device probe procedure. > + ret = PTR_ERR(data->supply); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "Failed to request regulator: %d\n", ret); > + return ret; dev_err_probe() ? -Sergey > + } > + > + ret = regulator_enable(data->supply); > + if (ret) { > + dev_err(dev, "Failed to enable regulator: %d\n", ret); > + return ret; > + } > + > /* Reset the mux if a reset GPIO is specified. */ > gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); > - if (IS_ERR(gpio)) > - return PTR_ERR(gpio); > + if (IS_ERR(gpio)) { > + ret = PTR_ERR(gpio); > + goto fail_cleanup; > + } > if (gpio) { > udelay(1); > gpiod_set_value_cansleep(gpio, 0); > @@ -556,7 +578,7 @@ static int pca954x_probe(struct i2c_client *client, > > ret = i2c_get_device_id(client, &id); > if (ret && ret != -EOPNOTSUPP) > - return ret; > + goto fail_cleanup; > > if (!ret && > (id.manufacturer_id != data->chip->id.manufacturer_id || > @@ -564,7 +586,8 @@ static int pca954x_probe(struct i2c_client *client, > dev_warn(dev, "unexpected device id %03x-%03x-%x\n", > id.manufacturer_id, id.part_id, > id.die_revision); > - return -ENODEV; > + ret = -ENODEV; > + goto fail_cleanup; > } > } > > @@ -583,7 +606,8 @@ static int pca954x_probe(struct i2c_client *client, > ret = pca954x_init(client, data); > if (ret < 0) { > dev_warn(dev, "probe failed\n"); > - return -ENODEV; > + ret = -ENODEV; > + goto fail_cleanup; > } > > ret = pca954x_irq_setup(muxc); > -- > 2.37.3 >
On 08/10/2022 13:50, Serge Semin wrote: > On Fri, Oct 07, 2022 at 09:53:50AM +0200, Patrick Rudolph wrote: >> Update the pca954x bindings to add support for the Maxim MAX735x/MAX736x >> chips. The functionality will be provided by the exisintg pca954x driver. >> >> While on it make the interrupts support conditionally as not all of the >> existing chips have interrupts. >> >> For chips that are powered off by default add an optional regulator >> called vdd-supply. >> >> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >> --- >> .../bindings/i2c/i2c-mux-pca954x.yaml | 39 ++++++++++++++++--- >> 1 file changed, 34 insertions(+), 5 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml >> index 9f1726d0356b..efad0a95806f 100644 >> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml >> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml >> @@ -4,21 +4,25 @@ >> $id: http://devicetree.org/schemas/i2c/i2c-mux-pca954x.yaml# >> $schema: http://devicetree.org/meta-schemas/core.yaml# >> >> -title: NXP PCA954x I2C bus switch >> +title: NXP PCA954x I2C and compatible bus switches >> >> maintainers: >> - Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> description: >> - The binding supports NXP PCA954x and PCA984x I2C mux/switch devices. >> - > >> -allOf: >> - - $ref: /schemas/i2c/i2c-mux.yaml# > > Why do you move the allOf statement to the bottom of the schema? Because it goes with 'ifs' at the bottom of the schema... > >> + The binding supports NXP PCA954x and PCA984x I2C mux/switch devices, >> + and the Maxim MAX735x and MAX736x I2C mux/switch devices. > > What about combining the sentence: "The binding supports NXP > PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices." ? > Currently it does look a bit bulky. Drop "The binding supports". Instead describe the hardware. > >> >> properties: >> compatible: >> oneOf: >> - enum: >> + - maxim,max7356 >> + - maxim,max7357 >> + - maxim,max7358 >> + - maxim,max7367 >> + - maxim,max7368 >> + - maxim,max7369 >> - nxp,pca9540 >> - nxp,pca9542 >> - nxp,pca9543 >> @@ -59,10 +63,33 @@ properties: >> description: if present, overrides i2c-mux-idle-disconnect >> $ref: /schemas/mux/mux-controller.yaml#/properties/idle-state >> >> + vdd-supply: >> + description: A voltage regulator supplying power to the chip. >> + >> required: >> - compatible >> - reg >> >> +allOf: >> + - $ref: /schemas/i2c/i2c-mux.yaml# >> + - if: >> + not: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - maxim,max7367 >> + - maxim,max7369 >> + - nxp,pca9542 >> + - nxp,pca9543 >> + - nxp,pca9544 >> + - nxp,pca9545 >> + then: > >> + properties: >> + interrupts: false >> + "#interrupt-cells": false >> + interrupt-controller: false > > I'd suggest to add an opposite definition. Evaluate the properties for > the devices which expect them being evaluated instead of falsing their > existence for the devices which don't support the interrupts. The properties rather should be defined in top-level than in "if", so I am not sure how would you want to achieve opposite way. Best regards, Krzysztof
On Sun, Oct 09, 2022 at 05:25:22PM +0200, Krzysztof Kozlowski wrote: > On 08/10/2022 13:50, Serge Semin wrote: > > On Fri, Oct 07, 2022 at 09:53:50AM +0200, Patrick Rudolph wrote: > >> Update the pca954x bindings to add support for the Maxim MAX735x/MAX736x > >> chips. The functionality will be provided by the exisintg pca954x driver. > >> > >> While on it make the interrupts support conditionally as not all of the > >> existing chips have interrupts. > >> > >> For chips that are powered off by default add an optional regulator > >> called vdd-supply. > >> > >> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > >> --- > >> .../bindings/i2c/i2c-mux-pca954x.yaml | 39 ++++++++++++++++--- > >> 1 file changed, 34 insertions(+), 5 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml > >> index 9f1726d0356b..efad0a95806f 100644 > >> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml > >> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml > >> @@ -4,21 +4,25 @@ > >> $id: http://devicetree.org/schemas/i2c/i2c-mux-pca954x.yaml# > >> $schema: http://devicetree.org/meta-schemas/core.yaml# > >> > >> -title: NXP PCA954x I2C bus switch > >> +title: NXP PCA954x I2C and compatible bus switches > >> > >> maintainers: > >> - Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> > >> description: > >> - The binding supports NXP PCA954x and PCA984x I2C mux/switch devices. > >> - > > > >> -allOf: > >> - - $ref: /schemas/i2c/i2c-mux.yaml# > > > > Why do you move the allOf statement to the bottom of the schema? > > Because it goes with 'ifs' at the bottom of the schema... Is there a requirement to move the allOf array to the bottom of the schema if it contains the 'if' statement? If only there were some kernel doc with all such implicit conventions... > > > > >> + The binding supports NXP PCA954x and PCA984x I2C mux/switch devices, > >> + and the Maxim MAX735x and MAX736x I2C mux/switch devices. > > > > What about combining the sentence: "The binding supports NXP > > PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices." ? > > Currently it does look a bit bulky. > > Drop "The binding supports". Instead describe the hardware. > > > > >> > >> properties: > >> compatible: > >> oneOf: > >> - enum: > >> + - maxim,max7356 > >> + - maxim,max7357 > >> + - maxim,max7358 > >> + - maxim,max7367 > >> + - maxim,max7368 > >> + - maxim,max7369 > >> - nxp,pca9540 > >> - nxp,pca9542 > >> - nxp,pca9543 > >> @@ -59,10 +63,33 @@ properties: > >> description: if present, overrides i2c-mux-idle-disconnect > >> $ref: /schemas/mux/mux-controller.yaml#/properties/idle-state > >> > >> + vdd-supply: > >> + description: A voltage regulator supplying power to the chip. > >> + > >> required: > >> - compatible > >> - reg > >> > >> +allOf: > >> + - $ref: /schemas/i2c/i2c-mux.yaml# > >> + - if: > >> + not: > >> + properties: > >> + compatible: > >> + contains: > >> + enum: > >> + - maxim,max7367 > >> + - maxim,max7369 > >> + - nxp,pca9542 > >> + - nxp,pca9543 > >> + - nxp,pca9544 > >> + - nxp,pca9545 > >> + then: > > > >> + properties: > >> + interrupts: false > >> + "#interrupt-cells": false > >> + interrupt-controller: false > > > > I'd suggest to add an opposite definition. Evaluate the properties for > > the devices which expect them being evaluated instead of falsing their > > existence for the devices which don't support the interrupts. > > The properties rather should be defined in top-level than in "if", so I > am not sure how would you want to achieve opposite way. With one more implicit convention like "preferably define the properties in the top-level than in if" of course I can't. Otherwise I thought something like this would work: +allOf: + - ... + - if: + properties: + compatible: + contains: + enum: [...] + then: + properties: + interrupts: ... + "#interrupt-cells": ... + interrupt-controller: ... ... - interrupts: - "#interrupt-cells": - interrupt-controller: ... With unevaluatedProperties set to false and evaluation performed for the particular compatibles such schema shall work with the same semantic. -Sergey > > > Best regards, > Krzysztof >
On 09/10/2022 14:03, Serge Semin wrote: > On Sun, Oct 09, 2022 at 05:25:22PM +0200, Krzysztof Kozlowski wrote: >> On 08/10/2022 13:50, Serge Semin wrote: >>> On Fri, Oct 07, 2022 at 09:53:50AM +0200, Patrick Rudolph wrote: >>>> Update the pca954x bindings to add support for the Maxim MAX735x/MAX736x >>>> chips. The functionality will be provided by the exisintg pca954x driver. >>>> >>>> While on it make the interrupts support conditionally as not all of the >>>> existing chips have interrupts. >>>> >>>> For chips that are powered off by default add an optional regulator >>>> called vdd-supply. >>>> >>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> >>>> --- >>>> .../bindings/i2c/i2c-mux-pca954x.yaml | 39 ++++++++++++++++--- >>>> 1 file changed, 34 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml >>>> index 9f1726d0356b..efad0a95806f 100644 >>>> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml >>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml >>>> @@ -4,21 +4,25 @@ >>>> $id: http://devicetree.org/schemas/i2c/i2c-mux-pca954x.yaml# >>>> $schema: http://devicetree.org/meta-schemas/core.yaml# >>>> >>>> -title: NXP PCA954x I2C bus switch >>>> +title: NXP PCA954x I2C and compatible bus switches >>>> >>>> maintainers: >>>> - Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> >>>> description: >>>> - The binding supports NXP PCA954x and PCA984x I2C mux/switch devices. >>>> - >>> >>>> -allOf: >>>> - - $ref: /schemas/i2c/i2c-mux.yaml# >>> >>> Why do you move the allOf statement to the bottom of the schema? >> > >> Because it goes with 'ifs' at the bottom of the schema... > > Is there a requirement to move the allOf array to the bottom of the > schema if it contains the 'if' statement? If only there were some > kernel doc with all such implicit conventions... It's just a convention, although quite logical because "ifs" can grow significantly, so putting it before properties is outside of context. Reader does not know yet to what this if applies. > >> >>> >>>> + The binding supports NXP PCA954x and PCA984x I2C mux/switch devices, >>>> + and the Maxim MAX735x and MAX736x I2C mux/switch devices. >>> >>> What about combining the sentence: "The binding supports NXP >>> PCA954x/PCA984x and Maxim MAX735x/MAX736x I2C mux/switch devices." ? >>> Currently it does look a bit bulky. >> >> Drop "The binding supports". Instead describe the hardware. >> >>> >>>> >>>> properties: >>>> compatible: >>>> oneOf: >>>> - enum: >>>> + - maxim,max7356 >>>> + - maxim,max7357 >>>> + - maxim,max7358 >>>> + - maxim,max7367 >>>> + - maxim,max7368 >>>> + - maxim,max7369 >>>> - nxp,pca9540 >>>> - nxp,pca9542 >>>> - nxp,pca9543 >>>> @@ -59,10 +63,33 @@ properties: >>>> description: if present, overrides i2c-mux-idle-disconnect >>>> $ref: /schemas/mux/mux-controller.yaml#/properties/idle-state >>>> >>>> + vdd-supply: >>>> + description: A voltage regulator supplying power to the chip. >>>> + >>>> required: >>>> - compatible >>>> - reg >>>> >>>> +allOf: >>>> + - $ref: /schemas/i2c/i2c-mux.yaml# >>>> + - if: >>>> + not: >>>> + properties: >>>> + compatible: >>>> + contains: >>>> + enum: >>>> + - maxim,max7367 >>>> + - maxim,max7369 >>>> + - nxp,pca9542 >>>> + - nxp,pca9543 >>>> + - nxp,pca9544 >>>> + - nxp,pca9545 >>>> + then: >>> >>>> + properties: >>>> + interrupts: false >>>> + "#interrupt-cells": false >>>> + interrupt-controller: false >>> >>> I'd suggest to add an opposite definition. Evaluate the properties for >>> the devices which expect them being evaluated instead of falsing their >>> existence for the devices which don't support the interrupts. >> > >> The properties rather should be defined in top-level than in "if", so I >> am not sure how would you want to achieve opposite way. > > With one more implicit convention like "preferably define the > properties in the top-level than in if" of course I can't. Otherwise I > thought something like this would work: > +allOf: > + - ... > + - if: > + properties: > + compatible: > + contains: > + enum: [...] > + then: > + properties: > + interrupts: ... > + "#interrupt-cells": ... > + interrupt-controller: ... > ... > - interrupts: > - "#interrupt-cells": > - interrupt-controller: ... > > With unevaluatedProperties set to false and evaluation performed for > the particular compatibles such schema shall work with the same > semantic. Yes, this will work, but defining properties inside "if" is usually not readable. Best regards, Krzysztof