| Message ID | cover.1619841953.git.lucas.p.stankus@gmail.com |
|---|---|
| Headers | show |
| Series | staging: iio: cdc: ad7746: add dt properties for capacitive channel setup | expand |
On Sat, 1 May 2021 09:32:03 -0300 Lucas Stankus <lucas.p.stankus@gmail.com> wrote: > This patch series aims to replace the platform_struct for the ad7746 driver > in favor of device tree bindings, creating the dt-binding 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. > > Changelog v2 -> v3: > - Add application reference note for the inverted EXCX dt bindings > - Replace macro with lookup table for setting the capacitive channel > excitation voltage > > Lucas Stankus (2): > dt-bindings: staging: iio: cdc: ad7746: add binding documentation for > AD7746 > staging: iio: cdc: ad7746: use dt for capacitive channel setup. Looks good to me. Thanks! Will leave time for Rob or anyone else to take a look etc before applying. If I seem to have lost it then feel free to give me a poke in a few weeks (it's happened a few times in the past :( Thanks, Jonathan > > .../bindings/iio/cdc/adi,ad7746.yaml | 77 +++++++++++++++++++ > drivers/staging/iio/cdc/ad7746.c | 54 ++++++++----- > drivers/staging/iio/cdc/ad7746.h | 28 ------- > 3 files changed, 110 insertions(+), 49 deletions(-) > create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml > delete mode 100644 drivers/staging/iio/cdc/ad7746.h >
On Sat, May 01, 2021 at 09:32:53AM -0300, Lucas Stankus wrote: > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c > index dfd71e99e872..531f1b96dea2 100644 > --- a/drivers/staging/iio/cdc/ad7746.c > +++ b/drivers/staging/iio/cdc/ad7746.c > @@ -18,8 +18,6 @@ > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > > -#include "ad7746.h" > - > /* > * AD7746 Register Definition > */ > @@ -676,10 +674,11 @@ 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 *dev = &client->dev; > struct ad7746_chip_info *chip; > struct iio_dev *indio_dev; > unsigned char regval = 0; > + unsigned int vdd_permille; > int ret = 0; > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); > @@ -703,26 +702,39 @@ 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; > - } > + if (device_property_read_bool(dev, "adi,exca-output-en")) { > + if (device_property_read_bool(dev, "adi,exca-output-invert")) > + regval |= AD7746_EXCSETUP_NEXCA; > + else > + regval |= AD7746_EXCSETUP_EXCA; > + } > > - if (pdata->excb_en) { > - if (pdata->excb_inv_en) > - regval |= AD7746_EXCSETUP_NEXCB; > - else > - regval |= AD7746_EXCSETUP_EXCB; > - } > + if (device_property_read_bool(dev, "adi,excb-output-en")) { > + if (device_property_read_bool(dev, "adi,excb-output-invert")) > + regval |= AD7746_EXCSETUP_NEXCB; > + else > + regval |= AD7746_EXCSETUP_EXCB; > + } > > - 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); > + ret = device_property_read_u32(dev, "adi,excitation-vdd-permille", > + &vdd_permille); > + if (!ret) { This test is reversed. I wonder if the static checkers can catch the uninitialized variable bug... It's probably better to write it as: if (device_property_read_u32(dev, "adi,excitation-vdd-permille", &vdd_permille) { So it matches the others. regards, dan carpenter; > + switch (vdd_permille) { > + case 125: > + regval |= AD7746_EXCSETUP_EXCLVL(0); > + break; > + case 250: > + regval |= AD7746_EXCSETUP_EXCLVL(1); > + break; > + case 375: > + regval |= AD7746_EXCSETUP_EXCLVL(2); > + break; > + case 500: > + regval |= AD7746_EXCSETUP_EXCLVL(3); > + break; > + default: > + break; > + } > } > > ret = i2c_smbus_write_byte_data(chip->client,
On Mon, May 03, 2021 at 01:07:20PM +0300, Dan Carpenter wrote: > On Sat, May 01, 2021 at 09:32:53AM -0300, Lucas Stankus wrote: > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c > > index dfd71e99e872..531f1b96dea2 100644 > > --- a/drivers/staging/iio/cdc/ad7746.c > > +++ b/drivers/staging/iio/cdc/ad7746.c > > @@ -18,8 +18,6 @@ > > #include <linux/iio/iio.h> > > #include <linux/iio/sysfs.h> > > > > -#include "ad7746.h" > > - > > /* > > * AD7746 Register Definition > > */ > > @@ -676,10 +674,11 @@ 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 *dev = &client->dev; > > struct ad7746_chip_info *chip; > > struct iio_dev *indio_dev; > > unsigned char regval = 0; > > + unsigned int vdd_permille; > > int ret = 0; > > > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); > > @@ -703,26 +702,39 @@ 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; > > - } > > + if (device_property_read_bool(dev, "adi,exca-output-en")) { > > + if (device_property_read_bool(dev, "adi,exca-output-invert")) > > + regval |= AD7746_EXCSETUP_NEXCA; > > + else > > + regval |= AD7746_EXCSETUP_EXCA; > > + } > > > > - if (pdata->excb_en) { > > - if (pdata->excb_inv_en) > > - regval |= AD7746_EXCSETUP_NEXCB; > > - else > > - regval |= AD7746_EXCSETUP_EXCB; > > - } > > + if (device_property_read_bool(dev, "adi,excb-output-en")) { > > + if (device_property_read_bool(dev, "adi,excb-output-invert")) > > + regval |= AD7746_EXCSETUP_NEXCB; > > + else > > + regval |= AD7746_EXCSETUP_EXCB; > > + } > > > > - 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); > > + ret = device_property_read_u32(dev, "adi,excitation-vdd-permille", > > + &vdd_permille); > > + if (!ret) { > > This test is reversed. I wonder if the static checkers can catch the > uninitialized variable bug... It's probably better to write it as: > > if (device_property_read_u32(dev, "adi,excitation-vdd-permille", > &vdd_permille) { > > So it matches the others. Oops. Sorry for the noise. I was wrong on this. I looked at the device_property_read_bool() code instead of the device_property_read_u32(). It's disappointing that the returns are reversed. regards, dan carpenter
On Sun, May 2, 2021 at 2:49 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sat, 1 May 2021 09:32:03 -0300 > Lucas Stankus <lucas.p.stankus@gmail.com> wrote: > > > This patch series aims to replace the platform_struct for the ad7746 driver > > in favor of device tree bindings, creating the dt-binding 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. > > > > Changelog v2 -> v3: > > - Add application reference note for the inverted EXCX dt bindings > > - Replace macro with lookup table for setting the capacitive channel > > excitation voltage > > > > Lucas Stankus (2): > > dt-bindings: staging: iio: cdc: ad7746: add binding documentation for > > AD7746 > > staging: iio: cdc: ad7746: use dt for capacitive channel setup. > > Looks good to me. Thanks! > > Will leave time for Rob or anyone else to take a look etc before applying. > > If I seem to have lost it then feel free to give me a poke in a few weeks > (it's happened a few times in the past :( > > Thanks, > > Jonathan > > > > .../bindings/iio/cdc/adi,ad7746.yaml | 77 +++++++++++++++++++ > > drivers/staging/iio/cdc/ad7746.c | 54 ++++++++----- > > drivers/staging/iio/cdc/ad7746.h | 28 ------- > > 3 files changed, 110 insertions(+), 49 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml > > delete mode 100644 drivers/staging/iio/cdc/ad7746.h > > > Hey Jonathan, It has been a week or so without any more feedback, so I'm just a pinging here to know if this can be applied. Thanks, Lucas
On Wed, 19 May 2021 17:59:31 -0300 Lucas Stankus <lucas.p.stankus@gmail.com> wrote: > On Sun, May 2, 2021 at 2:49 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Sat, 1 May 2021 09:32:03 -0300 > > Lucas Stankus <lucas.p.stankus@gmail.com> wrote: > > > > > This patch series aims to replace the platform_struct for the ad7746 driver > > > in favor of device tree bindings, creating the dt-binding 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. > > > > > > Changelog v2 -> v3: > > > - Add application reference note for the inverted EXCX dt bindings > > > - Replace macro with lookup table for setting the capacitive channel > > > excitation voltage > > > > > > Lucas Stankus (2): > > > dt-bindings: staging: iio: cdc: ad7746: add binding documentation for > > > AD7746 > > > staging: iio: cdc: ad7746: use dt for capacitive channel setup. > > > > Looks good to me. Thanks! > > > > Will leave time for Rob or anyone else to take a look etc before applying. > > > > If I seem to have lost it then feel free to give me a poke in a few weeks > > (it's happened a few times in the past :( > > > > Thanks, > > > > Jonathan > > > > > > .../bindings/iio/cdc/adi,ad7746.yaml | 77 +++++++++++++++++++ > > > drivers/staging/iio/cdc/ad7746.c | 54 ++++++++----- > > > drivers/staging/iio/cdc/ad7746.h | 28 ------- > > > 3 files changed, 110 insertions(+), 49 deletions(-) > > > create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml > > > delete mode 100644 drivers/staging/iio/cdc/ad7746.h > > > > > > > Hey Jonathan, > > It has been a week or so without any more feedback, so I'm just a pinging here > to know if this can be applied. oops. Indeed time I picked these up. Somehow I had them in the wrong colour in my horrendous colour based tracking system :( Now applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to do their worst. Thanks, Jonathan > > Thanks, > Lucas
This patch series aims to replace the platform_struct for the ad7746 driver in favor of device tree bindings, creating the dt-binding 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. Changelog v2 -> v3: - Add application reference note for the inverted EXCX dt bindings - Replace macro with lookup table for setting the capacitive channel excitation voltage Lucas Stankus (2): dt-bindings: staging: iio: cdc: ad7746: add binding documentation for AD7746 staging: iio: cdc: ad7746: use dt for capacitive channel setup. .../bindings/iio/cdc/adi,ad7746.yaml | 77 +++++++++++++++++++ drivers/staging/iio/cdc/ad7746.c | 54 ++++++++----- drivers/staging/iio/cdc/ad7746.h | 28 ------- 3 files changed, 110 insertions(+), 49 deletions(-) create mode 100644 Documentation/devicetree/bindings/iio/cdc/adi,ad7746.yaml delete mode 100644 drivers/staging/iio/cdc/ad7746.h