| Message ID | 20201031184854.745828-3-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dt-bindings:iio: yet more txt to yam conversions | expand |
On 1/11/2020 02:48, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Simple direct conversion from txt to yaml as part of a general aim of > converting all IIO bindings to this machine readable format. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Phil Reid <preid@electromag.com.au> Reviewed-by: Phil Reid <preid@electromag.com.au> > --- > .../bindings/iio/potentiometer/ad5272.txt | 27 ---------- > .../iio/potentiometer/adi,ad5272.yaml | 50 +++++++++++++++++++ > 2 files changed, 50 insertions(+), 27 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt > deleted file mode 100644 > index f9b2eef946aa..000000000000 > --- a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt > +++ /dev/null > @@ -1,27 +0,0 @@ > -* Analog Devices AD5272 digital potentiometer > - > -The node for this device must be a child node of a I2C controller, hence > -all mandatory properties for your controller must be specified. See directory: > - > - Documentation/devicetree/bindings/i2c > - > -for more details. > - > -Required properties: > - - compatible: Must be one of the following, depending on the model: > - adi,ad5272-020 > - adi,ad5272-050 > - adi,ad5272-100 > - adi,ad5274-020 > - adi,ad5274-100 > - > -Optional properties: > - - reset-gpios: GPIO specification for the RESET input. This is an > - active low signal to the AD5272. > - > -Example: > -ad5272: potentiometer@2f { > - reg = <0x2F>; > - compatible = "adi,ad5272-020"; > - reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; > -}; > diff --git a/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml > new file mode 100644 > index 000000000000..b9b7d383bff1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml > @@ -0,0 +1,50 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/potentiometer/adi,ad5272.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices AD5272 digital potentiometer > + > +maintainers: > + - Phil Reid <preid@electromag.com.au> > + > +description: | > + Datasheet: https://www.analog.com/en/products/ad5272.html > + > +properties: > + compatible: > + enum: > + - adi,ad5272-020 > + - adi,ad5272-050 > + - adi,ad5272-100 > + - adi,ad5274-020 > + - adi,ad5274-100 > + > + reg: > + maxItems: 1 > + > + reset-gpios: > + description: > + Active low signal to the AD5272 RESET input. > + > +additionalProperties: false > + > +required: > + - compatible > + - reg > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + potentiometer@2f { > + compatible = "adi,ad5272-020"; > + reg = <0x2F>; > + reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; > + }; > + }; > +... >
On Sat, Oct 31, 2020 at 06:48:10PM +0000, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Simple direct conversion from txt to yaml as part of a general aim of > converting all IIO bindings to this machine readable format. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Phil Reid <preid@electromag.com.au> > --- > .../bindings/iio/potentiometer/ad5272.txt | 27 ---------- > .../iio/potentiometer/adi,ad5272.yaml | 50 +++++++++++++++++++ > 2 files changed, 50 insertions(+), 27 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt > deleted file mode 100644 > index f9b2eef946aa..000000000000 > --- a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt > +++ /dev/null > @@ -1,27 +0,0 @@ > -* Analog Devices AD5272 digital potentiometer > - > -The node for this device must be a child node of a I2C controller, hence > -all mandatory properties for your controller must be specified. See directory: > - > - Documentation/devicetree/bindings/i2c > - > -for more details. > - > -Required properties: > - - compatible: Must be one of the following, depending on the model: > - adi,ad5272-020 > - adi,ad5272-050 > - adi,ad5272-100 > - adi,ad5274-020 > - adi,ad5274-100 > - > -Optional properties: > - - reset-gpios: GPIO specification for the RESET input. This is an > - active low signal to the AD5272. > - > -Example: > -ad5272: potentiometer@2f { > - reg = <0x2F>; > - compatible = "adi,ad5272-020"; > - reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; > -}; > diff --git a/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml > new file mode 100644 > index 000000000000..b9b7d383bff1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml > @@ -0,0 +1,50 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/potentiometer/adi,ad5272.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices AD5272 digital potentiometer > + > +maintainers: > + - Phil Reid <preid@electromag.com.au> > + > +description: | > + Datasheet: https://www.analog.com/en/products/ad5272.html > + > +properties: > + compatible: > + enum: > + - adi,ad5272-020 > + - adi,ad5272-050 > + - adi,ad5272-100 > + - adi,ad5274-020 > + - adi,ad5274-100 > + > + reg: > + maxItems: 1 > + > + reset-gpios: > + description: > + Active low signal to the AD5272 RESET input. Not a new problem, but active low or... > + > +additionalProperties: false > + > +required: > + - compatible > + - reg > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + potentiometer@2f { > + compatible = "adi,ad5272-020"; > + reg = <0x2F>; > + reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; active high? > + }; > + }; > +... > -- > 2.28.0 >
On Tue, 3 Nov 2020 10:10:39 -0600 Rob Herring <robh@kernel.org> wrote: > On Sat, Oct 31, 2020 at 06:48:10PM +0000, Jonathan Cameron wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > Simple direct conversion from txt to yaml as part of a general aim of > > converting all IIO bindings to this machine readable format. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Cc: Phil Reid <preid@electromag.com.au> > > --- > > .../bindings/iio/potentiometer/ad5272.txt | 27 ---------- > > .../iio/potentiometer/adi,ad5272.yaml | 50 +++++++++++++++++++ > > 2 files changed, 50 insertions(+), 27 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt > > deleted file mode 100644 > > index f9b2eef946aa..000000000000 > > --- a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt > > +++ /dev/null > > @@ -1,27 +0,0 @@ > > -* Analog Devices AD5272 digital potentiometer > > - > > -The node for this device must be a child node of a I2C controller, hence > > -all mandatory properties for your controller must be specified. See directory: > > - > > - Documentation/devicetree/bindings/i2c > > - > > -for more details. > > - > > -Required properties: > > - - compatible: Must be one of the following, depending on the model: > > - adi,ad5272-020 > > - adi,ad5272-050 > > - adi,ad5272-100 > > - adi,ad5274-020 > > - adi,ad5274-100 > > - > > -Optional properties: > > - - reset-gpios: GPIO specification for the RESET input. This is an > > - active low signal to the AD5272. > > - > > -Example: > > -ad5272: potentiometer@2f { > > - reg = <0x2F>; > > - compatible = "adi,ad5272-020"; > > - reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; > > -}; > > diff --git a/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml > > new file mode 100644 > > index 000000000000..b9b7d383bff1 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml > > @@ -0,0 +1,50 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/iio/potentiometer/adi,ad5272.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Analog Devices AD5272 digital potentiometer > > + > > +maintainers: > > + - Phil Reid <preid@electromag.com.au> > > + > > +description: | > > + Datasheet: https://www.analog.com/en/products/ad5272.html > > + > > +properties: > > + compatible: > > + enum: > > + - adi,ad5272-020 > > + - adi,ad5272-050 > > + - adi,ad5272-100 > > + - adi,ad5274-020 > > + - adi,ad5274-100 > > + > > + reg: > > + maxItems: 1 > > + > > + reset-gpios: > > + description: > > + Active low signal to the AD5272 RESET input. > > Not a new problem, but active low or... > > > + > > +additionalProperties: false > > + > > +required: > > + - compatible > > + - reg > > + > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + potentiometer@2f { > > + compatible = "adi,ad5272-020"; > > + reg = <0x2F>; > > + reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; > > active high? Good spot! @Phil. Looks like the driver is setting the reset line to 0 and then to 1 to come out of reset. So effectively inverting the logic. I'm tempted to be cynical and suggest we just drop the comment above and leave it vague but is there a better way we can clarify this? > > > + }; > > + }; > > +... > > -- > > 2.28.0 > >
On 4/11/2020 01:28, Jonathan Cameron wrote: > On Tue, 3 Nov 2020 10:10:39 -0600 > Rob Herring <robh@kernel.org> wrote: > >> On Sat, Oct 31, 2020 at 06:48:10PM +0000, Jonathan Cameron wrote: >>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>> >>> Simple direct conversion from txt to yaml as part of a general aim of >>> converting all IIO bindings to this machine readable format. >>> >>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>> Cc: Phil Reid <preid@electromag.com.au> >>> --- >>> .../bindings/iio/potentiometer/ad5272.txt | 27 ---------- >>> .../iio/potentiometer/adi,ad5272.yaml | 50 +++++++++++++++++++ >>> 2 files changed, 50 insertions(+), 27 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt >>> deleted file mode 100644 >>> index f9b2eef946aa..000000000000 >>> --- a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt >>> +++ /dev/null >>> @@ -1,27 +0,0 @@ >>> -* Analog Devices AD5272 digital potentiometer >>> - >>> -The node for this device must be a child node of a I2C controller, hence >>> -all mandatory properties for your controller must be specified. See directory: >>> - >>> - Documentation/devicetree/bindings/i2c >>> - >>> -for more details. >>> - >>> -Required properties: >>> - - compatible: Must be one of the following, depending on the model: >>> - adi,ad5272-020 >>> - adi,ad5272-050 >>> - adi,ad5272-100 >>> - adi,ad5274-020 >>> - adi,ad5274-100 >>> - >>> -Optional properties: >>> - - reset-gpios: GPIO specification for the RESET input. This is an >>> - active low signal to the AD5272. >>> - >>> -Example: >>> -ad5272: potentiometer@2f { >>> - reg = <0x2F>; >>> - compatible = "adi,ad5272-020"; >>> - reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; >>> -}; >>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml >>> new file mode 100644 >>> index 000000000000..b9b7d383bff1 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml >>> @@ -0,0 +1,50 @@ >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/iio/potentiometer/adi,ad5272.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Analog Devices AD5272 digital potentiometer >>> + >>> +maintainers: >>> + - Phil Reid <preid@electromag.com.au> >>> + >>> +description: | >>> + Datasheet: https://www.analog.com/en/products/ad5272.html >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - adi,ad5272-020 >>> + - adi,ad5272-050 >>> + - adi,ad5272-100 >>> + - adi,ad5274-020 >>> + - adi,ad5274-100 >>> + >>> + reg: >>> + maxItems: 1 >>> + >>> + reset-gpios: >>> + description: >>> + Active low signal to the AD5272 RESET input. >> >> Not a new problem, but active low or... >> >>> + >>> +additionalProperties: false >>> + >>> +required: >>> + - compatible >>> + - reg >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/gpio/gpio.h> >>> + i2c { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + potentiometer@2f { >>> + compatible = "adi,ad5272-020"; >>> + reg = <0x2F>; >>> + reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; >> >> active high? > > Good spot! @Phil. Looks like the driver is setting the reset line to > 0 and then to 1 to come out of reset. So effectively inverting the logic. > I'm tempted to be cynical and suggest we just drop the comment above and leave > it vague but is there a better way we can clarify this? Had a look at a few other iio drivers in regards how they handle the same thing. A few do the same thing, ie: the drivers are written to set gpio low to assert reset. So they need the device tree gpio config to be active high to work correctly. Not sure if this prevents users setting things up as open collector. I also reviewed my boards and they aren't using the reset line at the moment. So I've never properly tested that, but hte code looks ok. I'm happy to go the vague way. > >> >>> + }; >>> + }; >>> +... >>> -- >>> 2.28.0 >>> > >
On Tue, Nov 3, 2020 at 6:39 PM Phil Reid <preid@electromag.com.au> wrote: > > On 4/11/2020 01:28, Jonathan Cameron wrote: > > On Tue, 3 Nov 2020 10:10:39 -0600 > > Rob Herring <robh@kernel.org> wrote: > > > >> On Sat, Oct 31, 2020 at 06:48:10PM +0000, Jonathan Cameron wrote: > >>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >>> > >>> Simple direct conversion from txt to yaml as part of a general aim of > >>> converting all IIO bindings to this machine readable format. > >>> > >>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >>> Cc: Phil Reid <preid@electromag.com.au> > >>> --- > >>> .../bindings/iio/potentiometer/ad5272.txt | 27 ---------- > >>> .../iio/potentiometer/adi,ad5272.yaml | 50 +++++++++++++++++++ > >>> 2 files changed, 50 insertions(+), 27 deletions(-) > >>> > >>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt > >>> deleted file mode 100644 > >>> index f9b2eef946aa..000000000000 > >>> --- a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt > >>> +++ /dev/null > >>> @@ -1,27 +0,0 @@ > >>> -* Analog Devices AD5272 digital potentiometer > >>> - > >>> -The node for this device must be a child node of a I2C controller, hence > >>> -all mandatory properties for your controller must be specified. See directory: > >>> - > >>> - Documentation/devicetree/bindings/i2c > >>> - > >>> -for more details. > >>> - > >>> -Required properties: > >>> - - compatible: Must be one of the following, depending on the model: > >>> - adi,ad5272-020 > >>> - adi,ad5272-050 > >>> - adi,ad5272-100 > >>> - adi,ad5274-020 > >>> - adi,ad5274-100 > >>> - > >>> -Optional properties: > >>> - - reset-gpios: GPIO specification for the RESET input. This is an > >>> - active low signal to the AD5272. > >>> - > >>> -Example: > >>> -ad5272: potentiometer@2f { > >>> - reg = <0x2F>; > >>> - compatible = "adi,ad5272-020"; > >>> - reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; > >>> -}; > >>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml > >>> new file mode 100644 > >>> index 000000000000..b9b7d383bff1 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml > >>> @@ -0,0 +1,50 @@ > >>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > >>> +%YAML 1.2 > >>> +--- > >>> +$id: http://devicetree.org/schemas/iio/potentiometer/adi,ad5272.yaml# > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>> + > >>> +title: Analog Devices AD5272 digital potentiometer > >>> + > >>> +maintainers: > >>> + - Phil Reid <preid@electromag.com.au> > >>> + > >>> +description: | > >>> + Datasheet: https://www.analog.com/en/products/ad5272.html > >>> + > >>> +properties: > >>> + compatible: > >>> + enum: > >>> + - adi,ad5272-020 > >>> + - adi,ad5272-050 > >>> + - adi,ad5272-100 > >>> + - adi,ad5274-020 > >>> + - adi,ad5274-100 > >>> + > >>> + reg: > >>> + maxItems: 1 > >>> + > >>> + reset-gpios: > >>> + description: > >>> + Active low signal to the AD5272 RESET input. > >> > >> Not a new problem, but active low or... > >> > >>> + > >>> +additionalProperties: false > >>> + > >>> +required: > >>> + - compatible > >>> + - reg > >>> + > >>> +examples: > >>> + - | > >>> + #include <dt-bindings/gpio/gpio.h> > >>> + i2c { > >>> + #address-cells = <1>; > >>> + #size-cells = <0>; > >>> + > >>> + potentiometer@2f { > >>> + compatible = "adi,ad5272-020"; > >>> + reg = <0x2F>; > >>> + reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; > >> > >> active high? > > > > Good spot! @Phil. Looks like the driver is setting the reset line to > > 0 and then to 1 to come out of reset. So effectively inverting the logic. > > I'm tempted to be cynical and suggest we just drop the comment above and leave > > it vague but is there a better way we can clarify this? > > Had a look at a few other iio drivers in regards how they handle the same thing. > A few do the same thing, ie: the drivers are written to set gpio low to assert reset. > So they need the device tree gpio config to be active high to work correctly. > Not sure if this prevents users setting things up as open collector. The driver is wrong. 'gpiod_set_value(reset_gpio, 1);' should assert reset as '1' here is set to (reset) active state as defined in the DT. Given no upstream users, maybe it can be fixed... We need to make 'reset-gpios' implemented by a reset controller and stop letting drivers get it wrong. Rob
On 4/11/2020 11:12, Rob Herring wrote: > On Tue, Nov 3, 2020 at 6:39 PM Phil Reid <preid@electromag.com.au> wrote: >> >> On 4/11/2020 01:28, Jonathan Cameron wrote: >>> On Tue, 3 Nov 2020 10:10:39 -0600 >>> Rob Herring <robh@kernel.org> wrote: >>> >>>> On Sat, Oct 31, 2020 at 06:48:10PM +0000, Jonathan Cameron wrote: >>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>> >>>>> Simple direct conversion from txt to yaml as part of a general aim of >>>>> converting all IIO bindings to this machine readable format. >>>>> >>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>> Cc: Phil Reid <preid@electromag.com.au> >>>>> --- >>>>> .../bindings/iio/potentiometer/ad5272.txt | 27 ---------- >>>>> .../iio/potentiometer/adi,ad5272.yaml | 50 +++++++++++++++++++ >>>>> 2 files changed, 50 insertions(+), 27 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt >>>>> deleted file mode 100644 >>>>> index f9b2eef946aa..000000000000 >>>>> --- a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt >>>>> +++ /dev/null >>>>> @@ -1,27 +0,0 @@ >>>>> -* Analog Devices AD5272 digital potentiometer >>>>> - >>>>> -The node for this device must be a child node of a I2C controller, hence >>>>> -all mandatory properties for your controller must be specified. See directory: >>>>> - >>>>> - Documentation/devicetree/bindings/i2c >>>>> - >>>>> -for more details. >>>>> - >>>>> -Required properties: >>>>> - - compatible: Must be one of the following, depending on the model: >>>>> - adi,ad5272-020 >>>>> - adi,ad5272-050 >>>>> - adi,ad5272-100 >>>>> - adi,ad5274-020 >>>>> - adi,ad5274-100 >>>>> - >>>>> -Optional properties: >>>>> - - reset-gpios: GPIO specification for the RESET input. This is an >>>>> - active low signal to the AD5272. >>>>> - >>>>> -Example: >>>>> -ad5272: potentiometer@2f { >>>>> - reg = <0x2F>; >>>>> - compatible = "adi,ad5272-020"; >>>>> - reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; >>>>> -}; >>>>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml >>>>> new file mode 100644 >>>>> index 000000000000..b9b7d383bff1 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml >>>>> @@ -0,0 +1,50 @@ >>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>> +%YAML 1.2 >>>>> +--- >>>>> +$id: http://devicetree.org/schemas/iio/potentiometer/adi,ad5272.yaml# >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>> + >>>>> +title: Analog Devices AD5272 digital potentiometer >>>>> + >>>>> +maintainers: >>>>> + - Phil Reid <preid@electromag.com.au> >>>>> + >>>>> +description: | >>>>> + Datasheet: https://www.analog.com/en/products/ad5272.html >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + enum: >>>>> + - adi,ad5272-020 >>>>> + - adi,ad5272-050 >>>>> + - adi,ad5272-100 >>>>> + - adi,ad5274-020 >>>>> + - adi,ad5274-100 >>>>> + >>>>> + reg: >>>>> + maxItems: 1 >>>>> + >>>>> + reset-gpios: >>>>> + description: >>>>> + Active low signal to the AD5272 RESET input. >>>> >>>> Not a new problem, but active low or... >>>> >>>>> + >>>>> +additionalProperties: false >>>>> + >>>>> +required: >>>>> + - compatible >>>>> + - reg >>>>> + >>>>> +examples: >>>>> + - | >>>>> + #include <dt-bindings/gpio/gpio.h> >>>>> + i2c { >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + >>>>> + potentiometer@2f { >>>>> + compatible = "adi,ad5272-020"; >>>>> + reg = <0x2F>; >>>>> + reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; >>>> >>>> active high? >>> >>> Good spot! @Phil. Looks like the driver is setting the reset line to >>> 0 and then to 1 to come out of reset. So effectively inverting the logic. >>> I'm tempted to be cynical and suggest we just drop the comment above and leave >>> it vague but is there a better way we can clarify this? >> >> Had a look at a few other iio drivers in regards how they handle the same thing. >> A few do the same thing, ie: the drivers are written to set gpio low to assert reset. >> So they need the device tree gpio config to be active high to work correctly. >> Not sure if this prevents users setting things up as open collector. > > The driver is wrong. 'gpiod_set_value(reset_gpio, 1);' should assert > reset as '1' here is set to (reset) active state as defined in the DT. > > Given no upstream users, maybe it can be fixed... > > We need to make 'reset-gpios' implemented by a reset controller and > stop letting drivers get it wrong. > Yes I agree, the driver is wrong, think I just copied one of the other drivers for the pattern. I'd be happy to change it, there's probably few (if any) users. Having a software interface to assert the reset would be nice.
On 4/11/2020 12:53, Phil Reid wrote: > On 4/11/2020 11:12, Rob Herring wrote: >> On Tue, Nov 3, 2020 at 6:39 PM Phil Reid <preid@electromag.com.au> wrote: >>> >>> On 4/11/2020 01:28, Jonathan Cameron wrote: >>>> On Tue, 3 Nov 2020 10:10:39 -0600 >>>> Rob Herring <robh@kernel.org> wrote: >>>> >>>>> On Sat, Oct 31, 2020 at 06:48:10PM +0000, Jonathan Cameron wrote: >>>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>>> >>>>>> Simple direct conversion from txt to yaml as part of a general aim of >>>>>> converting all IIO bindings to this machine readable format. >>>>>> >>>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> >>>>>> Cc: Phil Reid <preid@electromag.com.au> >>>>>> --- >>>>>> .../bindings/iio/potentiometer/ad5272.txt | 27 ---------- >>>>>> .../iio/potentiometer/adi,ad5272.yaml | 50 +++++++++++++++++++ >>>>>> 2 files changed, 50 insertions(+), 27 deletions(-) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt >>>>>> deleted file mode 100644 >>>>>> index f9b2eef946aa..000000000000 >>>>>> --- a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt >>>>>> +++ /dev/null >>>>>> @@ -1,27 +0,0 @@ >>>>>> -* Analog Devices AD5272 digital potentiometer >>>>>> - >>>>>> -The node for this device must be a child node of a I2C controller, hence >>>>>> -all mandatory properties for your controller must be specified. See directory: >>>>>> - >>>>>> - Documentation/devicetree/bindings/i2c >>>>>> - >>>>>> -for more details. >>>>>> - >>>>>> -Required properties: >>>>>> - - compatible: Must be one of the following, depending on the model: >>>>>> - adi,ad5272-020 >>>>>> - adi,ad5272-050 >>>>>> - adi,ad5272-100 >>>>>> - adi,ad5274-020 >>>>>> - adi,ad5274-100 >>>>>> - >>>>>> -Optional properties: >>>>>> - - reset-gpios: GPIO specification for the RESET input. This is an >>>>>> - active low signal to the AD5272. >>>>>> - >>>>>> -Example: >>>>>> -ad5272: potentiometer@2f { >>>>>> - reg = <0x2F>; >>>>>> - compatible = "adi,ad5272-020"; >>>>>> - reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; >>>>>> -}; >>>>>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml >>>>>> new file mode 100644 >>>>>> index 000000000000..b9b7d383bff1 >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml >>>>>> @@ -0,0 +1,50 @@ >>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>>> +%YAML 1.2 >>>>>> +--- >>>>>> +$id: http://devicetree.org/schemas/iio/potentiometer/adi,ad5272.yaml# >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>> + >>>>>> +title: Analog Devices AD5272 digital potentiometer >>>>>> + >>>>>> +maintainers: >>>>>> + - Phil Reid <preid@electromag.com.au> >>>>>> + >>>>>> +description: | >>>>>> + Datasheet: https://www.analog.com/en/products/ad5272.html >>>>>> + >>>>>> +properties: >>>>>> + compatible: >>>>>> + enum: >>>>>> + - adi,ad5272-020 >>>>>> + - adi,ad5272-050 >>>>>> + - adi,ad5272-100 >>>>>> + - adi,ad5274-020 >>>>>> + - adi,ad5274-100 >>>>>> + >>>>>> + reg: >>>>>> + maxItems: 1 >>>>>> + >>>>>> + reset-gpios: >>>>>> + description: >>>>>> + Active low signal to the AD5272 RESET input. >>>>> >>>>> Not a new problem, but active low or... >>>>> >>>>>> + >>>>>> +additionalProperties: false >>>>>> + >>>>>> +required: >>>>>> + - compatible >>>>>> + - reg >>>>>> + >>>>>> +examples: >>>>>> + - | >>>>>> + #include <dt-bindings/gpio/gpio.h> >>>>>> + i2c { >>>>>> + #address-cells = <1>; >>>>>> + #size-cells = <0>; >>>>>> + >>>>>> + potentiometer@2f { >>>>>> + compatible = "adi,ad5272-020"; >>>>>> + reg = <0x2F>; >>>>>> + reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; >>>>> >>>>> active high? >>>> >>>> Good spot! @Phil. Looks like the driver is setting the reset line to >>>> 0 and then to 1 to come out of reset. So effectively inverting the logic. >>>> I'm tempted to be cynical and suggest we just drop the comment above and leave >>>> it vague but is there a better way we can clarify this? >>> >>> Had a look at a few other iio drivers in regards how they handle the same thing. >>> A few do the same thing, ie: the drivers are written to set gpio low to assert reset. >>> So they need the device tree gpio config to be active high to work correctly. >>> Not sure if this prevents users setting things up as open collector. >> >> The driver is wrong. 'gpiod_set_value(reset_gpio, 1);' should assert >> reset as '1' here is set to (reset) active state as defined in the DT. >> >> Given no upstream users, maybe it can be fixed... >> >> We need to make 'reset-gpios' implemented by a reset controller and >> stop letting drivers get it wrong. >> > > Yes I agree, the driver is wrong, think I just copied one of the other drivers for the pattern. > I'd be happy to change it, there's probably few (if any) users. > > Having a software interface to assert the reset would be nice. > > If there's no comments against the change, I'll submit a patch in the next day or so.
On Mon, 9 Nov 2020 10:56:47 +0800 Phil Reid <preid@electromag.com.au> wrote: > On 4/11/2020 12:53, Phil Reid wrote: > > On 4/11/2020 11:12, Rob Herring wrote: > >> On Tue, Nov 3, 2020 at 6:39 PM Phil Reid <preid@electromag.com.au> wrote: > >>> > >>> On 4/11/2020 01:28, Jonathan Cameron wrote: > >>>> On Tue, 3 Nov 2020 10:10:39 -0600 > >>>> Rob Herring <robh@kernel.org> wrote: > >>>> > >>>>> On Sat, Oct 31, 2020 at 06:48:10PM +0000, Jonathan Cameron wrote: > >>>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >>>>>> > >>>>>> Simple direct conversion from txt to yaml as part of a general aim of > >>>>>> converting all IIO bindings to this machine readable format. > >>>>>> > >>>>>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >>>>>> Cc: Phil Reid <preid@electromag.com.au> > >>>>>> --- > >>>>>> .../bindings/iio/potentiometer/ad5272.txt | 27 ---------- > >>>>>> .../iio/potentiometer/adi,ad5272.yaml | 50 +++++++++++++++++++ > >>>>>> 2 files changed, 50 insertions(+), 27 deletions(-) > >>>>>> > >>>>>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt > >>>>>> deleted file mode 100644 > >>>>>> index f9b2eef946aa..000000000000 > >>>>>> --- a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt > >>>>>> +++ /dev/null > >>>>>> @@ -1,27 +0,0 @@ > >>>>>> -* Analog Devices AD5272 digital potentiometer > >>>>>> - > >>>>>> -The node for this device must be a child node of a I2C controller, hence > >>>>>> -all mandatory properties for your controller must be specified. See directory: > >>>>>> - > >>>>>> - Documentation/devicetree/bindings/i2c > >>>>>> - > >>>>>> -for more details. > >>>>>> - > >>>>>> -Required properties: > >>>>>> - - compatible: Must be one of the following, depending on the model: > >>>>>> - adi,ad5272-020 > >>>>>> - adi,ad5272-050 > >>>>>> - adi,ad5272-100 > >>>>>> - adi,ad5274-020 > >>>>>> - adi,ad5274-100 > >>>>>> - > >>>>>> -Optional properties: > >>>>>> - - reset-gpios: GPIO specification for the RESET input. This is an > >>>>>> - active low signal to the AD5272. > >>>>>> - > >>>>>> -Example: > >>>>>> -ad5272: potentiometer@2f { > >>>>>> - reg = <0x2F>; > >>>>>> - compatible = "adi,ad5272-020"; > >>>>>> - reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; > >>>>>> -}; > >>>>>> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml > >>>>>> new file mode 100644 > >>>>>> index 000000000000..b9b7d383bff1 > >>>>>> --- /dev/null > >>>>>> +++ b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml > >>>>>> @@ -0,0 +1,50 @@ > >>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > >>>>>> +%YAML 1.2 > >>>>>> +--- > >>>>>> +$id: http://devicetree.org/schemas/iio/potentiometer/adi,ad5272.yaml# > >>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>>>>> + > >>>>>> +title: Analog Devices AD5272 digital potentiometer > >>>>>> + > >>>>>> +maintainers: > >>>>>> + - Phil Reid <preid@electromag.com.au> > >>>>>> + > >>>>>> +description: | > >>>>>> + Datasheet: https://www.analog.com/en/products/ad5272.html > >>>>>> + > >>>>>> +properties: > >>>>>> + compatible: > >>>>>> + enum: > >>>>>> + - adi,ad5272-020 > >>>>>> + - adi,ad5272-050 > >>>>>> + - adi,ad5272-100 > >>>>>> + - adi,ad5274-020 > >>>>>> + - adi,ad5274-100 > >>>>>> + > >>>>>> + reg: > >>>>>> + maxItems: 1 > >>>>>> + > >>>>>> + reset-gpios: > >>>>>> + description: > >>>>>> + Active low signal to the AD5272 RESET input. > >>>>> > >>>>> Not a new problem, but active low or... > >>>>> > >>>>>> + > >>>>>> +additionalProperties: false > >>>>>> + > >>>>>> +required: > >>>>>> + - compatible > >>>>>> + - reg > >>>>>> + > >>>>>> +examples: > >>>>>> + - | > >>>>>> + #include <dt-bindings/gpio/gpio.h> > >>>>>> + i2c { > >>>>>> + #address-cells = <1>; > >>>>>> + #size-cells = <0>; > >>>>>> + > >>>>>> + potentiometer@2f { > >>>>>> + compatible = "adi,ad5272-020"; > >>>>>> + reg = <0x2F>; > >>>>>> + reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; > >>>>> > >>>>> active high? > >>>> > >>>> Good spot! @Phil. Looks like the driver is setting the reset line to > >>>> 0 and then to 1 to come out of reset. So effectively inverting the logic. > >>>> I'm tempted to be cynical and suggest we just drop the comment above and leave > >>>> it vague but is there a better way we can clarify this? > >>> > >>> Had a look at a few other iio drivers in regards how they handle the same thing. > >>> A few do the same thing, ie: the drivers are written to set gpio low to assert reset. > >>> So they need the device tree gpio config to be active high to work correctly. > >>> Not sure if this prevents users setting things up as open collector. > >> > >> The driver is wrong. 'gpiod_set_value(reset_gpio, 1);' should assert > >> reset as '1' here is set to (reset) active state as defined in the DT. > >> > >> Given no upstream users, maybe it can be fixed... > >> > >> We need to make 'reset-gpios' implemented by a reset controller and > >> stop letting drivers get it wrong. > >> > > > > Yes I agree, the driver is wrong, think I just copied one of the other drivers for the pattern. > > I'd be happy to change it, there's probably few (if any) users. > > > > Having a software interface to assert the reset would be nice. > > > > > > If there's no comments against the change, I'll submit a patch in the next day or so. Hi Phil, So I've fixed the binding example up to have the sense as ACTIVE_LOW as discussed. If you have time to flip the sense round in the driver to match that it would be great and I'd propose we then mark that as suitable for stable so that we get it backported into existing trees. We could also spin a patch to the txt binding but perhaps that is more effort than is needed here. If I get time before you do I might send a proposed fix patch. Hopefully anyone then using it will get the sense right and we won't accidentally break anyone. At some point soon I'll also try and audit similar cases and hopefully we will get those fixed up as well. Obviously if anyone else wants to take a look at that it would be much appreciated! Thanks, Jonathan > >
On 23/11/2020 00:35, Jonathan Cameron wrote: >>> Having a software interface to assert the reset would be nice. >>> >>> >> If there's no comments against the change, I'll submit a patch in the next day or so. > Hi Phil, > > So I've fixed the binding example up to have the sense as ACTIVE_LOW as discussed. > If you have time to flip the sense round in the driver to match that it > would be great and I'd propose we then mark that as suitable for stable > so that we get it backported into existing trees. We could also spin > a patch to the txt binding but perhaps that is more effort than is needed > here. If I get time before you do I might send a proposed fix patch. > > Hopefully anyone then using it will get the sense right and we won't > accidentally break anyone. > > At some point soon I'll also try and audit similar cases and hopefully we > will get those fixed up as well. Obviously if anyone else wants to take > a look at that it would be much appreciated! > G'day Jonathan, Sorry this had slipped a bit on my list. I'll send a patch in next day or so.
diff --git a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt b/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt deleted file mode 100644 index f9b2eef946aa..000000000000 --- a/Documentation/devicetree/bindings/iio/potentiometer/ad5272.txt +++ /dev/null @@ -1,27 +0,0 @@ -* Analog Devices AD5272 digital potentiometer - -The node for this device must be a child node of a I2C controller, hence -all mandatory properties for your controller must be specified. See directory: - - Documentation/devicetree/bindings/i2c - -for more details. - -Required properties: - - compatible: Must be one of the following, depending on the model: - adi,ad5272-020 - adi,ad5272-050 - adi,ad5272-100 - adi,ad5274-020 - adi,ad5274-100 - -Optional properties: - - reset-gpios: GPIO specification for the RESET input. This is an - active low signal to the AD5272. - -Example: -ad5272: potentiometer@2f { - reg = <0x2F>; - compatible = "adi,ad5272-020"; - reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; -}; diff --git a/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml new file mode 100644 index 000000000000..b9b7d383bff1 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/potentiometer/adi,ad5272.yaml @@ -0,0 +1,50 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/potentiometer/adi,ad5272.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD5272 digital potentiometer + +maintainers: + - Phil Reid <preid@electromag.com.au> + +description: | + Datasheet: https://www.analog.com/en/products/ad5272.html + +properties: + compatible: + enum: + - adi,ad5272-020 + - adi,ad5272-050 + - adi,ad5272-100 + - adi,ad5274-020 + - adi,ad5274-100 + + reg: + maxItems: 1 + + reset-gpios: + description: + Active low signal to the AD5272 RESET input. + +additionalProperties: false + +required: + - compatible + - reg + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + + potentiometer@2f { + compatible = "adi,ad5272-020"; + reg = <0x2F>; + reset-gpios = <&gpio3 6 GPIO_ACTIVE_HIGH>; + }; + }; +...