diff mbox series

[v2,1/4] iio: adc: ad7887: convert dual-channel mode to DT/ACPI

Message ID 20201119100748.57689-1-alexandru.ardelean@analog.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/4] iio: adc: ad7887: convert dual-channel mode to DT/ACPI | expand

Commit Message

Alexandru Ardelean Nov. 19, 2020, 10:07 a.m. UTC 
This change converts the configuration of the dual-channel mode from the
old platform-data, to the device_property_present() function, which
supports both device-tree and ACPI configuration setups.

With this change the old platform_data include of the driver can be
removed.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

I'm wondering if this changeset is what was in mind here:
 https://lore.kernel.org/linux-iio/CA+U=DsqF5tu8Be9KXeyCWD2uHvV688Nc3n=z_Xi2J6H6DFJPRQ@mail.gmail.com/T/#mbe72e4da3acea3899d0d35402ea81e52a9bc34e6
This driver could have been simplified/reduced a whole lot more, but I'm
not sure about it. It's a bit of patch-noise, and later

Changelog v1 -> v2:
* dropped patch 'iio: adc: ad7887: convert driver to full DT probing'
  not adding the device_get_match_data() logic anymore
* added patch 'iio: adc: ad7887: remove matching code from driver'
  hooking the chip info directly to AD7887
* added patch 'iio: adc: ad7887: add OF match table'
  this just adds an OF table for DT and ACPI

 drivers/iio/adc/ad7887.c             | 10 +++++-----
 include/linux/platform_data/ad7887.h | 21 ---------------------
 2 files changed, 5 insertions(+), 26 deletions(-)
 delete mode 100644 include/linux/platform_data/ad7887.h

Comments

Andy Shevchenko Nov. 19, 2020, 3:08 p.m. UTC | #1 
On Thu, Nov 19, 2020 at 12:02 PM Alexandru Ardelean
<alexandru.ardelean@analog.com> wrote:
>
> This change converts the configuration of the dual-channel mode from the
> old platform-data, to the device_property_present() function, which
> supports both device-tree and ACPI configuration setups.
>
> With this change the old platform_data include of the driver can be
> removed.

I mostly like the part of getting rid of legacy platform data.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
(for patches 1-3 only)

> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>
> I'm wondering if this changeset is what was in mind here:
>  https://lore.kernel.org/linux-iio/CA+U=DsqF5tu8Be9KXeyCWD2uHvV688Nc3n=z_Xi2J6H6DFJPRQ@mail.gmail.com/T/#mbe72e4da3acea3899d0d35402ea81e52a9bc34e6
> This driver could have been simplified/reduced a whole lot more, but I'm
> not sure about it. It's a bit of patch-noise, and later
>
> Changelog v1 -> v2:
> * dropped patch 'iio: adc: ad7887: convert driver to full DT probing'
>   not adding the device_get_match_data() logic anymore
> * added patch 'iio: adc: ad7887: remove matching code from driver'
>   hooking the chip info directly to AD7887
> * added patch 'iio: adc: ad7887: add OF match table'
>   this just adds an OF table for DT and ACPI
>
>  drivers/iio/adc/ad7887.c             | 10 +++++-----
>  include/linux/platform_data/ad7887.h | 21 ---------------------
>  2 files changed, 5 insertions(+), 26 deletions(-)
>  delete mode 100644 include/linux/platform_data/ad7887.h
>
> diff --git a/drivers/iio/adc/ad7887.c b/drivers/iio/adc/ad7887.c
> index 4f6f0e0e03ee..06f684c053a0 100644
> --- a/drivers/iio/adc/ad7887.c
> +++ b/drivers/iio/adc/ad7887.c
> @@ -23,8 +23,6 @@
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>
> -#include <linux/platform_data/ad7887.h>
> -
>  #define AD7887_REF_DIS         BIT(5)  /* on-chip reference disable */
>  #define AD7887_DUAL            BIT(4)  /* dual-channel mode */
>  #define AD7887_CH_AIN1         BIT(3)  /* convert on channel 1, DUAL=1 */
> @@ -241,9 +239,9 @@ static void ad7887_reg_disable(void *data)
>
>  static int ad7887_probe(struct spi_device *spi)
>  {
> -       struct ad7887_platform_data *pdata = spi->dev.platform_data;
>         struct ad7887_state *st;
>         struct iio_dev *indio_dev;
> +       bool dual_mode;
>         uint8_t mode;
>         int ret;
>
> @@ -286,7 +284,9 @@ static int ad7887_probe(struct spi_device *spi)
>         mode = AD7887_PM_MODE4;
>         if (!st->reg)
>                 mode |= AD7887_REF_DIS;
> -       if (pdata && pdata->en_dual)
> +
> +       dual_mode = device_property_present(&spi->dev, "adi,dual-channel-mode");
> +       if (dual_mode)
>                 mode |= AD7887_DUAL;
>
>         st->tx_cmd_buf[0] = AD7887_CH_AIN0 | mode;
> @@ -298,7 +298,7 @@ static int ad7887_probe(struct spi_device *spi)
>         spi_message_init(&st->msg[AD7887_CH0]);
>         spi_message_add_tail(&st->xfer[0], &st->msg[AD7887_CH0]);
>
> -       if (pdata && pdata->en_dual) {
> +       if (dual_mode) {
>                 st->tx_cmd_buf[2] = AD7887_CH_AIN1 | mode;
>
>                 st->xfer[1].rx_buf = &st->data[0];
> diff --git a/include/linux/platform_data/ad7887.h b/include/linux/platform_data/ad7887.h
> deleted file mode 100644
> index 9b4dca6ae70b..000000000000
> --- a/include/linux/platform_data/ad7887.h
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * AD7887 SPI ADC driver
> - *
> - * Copyright 2010 Analog Devices Inc.
> - */
> -#ifndef IIO_ADC_AD7887_H_
> -#define IIO_ADC_AD7887_H_
> -
> -/**
> - * struct ad7887_platform_data - AD7887 ADC driver platform data
> - * @en_dual: Whether to use dual channel mode. If set to true AIN1 becomes the
> - *     second input channel, and Vref is internally connected to Vdd. If set to
> - *     false the device is used in single channel mode and AIN1/Vref is used as
> - *     VREF input.
> - */
> -struct ad7887_platform_data {
> -       bool en_dual;
> -};
> -
> -#endif /* IIO_ADC_AD7887_H_ */
> --
> 2.17.1
>
Jonathan Cameron Nov. 21, 2020, 3:36 p.m. UTC | #2 
On Thu, 19 Nov 2020 12:07:45 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change converts the configuration of the dual-channel mode from the
> old platform-data, to the device_property_present() function, which
> supports both device-tree and ACPI configuration setups.
> 
> With this change the old platform_data include of the driver can be
> removed.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Hi Alexandru,

The set looks good in general, but there is a an oddity in the driver.
If we don't provide the vref regulator, then the scale is set to reflect
the internal 2.5V reference.  Fiven the external reference only
works by overdriving the 2.5V (must be higher than that to have
an effect) I guess we could in theory clamp it to a minimum of
2.5V but anyone wiring up less than that would have built a crazy board
so we can probably ignore it.

However, as I read the datasheet in dual channel mode it should be set
to VDD not 2.5V.  Right now you could make it work in a DT file
by setting VREF==VDD regulator but that's inelegant.

If you agree with my logic, perhaps a follow up patch?

Jonathan


> ---
> 
> I'm wondering if this changeset is what was in mind here:
>  https://lore.kernel.org/linux-iio/CA+U=DsqF5tu8Be9KXeyCWD2uHvV688Nc3n=z_Xi2J6H6DFJPRQ@mail.gmail.com/T/#mbe72e4da3acea3899d0d35402ea81e52a9bc34e6
> This driver could have been simplified/reduced a whole lot more, but I'm
> not sure about it. It's a bit of patch-noise, and later
> 
> Changelog v1 -> v2:
> * dropped patch 'iio: adc: ad7887: convert driver to full DT probing'
>   not adding the device_get_match_data() logic anymore
> * added patch 'iio: adc: ad7887: remove matching code from driver'
>   hooking the chip info directly to AD7887
> * added patch 'iio: adc: ad7887: add OF match table'
>   this just adds an OF table for DT and ACPI
> 
>  drivers/iio/adc/ad7887.c             | 10 +++++-----
>  include/linux/platform_data/ad7887.h | 21 ---------------------
>  2 files changed, 5 insertions(+), 26 deletions(-)
>  delete mode 100644 include/linux/platform_data/ad7887.h
> 
> diff --git a/drivers/iio/adc/ad7887.c b/drivers/iio/adc/ad7887.c
> index 4f6f0e0e03ee..06f684c053a0 100644
> --- a/drivers/iio/adc/ad7887.c
> +++ b/drivers/iio/adc/ad7887.c
> @@ -23,8 +23,6 @@
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>  
> -#include <linux/platform_data/ad7887.h>
> -
>  #define AD7887_REF_DIS		BIT(5)	/* on-chip reference disable */
>  #define AD7887_DUAL		BIT(4)	/* dual-channel mode */
>  #define AD7887_CH_AIN1		BIT(3)	/* convert on channel 1, DUAL=1 */
> @@ -241,9 +239,9 @@ static void ad7887_reg_disable(void *data)
>  
>  static int ad7887_probe(struct spi_device *spi)
>  {
> -	struct ad7887_platform_data *pdata = spi->dev.platform_data;
>  	struct ad7887_state *st;
>  	struct iio_dev *indio_dev;
> +	bool dual_mode;
>  	uint8_t mode;
>  	int ret;
>  
> @@ -286,7 +284,9 @@ static int ad7887_probe(struct spi_device *spi)
>  	mode = AD7887_PM_MODE4;
>  	if (!st->reg)
>  		mode |= AD7887_REF_DIS;
> -	if (pdata && pdata->en_dual)
> +
> +	dual_mode = device_property_present(&spi->dev, "adi,dual-channel-mode");
> +	if (dual_mode)
>  		mode |= AD7887_DUAL;
>  
>  	st->tx_cmd_buf[0] = AD7887_CH_AIN0 | mode;
> @@ -298,7 +298,7 @@ static int ad7887_probe(struct spi_device *spi)
>  	spi_message_init(&st->msg[AD7887_CH0]);
>  	spi_message_add_tail(&st->xfer[0], &st->msg[AD7887_CH0]);
>  
> -	if (pdata && pdata->en_dual) {
> +	if (dual_mode) {
>  		st->tx_cmd_buf[2] = AD7887_CH_AIN1 | mode;
>  
>  		st->xfer[1].rx_buf = &st->data[0];
> diff --git a/include/linux/platform_data/ad7887.h b/include/linux/platform_data/ad7887.h
> deleted file mode 100644
> index 9b4dca6ae70b..000000000000
> --- a/include/linux/platform_data/ad7887.h
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * AD7887 SPI ADC driver
> - *
> - * Copyright 2010 Analog Devices Inc.
> - */
> -#ifndef IIO_ADC_AD7887_H_
> -#define IIO_ADC_AD7887_H_
> -
> -/**
> - * struct ad7887_platform_data - AD7887 ADC driver platform data
> - * @en_dual: Whether to use dual channel mode. If set to true AIN1 becomes the
> - *	second input channel, and Vref is internally connected to Vdd. If set to
> - *	false the device is used in single channel mode and AIN1/Vref is used as
> - *	VREF input.
> - */
> -struct ad7887_platform_data {
> -	bool en_dual;
> -};
> -
> -#endif /* IIO_ADC_AD7887_H_ */
Alexandru Ardelean Nov. 27, 2020, 12:02 p.m. UTC | #3 
On Sat, Nov 21, 2020 at 5:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 19 Nov 2020 12:07:45 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > This change converts the configuration of the dual-channel mode from the
> > old platform-data, to the device_property_present() function, which
> > supports both device-tree and ACPI configuration setups.
> >
> > With this change the old platform_data include of the driver can be
> > removed.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> Hi Alexandru,
>
> The set looks good in general, but there is a an oddity in the driver.
> If we don't provide the vref regulator, then the scale is set to reflect
> the internal 2.5V reference.  Fiven the external reference only
> works by overdriving the 2.5V (must be higher than that to have
> an effect) I guess we could in theory clamp it to a minimum of
> 2.5V but anyone wiring up less than that would have built a crazy board
> so we can probably ignore it.
>
> However, as I read the datasheet in dual channel mode it should be set
> to VDD not 2.5V.  Right now you could make it work in a DT file
> by setting VREF==VDD regulator but that's inelegant.
>
> If you agree with my logic, perhaps a follow up patch?

I haven't managed to get around to this one.
Will try next week.


>
> Jonathan
>
>
> > ---
> >
> > I'm wondering if this changeset is what was in mind here:
> >  https://lore.kernel.org/linux-iio/CA+U=DsqF5tu8Be9KXeyCWD2uHvV688Nc3n=z_Xi2J6H6DFJPRQ@mail.gmail.com/T/#mbe72e4da3acea3899d0d35402ea81e52a9bc34e6
> > This driver could have been simplified/reduced a whole lot more, but I'm
> > not sure about it. It's a bit of patch-noise, and later
> >
> > Changelog v1 -> v2:
> > * dropped patch 'iio: adc: ad7887: convert driver to full DT probing'
> >   not adding the device_get_match_data() logic anymore
> > * added patch 'iio: adc: ad7887: remove matching code from driver'
> >   hooking the chip info directly to AD7887
> > * added patch 'iio: adc: ad7887: add OF match table'
> >   this just adds an OF table for DT and ACPI
> >
> >  drivers/iio/adc/ad7887.c             | 10 +++++-----
> >  include/linux/platform_data/ad7887.h | 21 ---------------------
> >  2 files changed, 5 insertions(+), 26 deletions(-)
> >  delete mode 100644 include/linux/platform_data/ad7887.h
> >
> > diff --git a/drivers/iio/adc/ad7887.c b/drivers/iio/adc/ad7887.c
> > index 4f6f0e0e03ee..06f684c053a0 100644
> > --- a/drivers/iio/adc/ad7887.c
> > +++ b/drivers/iio/adc/ad7887.c
> > @@ -23,8 +23,6 @@
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/triggered_buffer.h>
> >
> > -#include <linux/platform_data/ad7887.h>
> > -
> >  #define AD7887_REF_DIS               BIT(5)  /* on-chip reference disable */
> >  #define AD7887_DUAL          BIT(4)  /* dual-channel mode */
> >  #define AD7887_CH_AIN1               BIT(3)  /* convert on channel 1, DUAL=1 */
> > @@ -241,9 +239,9 @@ static void ad7887_reg_disable(void *data)
> >
> >  static int ad7887_probe(struct spi_device *spi)
> >  {
> > -     struct ad7887_platform_data *pdata = spi->dev.platform_data;
> >       struct ad7887_state *st;
> >       struct iio_dev *indio_dev;
> > +     bool dual_mode;
> >       uint8_t mode;
> >       int ret;
> >
> > @@ -286,7 +284,9 @@ static int ad7887_probe(struct spi_device *spi)
> >       mode = AD7887_PM_MODE4;
> >       if (!st->reg)
> >               mode |= AD7887_REF_DIS;
> > -     if (pdata && pdata->en_dual)
> > +
> > +     dual_mode = device_property_present(&spi->dev, "adi,dual-channel-mode");
> > +     if (dual_mode)
> >               mode |= AD7887_DUAL;
> >
> >       st->tx_cmd_buf[0] = AD7887_CH_AIN0 | mode;
> > @@ -298,7 +298,7 @@ static int ad7887_probe(struct spi_device *spi)
> >       spi_message_init(&st->msg[AD7887_CH0]);
> >       spi_message_add_tail(&st->xfer[0], &st->msg[AD7887_CH0]);
> >
> > -     if (pdata && pdata->en_dual) {
> > +     if (dual_mode) {
> >               st->tx_cmd_buf[2] = AD7887_CH_AIN1 | mode;
> >
> >               st->xfer[1].rx_buf = &st->data[0];
> > diff --git a/include/linux/platform_data/ad7887.h b/include/linux/platform_data/ad7887.h
> > deleted file mode 100644
> > index 9b4dca6ae70b..000000000000
> > --- a/include/linux/platform_data/ad7887.h
> > +++ /dev/null
> > @@ -1,21 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-or-later */
> > -/*
> > - * AD7887 SPI ADC driver
> > - *
> > - * Copyright 2010 Analog Devices Inc.
> > - */
> > -#ifndef IIO_ADC_AD7887_H_
> > -#define IIO_ADC_AD7887_H_
> > -
> > -/**
> > - * struct ad7887_platform_data - AD7887 ADC driver platform data
> > - * @en_dual: Whether to use dual channel mode. If set to true AIN1 becomes the
> > - *   second input channel, and Vref is internally connected to Vdd. If set to
> > - *   false the device is used in single channel mode and AIN1/Vref is used as
> > - *   VREF input.
> > - */
> > -struct ad7887_platform_data {
> > -     bool en_dual;
> > -};
> > -
> > -#endif /* IIO_ADC_AD7887_H_ */
>
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7887.c b/drivers/iio/adc/ad7887.c
index 4f6f0e0e03ee..06f684c053a0 100644
--- a/drivers/iio/adc/ad7887.c
+++ b/drivers/iio/adc/ad7887.c
@@ -23,8 +23,6 @@ 
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 
-#include <linux/platform_data/ad7887.h>
-
 #define AD7887_REF_DIS		BIT(5)	/* on-chip reference disable */
 #define AD7887_DUAL		BIT(4)	/* dual-channel mode */
 #define AD7887_CH_AIN1		BIT(3)	/* convert on channel 1, DUAL=1 */
@@ -241,9 +239,9 @@  static void ad7887_reg_disable(void *data)
 
 static int ad7887_probe(struct spi_device *spi)
 {
-	struct ad7887_platform_data *pdata = spi->dev.platform_data;
 	struct ad7887_state *st;
 	struct iio_dev *indio_dev;
+	bool dual_mode;
 	uint8_t mode;
 	int ret;
 
@@ -286,7 +284,9 @@  static int ad7887_probe(struct spi_device *spi)
 	mode = AD7887_PM_MODE4;
 	if (!st->reg)
 		mode |= AD7887_REF_DIS;
-	if (pdata && pdata->en_dual)
+
+	dual_mode = device_property_present(&spi->dev, "adi,dual-channel-mode");
+	if (dual_mode)
 		mode |= AD7887_DUAL;
 
 	st->tx_cmd_buf[0] = AD7887_CH_AIN0 | mode;
@@ -298,7 +298,7 @@  static int ad7887_probe(struct spi_device *spi)
 	spi_message_init(&st->msg[AD7887_CH0]);
 	spi_message_add_tail(&st->xfer[0], &st->msg[AD7887_CH0]);
 
-	if (pdata && pdata->en_dual) {
+	if (dual_mode) {
 		st->tx_cmd_buf[2] = AD7887_CH_AIN1 | mode;
 
 		st->xfer[1].rx_buf = &st->data[0];
diff --git a/include/linux/platform_data/ad7887.h b/include/linux/platform_data/ad7887.h
deleted file mode 100644
index 9b4dca6ae70b..000000000000
--- a/include/linux/platform_data/ad7887.h
+++ /dev/null
@@ -1,21 +0,0 @@ 
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * AD7887 SPI ADC driver
- *
- * Copyright 2010 Analog Devices Inc.
- */
-#ifndef IIO_ADC_AD7887_H_
-#define IIO_ADC_AD7887_H_
-
-/**
- * struct ad7887_platform_data - AD7887 ADC driver platform data
- * @en_dual: Whether to use dual channel mode. If set to true AIN1 becomes the
- *	second input channel, and Vref is internally connected to Vdd. If set to
- *	false the device is used in single channel mode and AIN1/Vref is used as
- *	VREF input.
- */
-struct ad7887_platform_data {
-	bool en_dual;
-};
-
-#endif /* IIO_ADC_AD7887_H_ */