staging: iio: cdc: ad7746: remove platform_data in favor of device tree bindings
Subsystems (5)
cover staging: iio: cdc: ad7746: remove platform_data in favor of device tree bindings
This patch series aims to replace the platform_struct for the ad7746 driver in favor of device tree bindings, creating the dt-bindings documentation in the process.
Since the header file was only used to define the struct and the excitation level values, it was possible to remove the file entirely.
Lucas Stankus (3):
dt-bindings: staging: iio: cdc: ad7746: add binding documentation for
AD7746
staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output
staging: iio: cdc: ad7746: use dt binding to set the excitation level
.../bindings/iio/cdc/adi,ad7746.yaml | 79 +++++++++++++++++++ drivers/staging/iio/cdc/ad7746.c | 43 +++++----- drivers/staging/iio/cdc/ad7746.h | 28 ------- 3 files changed, 100 insertions(+), 50 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml delete mode 100644 drivers/staging/iio/cdc/ad7746.h
Signature · 1 line
2.31.1
PATCH 1/3 dt-bindings: staging: iio: cdc: ad7746: add binding documentation for AD7746
Add device tree binding documentation for AD7746 cdc in YAML format.
--- .../bindings/iio/cdc/adi,ad7746.yaml | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml new file +79 -0
@@ -0,0 +1,79 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/cdc/adi,ad7746.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: AD7746 24-Bit Capacitance-to-Digital Converter with Temperature Sensor + +maintainers: + - Michael Hennerich <[email protected]> + +description: | + AD7746 24-Bit Capacitance-to-Digital Converter with Temperature Sensor + + Specifications about the part can be found at: + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7291.pdf + +properties: + compatible: + enum: + - adi,ad7745 + - adi,ad7746 + - adi,ad7747 + + reg: + description: | + Physiscal address of the EXC set-up register. + maxItems: 1 + + adi,excitation-voltage-level: + description: | + Select the reference excitation voltage level used by the device. + With VDD being the power supply voltage, valid values are: + 0: +-VDD / 8 + 1: +-VDD / 4 + 2: +-VDD * 3 / 8 + 3: +-VDD / 2 + If left empty option 3 is selected. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3] + + adi,exca-output: + description: | + Sets the excitation output in the exca pin. + Valid values are: + 0: Disables output in the EXCA pin. + 1: Enables EXCA pin as the excitation output. + 2: Enables EXCA pin as the inverted excitation output. + If left empty the output is disabled. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2] + + adi,excb-output: + description: | + Analoguos to the adi,exca-output for the EXCB pin. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2] + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ad7746: cdc@0 { + compatible = "adi,ad7746"; + reg = <0>; + adi,excitation-voltage-level = <3>; + adi,exca-output = <0>; + adi,excb-output = <0>; + }; + }; +...
Signature · 1 line
2.31.1
Jonathan Cameron
maintainerRe: [PATCH 1/3] dt-bindings: staging: iio: cdc: ad7746: add binding documentation for AD7746
Quoted 3 lines · On Fri, 9 Apr 2021 15:50:10 -0300 Lucas Stankus <[email protected]>…
Add device tree binding documentation for AD7746 cdc in YAML format.
Signed-off-by Lucas Stankus <[email protected]>
Hi Lucas,
Good to see progress on this one after all these years :)
I think we can do a bit better though by making the attributes easy to comprehend without needing to refer to the documentation. Always good to avoid magic numbers if we can.
Suggestions inline.
Jonathan
Quoted 28 lines
--- .../bindings/iio/cdc/adi,ad7746.yaml | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yamlDocumentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml new file +17
-0@@ -0,0 +1,79 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/cdc/adi,ad7746.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: AD7746 24-Bit Capacitance-to-Digital Converter with Temperature Sensor + +maintainers: + - Michael Hennerich <[email protected]> + +description: | + AD7746 24-Bit Capacitance-to-Digital Converter with Temperature Sensor + + Specifications about the part can be found at: + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7291.pdf +
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml line 27
Quoted 10 lines
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml new file +10
-0+properties: + compatible: + enum: + - adi,ad7745 + - adi,ad7746 + - adi,ad7747 + + reg: + description: | + Physiscal address of the EXC set-up register.
reg in this case would be the i2c address.
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml line 30
Quoted 3 lines
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml new file +3
-0+ maxItems: 1 + + adi,excitation-voltage-level:
This isn't a level as such, it's a scale factor, or something like that and the naming should reflect that + the values should be real in some sense (multipliers so
perhaps something like adi,excitation-vdd-milicent ? schema/property-units.yaml includes -percent but that doesn't have enough precision.
enum [125, 250, 375, 500]
Quoted 8 lines
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml new file +8
-0+ description: | + Select the reference excitation voltage level used by the device. + With VDD being the power supply voltage, valid values are: + 0: +-VDD / 8 + 1: +-VDD / 4 + 2: +-VDD * 3 / 8 + 3: +-VDD / 2 + If left empty option 3 is selected.
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml line 48
Quoted 10 lines
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml new file +10
-0+ $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3] + + adi,exca-output: + description: | + Sets the excitation output in the exca pin. + Valid values are: + 0: Disables output in the EXCA pin. + 1: Enables EXCA pin as the excitation output. + 2: Enables EXCA pin as the inverted excitation output.
Hmm. Various ways we could do this and avoid the need for a enum representing several different things. Perhaps
adi,exa-output-en
adi,exa-output-invert
(appropriate checks so we can only have invert of the channel is enabled as otherwise it is less than meaningful)
Quoted 15 lines
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml new file +15
-0+ If left empty the output is disabled. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2] + + adi,excb-output: + description: | + Analoguos to the adi,exca-output for the EXCB pin. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2] + +required: + - compatible + - reg + +additionalProperties: false
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml line 73
Quoted 10 lines
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml new file +10
-0+ +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + ad7746: cdc@0 { + compatible = "adi,ad7746"; + reg = <0>;
That's very unlikely as an i2c address.
Quoted 6 lines
Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml new file +6
-0+ adi,excitation-voltage-level = <3>; + adi,exca-output = <0>; + adi,excb-output = <0>; + }; + }; +...
PATCH 2/3 staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output
In reply to Lucas StankusDitch platform_data fields in favor of device tree properties for configuring EXCA and EXCB output.
This also removes the fields from the platform_data struct, since they're not used anymore.
--- drivers/staging/iio/cdc/ad7746.c | 33 +++++++++++++++++--------------- drivers/staging/iio/cdc/ad7746.h | 4 ---- 2 files changed, 18 insertions(+), 19 deletions(-)
drivers/staging/iio/cdc/ad7746.c +18 -15
@@ -677,8 +677,10 @@ static int ad7746_probe(struct i2c_client *client, const struct i2c_device_id *id) {struct ad7746_platform_data *pdata = client->dev.platform_data; +struct device_node *np = client->dev.of_node;struct ad7746_chip_info *chip;struct iio_dev *indio_dev; +unsigned int exca_en, excb_en;unsigned char regval =0 ;int ret =0 ;
@@ -703,26 +705,27 @@ static int ad7746_probe(struct i2c_client *client, indio_dev->num_channels =ARRAY_SIZE (ad7746_channels); indio_dev->modes =INDIO_DIRECT_MODE ; -if (pdata) { -if (pdata->exca_en) { -if (pdata->exca_inv_en) - regval |=AD7746_EXCSETUP_NEXCA ; -else - regval |=AD7746_EXCSETUP_EXCA ; - } + ret =of_property_read_u32 (np,"adi,exca-output" , &exca_en); +if (!ret && exca_en) { +if (exca_en ==1 ) + regval |=AD7746_EXCSETUP_EXCA ; +else + regval |=AD7746_EXCSETUP_NEXCA ; + } -if (pdata->excb_en) { -if (pdata->excb_inv_en) - regval |=AD7746_EXCSETUP_NEXCB ; -else - regval |=AD7746_EXCSETUP_EXCB ; - } + ret =of_property_read_u32 (np,"adi,excb-output" , &excb_en); +if (!ret && excb_en) { +if (excb_en ==1 ) + regval |=AD7746_EXCSETUP_EXCB ; +else + regval |=AD7746_EXCSETUP_NEXCB ; + } +if (pdata) { regval |=AD7746_EXCSETUP_EXCLVL (pdata->exclvl); }else {dev_warn (&client->dev,"No platform data? using default\n" ); - regval =AD7746_EXCSETUP_EXCA |AD7746_EXCSETUP_EXCB | -AD7746_EXCSETUP_EXCLVL (3 ); + regval =AD7746_EXCSETUP_EXCLVL (3 ); } ret = i2c_smbus_write_byte_data(chip->client,
drivers/staging/iio/cdc/ad7746.h +0 -4
@@ -19,10 +19,6 @@struct ad7746_platform_data {unsigned char exclvl;/*Excitation Voltage Level */ -bool exca_en;/* enables EXCA pin as the excitation output */ -bool exca_inv_en;/* enables /EXCA pin as the excitation output */ -bool excb_en;/* enables EXCB pin as the excitation output */ -bool excb_inv_en;/* enables /EXCB pin as the excitation output */ };#endif /* IIO_CDC_AD7746_H_ */
Signature · 1 line
2.31.1
Re: [PATCH 2/3] staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output
Quoted 28 lines · Lucas Stankus
Ditch
platform_datafields in favor of device tree properties for configuring EXCA and EXCB output.
This also removes the fields from theplatform_datastruct, since they're not used anymore.Signed-off-by Lucas Stankus <[email protected]>--- drivers/staging/iio/cdc/ad7746.c | 33 +++++++++++++++++--------------- drivers/staging/iio/cdc/ad7746.h | 4 ---- 2 files changed, 18 insertions(+), 19 deletions(-)drivers/staging/iio/cdc/ad7746.c +2
-0@@ -677,8 +677,10 @@ static int ad7746_probe(struct i2c_client *client, const struct i2c_device_id *id) {struct ad7746_platform_data *pdata = client->dev.platform_data; +struct device_node *np = client->dev.of_node;struct ad7746_chip_info *chip;struct iio_dev *indio_dev; +unsigned int exca_en, excb_en;unsigned char regval =0 ;int ret =0 ;
drivers/staging/iio/cdc/ad7746.c static int ad7746_probe(struct i2c_client *client, · line 707
Quoted 3 lines
drivers/staging/iio/cdc/ad7746.c +0
-0@@ -703,26 +705,27 @@ static int ad7746_probe(struct i2c_client *client, indio_dev->num_channels =ARRAY_SIZE (ad7746_channels); indio_dev->modes =INDIO_DIRECT_MODE ;
[1]
drivers/staging/iio/cdc/ad7746.c static int ad7746_probe(struct i2c_client *client, · line 708
Quoted 8 lines
drivers/staging/iio/cdc/ad7746.c +1
-7-if (pdata) { -if (pdata->exca_en) { -if (pdata->exca_inv_en) - regval |=AD7746_EXCSETUP_NEXCA ; -else - regval |=AD7746_EXCSETUP_EXCA ; - } + ret =of_property_read_u32 (np,"adi,exca-output" , &exca_en);
maybe a better idea would be to use:
device_property_read_u32(dev, .... )
where:
dev = client->dev.;
this would make the driver a bit more friendly with both OF and ACPI
Quoted 17 lines
drivers/staging/iio/cdc/ad7746.c +10
-6+if (!ret && exca_en) { +if (exca_en ==1 ) + regval |=AD7746_EXCSETUP_EXCA ; +else + regval |=AD7746_EXCSETUP_NEXCA ; + } -if (pdata->excb_en) { -if (pdata->excb_inv_en) - regval |=AD7746_EXCSETUP_NEXCB ; -else - regval |=AD7746_EXCSETUP_EXCB ; - } + ret =of_property_read_u32 (np,"adi,excb-output" , &excb_en); +if (!ret && excb_en) { +if (excb_en ==1 ) + regval |=AD7746_EXCSETUP_EXCB ;
drivers/staging/iio/cdc/ad7746.c static int ad7746_probe(struct i2c_client *client, · line 725
Quoted 10 lines
drivers/staging/iio/cdc/ad7746.c +4
-2+else + regval |=AD7746_EXCSETUP_NEXCB ; + } +if (pdata) { regval |=AD7746_EXCSETUP_EXCLVL (pdata->exclvl); }else {dev_warn (&client->dev,"No platform data? using default\n" ); - regval =AD7746_EXCSETUP_EXCA |AD7746_EXCSETUP_EXCB | -AD7746_EXCSETUP_EXCLVL (3 );
This logic is problematic now.
Because no matter what you're setting in the DT, it always gets overridden here because there is no platform data.
Maybe a better idea is to do something like:
if (!pdata)
regval = AD7746_EXCSETUP_EXCLVL(3);
but this should be placed somewhere around [1]
Quoted 22 lines
drivers/staging/iio/cdc/ad7746.c +1
-0+ regval =AD7746_EXCSETUP_EXCLVL (3 ); } ret = i2c_smbus_write_byte_data(chip->client,drivers/staging/iio/cdc/ad7746.h +0
-5@@ -19,10 +19,6 @@struct ad7746_platform_data {unsigned char exclvl;/*Excitation Voltage Level */ -bool exca_en;/* enables EXCA pin as the excitation output */ -bool exca_inv_en;/* enables /EXCA pin as the excitation output */ -bool excb_en;/* enables EXCB pin as the excitation output */ -bool excb_inv_en;/* enables /EXCB pin as the excitation output */ };#endif /* IIO_CDC_AD7746_H_ */ --2.31.1
Re: [PATCH 2/3] staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output
Quoted 85 lines · Alexandru Ardelean
Quoted 28 lines · Lucas Stankus
Ditch
platform_datafields in favor of device tree properties for configuring EXCA and EXCB output.
This also removes the fields from theplatform_datastruct, since they're not used anymore.Signed-off-by Lucas Stankus <[email protected]>--- drivers/staging/iio/cdc/ad7746.c | 33 +++++++++++++++++--------------- drivers/staging/iio/cdc/ad7746.h | 4 ---- 2 files changed, 18 insertions(+), 19 deletions(-)drivers/staging/iio/cdc/ad7746.c +2
-0@@ -677,8 +677,10 @@ static int ad7746_probe(struct i2c_client *client, const struct i2c_device_id *id) {struct ad7746_platform_data *pdata = client->dev.platform_data; +struct device_node *np = client->dev.of_node;struct ad7746_chip_info *chip;struct iio_dev *indio_dev; +unsigned int exca_en, excb_en;unsigned char regval =0 ;int ret =0 ;drivers/staging/iio/cdc/ad7746.c static int ad7746_probe(struct i2c_client *client, · line 707
Quoted 3 lines
drivers/staging/iio/cdc/ad7746.c +0
-0@@ -703,26 +705,27 @@ static int ad7746_probe(struct i2c_client *client, indio_dev->num_channels =ARRAY_SIZE (ad7746_channels); indio_dev->modes =INDIO_DIRECT_MODE ;[1]
drivers/staging/iio/cdc/ad7746.c static int ad7746_probe(struct i2c_client *client, · line 708
Quoted 8 lines
drivers/staging/iio/cdc/ad7746.c +1
-7-if (pdata) { -if (pdata->exca_en) { -if (pdata->exca_inv_en) - regval |=AD7746_EXCSETUP_NEXCA ; -else - regval |=AD7746_EXCSETUP_EXCA ; - } + ret =of_property_read_u32 (np,"adi,exca-output" , &exca_en);maybe a better idea would be to use:
device_property_read_u32(dev, .... )
where:
dev =client->dev.;this would make the driver a bit more friendly with both OF and ACPI
Quoted 17 lines
drivers/staging/iio/cdc/ad7746.c +10
-6+if (!ret && exca_en) { +if (exca_en ==1 ) + regval |=AD7746_EXCSETUP_EXCA ; +else + regval |=AD7746_EXCSETUP_NEXCA ; + } -if (pdata->excb_en) { -if (pdata->excb_inv_en) - regval |=AD7746_EXCSETUP_NEXCB ; -else - regval |=AD7746_EXCSETUP_EXCB ; - } + ret =of_property_read_u32 (np,"adi,excb-output" , &excb_en); +if (!ret && excb_en) { +if (excb_en ==1 ) + regval |=AD7746_EXCSETUP_EXCB ;drivers/staging/iio/cdc/ad7746.c static int ad7746_probe(struct i2c_client *client, · line 725
Quoted 10 lines
drivers/staging/iio/cdc/ad7746.c +4
-2+else + regval |=AD7746_EXCSETUP_NEXCB ; + } +if (pdata) { regval |=AD7746_EXCSETUP_EXCLVL (pdata->exclvl); }else {dev_warn (&client->dev,"No platform data? using default\n" ); - regval =AD7746_EXCSETUP_EXCA |AD7746_EXCSETUP_EXCB | -AD7746_EXCSETUP_EXCLVL (3 );This logic is problematic now.
Because no matter what you're setting in the DT, it always gets overridden here because there is no platform data.
Quoted 5 lines
Maybe a better idea is to do something like:
if (!pdata)regval = AD7746_EXCSETUP_EXCLVL(3);but this should be placed somewhere around [1]
[ i can see that this logic will get corrected in the next patch] to add here a bit: the idea of a patch is that it should try to not introduce any [even temporary] breakage, even when it's in a series; if a driver was already broken, then it should get fixed via it's own patch; but no patch should introduce any breakages [if possible]
Quoted 24 lines
Quoted 24 lines
drivers/staging/iio/cdc/ad7746.c +1
-0+ regval =AD7746_EXCSETUP_EXCLVL (3 ); } ret = i2c_smbus_write_byte_data(chip->client,drivers/staging/iio/cdc/ad7746.h +0
-5@@ -19,10 +19,6 @@struct ad7746_platform_data {unsigned char exclvl;/*Excitation Voltage Level */ -bool exca_en;/* enables EXCA pin as the excitation output */ -bool exca_inv_en;/* enables /EXCA pin as the excitation output */ -bool excb_en;/* enables EXCB pin as the excitation output */ -bool excb_inv_en;/* enables /EXCB pin as the excitation output */ };#endif /* IIO_CDC_AD7746_H_ */ --2.31.1
Jonathan Cameron
maintainerRe: [PATCH 2/3] staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output
Quoted 91 lines · On Sat, 10 Apr 2021 19:15:39 +0300 Alexandru Ardelean <[email protected]>…
Quoted 89 lines · Alexandru Ardelean
Quoted 28 lines · Lucas Stankus
Ditch
platform_datafields in favor of device tree properties for configuring EXCA and EXCB output.
This also removes the fields from theplatform_datastruct, since they're not used anymore.Signed-off-by Lucas Stankus <[email protected]>--- drivers/staging/iio/cdc/ad7746.c | 33 +++++++++++++++++--------------- drivers/staging/iio/cdc/ad7746.h | 4 ---- 2 files changed, 18 insertions(+), 19 deletions(-)drivers/staging/iio/cdc/ad7746.c +2
-0@@ -677,8 +677,10 @@ static int ad7746_probe(struct i2c_client *client, const struct i2c_device_id *id) {struct ad7746_platform_data *pdata = client->dev.platform_data; +struct device_node *np = client->dev.of_node;struct ad7746_chip_info *chip;struct iio_dev *indio_dev; +unsigned int exca_en, excb_en;unsigned char regval =0 ;int ret =0 ;drivers/staging/iio/cdc/ad7746.c static int ad7746_probe(struct i2c_client *client, · line 707
Quoted 3 lines
drivers/staging/iio/cdc/ad7746.c +0
-0@@ -703,26 +705,27 @@ static int ad7746_probe(struct i2c_client *client, indio_dev->num_channels =ARRAY_SIZE (ad7746_channels); indio_dev->modes =INDIO_DIRECT_MODE ;[1]
drivers/staging/iio/cdc/ad7746.c static int ad7746_probe(struct i2c_client *client, · line 708
Quoted 8 lines
drivers/staging/iio/cdc/ad7746.c +1
-7-if (pdata) { -if (pdata->exca_en) { -if (pdata->exca_inv_en) - regval |=AD7746_EXCSETUP_NEXCA ; -else - regval |=AD7746_EXCSETUP_EXCA ; - } + ret =of_property_read_u32 (np,"adi,exca-output" , &exca_en);maybe a better idea would be to use:
device_property_read_u32(dev, .... )
where:
dev =client->dev.;this would make the driver a bit more friendly with both OF and ACPI
Quoted 17 lines
drivers/staging/iio/cdc/ad7746.c +10
-6+if (!ret && exca_en) { +if (exca_en ==1 ) + regval |=AD7746_EXCSETUP_EXCA ; +else + regval |=AD7746_EXCSETUP_NEXCA ; + } -if (pdata->excb_en) { -if (pdata->excb_inv_en) - regval |=AD7746_EXCSETUP_NEXCB ; -else - regval |=AD7746_EXCSETUP_EXCB ; - } + ret =of_property_read_u32 (np,"adi,excb-output" , &excb_en); +if (!ret && excb_en) { +if (excb_en ==1 ) + regval |=AD7746_EXCSETUP_EXCB ;drivers/staging/iio/cdc/ad7746.c static int ad7746_probe(struct i2c_client *client, · line 725
Quoted 10 lines
drivers/staging/iio/cdc/ad7746.c +4
-2+else + regval |=AD7746_EXCSETUP_NEXCB ; + } +if (pdata) { regval |=AD7746_EXCSETUP_EXCLVL (pdata->exclvl); }else {dev_warn (&client->dev,"No platform data? using default\n" ); - regval =AD7746_EXCSETUP_EXCA |AD7746_EXCSETUP_EXCB | -AD7746_EXCSETUP_EXCLVL (3 );This logic is problematic now.
Because no matter what you're setting in the DT, it always gets overridden here because there is no platform data.Maybe a better idea is to do something like:
if (!pdata)regval = AD7746_EXCSETUP_EXCLVL(3);
Quoted 7 lines
Quoted 1 line
but this should be placed somewhere around [1]
[ i can see that this logic will get corrected in the next patch] to add here a bit: the idea of a patch is that it should try to not introduce any [even temporary] breakage, even when it's in a series; if a driver was already broken, then it should get fixed via it's own patch; but no patch should introduce any breakages [if possible]
The two patches are small enough I'd be fine with them being merged into one that avoiding any special handling. Just add a note to the patch description to say that it was done in one patch for this reason.
Jonathan
Quoted 25 lines
Quoted 25 lines
Quoted 25 lines
drivers/staging/iio/cdc/ad7746.c +1
-0+ regval =AD7746_EXCSETUP_EXCLVL (3 ); } ret = i2c_smbus_write_byte_data(chip->client,drivers/staging/iio/cdc/ad7746.h +0
-5@@ -19,10 +19,6 @@struct ad7746_platform_data {unsigned char exclvl;/*Excitation Voltage Level */ -bool exca_en;/* enables EXCA pin as the excitation output */ -bool exca_inv_en;/* enables /EXCA pin as the excitation output */ -bool excb_en;/* enables EXCB pin as the excitation output */ -bool excb_inv_en;/* enables /EXCB pin as the excitation output */ };#endif /* IIO_CDC_AD7746_H_ */ --2.31.1
A message that is not in the archive (sent off-list or not yet received): CAHp75Ve2NBMyQf7jw63a=4r135ShGEoRjZ+CUr36DC+gH39d7A@mail.gmail.com
A message that is not in the archive (sent off-list or not yet received): CAHp75VcMsMvSrbP3tkcivvd+s=8drqiCt-xmk+HxhLS87w6zYw@mail.gmail.com
Jonathan Cameron
maintainerRe: [PATCH 2/3] staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output
Quoted 13 lines · On Sat, 10 Apr 2021 13:05:14 +0300 Andy Shevchenko <[email protected]>…
Quoted 10 lines · Andy Shevchenko
Quoted 4 lines · Lucas Stankus
Ditch
platform_datafields in favor of device tree properties for configuring EXCA and EXCB output.
This also removes the fields from theplatform_datastruct, since they're not used anymore.
Quoted 6 lines
Quoted 6 lines
As far as I read the old code it’s possible to leave pins untouched, not anymore the case after this patch. What datasheet tells about it? Please elaborate in the commit message and add a Datasheet: tag as a reference.
Default is to have them disabled, so if you switch to separate -en vs -invert lack of either will correspond to the power on default and simplify things somewhat.
Quoted 105 lines
Okay, I see now. But can you simple use switch case or so, because currently code is not so understandable from the first glance?
Quoted 98 lines
Quoted 86 lines
Signed-off-by Lucas Stankus <[email protected]>--- drivers/staging/iio/cdc/ad7746.c | 33 +++++++++++++++++--------------- drivers/staging/iio/cdc/ad7746.h | 4 ---- 2 files changed, 18 insertions(+), 19 deletions(-)drivers/staging/iio/cdc/ad7746.c +18
-15@@ -677,8 +677,10 @@ static int ad7746_probe(struct i2c_client *client, const struct i2c_device_id *id) {struct ad7746_platform_data *pdata = client->dev.platform_data; +struct device_node *np = client->dev.of_node;struct ad7746_chip_info *chip;struct iio_dev *indio_dev; +unsigned int exca_en, excb_en;unsigned char regval =0 ;int ret =0 ;@@ -703,26 +705,27 @@ static int ad7746_probe(struct i2c_client *client, indio_dev->num_channels =ARRAY_SIZE (ad7746_channels); indio_dev->modes =INDIO_DIRECT_MODE ; -if (pdata) { -if (pdata->exca_en) { -if (pdata->exca_inv_en) - regval |=AD7746_EXCSETUP_NEXCA ; -else - regval |=AD7746_EXCSETUP_EXCA ; - } + ret =of_property_read_u32 (np,"adi,exca-output" , &exca_en); +if (!ret && exca_en) { +if (exca_en ==1 ) + regval |=AD7746_EXCSETUP_EXCA ; +else + regval |=AD7746_EXCSETUP_NEXCA ; + } -if (pdata->excb_en) { -if (pdata->excb_inv_en) - regval |=AD7746_EXCSETUP_NEXCB ; -else - regval |=AD7746_EXCSETUP_EXCB ; - } + ret =of_property_read_u32 (np,"adi,excb-output" , &excb_en); +if (!ret && excb_en) { +if (excb_en ==1 ) + regval |=AD7746_EXCSETUP_EXCB ; +else + regval |=AD7746_EXCSETUP_NEXCB ; + } +if (pdata) { regval |=AD7746_EXCSETUP_EXCLVL (pdata->exclvl); }else {dev_warn (&client->dev,"No platform data? using default\n"); - regval =AD7746_EXCSETUP_EXCA |AD7746_EXCSETUP_EXCB | -AD7746_EXCSETUP_EXCLVL (3 ); + regval =AD7746_EXCSETUP_EXCLVL (3 ); }ret = i2c_smbus_write_byte_data(chip->client,drivers/staging/iio/cdc/ad7746.h +0
-1@@ -19,10 +19,6 @@struct ad7746_platform_data {unsigned char exclvl;/*Excitation Voltage Level */ -bool exca_en;/* enables EXCA pin as the excitation output */
- boolexca_inv_en; /* enables /EXCA pin as the excitation
output */
- boolexcb_en; /* enables EXCB pin as the excitation
output */
- boolexcb_inv_en; /* enables /EXCB pin as the excitation
output */
};Quoted 5 lines
#endif /*
IIO_CDC_AD7746_H_*/
--
2.31.1--
With Best Regards,
Andy Shevchenko
PATCH 3/3 staging: iio: cdc: ad7746: use dt binding to set the excitation level
In reply to Lucas StankusSet device excitation level using properties from device tree binding instead of using platform_data.
As this replaces the last instance where the platform_data struct was used, remove ad7746.h header file since it's no longer needed.
--- drivers/staging/iio/cdc/ad7746.c | 16 ++++++---------- drivers/staging/iio/cdc/ad7746.h | 24 ------------------------ 2 files changed, 6 insertions(+), 34 deletions(-) delete mode 100644 drivers/staging/iio/cdc/ad7746.h
drivers/staging/iio/cdc/ad7746.c +6 -10
@@ -18,8 +18,6 @@#include <linux/iio/iio.h> #include <linux/iio/sysfs.h> -#include "ad7746.h" -/* * AD7746 Register Definition */
@@ -676,11 +674,10 @@ static const struct iio_info ad7746_info = {static int ad7746_probe (struct i2c_client *client,const struct i2c_device_id *id) { -struct ad7746_platform_data *pdata = client->dev.platform_data;struct device_node *np = client->dev.of_node;struct ad7746_chip_info *chip;struct iio_dev *indio_dev; -unsigned int exca_en, excb_en; +unsigned int exca_en, excb_en, exclvl;unsigned char regval =0 ;int ret =0 ;
@@ -721,12 +718,11 @@ static int ad7746_probe(struct i2c_client *client, regval |=AD7746_EXCSETUP_NEXCB ; } -if (pdata) { - regval |=AD7746_EXCSETUP_EXCLVL (pdata->exclvl); - }else { -dev_warn (&client->dev,"No platform data? using default\n" ); - regval =AD7746_EXCSETUP_EXCLVL (3 ); - } + ret =of_property_read_u32 (np,"adi,excitation-voltage-level" , &exclvl); +if (!ret) + regval |=AD7746_EXCSETUP_EXCLVL (exclvl); +else + regval |=AD7746_EXCSETUP_EXCLVL (3 ); ret =i2c_smbus_write_byte_data (chip->client,AD7746_REG_EXC_SETUP , regval);
drivers/staging/iio/cdc/ad7746.h deleted +0 -24
@@ -1,24 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -/* -* AD7746 capacitive sensor driver supporting AD7745, AD7746 and AD7747 -* -* Copyright 2011 Analog Devices Inc. -*/ - -#ifndef IIO_CDC_AD7746_H_ -#define IIO_CDC_AD7746_H_ - -/* -* TODO: struct ad7746_platform_data needs to go into include/linux/iio -*/ - -#define AD7466_EXCLVL_0 0/* +-VDD/8 */ -#define AD7466_EXCLVL_1 1/* +-VDD/4 */ -#define AD7466_EXCLVL_2 2/* +-VDD * 3/8 */ -#define AD7466_EXCLVL_3 3/* +-VDD/2 */ - -struct ad7746_platform_data { -unsigned char exclvl;/*Excitation Voltage Level */ -}; - -#endif /* IIO_CDC_AD7746_H_ */
Signature · 1 line
2.31.1
Quoted 7 lines · Lucas Stankus
This patch series aims to replace the
platform_structfor the ad7746 driver in favor of device tree bindings, creating the dt-bindings documentation in the process.Since the header file was only used to define the struct and the excitation level values, it was possible to remove the file entirely.
From my side: I need to get better at understanding IIO and how to place some logic of devices into IIO,
I don't know if there is a better approach at converting the current platform_data into DT/OF.
Maybe Jonathan [or someone else] has some better ideas. Otherwise the overall approach looks fine from my side.
Quoted 17 lines
Lucas Stankus (3):
dt-bindings: staging: iio: cdc: ad7746: add binding documentation forAD7746staging: iio: cdc: ad7746: use dt bindings to set the EXCx pins output
staging: iio: cdc: ad7746: use dt binding to set the excitation level.../bindings/iio/cdc/adi,ad7746.yaml | 79 +++++++++++++++++++ drivers/staging/iio/cdc/ad7746.c | 43 +++++----- drivers/staging/iio/cdc/ad7746.h | 28 ------- 3 files changed, 100 insertions(+), 50 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml delete mode 100644 drivers/staging/iio/cdc/ad7746.h--
2.31.1