[v2,3/6] iio: ad7949: Support configuration read-back
diff mbox series

Message ID 1556813672-49861-3-git-send-email-adam.michaelis@rockwellcollins.com
State New
Headers show
Series
  • [v2,1/6] iio: ad7949: Support internal Vref
Related show

Commit Message

Adam Michaelis May 2, 2019, 4:14 p.m. UTC
Adds device tree parameter to set the configuration read-back bit
in the configuration register to tell the AD7949 to include the value of
the configuration register at the time the current sample was acquired
when reading from the part.

Further work must be done to make read-back information available to
consumer.

Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
	V2: Add some defines to reduce use of magic numbers.
---
 drivers/iio/adc/ad7949.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Jonathan Cameron May 11, 2019, 10:31 a.m. UTC | #1
On Tue, 7 May 2019 14:53:32 -0500
Adam Michaelis <adam.michaelis@collins.com> wrote:

> On Sun, May 5, 2019 at 9:42 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu,  2 May 2019 11:14:29 -0500
> > Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
> >  
> > > Adds device tree parameter to set the configuration read-back bit
> > > in the configuration register to tell the AD7949 to include the value of
> > > the configuration register at the time the current sample was acquired
> > > when reading from the part.
> > >
> > > Further work must be done to make read-back information available to
> > > consumer.  
> >
> > This needs some explanation of why it is useful at all. I'm certainly unclear
> > on why it would be useful to configure this at boot time.
> >
> > Code looks fine.
> >
> > Jonathan
> >  
> The configuration read-back feature is being maintained from the
> original version of this driver. Before adding the device tree entry,
> there was no way to change this setting other than debugfs raw access
> to the SPI interface, and there is still no access to the returned
> configuration data should the feature be enabled. I would be willing
> to remove the feature altogether, but wanted to tread softly on
> existing features.
Ah. Makes sense.  My gut feeling is to drop it.

Anyone at Analog have a view on this?

Thanks,

Jonathan

> 
> Adam
> > >
> > > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > > ---
> > >       V2: Add some defines to reduce use of magic numbers.
> > > ---
> > >  drivers/iio/adc/ad7949.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > index afc1361af5fb..7820e1097787 100644
> > > --- a/drivers/iio/adc/ad7949.c
> > > +++ b/drivers/iio/adc/ad7949.c
> > > @@ -69,6 +69,7 @@ struct ad7949_adc_spec {
> > >   * @iio_dev: reference to iio structure
> > >   * @spi: reference to spi structure
> > >   * @ref_sel: selected reference voltage source
> > > + * @cfg_readback: whether reads will include configuration data
> > >   * @resolution: resolution of the chip
> > >   * @cfg: copy of the configuration register
> > >   * @current_channel: current channel in use
> > > @@ -80,6 +81,7 @@ struct ad7949_adc_chip {
> > >       struct iio_dev *indio_dev;
> > >       struct spi_device *spi;
> > >       enum ad7949_ref_sel ref_sel;
> > > +     bool cfg_readback;
> > >       u8 resolution;
> > >       u16 cfg;
> > >       unsigned int current_channel;
> > > @@ -283,7 +285,11 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> > >                       AD7949_CFG_REF_SEL_MASK;
> > >       adc_config |= (AD7949_CFG_SEQ_DISABLED << AD7949_CFG_SEQ_SHIFT) &
> > >                       AD7949_CFG_SEQ_MASK;
> > > -     adc_config |= AD7949_CFG_READBACK_DIS;
> > > +
> > > +     if (ad7949_adc->cfg_readback)
> > > +             adc_config |= AD7949_CFG_READBACK_EN;
> > > +     else
> > > +             adc_config |= AD7949_CFG_READBACK_DIS;
> > >
> > >       ret = ad7949_spi_write_cfg(ad7949_adc,
> > >                       adc_config,
> > > @@ -331,6 +337,10 @@ static int ad7949_spi_probe(struct spi_device *spi)
> > >       indio_dev->num_channels = spec->num_channels;
> > >       ad7949_adc->resolution = spec->resolution;
> > >
> > > +     ad7949_adc->cfg_readback = of_property_read_bool(
> > > +                     ad7949_adc->indio_dev->dev.of_node,
> > > +                     "adi,cfg-readback");
> > > +
> > >       ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
> > >                       "adi,reference-select",
> > >                       &temp);  
> >  
> 
> --
Popa, Stefan Serban May 13, 2019, 10:04 a.m. UTC | #2
On Sb, 2019-05-11 at 11:31 +0100, Jonathan Cameron wrote:
> [External]
> 
> 
> On Tue, 7 May 2019 14:53:32 -0500
> Adam Michaelis <adam.michaelis@collins.com> wrote:
> 
> > 
> > On Sun, May 5, 2019 at 9:42 AM Jonathan Cameron <jic23@kernel.org>
> > wrote:
> > > 
> > > 
> > > On Thu,  2 May 2019 11:14:29 -0500
> > > Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
> > > 
> > > > 
> > > > Adds device tree parameter to set the configuration read-back bit
> > > > in the configuration register to tell the AD7949 to include the
> > > > value of
> > > > the configuration register at the time the current sample was
> > > > acquired
> > > > when reading from the part.
> > > > 
> > > > Further work must be done to make read-back information available
> > > > to
> > > > consumer.
> > > This needs some explanation of why it is useful at all. I'm certainly
> > > unclear
> > > on why it would be useful to configure this at boot time.
> > > 
> > > Code looks fine.
> > > 
> > > Jonathan
> > > 
> > The configuration read-back feature is being maintained from the
> > original version of this driver. Before adding the device tree entry,
> > there was no way to change this setting other than debugfs raw access
> > to the SPI interface, and there is still no access to the returned
> > configuration data should the feature be enabled. I would be willing
> > to remove the feature altogether, but wanted to tread softly on
> > existing features.
> Ah. Makes sense.  My gut feeling is to drop it.
> 
> Anyone at Analog have a view on this?
> 
> Thanks,
> 
> Jonathan
> 

Hi, 

It is not obvious to me why this feature would be useful. I would not add
it.

Regards,
-Stefan

> > 
> > 
> > Adam
> > > 
> > > > 
> > > > 
> > > > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > > > ---
> > > >       V2: Add some defines to reduce use of magic numbers.
> > > > ---
> > > >  drivers/iio/adc/ad7949.c | 12 +++++++++++-
> > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > > index afc1361af5fb..7820e1097787 100644
> > > > --- a/drivers/iio/adc/ad7949.c
> > > > +++ b/drivers/iio/adc/ad7949.c
> > > > @@ -69,6 +69,7 @@ struct ad7949_adc_spec {
> > > >   * @iio_dev: reference to iio structure
> > > >   * @spi: reference to spi structure
> > > >   * @ref_sel: selected reference voltage source
> > > > + * @cfg_readback: whether reads will include configuration data
> > > >   * @resolution: resolution of the chip
> > > >   * @cfg: copy of the configuration register
> > > >   * @current_channel: current channel in use
> > > > @@ -80,6 +81,7 @@ struct ad7949_adc_chip {
> > > >       struct iio_dev *indio_dev;
> > > >       struct spi_device *spi;
> > > >       enum ad7949_ref_sel ref_sel;
> > > > +     bool cfg_readback;
> > > >       u8 resolution;
> > > >       u16 cfg;
> > > >       unsigned int current_channel;
> > > > @@ -283,7 +285,11 @@ static int ad7949_spi_init(struct
> > > > ad7949_adc_chip *ad7949_adc)
> > > >                       AD7949_CFG_REF_SEL_MASK;
> > > >       adc_config |= (AD7949_CFG_SEQ_DISABLED <<
> > > > AD7949_CFG_SEQ_SHIFT) &
> > > >                       AD7949_CFG_SEQ_MASK;
> > > > -     adc_config |= AD7949_CFG_READBACK_DIS;
> > > > +
> > > > +     if (ad7949_adc->cfg_readback)
> > > > +             adc_config |= AD7949_CFG_READBACK_EN;
> > > > +     else
> > > > +             adc_config |= AD7949_CFG_READBACK_DIS;
> > > > 
> > > >       ret = ad7949_spi_write_cfg(ad7949_adc,
> > > >                       adc_config,
> > > > @@ -331,6 +337,10 @@ static int ad7949_spi_probe(struct spi_device
> > > > *spi)
> > > >       indio_dev->num_channels = spec->num_channels;
> > > >       ad7949_adc->resolution = spec->resolution;
> > > > 
> > > > +     ad7949_adc->cfg_readback = of_property_read_bool(
> > > > +                     ad7949_adc->indio_dev->dev.of_node,
> > > > +                     "adi,cfg-readback");
> > > > +
> > > >       ret = of_property_read_u32(ad7949_adc->indio_dev-
> > > > >dev.of_node,
> > > >                       "adi,reference-select",
> > > >                       &temp);
> > --
Adam Michaelis May 13, 2019, 2:52 p.m. UTC | #3
On Mon, May 13, 2019 at 5:04 AM Popa, Stefan Serban
<StefanSerban.Popa@analog.com> wrote:
>
> On Sb, 2019-05-11 at 11:31 +0100, Jonathan Cameron wrote:
> > [External]
> >
> >
> > On Tue, 7 May 2019 14:53:32 -0500
> > Adam Michaelis <adam.michaelis@collins.com> wrote:
> >
> > >
> > > On Sun, May 5, 2019 at 9:42 AM Jonathan Cameron <jic23@kernel.org>
> > > wrote:
> > > >
> > > >
> > > > On Thu,  2 May 2019 11:14:29 -0500
> > > > Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
> > > >
> > > > >
> > > > > Adds device tree parameter to set the configuration read-back bit
> > > > > in the configuration register to tell the AD7949 to include the
> > > > > value of
> > > > > the configuration register at the time the current sample was
> > > > > acquired
> > > > > when reading from the part.
> > > > >
> > > > > Further work must be done to make read-back information available
> > > > > to
> > > > > consumer.
> > > > This needs some explanation of why it is useful at all. I'm certainly
> > > > unclear
> > > > on why it would be useful to configure this at boot time.
> > > >
> > > > Code looks fine.
> > > >
> > > > Jonathan
> > > >
> > > The configuration read-back feature is being maintained from the
> > > original version of this driver. Before adding the device tree entry,
> > > there was no way to change this setting other than debugfs raw access
> > > to the SPI interface, and there is still no access to the returned
> > > configuration data should the feature be enabled. I would be willing
> > > to remove the feature altogether, but wanted to tread softly on
> > > existing features.
> > Ah. Makes sense.  My gut feeling is to drop it.
> >
> > Anyone at Analog have a view on this?
> >
> > Thanks,
> >
> > Jonathan
> >
>
> Hi,
>
> It is not obvious to me why this feature would be useful. I would not add
> it.
>
> Regards,
> -Stefan
>

I have removed this feature in V3 of the patch series.

Thank you for the feedback,
Adam

> > >
> > >
> > > Adam
> > > >
> > > > >
> > > > >
> > > > > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > > > > ---
> > > > >       V2: Add some defines to reduce use of magic numbers.
> > > > > ---
> > > > >  drivers/iio/adc/ad7949.c | 12 +++++++++++-
> > > > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > > > > index afc1361af5fb..7820e1097787 100644
> > > > > --- a/drivers/iio/adc/ad7949.c
> > > > > +++ b/drivers/iio/adc/ad7949.c
> > > > > @@ -69,6 +69,7 @@ struct ad7949_adc_spec {
> > > > >   * @iio_dev: reference to iio structure
> > > > >   * @spi: reference to spi structure
> > > > >   * @ref_sel: selected reference voltage source
> > > > > + * @cfg_readback: whether reads will include configuration data
> > > > >   * @resolution: resolution of the chip
> > > > >   * @cfg: copy of the configuration register
> > > > >   * @current_channel: current channel in use
> > > > > @@ -80,6 +81,7 @@ struct ad7949_adc_chip {
> > > > >       struct iio_dev *indio_dev;
> > > > >       struct spi_device *spi;
> > > > >       enum ad7949_ref_sel ref_sel;
> > > > > +     bool cfg_readback;
> > > > >       u8 resolution;
> > > > >       u16 cfg;
> > > > >       unsigned int current_channel;
> > > > > @@ -283,7 +285,11 @@ static int ad7949_spi_init(struct
> > > > > ad7949_adc_chip *ad7949_adc)
> > > > >                       AD7949_CFG_REF_SEL_MASK;
> > > > >       adc_config |= (AD7949_CFG_SEQ_DISABLED <<
> > > > > AD7949_CFG_SEQ_SHIFT) &
> > > > >                       AD7949_CFG_SEQ_MASK;
> > > > > -     adc_config |= AD7949_CFG_READBACK_DIS;
> > > > > +
> > > > > +     if (ad7949_adc->cfg_readback)
> > > > > +             adc_config |= AD7949_CFG_READBACK_EN;
> > > > > +     else
> > > > > +             adc_config |= AD7949_CFG_READBACK_DIS;
> > > > >
> > > > >       ret = ad7949_spi_write_cfg(ad7949_adc,
> > > > >                       adc_config,
> > > > > @@ -331,6 +337,10 @@ static int ad7949_spi_probe(struct spi_device
> > > > > *spi)
> > > > >       indio_dev->num_channels = spec->num_channels;
> > > > >       ad7949_adc->resolution = spec->resolution;
> > > > >
> > > > > +     ad7949_adc->cfg_readback = of_property_read_bool(
> > > > > +                     ad7949_adc->indio_dev->dev.of_node,
> > > > > +                     "adi,cfg-readback");
> > > > > +
> > > > >       ret = of_property_read_u32(ad7949_adc->indio_dev-
> > > > > >dev.of_node,
> > > > >                       "adi,reference-select",
> > > > >                       &temp);
> > > --
Couret Charles-Antoine May 24, 2019, 12:02 p.m. UTC | #4
Le 07/05/2019 à 21:53, Adam Michaelis a écrit :
> On Sun, May 5, 2019 at 9:42 AM Jonathan Cameron <jic23@kernel.org> wrote:
>> On Thu,  2 May 2019 11:14:29 -0500
>> Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
>>
>>> Adds device tree parameter to set the configuration read-back bit
>>> in the configuration register to tell the AD7949 to include the value of
>>> the configuration register at the time the current sample was acquired
>>> when reading from the part.
>>>
>>> Further work must be done to make read-back information available to
>>> consumer.
>> This needs some explanation of why it is useful at all. I'm certainly unclear
>> on why it would be useful to configure this at boot time.
>>
>> Code looks fine.
>>
>> Jonathan
>>
> The configuration read-back feature is being maintained from the
> original version of this driver. Before adding the device tree entry,
> there was no way to change this setting other than debugfs raw access
> to the SPI interface, and there is still no access to the returned
> configuration data should the feature be enabled. I would be willing
> to remove the feature altogether, but wanted to tread softly on
> existing features.

Hi,

I added this feature for debug purpose but it is not used in our case 
anymore because the driver and the device are working as expected.

But maybe we can use it the check if the config is correctly applied? I 
don't know, it is probably useless to keep this feature here.

Regards,

Charles-Antoine Couret

Patch
diff mbox series

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index afc1361af5fb..7820e1097787 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -69,6 +69,7 @@  struct ad7949_adc_spec {
  * @iio_dev: reference to iio structure
  * @spi: reference to spi structure
  * @ref_sel: selected reference voltage source
+ * @cfg_readback: whether reads will include configuration data
  * @resolution: resolution of the chip
  * @cfg: copy of the configuration register
  * @current_channel: current channel in use
@@ -80,6 +81,7 @@  struct ad7949_adc_chip {
 	struct iio_dev *indio_dev;
 	struct spi_device *spi;
 	enum ad7949_ref_sel ref_sel;
+	bool cfg_readback;
 	u8 resolution;
 	u16 cfg;
 	unsigned int current_channel;
@@ -283,7 +285,11 @@  static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 			AD7949_CFG_REF_SEL_MASK;
 	adc_config |= (AD7949_CFG_SEQ_DISABLED << AD7949_CFG_SEQ_SHIFT) &
 			AD7949_CFG_SEQ_MASK;
-	adc_config |= AD7949_CFG_READBACK_DIS;
+
+	if (ad7949_adc->cfg_readback)
+		adc_config |= AD7949_CFG_READBACK_EN;
+	else
+		adc_config |= AD7949_CFG_READBACK_DIS;
 
 	ret = ad7949_spi_write_cfg(ad7949_adc,
 			adc_config,
@@ -331,6 +337,10 @@  static int ad7949_spi_probe(struct spi_device *spi)
 	indio_dev->num_channels = spec->num_channels;
 	ad7949_adc->resolution = spec->resolution;
 
+	ad7949_adc->cfg_readback = of_property_read_bool(
+			ad7949_adc->indio_dev->dev.of_node,
+			"adi,cfg-readback");
+
 	ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
 			"adi,reference-select",
 			&temp);