Re: [PATCH 4/5] iio: light: lm3533-als: remove explicit parent assignment

[Ardelean, Alexandru]

On Fri, 2020-05-29 at 12:16 +0200, Johan Hovold wrote:
> [External]
> 
> On Fri, May 22, 2020 at 11:22:07AM +0300, Alexandru Ardelean wrote:
> > This assignment is the more peculiar of the bunch as it assigns the parent
> > of the platform-device's device (i.e. pdev->dev.parent) as the IIO device's
> > parent.
> > 
> > It's unclear whether this is intentional or not.
> > Hence it is in it's own patch.
> 
> Yeah, we have a few mfd drivers whose child drivers registers their
> class devices directly under the parent mfd device rather than the
> corresponding child platform device.
> 
> Since it's done consistently I think you need to update them all if you
> really want to change this. 
> 
> And it may not be worth it since at least in theory someone could now be
> relying on this topology.

Thanks for the feedback.
I guess, it could make sense to do here:
  devm_iio_device_alloc(pdev->dev.parent, ...)

Currently it's:
  devm_iio_device_alloc(>dev, ...)

That would make it slightly more consistent.
i.e. the life-time of the object would be attached to the parent of the platform
device, versus the platform-device.

Currently, as it is, the allocation [of the IIO device] is tied the platform-
device, and the IIO registration to the parent (of the platform-device).
I'm not super-familiar with the internals here, but does this sound a bit wrong?
Is there a chance where the IIO device could be de-allocated, while registered?


> 
> > Signed-off-by: Alexandru Ardelean 
> > ---
> >  drivers/iio/light/lm3533-als.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/iio/light/lm3533-als.c b/drivers/iio/light/lm3533-als.c
> > index bc196c212881..0f380ec8d30c 100644
> > --- a/drivers/iio/light/lm3533-als.c
> > +++ b/drivers/iio/light/lm3533-als.c
> > @@ -852,7 +852,6 @@ static int lm3533_als_probe(struct platform_device
> > *pdev)
> > indio_dev->channels = lm3533_als_channels;
> > indio_dev->num_channels = ARRAY_SIZE(lm3533_als_channels);
> > indio_dev->name = dev_name(>dev);
> > -   indio_dev->dev.parent = pdev->dev.parent;
> > indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> > als = iio_priv(indio_dev);
> 
> Johan
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad2s1210: Fix SPI reading

[Ardelean, Alexandru]

On Sun, 2020-05-03 at 12:37 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Wed, 29 Apr 2020 10:21:29 +0300
> Alexandru Ardelean  wrote:
> 
> > From: Dragos Bogdan 
> > 
> > If the serial interface is used, the 8-bit address should be latched using
> > the rising edge of the WR/FSYNC signal.
> > 
> > This basically means that a CS change is required between the first byte
> > sent, and the second one.
> > This change splits the single-transfer transfer of 2 bytes into 2 transfers
> > with a single byte, and CS change in-between.
> > 
> > Signed-off-by: Dragos Bogdan 
> > Signed-off-by: Alexandru Ardelean 
> 
> Fixes tag would have been nice. I've had a go by picking a patch where I
> refactored this code, but I think the issue probably predates that one.
> Its in 2011 so I doubt anyone will try going past that with backports ;)
> 

Apologies again for not considering Fixes tag.
At this point, I feel bad for repeating the apology, as it may sound like hollow

words.
But, I guess this could have skipped going through the fixes route.
The patch has been living in our tree for a while.

> Applied to the fixes-togreg branch of iio.git and marked for stable.
> 
> I'm guessing this means you have hardware and hope to get this one out
> of staging shortly? *crosses fingers* :)

Sorry, but not at this point in time.
I just fished this from our tree.
I also handle our kernel upgrades [on our side], and whenever I do an update,
some upstreamed patches disappear from our tree, and some stand-out and I wonder
how come this wasn't sent upstream.
This is one of them.

I don't know if I'll be able to find someone [in the near future] to allocate
this to [for moving out-of-staging].
Right now, the priority [on our side] is the high-speed converters.


> 
> Jonathan
> 
> > ---
> >  drivers/staging/iio/resolver/ad2s1210.c | 17 -
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c
> > b/drivers/staging/iio/resolver/ad2s1210.c
> > index 4b25a3a314ed..ed404355ea4c 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -130,17 +130,24 @@ static int ad2s1210_config_write(struct ad2s1210_state
> > *st, u8 data)
> >  static int ad2s1210_config_read(struct ad2s1210_state *st,
> > unsigned char address)
> >  {
> > -   struct spi_transfer xfer = {
> > -   .len = 2,
> > -   .rx_buf = st->rx,
> > -   .tx_buf = st->tx,
> > +   struct spi_transfer xfers[] = {
> > +   {
> > +   .len = 1,
> > +   .rx_buf = >rx[0],
> > +   .tx_buf = >tx[0],
> > +   .cs_change = 1,
> > +   }, {
> > +   .len = 1,
> > +   .rx_buf = >rx[1],
> > +   .tx_buf = >tx[1],
> > +   },
> > };
> > int ret = 0;
> >  
> > ad2s1210_set_mode(MOD_CONFIG, st);
> > st->tx[0] = address | AD2S1210_MSB_IS_HIGH;
> > st->tx[1] = AD2S1210_REG_FAULT;
> > -   ret = spi_sync_transfer(st->sdev, , 1);
> > +   ret = spi_sync_transfer(st->sdev, xfers, 2);
> > if (ret < 0)
> > return ret;
> >  
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad5933: rework probe to use devm_ function variants

[Ardelean, Alexandru]

On Sat, 2020-05-02 at 19:25 +0100, Jonathan Cameron wrote:
> On Tue, 28 Apr 2020 12:31:28 +0300
> Alexandru Ardelean  wrote:
> 
> > This change cleans up the driver's probe function to use only devm_
> > function variants. This also gets rid of the remove function and moves the
> > clock & regulator de-initializations to the 'ad5933_cleanup()' callback.
> > 
> > Signed-off-by: Alexandru Ardelean 
> 
> Basic rule of thumb. Whatever you register with devm_add_action_or_reset
> should only cleanup one one thing done in the probe path.
> There is almost always a race if you do more than one bit of cleanup
> per such callback + it's harder to review as it fails the 'obviously correct
> test'.

I did not know that.
That idea missed me up until now.

Will re-spin.
Thanks
Alex

> 
> Jonathan
> 
> > ---
> >  .../staging/iio/impedance-analyzer/ad5933.c   | 59 ---
> >  1 file changed, 23 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > index af0bcf95ee8a..06a6dcd7883b 100644
> > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > @@ -602,11 +602,12 @@ static const struct iio_buffer_setup_ops
> > ad5933_ring_setup_ops = {
> > .postdisable = ad5933_ring_postdisable,
> >  };
> >  
> > -static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> > +static int ad5933_register_ring_funcs_and_init(struct device *dev,
> > +  struct iio_dev *indio_dev)
> >  {
> > struct iio_buffer *buffer;
> >  
> > -   buffer = iio_kfifo_allocate();
> > +   buffer = devm_iio_kfifo_allocate(dev);
> > if (!buffer)
> > return -ENOMEM;
> >  
> > @@ -676,6 +677,14 @@ static void ad5933_work(struct work_struct *work)
> > }
> >  }
> >  
> > +static void ad5933_cleanup(void *data)
> > +{
> > +   struct ad5933_state *st = data;
> > +
> > +   clk_disable_unprepare(st->mclk);
> > +   regulator_disable(st->reg);
> 
> Please do two separate callbacks so that these can be handled
> in the correct places.  I.e. you do something then immediately
> register the handler to undo it.
> 
> Currently you can end up disabling a clock you haven't enabled
> (which I am fairly sure will give you an error message).
> 
> > +}
> > +
> >  static int ad5933_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> >  {
> > @@ -703,23 +712,28 @@ static int ad5933_probe(struct i2c_client *client,
> > dev_err(>dev, "Failed to enable specified VDD
> > supply\n");
> > return ret;
> > }
> > +
> > +   ret = devm_add_action_or_reset(>dev, ad5933_cleanup, st);
> > +   if (ret)
> > +   return ret;
> > +
> > ret = regulator_get_voltage(st->reg);
> >  
> > if (ret < 0)
> > -   goto error_disable_reg;
> > +   return ret;
> >  
> > st->vref_mv = ret / 1000;
> >  
> > st->mclk = devm_clk_get(>dev, "mclk");
> > if (IS_ERR(st->mclk) && PTR_ERR(st->mclk) != -ENOENT) {
> > ret = PTR_ERR(st->mclk);
> > -   goto error_disable_reg;
> > +   return ret;
> > }
> >  
> > if (!IS_ERR(st->mclk)) {
> > ret = clk_prepare_enable(st->mclk);
> > if (ret < 0)
> > -   goto error_disable_reg;
> > +   return ret;
> > ext_clk_hz = clk_get_rate(st->mclk);
> > }
> >  
> > @@ -742,41 +756,15 @@ static int ad5933_probe(struct i2c_client *client,
> > indio_dev->channels = ad5933_channels;
> > indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
> >  
> > -   ret = ad5933_register_ring_funcs_and_init(indio_dev);
> > +   ret = ad5933_register_ring_funcs_and_init(>dev, indio_dev);
> > if (ret)
> > -   goto error_disable_mclk;
> > +   return ret;
> >  
> > ret = ad5933_setup(st);
> > if (ret)
> > -   goto error_unreg_ring;
> > -
> > -   ret = iio_device_register(indio_dev);
> > -   if (ret)
> > -   goto error_unreg_ring;
> > -
> > -   return 0;
> > -
> > -error_unreg_ring:
> > -   iio_kfifo_free(indio_dev->buffer);
> > -error_disable_mclk:
> > -   clk_disable_unprepare(st->mclk);
> > -error_disable_reg:
> > -   regulator_disable(st->reg);
> > -
> > -   return ret;
> > -}
> > -
> > -static int ad5933_remove(struct i2c_client *client)
> > -{
> > -   struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > -   struct ad5933_state *st = iio_priv(indio_dev);
> > -
> > -   iio_device_unregister(indio_dev);
> > -   iio_kfifo_free(indio_dev->buffer);
> > -   regulator_disable(st->reg);
> > -   clk_disable_unprepare(st->mclk);
> > +   return ret;
> >  
> > -   return 0;
> > +   return devm_iio_device_register(>dev, indio_dev);
> >  }
> >  
> >  static const struct i2c_device_id ad5933_id[] = {
> > @@ -801,7 +789,6 @@ static struct i2c_driver ad5933_driver = {
> > 

Re: [PATCH 2/3] iio: make use of iio_device_attach_kfifo_buffer() where straightforward

[Ardelean, Alexandru]

On Tue, 2020-04-07 at 11:26 +0200, Markus Elfring wrote:
> [External]
> 
> How do you think about a patch subject like “iio: Increase use of
> iio_device_attach_kfifo_buffer()”?
> 
> 
> > This change does that.
> 
> I suggest to improve also this commit message.
> 
> * Would you like to consider a wording like “Convert a specific function call
>   combination to a better programming interface.”?
> 
> * Do you imagine any more software fine-tuning because of related
>   collateral evolution?
> 

I'll see.
This patchset is kind of stopped.
Will need a rework for it.

> Regards,
> Markus
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging: iio: ad5933: use iio_device_attach_kfifo_buffer() helper

[Ardelean, Alexandru]

On Sun, 2020-04-05 at 11:49 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Wed, 1 Apr 2020 15:59:36 +0300
> Alexandru Ardelean  wrote:
> 
> > This driver calls iio_kfifo_allocate() vs devm_iio_kfifo_allocate(). But
> > the conversion is still simpler here, and cleans-up/reduces some error
> > paths.
> > 
> > Signed-off-by: Alexandru Ardelean 
> 
> This mixes devm managed stuff an unmanaged.  Hence it fails the 'obviously
> correct'
> test.  If you wanted to do this you'd first need to sort out the unmanaged
> bits to be automatically unwound (regulators and clocks). Or potentially
> reorder
> the driver so those happen after this allocation is done.
> 

Yeah.
I was a bit sloppy here.
I think tried a broader cleanup/rework would be a better idea here.


> Thanks,
> 
> Jonathan
> 
> > ---
> >  .../staging/iio/impedance-analyzer/ad5933.c   | 28 ---
> >  1 file changed, 5 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > index af0bcf95ee8a..7bde93c6dd74 100644
> > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > @@ -602,22 +602,6 @@ static const struct iio_buffer_setup_ops
> > ad5933_ring_setup_ops = {
> > .postdisable = ad5933_ring_postdisable,
> >  };
> >  
> > -static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> > -{
> > -   struct iio_buffer *buffer;
> > -
> > -   buffer = iio_kfifo_allocate();
> > -   if (!buffer)
> > -   return -ENOMEM;
> > -
> > -   iio_device_attach_buffer(indio_dev, buffer);
> > -
> > -   /* Ring buffer functions - here trigger setup related */
> > -   indio_dev->setup_ops = _ring_setup_ops;
> > -
> > -   return 0;
> > -}
> > -
> >  static void ad5933_work(struct work_struct *work)
> >  {
> > struct ad5933_state *st = container_of(work,
> > @@ -738,26 +722,25 @@ static int ad5933_probe(struct i2c_client *client,
> > indio_dev->dev.parent = >dev;
> > indio_dev->info = _info;
> > indio_dev->name = id->name;
> > -   indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE);
> > +   indio_dev->modes = INDIO_DIRECT_MODE;
> > indio_dev->channels = ad5933_channels;
> > indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
> >  
> > -   ret = ad5933_register_ring_funcs_and_init(indio_dev);
> > +   ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE,
> > +_ring_setup_ops);
> > if (ret)
> > goto error_disable_mclk;
> >  
> > ret = ad5933_setup(st);
> > if (ret)
> > -   goto error_unreg_ring;
> > +   goto error_disable_mclk;
> >  
> > ret = iio_device_register(indio_dev);
> > if (ret)
> > -   goto error_unreg_ring;
> > +   goto error_disable_mclk;
> >  
> > return 0;
> >  
> > -error_unreg_ring:
> > -   iio_kfifo_free(indio_dev->buffer);
> >  error_disable_mclk:
> > clk_disable_unprepare(st->mclk);
> >  error_disable_reg:
> > @@ -772,7 +755,6 @@ static int ad5933_remove(struct i2c_client *client)
> > struct ad5933_state *st = iio_priv(indio_dev);
> >  
> > iio_device_unregister(indio_dev);
> > -   iio_kfifo_free(indio_dev->buffer);
> > regulator_disable(st->reg);
> > clk_disable_unprepare(st->mclk);
> >  
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] iio: kfifo: add iio_device_attach_kfifo_buffer() helper

[Ardelean, Alexandru]

On Sun, 2020-04-05 at 11:46 +0100, Jonathan Cameron wrote:
> [External]
> 
> On Wed, 1 Apr 2020 15:59:34 +0300
> Alexandru Ardelean  wrote:
> 
> > This change adds the iio_device_attach_kfifo_buffer() helper/short-hand,
> > which groups the simple routine of allocating a kfifo buffers via
> > devm_iio_kfifo_allocate() and calling iio_device_attach_buffer().
> > 
> > The mode_flags parameter is required. The setup_ops parameter is optional.
> > 
> > This function will be a bit more useful when needing to define multiple
> > buffers per IIO device.
> > 
> > One requirement [that is more a recommendation] for this helper, is to call
> > it after 'indio_dev' has been populated.
> > 
> > Also, one consequence related to using this helper is that the resource
> > management of the buffer will be tied to 'indio_dev->dev'. Previously it
> > was open-coded, and each driver does it slightly differently. Most of them
> > tied it to the parent device, some of them to 'indio_dev->dev'.
> > This shouldn't be a problem, and may be a good idea when adding more
> > buffers per-device.
> 
> I'm glad you highlighted this subtlety.  I'm not sure it's safe in all cases
> because the result is that the managed cleanup for this will occur once we
> get to the cleanup for devm_iio_device_alloc and we release the indio_dev->dev
> 
> That would put it 'after' any other devm calls that are still hung off the
> parent
> device.
> 
> Now the question is whether that ever causes us problems... See next patch.
> It potentially does.  I think we need to provide the dev separately even
> if it feels a bit silly to do so.  Scope management is complex so I don't
> really want to force people to mix and match between different devices
> and so get it wrong by accident.
> 
> The other issue is that it's not readily apparent from the naming that
> this function is registering stuff that is cleaned up automatically or
> that it even allocates anything that might need that..
> 
> devm_iio_device_attach_new_kfifo_buffer maybe?
> 
> I'm sort of wondering if we should do what dma did and have
> 
> iiom_device_attach_new_kfifo_buffer to indicate it's managed in the
> scope of the iio device?
> 
> What do people think?
> 
> However, see patch 2 before commenting.  Reality is I'm not sure forcing
> managed calls to hang off iio_dev->dev is a good idea (at this stage given
> where we are).

What I am really after with this patch is to hide away these:
 iio_kfifo_free(indio_dev->buffer);
 iio_buffer_set_attrs(indio_dev->buffer, _fifo_attributes); 
i.e. not have 'indio_dev->buffer' open-coded in drivers, and hide it in IIO core
somewhere.
Some ideas can go in parallel [like this one] to add support for multiple
buffers.

So, I will think of a better [less sloppy] V2 for this.

One intermediate alternative is to do 'iio_device_kfifo_free(indio_dev)', but
I'll still try to think of a better devm_ approach.
devm_iio_device_attach_new_kfifo_buffer() sounds a bit long but may work.
iiom_device_attach_new_kfifo_buffer() can also work.

What if we just default attaching to the parent device?

Would it work to also attach the parent device in devm_iio_device_alloc() by
default?
Or change 'iio_device_alloc()' to take a parent device as argument?
Which for devm_iio_device_alloc(dev,...) would implicitly mean that 'dev' is
'parent'?

These are just some thoughts.


> 
> Thanks
> 
> Jonathan
> 
> 
> > Signed-off-by: Alexandru Ardelean 
> > ---
> >  drivers/iio/buffer/kfifo_buf.c | 37 ++
> >  include/linux/iio/kfifo_buf.h  |  4 
> >  2 files changed, 41 insertions(+)
> > 
> > diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> > index 3150f8ab984b..05b7c5fc6f1d 100644
> > --- a/drivers/iio/buffer/kfifo_buf.c
> > +++ b/drivers/iio/buffer/kfifo_buf.c
> > @@ -228,4 +228,41 @@ void devm_iio_kfifo_free(struct device *dev, struct
> > iio_buffer *r)
> >  }
> >  EXPORT_SYMBOL(devm_iio_kfifo_free);
> >  
> > +/**
> > + * iio_device_attach_kfifo_buffer - Allocate a kfifo buffer & attach it to
> > an IIO device
> > + * @indio_dev: The device the buffer should be attached to
> > + * @mode_flags: The mode flags for this buffer (INDIO_BUFFER_SOFTWARE
> > and/or
> > + * INDIO_BUFFER_TRIGGERED).
> > + * @setup_ops: The setup_ops required to configure the HW part of the
> > buffer (optional)
> > + *
> > + * This function allocates a kfifo buffer via devm_iio_kfifo_allocate() and
> > + * attaches it to the IIO device via iio_device_attach_buffer().
> > + * This is meant to be a bit of a short-hand/helper function as many driver
> > + * seem to do this.
> > + */
> > +int iio_device_attach_kfifo_buffer(struct iio_dev *indio_dev,
> > +  int mode_flags,
> > +  const struct iio_buffer_setup_ops *setup_ops)
> > +{
> > +   struct iio_buffer *buffer;
> > +
> > +   if (mode_flags)
> > +   mode_flags &= kfifo_access_funcs.modes;
> > +
> > +   if 

Re: [PATCH] staging: iio: adc: ad7192: Align with parenthesis

[Ardelean, Alexandru]

On Wed, 2020-03-04 at 00:09 +0530, Nishant Malpani wrote:
> [External]
> 
> This patch fixes the checkpatch.pl warning:
> 
> CHECK: Alignment should match open parenthesis
> +static void ad7192_get_available_filter_freq(struct ad7192_state *st,
> +   int *freq)
> 

This patch needs to be updated a bit.
The driver has moved out of staging.
Path is now: ./drivers/iio/adc/ad7192.c

> Signed-off-by: Nishant Malpani 
> ---
>  drivers/staging/iio/adc/ad7192.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7192.c
> b/drivers/staging/iio/adc/ad7192.c
> index bf3e2a9cc07f..20fe7464da7f 100644
> --- a/drivers/staging/iio/adc/ad7192.c
> +++ b/drivers/staging/iio/adc/ad7192.c
> @@ -477,7 +477,7 @@ static ssize_t ad7192_set(struct device *dev,
>  }
>  
>  static void ad7192_get_available_filter_freq(struct ad7192_state *st,
> - int *freq)
> +  int *freq)
>  {
>   unsigned int fadc;
>  
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240

[Ardelean, Alexandru]

On Wed, 2019-12-04 at 17:00 +, Mark Brown wrote:
> On Wed, Dec 04, 2019 at 07:18:15AM +0000, Ardelean, Alexandru wrote:
> 
> > One example (for spi-cpha):
> > if (of_property_read_u32(nc, "spi-cpha", ) == 0) {
> > spi->mode |= SPI_CPHA_OVERRIDE;
> > if (tmp)
> > spi->mode |= SPI_CPHA;
> 
> We could also do this with a separate flag saying that the wire
> format is forced from DT rather than having one per setting.
> 

I will admit that [since you seem to incline towards this approach], I am
also inclined to consider it over the spi-mode-conv driver.
And also since you're saying that the driver would be harder to achieve.

I'll see about an implementation for this flag and come back for a review
[on it].
[Until then] I also owe you some more "delay_usecs" cleanup in other
subsystems.

Thanks
Alex

> > Or maybe, a more complete solution would be an "spi-mode-conv" driver.
> > Similar to the fixed-factor-clock clk driver, which just does a
> > computation
> > based on values from the DT.
> > To tell the truth, this would be a great idea, because we have
> > something
> > like a passive 3-wire-to-4-wire HDL converter. This requires that the
> > driver be configured in 3-wire mode, the SPI controller in normal 4-
> > wire.
> > That's because the SPI framework does a validation of the supported
> > modes
> > (for the SPI controller) and invalidates what the device wants (which
> > is
> > very reasonable).
> 
> This is harder to achieve here because we don't have drivers for
> random bits of the wire format...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 1/2] staging: iio: accel: adis16240: enforce SPI mode on probe function

[Ardelean, Alexandru]

On Sun, 2019-12-01 at 11:42 +, Jonathan Cameron wrote:
> [External]
> 
> On Mon, 25 Nov 2019 07:55:39 +
> "Ardelean, Alexandru"  wrote:
> 
> > On Sat, 2019-11-23 at 20:35 -0300, Rodrigo Carvalho wrote:
> > > [External]
> > > 
> > > According to the datasheet, this driver supports only SPI mode 3,
> > > so we should enforce it and call spi_setup() on probe function.
> > > 
> > > Signed-off-by: Rodrigo Ribeiro Carvalho 
> > > ---
> > > V5:
> > >   - Add this patch to the patchset
> > > 
> > >  drivers/staging/iio/accel/adis16240.c | 7 +++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/staging/iio/accel/adis16240.c
> > > b/drivers/staging/iio/accel/adis16240.c
> > > index 82099db4bf0c..77b6b81767b9 100644
> > > --- a/drivers/staging/iio/accel/adis16240.c
> > > +++ b/drivers/staging/iio/accel/adis16240.c
> > > @@ -400,6 +400,13 @@ static int adis16240_probe(struct spi_device
> > > *spi)
> > >   indio_dev->num_channels = ARRAY_SIZE(adis16240_channels);
> > >   indio_dev->modes = INDIO_DIRECT_MODE;
> > >  
> > > + spi->mode = SPI_MODE_3;  
> > 
> > A generic question from me here, since I am not sure.
> > 
> > Would this limit the configurations of this chip on the board?
> > In case there is some level-inverter [for various weird reasons] on the
> > board, this may not work, because the SPI controller would need CPOL to
> > be
> > 0.
> > 
> > Not sure if this question is valid, or whether we need to care about
> > such
> > configurations.
> 
> It's a good question as this sort of trick is used sometimes. Let's see
> what responses we get to the other branch of this thread before moving
> forwards
> with this.
> 

Coming back here.
Apologies to Rodrigo. I do realize that I delayed this a bit too much.

Let's have this series as-is here, and then we can see about a more generic
SPI Mode Converter driver that rounds-up all these weird boards.
Or, if we don't do any SPI mode converter drivers, then we can handle this
on a case-by-case basis/driver.


> Jonathan
> 
> 
> > Thanks
> > Alex
> > 
> > > + ret = spi_setup(spi);
> > > + if (ret) {
> > > + dev_err(>dev, "spi_setup failed!\n");
> > > + return ret;
> > > + }
> > > +
> > >   ret = adis_init(st, indio_dev, spi, _data);
> > >   if (ret)
> > >   return ret;  
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240

[Ardelean, Alexandru]

On Tue, 2019-12-03 at 16:51 +, Jonathan Cameron wrote:
> On Tue, 3 Dec 2019 16:38:50 +
> Mark Brown  wrote:
> 
> > On Sun, Dec 01, 2019 at 11:40:32AM +, Jonathan Cameron wrote:
> > 
> > > +CC Mark as we probably need a more general view point on
> > > the question of whether SPI mode should be enforced by binding
> > > or in the driver.  
> > 
> > Not sure I see the question here, I think I was missing a bit of
> > the conversation?  It's perfectly fine for a driver to specify a
> > mode, if the hardware always uses some unusual mode then there's
> > no sense in forcing every single DT to set the same mode.  On the
> > other hand if there's some configuration for the driver that was
> > handling some board specific configuration that there's already
> > some generic SPI support for setting then it seems odd to have a
> > custom driver specific configuration mechanism.
> > 
> 
> If the driver picks a mode because that's what it says on the datasheet
> it prevents odd board configurations from working.  The question
> becomes whether it makes sense in general to assume those odd board
> conditions don't exist until we actually have one, or to assume that
> they might and push the burden on to all DT files.
> 
> Traditionally in IIO at least we've mostly taken the view the DT
> should be right and complete and had bindings state what normal
> parameters must be for it to work (assuming no inverters etc)
> 
> If we encode it in the driver, and we later meet such a board we
> end up with a custom dance to query the DT parameters again and
> only override if present.
> 
> We can't rely on the core SPI handling because I don't think
> there is any means of specifying a default.
> 
> We can adopt the view that in general these weird boards with inverters
> are weird and just handle them when they occur.  Sounds like that is your
> preference, at least for new parts.
> 
> For old ones we have no idea if there are boards out there using
> them with inverters so easiest is probably to just carry on putting them
> in the DT bindings.

There might be a few other options, which would require some SPI OF change.

One example (for spi-cpha):
if (of_property_read_u32(nc, "spi-cpha", ) == 0) {
spi->mode |= SPI_CPHA_OVERRIDE;
if (tmp)
spi->mode |= SPI_CPHA;
} else 
 if (of_property_read_bool(nc, "spi-cpha"))
spi->mode |= SPI_CPHA;

Or another option could be:
if (of_property_read_bool(nc, "spi-cpha-override")) {
spi->mode |= SPI_CPHA_OVERRIDE;
if
(of_property_read_bool(nc, "spi-cpha"))
spi->mode |= SPI_CPHA;


Naturally, this would require that spi_setup() checks SPI_CPHA_OVERRIDE and
doesn't set SPI_CPHA if SPI_CPHA_OVERRIDE is set. 

Or maybe, a more complete solution would be an "spi-mode-conv" driver.
Similar to the fixed-factor-clock clk driver, which just does a computation
based on values from the DT.

To tell the truth, this would be a great idea, because we have something
like a passive 3-wire-to-4-wire HDL converter. This requires that the
driver be configured in 3-wire mode, the SPI controller in normal 4-wire.
That's because the SPI framework does a validation of the supported modes
(for the SPI controller) and invalidates what the device wants (which is
very reasonable).

An "spi-mode-conv" driver would also handle this 3-wire-4-wire dance, and
the level inversions, and other (similar) things.

Thoughts?
Alex

> 
> Jonathan
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/4] iio: adc: Add support for AD7091R5 ADC

[Ardelean, Alexandru]

On Tue, 2019-10-29 at 18:29 +0200, Beniamin Bia wrote:
> [External]
> 
> From: Paul Cercueil 
> 
> AD7091 is 4-Channel, I2C, Ultra Low Power,12-Bit ADC.
> 
> Datasheet:
> Link: 
> https://www.analog.com/media/en/technical-documentation/data-sheets/ad7091r-5.pdf
> 
> Signed-off-by: Paul Cercueil 
> Co-developed-by: Beniamin Bia 
> Signed-off-by: Beniamin Bia 
> ---
> Changes in v2:
> -blank lines removed
> -prefix added to macros
> -comments rework
> -error checking syntax changed
> -iio mutex replaced by a mutex
> -device remove function was removed and later replaced by devm_add_action
> -regmap include removed from header
> 
>  drivers/iio/adc/Kconfig|   7 +
>  drivers/iio/adc/Makefile   |   1 +
>  drivers/iio/adc/ad7091r-base.c | 264 +
>  drivers/iio/adc/ad7091r-base.h |  25 
>  drivers/iio/adc/ad7091r5.c | 102 +
>  5 files changed, 399 insertions(+)
>  create mode 100644 drivers/iio/adc/ad7091r-base.c
>  create mode 100644 drivers/iio/adc/ad7091r-base.h
>  create mode 100644 drivers/iio/adc/ad7091r5.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7e3286265a38..80b1b9551749 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -22,6 +22,13 @@ config AD7124
> To compile this driver as a module, choose M here: the module
> will be
> called ad7124.
>  
> +config AD7091R5
> + tristate "Analog Devices AD7091R5 ADC Driver"
> + depends on I2C
> + select REGMAP_I2C
> + help
> +   Say yes here to build support for Analog Devices AD7091R-5 ADC.
> +

Sorry for the lateness here.
Is it too late to mention to put this before the AD7124 driver?
Same question for Makefile.

This is to keep things alphabetically sorted.

Thanks
Alex

>  config AD7266
>   tristate "Analog Devices AD7265/AD7266 ADC driver"
>   depends on SPI_MASTER
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index ef9cc485fb67..55e44735aaac 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -6,6 +6,7 @@
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
>  obj-$(CONFIG_AD7124) += ad7124.o
> +obj-$(CONFIG_AD7091R5) += ad7091r5.o ad7091r-base.o
>  obj-$(CONFIG_AD7266) += ad7266.o
>  obj-$(CONFIG_AD7291) += ad7291.o
>  obj-$(CONFIG_AD7298) += ad7298.o
> diff --git a/drivers/iio/adc/ad7091r-base.c b/drivers/iio/adc/ad7091r-
> base.c
> new file mode 100644
> index ..c2500f614d54
> --- /dev/null
> +++ b/drivers/iio/adc/ad7091r-base.c
> @@ -0,0 +1,264 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AD7091RX Analog to Digital converter driver
> + *
> + * Copyright 2014-2019 Analog Devices Inc.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "ad7091r-base.h"
> +
> +#define AD7091R_REG_RESULT  0
> +#define AD7091R_REG_CHANNEL 1
> +#define AD7091R_REG_CONF2
> +#define AD7091R_REG_ALERT   3
> +#define AD7091R_REG_CH_LOW_LIMIT(ch) ((ch) * 3 + 4)
> +#define AD7091R_REG_CH_HIGH_LIMIT(ch) ((ch) * 3 + 5)
> +#define AD7091R_REG_CH_HYSTERESIS(ch) ((ch) * 3 + 6)
> +
> +/* AD7091R_REG_RESULT */
> +#define AD7091R_REG_RESULT_CH_ID(x)  (((x) >> 13) & 0x3)
> +#define AD7091R_REG_RESULT_CONV_RESULT(x)   ((x) & 0xfff)
> +
> +/* AD7091R_REG_CONF */
> +#define AD7091R_REG_CONF_AUTO   BIT(8)
> +#define AD7091R_REG_CONF_CMDBIT(10)
> +
> +#define AD7091R_REG_CONF_MODE_MASK  \
> + (AD7091R_REG_CONF_AUTO | AD7091R_REG_CONF_CMD)
> +
> +enum ad7091r_mode {
> + AD7091R_MODE_SAMPLE,
> + AD7091R_MODE_COMMAND,
> + AD7091R_MODE_AUTOCYCLE,
> +};
> +
> +struct ad7091r_state {
> + struct device *dev;
> + struct regmap *map;
> + const struct ad7091r_chip_info *chip_info;
> + enum ad7091r_mode mode;
> + struct mutex lock;
> +};
> +
> +static int ad7091r_set_mode(struct ad7091r_state *st, enum ad7091r_mode
> mode)
> +{
> + int ret;
> +
> + switch (mode) {
> + case AD7091R_MODE_SAMPLE:
> + ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
> +  AD7091R_REG_CONF_MODE_MASK, 0);
> + break;
> + case AD7091R_MODE_COMMAND:
> + ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
> +  AD7091R_REG_CONF_MODE_MASK,
> +  AD7091R_REG_CONF_CMD);
> + break;
> + case AD7091R_MODE_AUTOCYCLE:
> + ret = regmap_update_bits(st->map, AD7091R_REG_CONF,
> +  AD7091R_REG_CONF_MODE_MASK,
> +  AD7091R_REG_CONF_AUTO);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (ret)
> + return ret;
> +
> + st->mode = mode;
> +
> + return ret;
> +}
> +
> +static int ad7091r_set_channel(struct 

Re: [PATCH v5 1/2] staging: iio: accel: adis16240: enforce SPI mode on probe function

[Ardelean, Alexandru]

On Sat, 2019-11-23 at 20:35 -0300, Rodrigo Carvalho wrote:
> [External]
> 
> According to the datasheet, this driver supports only SPI mode 3,
> so we should enforce it and call spi_setup() on probe function.
> 
> Signed-off-by: Rodrigo Ribeiro Carvalho 
> ---
> V5:
>   - Add this patch to the patchset
> 
>  drivers/staging/iio/accel/adis16240.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/staging/iio/accel/adis16240.c
> b/drivers/staging/iio/accel/adis16240.c
> index 82099db4bf0c..77b6b81767b9 100644
> --- a/drivers/staging/iio/accel/adis16240.c
> +++ b/drivers/staging/iio/accel/adis16240.c
> @@ -400,6 +400,13 @@ static int adis16240_probe(struct spi_device *spi)
>   indio_dev->num_channels = ARRAY_SIZE(adis16240_channels);
>   indio_dev->modes = INDIO_DIRECT_MODE;
>  
> + spi->mode = SPI_MODE_3;

A generic question from me here, since I am not sure.

Would this limit the configurations of this chip on the board?
In case there is some level-inverter [for various weird reasons] on the
board, this may not work, because the SPI controller would need CPOL to be
0.

Not sure if this question is valid, or whether we need to care about such
configurations.

Thanks
Alex

> + ret = spi_setup(spi);
> + if (ret) {
> + dev_err(>dev, "spi_setup failed!\n");
> + return ret;
> + }
> +
>   ret = adis_init(st, indio_dev, spi, _data);
>   if (ret)
>   return ret;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240

[Ardelean, Alexandru]

On Sat, 2019-11-23 at 11:41 +, Jonathan Cameron wrote:
> On Sat, 23 Nov 2019 02:19:27 -0300
> Rodrigo Carvalho  wrote:
> 
> > This patch add device tree binding documentation for ADIS16240.
> > 
> > Signed-off-by: Rodrigo Ribeiro Carvalho 

My bad for the late timing on this.
I'm slightly more fresh on Mondays.
But I will get overloaded with work in a few hours, so I may not have time
ot respond.

> No problem with this patch, but I definitely want to see an accompanying
> one enforcing the SPI mode in the driver.
> 

So, then the binding should probably also define spi-cpol & spi-cpha
as mandatory.
Maybe, the driver would do a check and print a warning.

I'm noticing that this device uses SPI mode 3, but this DT binding defaults
to SPI mode 0

> Right now the driver doesn't set it and so I'm fairly sure not putting
> it in the binding will leave you with a non working device.
> 
> The right option if only one option is supported is for the driver
> to call spi_setup with the relevant options.
> 

What if the board uses some level inverters [because of some weird reason]
and that messes up with the SPI mode?
It's not common, but it is possible.

> Thanks,
> 
> Jonathan
> 
> > ---
> > V4:
> >- Remove spi-cpha and spi-cpol in binding example, since this driver
> > supports only one timing mode.
> >  .../bindings/iio/accel/adi,adis16240.yaml | 49 +++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > new file mode 100644
> > index ..8e902f7c49e6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ADIS16240 Programmable Impact Sensor and Recorder driver
> > +
> > +maintainers:
> > +  - Alexandru Ardelean 
> > +
> > +description: |
> > +  ADIS16240 Programmable Impact Sensor and Recorder driver that
> > supports
> > +  SPI interface.
> > +https://www.analog.com/en/products/adis16240.html
> > +
> > +properties:
> > +  compatible:
> > +enum:
> > +  - adi,adis16240
> > +
> > +  reg:
> > +maxItems: 1
> > +
> > +  interrupts:
> > +maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +examples:
> > +  - |
> > +#include 
> > +#include 
> > +spi0 {
> > +#address-cells = <1>;
> > +#size-cells = <0>;
> > +
> > +/* Example for a SPI device node */
> > +accelerometer@0 {
> > +compatible = "adi,adis16240";
> > +reg = <0>;
> > +spi-max-frequency = <250>;
> > +interrupt-parent = <>;
> > +interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > +};
> > +};
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad9834: add a check for devm_clk_get

[Ardelean, Alexandru]

On Wed, 2019-10-16 at 22:25 +0800, Chuhong Yuan wrote:
> ad9834_probe misses a check for devm_clk_get and may cause problems.
> Add a check like what ad9832 does to fix it.
> 

This could also use a Fixes tag, but not a big deal.

Reviewed-by: Alexandru Ardelean 

> Signed-off-by: Chuhong Yuan 
> ---
>  drivers/staging/iio/frequency/ad9834.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/staging/iio/frequency/ad9834.c
> b/drivers/staging/iio/frequency/ad9834.c
> index 038d6732c3fd..23026978a5a5 100644
> --- a/drivers/staging/iio/frequency/ad9834.c
> +++ b/drivers/staging/iio/frequency/ad9834.c
> @@ -417,6 +417,10 @@ static int ad9834_probe(struct spi_device *spi)
>   st = iio_priv(indio_dev);
>   mutex_init(>lock);
>   st->mclk = devm_clk_get(>dev, NULL);
> + if (IS_ERR(st->mclk)) {
> + ret = PTR_ERR(st->mclk);
> + goto error_disable_reg;
> + }
>  
>   ret = clk_prepare_enable(st->mclk);
>   if (ret) {
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] dt-bindings: iio: accel: add binding documentation for ADIS16240

[Ardelean, Alexandru]

On Thu, 2019-09-12 at 18:39 -0300, Rodrigo Carvalho wrote:
> This patch add device tree binding documentation for ADIS16240.
> 
> Signed-off-by: Rodrigo Ribeiro Carvalho 
> ---
> V2:
>   - Remove true constant for spi-cpha and spi-cpol
>   - Add description field for spi-cpha and spi-cpol
>   - Add maxItems field for spi-cpha and spi-cpol
> 
>  .../bindings/iio/accel/adi,adis16240.yaml | 61 +++
>  1 file changed, 61 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> new file mode 100644
> index ..4b1bd2419604
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adis16240.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/accel/adi,adis16240.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADIS16240 Programmable Impact Sensor and Recorder driver
> +
> +maintainers:
> +  - Alexandru Ardelean 
> +
> +description: |
> +  ADIS16240 Programmable Impact Sensor and Recorder driver that supports
> +  SPI interface.
> +https://www.analog.com/en/products/adis16240.html
> +
> +properties:
> +  compatible:
> +enum:
> +  - adi,adis16240
> +
> +  reg:
> +maxItems: 1
> +
> +  spi-cpha:
> +description: |
> +  See Documentation/devicetree/bindings/spi/spi-controller.yaml
> +maxItems: 1

Description for standard properties is not required.

For spi-cpha/cpol just "true" seems sufficient.

So

 spi-cpha: true

 spi-cpol: true

> +
> +  spi-cpol: |
> +description: |
> +  See Documentation/devicetree/bindings/spi/spi-controller.yaml
> +maxItems: 1
> +
> +  interrupts:
> +maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +

If spi-cpha & spi-cpol are true, they should typically be also required.
Though, I think Rob would answer things better here.

> +examples:
> +  - |
> +#include 
> +#include 
> +spi0 {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +/* Example for a SPI device node */
> +accelerometer@0 {
> +compatible = "adi,adis16240";
> +reg = <0>;
> +spi-max-frequency = <250>;
> +spi-cpol;
> +spi-cpha;
> +interrupt-parent = <>;
> +interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +};
> +};
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: iio: accel: adis16240: Improve readability on write_raw function

[Ardelean, Alexandru]

On Tue, 2019-08-13 at 16:31 -0300, Rodrigo Ribeiro wrote:
> [External]
> 
> Replace shift and minus operation by GENMASK macro and remove the local
> variables used to store intermediate data.
> 

Reviewed-by: Alexandru Ardelean 

> Signed-off-by: Rodrigo Ribeiro Carvalho 
> ---
> v2:
>- Leave switch statement instead of replace by if statement
>  drivers/staging/iio/accel/adis16240.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/accel/adis16240.c 
> b/drivers/staging/iio/accel/adis16240.c
> index 62f4b3b1b457..82099db4bf0c 100644
> --- a/drivers/staging/iio/accel/adis16240.c
> +++ b/drivers/staging/iio/accel/adis16240.c
> @@ -309,15 +309,12 @@ static int adis16240_write_raw(struct iio_dev 
> *indio_dev,
>  long mask)
>  {
>   struct adis *st = iio_priv(indio_dev);
> - int bits = 10;
> - s16 val16;
>   u8 addr;
>  
>   switch (mask) {
>   case IIO_CHAN_INFO_CALIBBIAS:
> - val16 = val & ((1 << bits) - 1);
>   addr = adis16240_addresses[chan->scan_index][0];
> - return adis_write_reg_16(st, addr, val16);
> + return adis_write_reg_16(st, addr, val & GENMASK(9, 0));
>   }
>   return -EINVAL;
>  }
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad2s1210: Use device-managed API

[Ardelean, Alexandru]

On Fri, 2019-07-26 at 19:07 +0800, Chuhong Yuan wrote:
> [External]
> 
> Use device-managed API to simplify the code.
> The remove function is redundant now and can
> be deleted.

Reviewed-by: Alexandru Ardelean 

> 
> Signed-off-by: Chuhong Yuan 
> ---
>  drivers/staging/iio/resolver/ad2s1210.c | 12 +---
>  1 file changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c 
> b/drivers/staging/iio/resolver/ad2s1210.c
> index 0c1bd108c386..4b25a3a314ed 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -671,7 +671,7 @@ static int ad2s1210_probe(struct spi_device *spi)
>   indio_dev->num_channels = ARRAY_SIZE(ad2s1210_channels);
>   indio_dev->name = spi_get_device_id(spi)->name;
>  
> - ret = iio_device_register(indio_dev);
> + ret = devm_iio_device_register(>dev, indio_dev);
>   if (ret)
>   return ret;
>  
> @@ -683,15 +683,6 @@ static int ad2s1210_probe(struct spi_device *spi)
>   return 0;
>  }
>  
> -static int ad2s1210_remove(struct spi_device *spi)
> -{
> - struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -
> - iio_device_unregister(indio_dev);
> -
> - return 0;
> -}
> -
>  static const struct of_device_id ad2s1210_of_match[] = {
>   { .compatible = "adi,ad2s1210", },
>   { }
> @@ -710,7 +701,6 @@ static struct spi_driver ad2s1210_driver = {
>   .of_match_table = of_match_ptr(ad2s1210_of_match),
>   },
>   .probe = ad2s1210_probe,
> - .remove = ad2s1210_remove,
>   .id_table = ad2s1210_id,
>  };
>  module_spi_driver(ad2s1210_driver);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging:iio:adc:ad7280a: add of_match_table entry

[Ardelean, Alexandru]

On Fri, 2019-07-26 at 01:38 +0530, Kartik Kulkarni wrote:
> Add the of_device_id struct and the respective
> of_match_device entry to complete device tree support.
> 

This would be a [V2] I suppose.

This change also does the rename of the driver name in a single go.
Since it's a trivial change, it's fine from my side.

Reviewed-by: Alexandru Ardelean 


> Signed-off-by: Kartik Kulkarni 
> Reviewed-by: Matheus Tavares 
> ---
>  drivers/staging/iio/adc/ad7280a.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7280a.c 
> b/drivers/staging/iio/adc/ad7280a.c
> index 19a5f244dcae..ded0ba093a28 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -1027,9 +1027,16 @@ static const struct spi_device_id ad7280_id[] = {
>  };
>  MODULE_DEVICE_TABLE(spi, ad7280_id);
>  
> +static const struct of_device_id ad7280_of_match[] = {
> + { .compatible = "adi,ad7280a", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad7280_of_match);
> +
>  static struct spi_driver ad7280_driver = {
>   .driver = {
> - .name   = "ad7280",
> + .name   = "ad7280a",
> + .of_match_table = ad7280_of_match,
>   },
>   .probe  = ad7280_probe,
>   .id_table   = ad7280_id,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] dt-bindings: iio: adc: add adi,ad7780.yaml binding

[Ardelean, Alexandru]

On Wed, 2019-06-05 at 17:35 -0300, Renato Lui Geh wrote:
> [External]
> 
> 
> On 05/26, Jonathan Cameron wrote:
> > On Fri, 24 May 2019 22:26:30 -0300
> > Renato Lui Geh  wrote:
> > 
> > > This patch adds a YAML binding for the Analog Devices AD7780/1 and
> > > AD7170/1 analog-to-digital converters.
> > > 
> > > Signed-off-by: Renato Lui Geh 
> > Looks good to me, but I'm still finding my feet with these so will
> > leave it for a few days for others to have time to comment.
> > 
> > Michael, looking for a quick reply from you to say if you are happy
> > being explicitly listed as maintainer for this one, or if you'd
> > rather land it on someone else.  Same applies for patch 2.
> > 
> > Renato, if I seem to have forgotten this in a week or so, feel
> > free to give me a poke. I've been known to loose patches entirely!
> 
> Hi Jonathan,
> 
> Just here to give you a poke. :)
> 
> By the way, in these cases, which would be easier for you? To send you
> an email like I'm doing right now on last week's thread; or to resend
> the entire patch(set)?
> 

I think in this case, maybe let's wait a bit longer.
Jonathan has not been active recently.

I think a [RESEND] would be a good idea when he gets back/active and misses 
your patchset.

Thanks
Alex

> Thanks,
> Renato
> > Thanks,
> > 
> > Jonathan
> > > ---
> > > Changes in v2:
> > >  - vref-supply to avdd-supply
> > >  - remove avdd-supply from required list
> > >  - include adc block in an spi block
> > > 
> > >  .../bindings/iio/adc/adi,ad7780.txt   | 48 --
> > >  .../bindings/iio/adc/adi,ad7780.yaml  | 87 +++
> > >  2 files changed, 87 insertions(+), 48 deletions(-)
> > >  delete mode 100644 
> > > Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> > >  create mode 100644 
> > > Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> > > b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> > > deleted file mode 100644
> > > index 440e52555349..
> > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> > > +++ /dev/null
> > > @@ -1,48 +0,0 @@
> > > -* Analog Devices AD7170/AD7171/AD7780/AD7781
> > > -
> > > -Data sheets:
> > > -
> > > -- AD7170:
> > > -* 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7170.pdf
> > > -- AD7171:
> > > -* 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7171.pdf
> > > -- AD7780:
> > > -* 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ad7780.pdf
> > > -- AD7781:
> > > -* 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7781.pdf
> > > -
> > > -Required properties:
> > > -
> > > -- compatible: should be one of
> > > -* "adi,ad7170"
> > > -* "adi,ad7171"
> > > -* "adi,ad7780"
> > > -* "adi,ad7781"
> > > -- reg: spi chip select number for the device
> > > -- vref-supply: the regulator supply for the ADC reference voltage
> > > -
> > > -Optional properties:
> > > -
> > > -- powerdown-gpios:  must be the device tree identifier of the PDRST pin. 
> > > If
> > > -specified, it will be asserted during driver probe. As 
> > > the
> > > -line is active high, it should be marked 
> > > GPIO_ACTIVE_HIGH.
> > > -- adi,gain-gpios:   must be the device tree identifier of the GAIN pin. 
> > > Only for
> > > -the ad778x chips. If specified, it will be asserted 
> > > during
> > > -driver probe. As the line is active low, it should be 
> > > marked
> > > -GPIO_ACTIVE_LOW.
> > > -- adi,filter-gpios: must be the device tree identifier of the FILTER 
> > > pin. Only
> > > -for the ad778x chips. If specified, it will be asserted
> > > -during driver probe. As the line is active low, it 
> > > should be
> > > -marked GPIO_ACTIVE_LOW.
> > > -
> > > -Example:
> > > -
> > > -adc@0 {
> > > -compatible =  "adi,ad7780";
> > > -reg = <0>;
> > > -vref-supply = <_supply>
> > > -
> > > -powerdown-gpios  = < 12 GPIO_ACTIVE_HIGH>;
> > > -adi,gain-gpios   = <  5 GPIO_ACTIVE_LOW>;
> > > -adi,filter-gpios = < 15 GPIO_ACTIVE_LOW>;
> > > -};
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> > > b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> > > new file mode 100644
> > > index ..d1109416963c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> > > @@ -0,0 +1,87 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/adc/adi,ad7780.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Analog Devices AD7170/AD7171/AD7780/AD7781 analog to digital 
> > > converters
> > > +
> > > +maintainers:
> > > +  - Michael 

Re: [PATCH] staging: iio: adis16240: add of_match_table entry

[Ardelean, Alexandru]

On Fri, 2019-05-24 at 10:50 -0300, Marcelo Schmitt wrote:
> [External]
> 
> 
> Hi Alexandru,
> 
> On 05/24, Alexandru Ardelean wrote:
> > On Fri, May 24, 2019 at 6:30 AM Rodrigo Ribeiro  
> > wrote:
> > > 
> > > This patch adds of_match_table entry in device driver in order to
> > > enable spi fallback probing.
> > > 
> > > Signed-off-by: Rodrigo Ribeiro 
> > > Reviewed-by: Marcelo Schmitt 
> > > ---
> > >  drivers/staging/iio/accel/adis16240.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/staging/iio/accel/adis16240.c 
> > > b/drivers/staging/iio/accel/adis16240.c
> > > index 8c6d23604eca..b80c8529784b 100644
> > > --- a/drivers/staging/iio/accel/adis16240.c
> > > +++ b/drivers/staging/iio/accel/adis16240.c
> > > @@ -444,6 +444,7 @@ MODULE_DEVICE_TABLE(of, adis16240_of_match);
> > >  static struct spi_driver adis16240_driver = {
> > > .driver = {
> > > .name = "adis16240",
> > > +   .of_match_table = adis16240_of_match,
> > 
> > This patch is missing the actual table.
> 
> Struct with compatible devices table was included separately in a
> previous patch at commit d9e533b6c0a26c7ef8116b7f3477c164c07bb6fb.
> Yeah, I also thought it was missing the match table the first time I was
> this patch. It's really confusing when we have two patches, one
> depending on another, that are not part of the same patch set. We're
> trying to avoid things like this the most but that slipped out from our
> internal review. We're sorry about that.

No worries.

It happens to me too.

Thanks
Alex

> 
> > 
> > > },
> > > .probe = adis16240_probe,
> > > .remove = adis16240_remove,
> > > --
> > > 2.20.1
> > > 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] dt-bindings: iio: adc: add adi,ad7780.yaml binding

[Ardelean, Alexandru]

On Sun, 2019-05-19 at 12:32 +0100, Jonathan Cameron wrote:
> [External]
> 
> 
> On Sat, 18 May 2019 19:41:12 -0300
> Renato Lui Geh  wrote:
> 
> > This patch adds a YAML binding for the Analog Devices AD7780/1 and
> > AD7170/1 analog-to-digital converters.
> > 
> > Signed-off-by: Renato Lui Geh 
> 
> One comment inline.  I'll also be needing an ack from Analog on this,
> preferably Michael's.
> 
> Thanks,
> 
> Jonathan
> > ---
> >  .../bindings/iio/adc/adi,ad7780.txt   | 48 ---
> >  .../bindings/iio/adc/adi,ad7780.yaml  | 85 +++

You should also update the MAINTAINERS file.
Maybe in a following patch.
It looks like there is not entry in there, so maybe you need to add a new
one.

Something like:


ANALOG DEVICES INC AD7780 DRIVER
M:  Michael Hennerich 
M:  Renato Lui Geh 
L:  linux-...@vger.kernel.org
W:  http://ez.analog.com/community/linux-device-drivers
S:  Supported
F:  drivers/iio/adc/ad7780.c
F:  Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml

This should be after this block
ANALOG DEVICES INC AD7768-1 DRIVER

Note that I added you as a co-maintainer.
If you want, you do not need to add that line.

> >  2 files changed, 85 insertions(+), 48 deletions(-)
> >  delete mode 100644
> > Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> >  create mode 100644
> > Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> > b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> > deleted file mode 100644
> > index 440e52555349..
> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> > +++ /dev/null
> > @@ -1,48 +0,0 @@
> > -* Analog Devices AD7170/AD7171/AD7780/AD7781
> > -
> > -Data sheets:
> > -
> > -- AD7170:
> > - * 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7170.pdf
> > -- AD7171:
> > - * 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7171.pdf
> > -- AD7780:
> > - * 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ad7780.pdf
> > -- AD7781:
> > - * 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7781.pdf
> > -
> > -Required properties:
> > -
> > -- compatible: should be one of
> > - * "adi,ad7170"
> > - * "adi,ad7171"
> > - * "adi,ad7780"
> > - * "adi,ad7781"
> > -- reg: spi chip select number for the device
> > -- vref-supply: the regulator supply for the ADC reference voltage
> > -
> > -Optional properties:
> > -
> > -- powerdown-gpios:  must be the device tree identifier of the PDRST
> > pin. If
> > - specified, it will be asserted during driver probe.
> > As the
> > - line is active high, it should be marked
> > GPIO_ACTIVE_HIGH.
> > -- adi,gain-gpios:   must be the device tree identifier of the GAIN
> > pin. Only for
> > - the ad778x chips. If specified, it will be asserted
> > during
> > - driver probe. As the line is active low, it should be
> > marked
> > - GPIO_ACTIVE_LOW.
> > -- adi,filter-gpios: must be the device tree identifier of the FILTER
> > pin. Only
> > - for the ad778x chips. If specified, it will be
> > asserted
> > - during driver probe. As the line is active low, it
> > should be
> > - marked GPIO_ACTIVE_LOW.
> > -
> > -Example:
> > -
> > -adc@0 {
> > - compatible =  "adi,ad7780";
> > - reg = <0>;
> > - vref-supply = <_supply>
> > -
> > - powerdown-gpios  = < 12 GPIO_ACTIVE_HIGH>;
> > - adi,gain-gpios   = <  5 GPIO_ACTIVE_LOW>;
> > - adi,filter-gpios = < 15 GPIO_ACTIVE_LOW>;
> > -};
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> > b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> > new file mode 100644
> > index ..931bc4f8ec04
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> > @@ -0,0 +1,85 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/adi,ad7780.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices AD7170/AD7171/AD7780/AD7781 analog to digital
> > converters
> > +
> > +maintainers:
> > +  - Michael Hennerich 
> > +
> > +description: |
> > +  The ad7780 is a sigma-delta analog to digital converter. This driver
> > provides
> > +  reading voltage values and status bits from both the ad778x and
> > ad717x series.
> > +  Its interface also allows writing on the FILTER and GAIN GPIO pins
> > on the
> > +  ad778x.
> > +
> > +  Specifications on the converters can be found at:
> > +AD7170:
> > +  
> > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7170.pdf
> > +AD7171:
> > +  
> > 

Re: [PATCH 03/16] lib,treewide: add new match_string() helper/macro

[Ardelean, Alexandru]

On Fri, 2019-05-10 at 17:34 +0300, andriy.shevche...@linux.intel.com wrote:
> [External]
> 
> 
> On Fri, May 10, 2019 at 09:15:27AM +0000, Ardelean, Alexandru wrote:
> > On Wed, 2019-05-08 at 16:22 +0300, Alexandru Ardelean wrote:
> > > On Wed, 2019-05-08 at 15:18 +0200, Greg KH wrote:
> > > > On Wed, May 08, 2019 at 04:11:28PM +0300, Andy Shevchenko wrote:
> > > > > On Wed, May 08, 2019 at 02:28:29PM +0300, Alexandru Ardelean
> > > > > wrote:
> > > > > Can you split include/linux/ change from the rest?
> > > > 
> > > > That would break the build, why do you want it split out?  This
> > > > makes
> > > > sense all as a single patch to me.
> > > > 
> > > 
> > > Not really.
> > > It would be just be the new match_string() helper/macro in a new
> > > commit.
> > > And the conversions of the simple users of match_string() (the ones
> > > using
> > > ARRAY_SIZE()) in another commit.
> > > 
> > 
> > I should have asked in my previous reply.
> > Leave this as-is or re-formulate in 2 patches ?
> 
> Depends on on what you would like to spend your time: collecting Acks for
> all
> pieces in treewide patch or send new API first followed up by per driver
> /
> module update in next cycle.

I actually would have preferred new API first, with the current
`match_string()` -> `__match_string()` rename from the start, but I wasn't
sure. I am still navigating through how feedbacks are working in this
realm.

I'll send a V2 with the API change-first/only; should be a smaller list.
Then see about follow-ups/changes per subsystems.

> 
> I also have no strong preference.
> And I think it's good to add Heikki Krogerus to Cc list for both patch
> series,
> since he is the author of sysfs variant and may have something to comment
> on
> the rest.

Thanks for the reference.

> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 09/16] mmc: sdhci-xenon: use new match_string() helper/macro

[Ardelean, Alexandru]

On Fri, 2019-05-10 at 14:01 +0300, Dan Carpenter wrote:
> [External]
> 
> 
> On Fri, May 10, 2019 at 09:13:26AM +0000, Ardelean, Alexandru wrote:
> > On Wed, 2019-05-08 at 16:26 +0300, Alexandru Ardelean wrote:
> > > On Wed, 2019-05-08 at 15:20 +0300, Dan Carpenter wrote:
> > > > 
> > > > 
> > > > On Wed, May 08, 2019 at 02:28:35PM +0300, Alexandru Ardelean wrote:
> > > > > -static const char * const phy_types[] = {
> > > > > - "emmc 5.0 phy",
> > > > > - "emmc 5.1 phy"
> > > > > -};
> > > > > -
> > > > >  enum xenon_phy_type_enum {
> > > > >   EMMC_5_0_PHY,
> > > > >   EMMC_5_1_PHY,
> > > > >   NR_PHY_TYPES
> > > > 
> > > > There is no need for NR_PHY_TYPES now so you could remove that as
> > > > well.
> > > > 
> > > 
> > > I thought the same.
> > > The only reason to keep NR_PHY_TYPES, is for potential future
> > > patches,
> > > where it would be just 1 addition
> > > 
> > >  enum xenon_phy_type_enum {
> > >   EMMC_5_0_PHY,
> > >   EMMC_5_1_PHY,
> > > +  EMMC_5_2_PHY,
> > >   NR_PHY_TYPES
> > >   }
> > > 
> > > Depending on style/preference of how to do enums (allow comma on last
> > > enum
> > > or not allow comma on last enum value), adding new enum values woudl
> > > be 2
> > > additions + 1 deletion lines.
> > > 
> > >  enum xenon_phy_type_enum {
> > >   EMMC_5_0_PHY,
> > > -  EMMC_5_1_PHY
> > > +  EMM
> > > C_5_1_PHY,
> > > +  EMMC_5_2_PHY
> > >  }
> > > 
> > > Either way (leave NR_PHY_TYPES or remove NR_PHY_TYPES) is fine from
> > > my
> > > side.
> > > 
> > 
> > Preference on this ?
> > If no objection [nobody insists] I would keep.
> > 
> > I don't feel strongly about it [dropping NR_PHY_TYPES or not].
> 
> If you end up resending the series could you remove it, but if not then
> it's not worth it.

ack

thanks
Alex

> 
> regards,
> dan carpenter
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/16] lib,treewide: add new match_string() helper/macro

[Ardelean, Alexandru]

On Wed, 2019-05-08 at 16:22 +0300, Alexandru Ardelean wrote:
> On Wed, 2019-05-08 at 15:18 +0200, Greg KH wrote:
> > 
> > 
> > On Wed, May 08, 2019 at 04:11:28PM +0300, Andy Shevchenko wrote:
> > > On Wed, May 08, 2019 at 02:28:29PM +0300, Alexandru Ardelean wrote:
> > > > This change re-introduces `match_string()` as a macro that uses
> > > > ARRAY_SIZE() to compute the size of the array.
> > > > The macro is added in all the places that do
> > > > `match_string(_a, ARRAY_SIZE(_a), s)`, since the change is pretty
> > > > straightforward.
> > > 
> > > Can you split include/linux/ change from the rest?
> > 
> > That would break the build, why do you want it split out?  This makes
> > sense all as a single patch to me.
> > 
> 
> Not really.
> It would be just be the new match_string() helper/macro in a new commit.
> And the conversions of the simple users of match_string() (the ones using
> ARRAY_SIZE()) in another commit.
> 

I should have asked in my previous reply.
Leave this as-is or re-formulate in 2 patches ?

No strong preference from my side.

Thanks
Alex

> Thanks
> Alex
> 
> > thanks,
> > 
> > greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 09/16] mmc: sdhci-xenon: use new match_string() helper/macro

[Ardelean, Alexandru]

On Wed, 2019-05-08 at 16:26 +0300, Alexandru Ardelean wrote:
> On Wed, 2019-05-08 at 15:20 +0300, Dan Carpenter wrote:
> > 
> > 
> > On Wed, May 08, 2019 at 02:28:35PM +0300, Alexandru Ardelean wrote:
> > > -static const char * const phy_types[] = {
> > > - "emmc 5.0 phy",
> > > - "emmc 5.1 phy"
> > > -};
> > > -
> > >  enum xenon_phy_type_enum {
> > >   EMMC_5_0_PHY,
> > >   EMMC_5_1_PHY,
> > >   NR_PHY_TYPES
> > 
> > There is no need for NR_PHY_TYPES now so you could remove that as well.
> > 
> 
> I thought the same.
> The only reason to keep NR_PHY_TYPES, is for potential future patches,
> where it would be just 1 addition
> 
>  enum xenon_phy_type_enum {
>   EMMC_5_0_PHY,
>   EMMC_5_1_PHY,
> +  EMMC_5_2_PHY,
>   NR_PHY_TYPES
>   }
> 
> Depending on style/preference of how to do enums (allow comma on last
> enum
> or not allow comma on last enum value), adding new enum values woudl be 2
> additions + 1 deletion lines.
> 
>  enum xenon_phy_type_enum {
>   EMMC_5_0_PHY,
> -  EMMC_5_1_PHY
> +  EMM
> C_5_1_PHY,
> +  EMMC_5_2_PHY
>  }
> 
> Either way (leave NR_PHY_TYPES or remove NR_PHY_TYPES) is fine from my
> side.
> 

Preference on this ?
If no objection [nobody insists] I would keep.

I don't feel strongly about it [dropping NR_PHY_TYPES or not].

Thanks
Alex

> Thanks
> Alex
> 
> > regards,
> > dan carpenter
> > 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 09/16] mmc: sdhci-xenon: use new match_string() helper/macro

[Ardelean, Alexandru]

On Wed, 2019-05-08 at 15:20 +0300, Dan Carpenter wrote:
> 
> 
> On Wed, May 08, 2019 at 02:28:35PM +0300, Alexandru Ardelean wrote:
> > -static const char * const phy_types[] = {
> > - "emmc 5.0 phy",
> > - "emmc 5.1 phy"
> > -};
> > -
> >  enum xenon_phy_type_enum {
> >   EMMC_5_0_PHY,
> >   EMMC_5_1_PHY,
> >   NR_PHY_TYPES
> 
> There is no need for NR_PHY_TYPES now so you could remove that as well.
> 

I thought the same.
The only reason to keep NR_PHY_TYPES, is for potential future patches,
where it would be just 1 addition

 enum xenon_phy_type_enum {
  EMMC_5_0_PHY,
  EMMC_5_1_PHY,
+  EMMC_5_2_PHY,
  NR_PHY_TYPES
  }

Depending on style/preference of how to do enums (allow comma on last enum
or not allow comma on last enum value), adding new enum values woudl be 2
additions + 1 deletion lines.

 enum xenon_phy_type_enum {
  EMMC_5_0_PHY,
-  EMMC_5_1_PHY
+  EMM
C_5_1_PHY,
+  EMMC_5_2_PHY
 }

Either way (leave NR_PHY_TYPES or remove NR_PHY_TYPES) is fine from my
side.

Thanks
Alex

> regards,
> dan carpenter
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/16] lib,treewide: add new match_string() helper/macro

[Ardelean, Alexandru]

On Wed, 2019-05-08 at 15:18 +0200, Greg KH wrote:
> 
> 
> On Wed, May 08, 2019 at 04:11:28PM +0300, Andy Shevchenko wrote:
> > On Wed, May 08, 2019 at 02:28:29PM +0300, Alexandru Ardelean wrote:
> > > This change re-introduces `match_string()` as a macro that uses
> > > ARRAY_SIZE() to compute the size of the array.
> > > The macro is added in all the places that do
> > > `match_string(_a, ARRAY_SIZE(_a), s)`, since the change is pretty
> > > straightforward.
> > 
> > Can you split include/linux/ change from the rest?
> 
> That would break the build, why do you want it split out?  This makes
> sense all as a single patch to me.
> 

Not really.
It would be just be the new match_string() helper/macro in a new commit.
And the conversions of the simple users of match_string() (the ones using
ARRAY_SIZE()) in another commit.

Thanks
Alex

> thanks,
> 
> greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/16] lib: fix match_string() helper on -1 array size

[Ardelean, Alexandru]

On Wed, 2019-05-08 at 14:28 +0300, Alexandru Ardelean wrote:
> The documentation the `_match_string()` helper mentions that `n`
> should be:
>  * @n: number of strings in the array or -1 for NULL terminated arrays
> 
> The behavior of the function is different, in the sense that it exits on
> the first NULL element in the array, regardless of whether `n` is -1 or a
> positive number.
> 
> This patch changes the behavior, to exit the loop when a NULL element is
> found and n == -1. Essentially, this aligns the behavior with the
> doc-string.
> 
> There are currently many users of `match_string()`, and so, in order to
> go
> through them, the next patches in the series will focus on doing some
> cosmetic changes, which are aimed at grouping the users of
> `match_string()`.
> 

This is the duplicate & should be dropped.
Sorry for this.

> Signed-off-by: Alexandru Ardelean 
> ---
>  lib/string.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/string.c b/lib/string.c
> index 3ab861c1a857..76edb7bf76cb 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -648,8 +648,11 @@ int match_string(const char * const *array, size_t
> n, const char *string)
>  
>   for (index = 0; index < n; index++) {
>   item = array[index];
> - if (!item)
> + if (!item) {
> + if (n != (size_t)-1)
> + continue;
>   break;
> + }
>   if (!strcmp(item, string))
>   return index;
>   }
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] staging: iio: ad7150: use FIELD_GET and GENMASK

[Ardelean, Alexandru]

On Tue, 2019-05-07 at 17:44 -0300, Melissa Wen wrote:
> [External]
> 
> 
> On 05/06, Ardelean, Alexandru wrote:
> > On Sat, 2019-05-04 at 13:43 +0300, Alexandru Ardelean wrote:
> > > [External]
> > > 
> > > 
> > > On Sat, May 4, 2019 at 1:25 AM Melissa Wen 
> > > wrote:
> > > > 
> > > > Use the bitfield macro FIELD_GET, and GENMASK to do the shift and
> > > > mask
> > > > in
> > > > one go. This makes the code more readable than explicit masking
> > > > followed
> > > > by a shift.
> > > > 
> > > 
> > > This looks neat.
> > > I'd have to remember to ack it from my work email.
> > 
> > Acked-by: Alexandru Ardelean 
> > 
> > > 
> > > One minor comment inline, which would be the object of a new patch.
> > > 
> > > > Signed-off-by: Melissa Wen 
> > > > ---
> > > >  drivers/staging/iio/cdc/ad7150.c | 6 +-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/staging/iio/cdc/ad7150.c
> > > > b/drivers/staging/iio/cdc/ad7150.c
> > > > index 24601ba7db88..4ba46fb6ac02 100644
> > > > --- a/drivers/staging/iio/cdc/ad7150.c
> > > > +++ b/drivers/staging/iio/cdc/ad7150.c
> > > > @@ -5,6 +5,7 @@
> > > >   * Copyright 2010-2011 Analog Devices Inc.
> > > >   */
> > > > 
> > > > +#include 
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > @@ -44,6 +45,9 @@
> > > >  #define AD7150_SN0_REG 0x16
> > > >  #define AD7150_ID_REG  0x17
> > > > 
> > > > +/* AD7150 masks */
> > > > +#define AD7150_THRESHTYPE_MSK  GENMASK(6, 5)
> > > > +
> > > >  /**
> > > >   * struct ad7150_chip_info - instance specific chip data
> > > >   * @client: i2c client for this device
> > > > @@ -136,7 +140,7 @@ static int ad7150_read_event_config(struct
> > > > iio_dev
> > > > *indio_dev,
> > > > if (ret < 0)
> > > > return ret;
> > > > 
> > > > -   threshtype = (ret >> 5) & 0x03;
> > > > +   threshtype = FIELD_GET(AD7150_THRESHTYPE_MSK, ret);
> > > > adaptive = !!(ret & 0x80);
> > > 
> > > Why not also do something similar for the `adaptive` value ?
> 
> Hi Alexandru,
> 
> Yes, I'm working on it!  However, taking a look at the driver datasheet
> (Table
> 13, page 19), there seems to be a little mistake in choosing the variable
> name
> and its meaning,  since when bit(7) is 1 (true) the threshold is fixed,
> and not
> adaptive. For this reason, I removed it from this patchset to mature the
> solution. I will send it as a bug fix if I prove it's necessary.
> Do you have any advice or feeling about it to share?
> 

Good find.
Since this is a bug-fix, typically it's good to fix the code as-is now
[even if it isn't neat code], and then do the conversions to neat code.

Bug-fixes always take priority over cosmetic changes.
So, I would send the bug-fix as-soon-as-possible, and then update things.


> P.s.: Sorry for having previously sent an email with HTML.
> 
> Melissa
> 
> > > 
> > > > 
> > > > switch (type) {
> > > > --
> > > > 2.20.1
> > > > 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/4] staging: iio: ad7150: use FIELD_GET and GENMASK

[Ardelean, Alexandru]

On Sat, 2019-05-04 at 13:43 +0300, Alexandru Ardelean wrote:
> [External]
> 
> 
> On Sat, May 4, 2019 at 1:25 AM Melissa Wen  wrote:
> > 
> > Use the bitfield macro FIELD_GET, and GENMASK to do the shift and mask
> > in
> > one go. This makes the code more readable than explicit masking
> > followed
> > by a shift.
> > 
> 
> This looks neat.
> I'd have to remember to ack it from my work email.

Acked-by: Alexandru Ardelean 

> 
> One minor comment inline, which would be the object of a new patch.
> 
> > Signed-off-by: Melissa Wen 
> > ---
> >  drivers/staging/iio/cdc/ad7150.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/iio/cdc/ad7150.c
> > b/drivers/staging/iio/cdc/ad7150.c
> > index 24601ba7db88..4ba46fb6ac02 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -5,6 +5,7 @@
> >   * Copyright 2010-2011 Analog Devices Inc.
> >   */
> > 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -44,6 +45,9 @@
> >  #define AD7150_SN0_REG 0x16
> >  #define AD7150_ID_REG  0x17
> > 
> > +/* AD7150 masks */
> > +#define AD7150_THRESHTYPE_MSK  GENMASK(6, 5)
> > +
> >  /**
> >   * struct ad7150_chip_info - instance specific chip data
> >   * @client: i2c client for this device
> > @@ -136,7 +140,7 @@ static int ad7150_read_event_config(struct iio_dev
> > *indio_dev,
> > if (ret < 0)
> > return ret;
> > 
> > -   threshtype = (ret >> 5) & 0x03;
> > +   threshtype = FIELD_GET(AD7150_THRESHTYPE_MSK, ret);
> > adaptive = !!(ret & 0x80);
> 
> Why not also do something similar for the `adaptive` value ?
> 
> > 
> > switch (type) {
> > --
> > 2.20.1
> > 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly

[Ardelean, Alexandru]

On Sat, 2019-03-02 at 19:08 +, Jonathan Cameron wrote:
> [External]
> 
> 
> On Sat, 2 Mar 2019 19:07:16 +
> Jonathan Cameron  wrote:
> 
> > On Fri, 1 Mar 2019 07:17:04 +
> > "Ardelean, Alexandru"  wrote:
> > 
> > > On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> > > > 
> > > > 
> > > > The AD7780 driver contains status pattern bits designed for
> > > > checking
> > > > whether serial transfers have been correctly performed. Pattern
> > > > macros
> > > > were previously generated through bit fields. This patch sets good
> > > > pattern values directly and masks through GENMASK.
> > > > 
> > > > Signed-off-by: Renato Lui Geh 
> > > > ---
> > > >  drivers/staging/iio/adc/ad7780.c | 20 +---
> > > >  1 file changed, 9 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > > b/drivers/staging/iio/adc/ad7780.c
> > > > index 7a68e90ddf14..56c49e28f432 100644
> > > > --- a/drivers/staging/iio/adc/ad7780.c
> > > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > > @@ -17,6 +17,7 @@
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > +#include 
> > > > 
> > > >  #include 
> > > >  #include 
> > > > @@ -28,16 +29,13 @@
> > > >  #define AD7780_ID1 BIT(4)
> > > >  #define AD7780_ID0 BIT(3)
> > > >  #define AD7780_GAINBIT(2)
> > > > -#define AD7780_PAT1BIT(1)
> > > > -#define AD7780_PAT0BIT(0)
> > > 
> > > I don't see a problem to leave the bitfields;  they can be read &
> > > matched
> > > easier with the datasheet.
> > > 
> > > > 
> > > > -#define AD7780_PATTERN (AD7780_PAT0)
> > > > -#define AD7780_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1)
> > > > 
> > > > -#define AD7170_PAT2BIT(2)
> > > > +#define AD7780_PATTERN_GOOD1
> > > 
> > > It was also nice before that the PAT0..PAT2 bitfields were used to
> > > define a
> > > good pattern, since it's easier to match with the datasheet.
> > 
> > This one was much suggestion.  Not particularly important one.
> 
> Not enough sleep this week clearly :)
> This one was _my_ suggestion.

Ok.
I assumed a bit I missed a discussion somewhere.

Let's have this as-is here.
I don't mind it much either.
My [personal] feeling is that [in the context of a move-out-of-staging-of-
this-driver] this patch is a bit of noise.

If the series were more tidy-up, then I probably would not have replied.

> 
> > 
> > Personally if a datasheet is pointlessly confusing I tend to ignore it.
> > This is a two bit field as the bits don't have independent meaning!
> > 
> > I'm not strongly tied to it though and as it's an Analog driver and
> > you all do a good job maintaining the set I'll go with your preference!
> > I do prefer the naming of PATTERN_GOOD though if nothing else!
> > > 
> > > 
> > > > +#define AD7780_PATTERN_MASKGENMASK(1, 0)
> > > 
> > > I like the general usage of GENMASK, but I'm not sure in this case
> > > it's
> > > worth doing. Maybe I missed a discussion somewhere, about doing this
> > > change, but it is mostly a cosmetic without any functional change.
> > > 
> > > 
> > > > 
> > > > -#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> > > > -#define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 |
> > > > AD7170_PAT2)
> > > > +#define AD7170_PATTERN_GOOD5
> > > > +#define AD7170_PATTERN_MASKGENMASK(2, 0)
> > > > 
> > > >  #define AD7780_GAIN_MIDPOINT   64
> > > >  #define AD7780_FILTER_MIDPOINT 13350
> > > > @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> > > > ad7780_sigma_delta_info = {
> > > >  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > > > [ID_AD7170] = {
> > > > .channel = AD7170_CHANNEL(12, 24),
> > > > -   .pattern = AD7170_PATTERN,
> > > > +   .pattern = AD7170_PATTERN_GOOD,
> > > > .pattern_mask = AD7170_PATTERN_MASK,
> > > > .is_ad778x = false,
> > > >  

Re: [PATCH v4 4/9] staging:iio:ad7780: add chip ID values and mask

[Ardelean, Alexandru]

On Sun, 2019-03-03 at 14:53 +, Jonathan Cameron wrote:
> [External]
> 
> 
> On Sun, 3 Mar 2019 11:01:09 -0300
> Renato Lui Geh  wrote:
> 
> > On 03/01, Ardelean, Alexandru wrote:
> > > On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> > > > 
> > > > 
> > > > The ad7780 supports both the ad778x and ad717x families. Each chip
> > > > has
> > > > a corresponding ID. This patch provides a mask for extracting ID
> > > > values
> > > > from the status bits and also macros for the correct values for the
> > > > ad7170, ad7171, ad7780 and ad7781.
> > > > 
> > > > Signed-off-by: Renato Lui Geh 
> > > > ---
> > > >  drivers/staging/iio/adc/ad7780.c | 8 ++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > > b/drivers/staging/iio/adc/ad7780.c
> > > > index 56c49e28f432..ad7617a3a141 100644
> > > > --- a/drivers/staging/iio/adc/ad7780.c
> > > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > > @@ -26,10 +26,14 @@
> > > >  #define AD7780_RDY BIT(7)
> > > >  #define AD7780_FILTER  BIT(6)
> > > >  #define AD7780_ERR BIT(5)
> > > > -#define AD7780_ID1 BIT(4)
> > > > -#define AD7780_ID0 BIT(3)
> > > >  #define AD7780_GAINBIT(2)
> > > > 
> > > > +#define AD7170_ID  0
> > > > +#define AD7171_ID  1
> > > > +#define AD7780_ID  1
> > > > +#define AD7781_ID  0
> > > > +
> > > > +#define AD7780_ID_MASK (BIT(3) | BIT(4))
> > > 
> > > This also doesn't have any functionality change.
> > > The AD7170_ID, AD7171_ID, AD7780_ID & AD7781_ID IDs are also unused
> > > (maybe
> > > in a later patch they are ?).
> > 
> > They aren't. I added them following a previous review suggestion.
> > Should
> > I remove them?
> 
> Can we check them?  It's always useful to confirm that the device is
> the one you think it is.  Then we can either use what is there
> with a suitable warning, or if that is tricky just fault out as the
> dt is giving us the wrong part number.
> 
> J

I guess `dev_warn_ratelimited()` could be used to make sure syslog isn't
spammed-to-death when doing multiple conversions, and the ID isn't correct.
Since these IDs are read after you get a sample, I'm a bit fearful of log-
spam.

I wouldn't throw an error in the ad7780_postprocess_sample() for this, but
warning [with rate-limit] sounds reasonable.

> 
> > > 
> > > I would also leave the AD7780_ID1 & AD7780_ID0 definitions in place,
> > > since
> > > they're easier matched with the datasheet.
> > > 
> > > > 
> > > >  #define AD7780_PATTERN_GOOD1
> > > >  #define AD7780_PATTERN_MASKGENMASK(1, 0)
> > > > --
> > > > 2.21.0
> > > > 
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 5/9] staging: iio: ad7780: move regulator to after GPIO init

[Ardelean, Alexandru]

On Sat, 2019-03-02 at 19:11 +, Jonathan Cameron wrote:
> [External]
> 
> 
> On Fri, 1 Mar 2019 07:38:45 +
> "Ardelean, Alexandru"  wrote:
> 
> > On Thu, 2019-02-28 at 11:25 -0300, Renato Lui Geh wrote:
> > > 
> > > 
> > > To maintain consistency between ad7780_probe and ad7780_remove
> > > orders,
> > > regulator initialization has been moved to after GPIO
> > > initializations.
> > > 
> > > Signed-off-by: Renato Lui Geh 
> > > ---
> > >  drivers/staging/iio/adc/ad7780.c | 26 +-
> > >  1 file changed, 13 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > b/drivers/staging/iio/adc/ad7780.c
> > > index ad7617a3a141..12aef0f101bc 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -255,16 +255,6 @@ static int ad7780_probe(struct spi_device *spi)
> > > 
> > > ad_sd_init(>sd, indio_dev, spi,
> > > _sigma_delta_info);
> > > 
> > > -   st->reg = devm_regulator_get(>dev, "avdd");
> > > -   if (IS_ERR(st->reg))
> > > -   return PTR_ERR(st->reg);
> > > -
> > > -   ret = regulator_enable(st->reg);
> > > -   if (ret) {
> > > -   dev_err(>dev, "Failed to enable specified AVdd
> > > supply\n");
> > > -   return ret;
> > > -   }
> > > -
> > > st->chip_info =
> > > _chip_info_tbl[spi_get_device_id(spi)-
> > > > driver_data];
> > > 
> > > @@ -284,7 +274,7 @@ static int ad7780_probe(struct spi_device *spi)
> > > ret = PTR_ERR(st->powerdown_gpio);
> > > dev_err(>dev, "Failed to request powerdown GPIO:
> > > %d\n",
> > > ret);
> > > -   goto error_disable_reg;
> > > +   return ret;
> > > }
> > > 
> > > if (st->chip_info->is_ad778x) {
> > > @@ -295,7 +285,7 @@ static int ad7780_probe(struct spi_device *spi)
> > > ret = PTR_ERR(st->gain_gpio);
> > > dev_err(>dev, "Failed to request gain
> > > GPIO:
> > > %d\n",
> > > ret);
> > > -   goto error_disable_reg;
> > > +   return ret;
> > > }
> > > 
> > > st->filter_gpio = devm_gpiod_get_optional(>dev,
> > > @@ -306,10 +296,20 @@ static int ad7780_probe(struct spi_device *spi)
> > > dev_err(>dev,
> > > "Failed to request filter GPIO:
> > > %d\n",
> > > ret);
> > > -   goto error_disable_reg;
> > > +   return ret;
> > > }
> > > }
> > > 
> > > +   st->reg = devm_regulator_get(>dev, "avdd");
> > > +   if (IS_ERR(st->reg))
> > > +   return PTR_ERR(st->reg);
> > > +
> > > +   ret = regulator_enable(st->reg);
> > > +   if (ret) {
> > > +   dev_err(>dev, "Failed to enable specified AVdd
> > > supply\n");
> > > +   return ret;
> > > +   }
> > > +
> > 
> > I'm probably missing something, but other than the fact that this moves
> > the
> > regulator init after the GPIOs init, it doesn't change much.
> > The order of the probe & remove is more-or-less the same.
> > The GPIOs will be free'd via devm_ API/stuff.
> 
> This is another one from me.  I'm a fanatic at times when it comes
> to probe and remove orders being precise reverses.  It just makes
> review easier.  Nice to not actually have to think.
> 
> So I agree there is no 'actual' effect but it is in my view
> still worth doing.

Ack.
Let's leave it

> 
> Jonathan
> 
> > 
> > 
> > > ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> > > if (ret)
> > > goto error_disable_reg;
> > > --
> > > 2.21.0
> > > 
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 0/9] staging: iio: ad7780: move out of staging

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:23 -0300, Renato Lui Geh wrote:
> 

The patch-series is a bit big.
I guess that the intent is to move this out-of-staging, but various patches
are holding this in it's place.
For patch series above a certain size, you could get many re-spins
[V2,3,4... so on].

You could send some of the changes as individual patches, or group them in
series of 1,2 or 3 patches. That way, you "parallelize" patch sending, and
when you get reviews on each patch, you can re-spin them individually.
You'll find over time that certain patches get accepted on V1, others on V2
and some on V7 [ hopefully, there isn't any frustration at that point ].

Well, this is a technique I use to distribute some of my upstream-patch-
work, so that I can switch easier between internal-work & upstreaming-work.

Coming back to this patch-series.
My general input, is that the patches are fine over-all; some are just
cosmetics/noise/a-different-way-of-doing-things-for-this-driver, and those
usually can be left to preference [of the maintainer usually].

I do suggest to not hurry when re-spinning patches, and not change too much
the number of patches in a new series. That can complicate things
sometimes. But, if doing small patch-series or individual patches, you
won't have this problem too much.

Thanks
Alex

> 
> This series of patches contains the following:
>  - Adds user input for the 'gain' and 'filter' GPIO pins for the ad778x
>family chips;
>  - Filter reading for the ad778x;
>  - Sets pattern macro values and mask for PATTERN status bits;
>  - Adds ID values for the ad7170, ad7171, ad7780 and ad7781 for ID
>status bits checking;
>  - Moves regulator initialization to after GPIO init to maintain
>consistency between probe and remove;
>  - Copyright edits, adding SPDX identifier and new copyright holder;
>  - Moves the ad7780 driver out of staging to the mainline;
>  - Adds device tree binding for the ad7780 driver.
> 
> Renato Lui Geh (9):
>   staging: iio: ad7780: add gain & filter gpio support
>   staging: iio: ad7780: add filter reading to ad778x
>   staging: iio: ad7780: set pattern values and masks directly
>   staging:iio:ad7780: add chip ID values and mask
>   staging: iio: ad7780: move regulator to after GPIO init
>   staging: iio: ad7780: add SPDX identifier
>   staging: iio: ad7780: add new copyright holder
>   staging: iio: ad7780: moving ad7780 out of staging
>   staging: iio: ad7780: add device tree binding
> 
> Changelog:
> *v3
>  - SPDX and regulator init as patches
>  - Renamed filter to odr and ad778x_filter to ad778x_odr_avail
>  - Removed unnecessary regulator disabling
>  - Removed unnecessary AD_SD_CHANNEL macro
>  - Changed unsigned int to unsigned long long to avoid overflow
> *v4
>  - Split gain & filter patch into two, with the new commit adding only
>filter reading
>  - Changed pattern values to direct values, and added pattern mask
>  - Added ID values and mask
>  - Added new copyright holder
>  - Added device tree binding to the ad7780 driver
> 
>  .../bindings/iio/adc/adi,ad7780.txt   |  48 +++
>  drivers/iio/adc/Kconfig   |  12 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/ad7780.c  | 365 ++
>  drivers/staging/iio/adc/Kconfig   |  13 -
>  drivers/staging/iio/adc/Makefile  |   1 -
>  drivers/staging/iio/adc/ad7780.c  | 277 -
>  7 files changed, 426 insertions(+), 291 deletions(-)
>  create mode 100644
> Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
>  create mode 100644 drivers/iio/adc/ad7780.c
>  delete mode 100644 drivers/staging/iio/adc/ad7780.c
> 
> --
> 2.21.0
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 4/9] staging:iio:ad7780: add chip ID values and mask

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> 
> 
> The ad7780 supports both the ad778x and ad717x families. Each chip has
> a corresponding ID. This patch provides a mask for extracting ID values
> from the status bits and also macros for the correct values for the
> ad7170, ad7171, ad7780 and ad7781.
> 
> Signed-off-by: Renato Lui Geh 
> ---
>  drivers/staging/iio/adc/ad7780.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 56c49e28f432..ad7617a3a141 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -26,10 +26,14 @@
>  #define AD7780_RDY BIT(7)
>  #define AD7780_FILTER  BIT(6)
>  #define AD7780_ERR BIT(5)
> -#define AD7780_ID1 BIT(4)
> -#define AD7780_ID0 BIT(3)
>  #define AD7780_GAINBIT(2)
> 
> +#define AD7170_ID  0
> +#define AD7171_ID  1
> +#define AD7780_ID  1
> +#define AD7781_ID  0
> +
> +#define AD7780_ID_MASK (BIT(3) | BIT(4))

This also doesn't have any functionality change.
The AD7170_ID, AD7171_ID, AD7780_ID & AD7781_ID IDs are also unused (maybe
in a later patch they are ?).

I would also leave the AD7780_ID1 & AD7780_ID0 definitions in place, since
they're easier matched with the datasheet.

> 
>  #define AD7780_PATTERN_GOOD1
>  #define AD7780_PATTERN_MASKGENMASK(1, 0)
> --
> 2.21.0
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 5/9] staging: iio: ad7780: move regulator to after GPIO init

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:25 -0300, Renato Lui Geh wrote:
> 
> 
> To maintain consistency between ad7780_probe and ad7780_remove orders,
> regulator initialization has been moved to after GPIO initializations.
> 
> Signed-off-by: Renato Lui Geh 
> ---
>  drivers/staging/iio/adc/ad7780.c | 26 +-
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index ad7617a3a141..12aef0f101bc 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -255,16 +255,6 @@ static int ad7780_probe(struct spi_device *spi)
> 
> ad_sd_init(>sd, indio_dev, spi, _sigma_delta_info);
> 
> -   st->reg = devm_regulator_get(>dev, "avdd");
> -   if (IS_ERR(st->reg))
> -   return PTR_ERR(st->reg);
> -
> -   ret = regulator_enable(st->reg);
> -   if (ret) {
> -   dev_err(>dev, "Failed to enable specified AVdd
> supply\n");
> -   return ret;
> -   }
> -
> st->chip_info =
> _chip_info_tbl[spi_get_device_id(spi)-
> >driver_data];
> 
> @@ -284,7 +274,7 @@ static int ad7780_probe(struct spi_device *spi)
> ret = PTR_ERR(st->powerdown_gpio);
> dev_err(>dev, "Failed to request powerdown GPIO:
> %d\n",
> ret);
> -   goto error_disable_reg;
> +   return ret;
> }
> 
> if (st->chip_info->is_ad778x) {
> @@ -295,7 +285,7 @@ static int ad7780_probe(struct spi_device *spi)
> ret = PTR_ERR(st->gain_gpio);
> dev_err(>dev, "Failed to request gain GPIO:
> %d\n",
> ret);
> -   goto error_disable_reg;
> +   return ret;
> }
> 
> st->filter_gpio = devm_gpiod_get_optional(>dev,
> @@ -306,10 +296,20 @@ static int ad7780_probe(struct spi_device *spi)
> dev_err(>dev,
> "Failed to request filter GPIO: %d\n",
> ret);
> -   goto error_disable_reg;
> +   return ret;
> }
> }
> 
> +   st->reg = devm_regulator_get(>dev, "avdd");
> +   if (IS_ERR(st->reg))
> +   return PTR_ERR(st->reg);
> +
> +   ret = regulator_enable(st->reg);
> +   if (ret) {
> +   dev_err(>dev, "Failed to enable specified AVdd
> supply\n");
> +   return ret;
> +   }
> +

I'm probably missing something, but other than the fact that this moves the
regulator init after the GPIOs init, it doesn't change much.
The order of the probe & remove is more-or-less the same.
The GPIOs will be free'd via devm_ API/stuff.


> ret = ad_sd_setup_buffer_and_trigger(indio_dev);
> if (ret)
> goto error_disable_reg;
> --
> 2.21.0
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> 
> 
> The AD7780 driver contains status pattern bits designed for checking
> whether serial transfers have been correctly performed. Pattern macros
> were previously generated through bit fields. This patch sets good
> pattern values directly and masks through GENMASK.
> 
> Signed-off-by: Renato Lui Geh 
> ---
>  drivers/staging/iio/adc/ad7780.c | 20 +---
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 7a68e90ddf14..56c49e28f432 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  #include 
>  #include 
> @@ -28,16 +29,13 @@
>  #define AD7780_ID1 BIT(4)
>  #define AD7780_ID0 BIT(3)
>  #define AD7780_GAINBIT(2)
> -#define AD7780_PAT1BIT(1)
> -#define AD7780_PAT0BIT(0)

I don't see a problem to leave the bitfields;  they can be read & matched
easier with the datasheet.

> 
> -#define AD7780_PATTERN (AD7780_PAT0)
> -#define AD7780_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1)
> 
> -#define AD7170_PAT2BIT(2)

> +#define AD7780_PATTERN_GOOD1

It was also nice before that the PAT0..PAT2 bitfields were used to define a
good pattern, since it's easier to match with the datasheet.


> +#define AD7780_PATTERN_MASKGENMASK(1, 0)

I like the general usage of GENMASK, but I'm not sure in this case it's
worth doing. Maybe I missed a discussion somewhere, about doing this
change, but it is mostly a cosmetic without any functional change.


> 
> -#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> -#define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> +#define AD7170_PATTERN_GOOD5
> +#define AD7170_PATTERN_MASKGENMASK(2, 0)
> 
>  #define AD7780_GAIN_MIDPOINT   64
>  #define AD7780_FILTER_MIDPOINT 13350
> @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
>  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> [ID_AD7170] = {
> .channel = AD7170_CHANNEL(12, 24),
> -   .pattern = AD7170_PATTERN,
> +   .pattern = AD7170_PATTERN_GOOD,
> .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7171] = {
> .channel = AD7170_CHANNEL(16, 24),
> -   .pattern = AD7170_PATTERN,
> +   .pattern = AD7170_PATTERN_GOOD,
> .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7780] = {
> .channel = AD7780_CHANNEL(24, 32),
> -   .pattern = AD7780_PATTERN,
> +   .pattern = AD7780_PATTERN_GOOD,
> .pattern_mask = AD7780_PATTERN_MASK,
> .is_ad778x = true,
> },
> [ID_AD7781] = {
> .channel = AD7780_CHANNEL(20, 32),
> -   .pattern = AD7780_PATTERN,
> +   .pattern = AD7780_PATTERN_GOOD,
> .pattern_mask = AD7780_PATTERN_MASK,
> .is_ad778x = true,
> },
> --
> 2.21.0
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 2/9] staging: iio: ad7780: add filter reading to ad778x

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> 
> 
> This patch adds the new feature of reading the filter odr value for
> ad778x chips. This value is stored in the chip's state struct whenever a
> read or write call is performed on the chip's driver.
> 
> This feature requires sharing SAMP_FREQ. Since the ad717x does not have
> a filter option, the driver only shares the relevant info mask for the
> ad778x family.
> 
> Signed-off-by: Renato Lui Geh 
> ---
> Changes in v4:
>  - As mentioned in the previous patch, this was originally as part of
>the 'add gain & filter gpio support' patch
> 

This needs the  ad778x_odr_avail[2] array to be moved to this patch.
Otherwise from what I can tell, this looks good.

>  drivers/staging/iio/adc/ad7780.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 87fbcf510d45..7a68e90ddf14 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -59,6 +59,7 @@ struct ad7780_state {
> struct gpio_desc*gain_gpio;
> struct gpio_desc*filter_gpio;
> unsigned intgain;
> +   unsigned intodr;
> unsigned intint_vref_mv;
> 
> struct ad_sigma_delta sd;
> @@ -121,6 +122,9 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_OFFSET:
> *val = -(1 << (chan->scan_type.realbits - 1));
> return IIO_VAL_INT;
> +   case IIO_CHAN_INFO_SAMP_FREQ:
> +   *val = st->odr;
> +   return IIO_VAL_INT;
> default:
> break;
> }
> @@ -163,6 +167,7 @@ static int ad7780_write_raw(struct iio_dev
> *indio_dev,
> val = 0;
> else
> val = 1;
> +   st->odr = ad778x_odr_avail[val];
> gpiod_set_value(st->filter_gpio, val);
> break;
> default:
> @@ -184,6 +189,7 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
> 
> if (chip_info->is_ad778x) {
> st->gain = ad778x_gain[raw_sample & AD7780_GAIN];
> +   st->odr = ad778x_odr_avail[raw_sample & AD7780_FILTER];
> }
> 
> return 0;
> @@ -196,17 +202,19 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
>  };
> 
>  #define AD7780_CHANNEL(bits, wordsize) \
> +   AD_SD_CHANNEL(1, 0, 0, bits, 32, wordsize - bits)
> +#define AD7170_CHANNEL(bits, wordsize) \
> AD_SD_CHANNEL_NO_SAMP_FREQ(1, 0, 0, bits, 32, wordsize - bits)
> 
>  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> [ID_AD7170] = {
> -   .channel = AD7780_CHANNEL(12, 24),
> +   .channel = AD7170_CHANNEL(12, 24),
> .pattern = AD7170_PATTERN,
> .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7171] = {
> -   .channel = AD7780_CHANNEL(16, 24),
> +   .channel = AD7170_CHANNEL(16, 24),
> .pattern = AD7170_PATTERN,
> .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> --
> 2.21.0
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 1/9] staging: iio: ad7780: add gain & filter gpio support

[Ardelean, Alexandru]

On Thu, 2019-02-28 at 11:23 -0300, Renato Lui Geh wrote:
> 
> 
> Previously, the AD7780 driver only supported gpio for the 'powerdown'
> pin. This commit adds suppport for the 'gain' and 'filter' pin.
> 
> Signed-off-by: Renato Lui Geh 
> Signed-off-by: Giuliano Belinassi 
> Co-developed-by: Giuliano Belinassi 
> ---
> Changes in v3:
>  - Renamed ad7780_chip_info's filter to odr
>  - Renamed ad778x_filter to ad778x_odr_avail
>  - Changed vref variable from unsigned int to unsigned long long to avoid
>overflow
>  - Removed unnecessary AD_SD_CHANNEL macro
> Changes in v4:
>  - Removed useless macro
>  - Added default case for switch to suppress warning
>  - Removed chunks belonging to filter reading, adding these as a
>patch for itself
> 
>  drivers/staging/iio/adc/ad7780.c | 90 +---
>  1 file changed, 84 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c4a85789c2db..87fbcf510d45 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -39,6 +39,12 @@
>  #define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
>  #define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> 
> +#define AD7780_GAIN_MIDPOINT   64
> +#define AD7780_FILTER_MIDPOINT 13350
> +
> +static const unsigned int ad778x_gain[2]  = { 1, 128 };
> +static const unsigned int ad778x_odr_avail[2] = { 1, 16700 };

ad778x_odr_avail[2] is not used in this patch, so it should probably go
into the next one 
(i.e. staging: iio: ad7780: add filter reading to ad778x )

one good way of catching stuff like this is to do interactive rebase and
compile your driver on each patch to see if the compiler catches this;
i suspect the compiler would have thrown an error for this change


> 
>  struct ad7780_chip_info
> struct iio_chan_specchannel;
> unsigned intpattern_mask;
> @@ -50,7 +56,10 @@ struct ad7780_state {
> const struct ad7780_chip_info   *chip_info;
> struct regulator*reg;
> struct gpio_desc*powerdown_gpio;
> -   unsigned intgain;
> +   struct gpio_desc*gain_gpio;
> +   struct gpio_desc*filter_gpio;
> +   unsigned intgain;
> +   unsigned intint_vref_mv;
> 
> struct ad_sigma_delta sd;
>  };
> @@ -104,17 +113,65 @@ static int ad7780_read_raw(struct iio_dev
> *indio_dev,
> voltage_uv = regulator_get_voltage(st->reg);
> if (voltage_uv < 0)
> return voltage_uv;
> -   *val = (voltage_uv / 1000) * st->gain;
> +   voltage_uv /= 1000;
> +   *val = voltage_uv * st->gain;
> *val2 = chan->scan_type.realbits - 1;
> +   st->int_vref_mv = voltage_uv;
> return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
> *val = -(1 << (chan->scan_type.realbits - 1));
> return IIO_VAL_INT;
> +   default:
> +   break;

The indentation of the break statement is inconsistent with other places.
Still, it does not add much value adding this change as-is, since it does
not change any behavior, and is not an element needed by this change (i.e.
adding gain & filter support via gpios)

> }
> 
> return -EINVAL;
>  }
> 
> +static int ad7780_write_raw(struct iio_dev *indio_dev,
> +   struct iio_chan_spec const *chan,
> +   int val,
> +   int val2,
> +   long m)
> +{
> +   struct ad7780_state *st = iio_priv(indio_dev);
> +   const struct ad7780_chip_info *chip_info = st->chip_info;
> +   unsigned long long vref;
> +   unsigned int full_scale, gain;
> +
> +   if (!chip_info->is_ad778x)
> +   return 0;
> +
> +   switch (m) {
> +   case IIO_CHAN_INFO_SCALE:
> +   if (val != 0)
> +   return -EINVAL;
> +
> +   vref = st->int_vref_mv * 100LL;
> +   full_scale = 1 << (chip_info->channel.scan_type.realbits
> - 1);
> +   gain = DIV_ROUND_CLOSEST(vref, full_scale);
> +   gain = DIV_ROUND_CLOSEST(gain, val2);
> +   st->gain = gain;
> +   if (gain < AD7780_GAIN_MIDPOINT)
> +   gain = 0;
> +   else
> +   gain = 1;
> +   gpiod_set_value(st->gain_gpio, gain);
> +   break;
> +   case IIO_CHAN_INFO_SAMP_FREQ:
> +   if (1000*val + val2/1000 < AD7780_FILTER_MIDPOINT)
> +   val = 0;
> +   else
> +   val = 1;
> +   gpiod_set_value(st->filter_gpio, val);
> +   break;
> +   default:
> +   break;
> +   }
> +
> +   return 0;
> +}
> +
> 

Re: [PATCH 0/3] Add devicetree support for ad5933

[Ardelean, Alexandru]

On Mon, 2018-12-10 at 21:27 +, Jonathan Cameron wrote:
> On Sat, 8 Dec 2018 22:10:43 +0100
> Greg KH  wrote:
> 
> > On Sat, Dec 08, 2018 at 04:56:45PM -0200, Marcelo Schmitt wrote:
> > > Parts of this work came from contributions of Alexandru Ardelean and
> > > Dragos Bogdan, I and Gabriel would like to thank for the insights
> > > provided by their previous patches. Maybe it would be the case to add
> > > them as co-authors of this patch set.  
> > 
> > That's what the Co-developed-by: tag is for, please use it :)
> > 
> 
> Alexandru, Dragos.  No idea how involved you were in the actual
> patch...
> 
> Work amongst amongst the 4 of you what you would like to do and
> let us know!
> 

Hey,

To give a bit of context, I sent some patches some time ago that deal with
this, but haven't had time to re-spin them [based on Jonathan's comments].

I've discussed with Dragos.
>From our side, any version that is decided with respect to the `Co-
developed-by:` tag is fine. i.e. we don't need to be mentioned in the
commits.

I hope this is a sufficiently good answer [for the whole legal stuff].

> Patches look fine, though we need to let the DT maintainers have a few
> days at least to get around to looking if they want to (they don't always
> on simple bindings like these).
> 

Thanks
Alex

> Jonathan
>  
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad7780: Add gain & filter gpio support

[Ardelean, Alexandru]

On Tue, 2018-11-27 at 06:11 -0500, Popa, Stefan Serban wrote:
> On Lu, 2018-11-26 at 17:24 -0200, Giuliano Belinassi wrote:
> Hi, please see bellow
> 

One note from me here.

> > Hi, thank you for the review
> > 
> > > 
> > > On Thu, 22 Nov 2018 11:01:00 +
> > > "Popa, Stefan Serban"  wrote:
> > > > 
> > > > I think that instead of setting the gain directly, we should use
> > > > the IIO_CHAN_INFO_SCALE attribute. At page 12 of the ad7780
> > > > datasheet
> > > > there
> > > > is a formula from which the output code can be calculated:
> > > > Code = 2^(N − 1)
> > > > × [(AIN × Gain /VREF) + 1]. So, by setting the scale from user
> > > > space,
> > > > the
> > > > driver can calculate the correct gain by using the formula above.
> > > > Also, it
> > > > would be useful to introduce scale available.
> > > > Furthermore, there is a new
> > > > ad7124 adc driver which does this exact thing. Take a look here:
> > > > http
> > > > s://gi
> > > > thub.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/ad7124.
> > > > c#
> > > > L337.
> > 
> > We have some questions about the code you provided to us:
> >   1-) What is exactly the inputs for the write_raw function?
> 
> In your write_raw() function you need to add IIO_CHAN_INFO_SCALE case.
> Setting the scale from user space looks something like this:
> root:/sys/bus/iio/devices/iio:device0> echo 0.000298 > in_voltage_scale .
> Furthermore, in your write_raw() function, val=0 and val2=298.
> Knowing that full_scale_voltage = Vref / (gain * scale), we can calculate
> the gain = Vref / (full_scale_voltage * scale). We only support two gains
> (1 and 128), so we need to determine which one fits better with the
> desired
> scale. Finally, all we have left to do is to set the gain. 
>  
> >   2-) Could you give more details about the math around lines 346-348?
> > Is it correct to assume that the multiplication at line 346 won't
> > overflow? (vref is an uint)
> 
> It is correct that Vref is in microvolts, so for example, Vref of 2.5V  =
> 25uV. It won't overflow since we use the Vref as nominator, while
> full_scale_voltage and scale are the denominators.
> 

[Regarding the AD7124] I guess I should be noted that the code can
overflow, but in this case it shouldn't, because (according to the device
datasheet) the maximum VREF can be 3600 mV.
A user could specify (in the devicetree) something larger than 4300 mV (and
that would overflow) but that would also make the readings useless as the
external VREF would be invalid ; and there is also the risk of frying the
chip in that case [something you really can't protect the user against].

The internal VREF however is 2500 mV, so things should be fine from that
point of view.

Typically, in datasheets (at least from Analog Devices) it's good to take a
look at the specifications sections.
[For AD7124] You will see that the internal VREF [page 8] is 2.5V (with
approximation of +/-0.2%) and for external reference it goes all the way up
to AVDD, which has typical values of 2.9V - 3.6V.
So, for u32 this code should be fine and not overflow.

One small thing that can be confusing about that code in AD7124 is that it
gets multiplied with 100LL (which is signed long-long), but that should
be fine, since the operation should be converted to u32 (unsigned int)
representation [by being assigned to vref], which ends up being fine in the
end.

> > 
> > And regarding our code:
> >   1-) The val in our write_raw function should be, in case of GAIN, a
> > number that best approximate the actual gain value of 1 or 128? For
> > instance, if the user inputs 126, we should default to 128?
> 
> We should not allow the the user to input the gain, he needs to input the
> scale (see the mail from Jonathan and the above explanation). However, if
> the calculated gain is one that is not supported, such as 126, we will
> set
> the closest matching value, in this case 128.
> 
> >   2-) In the case of FILTER, is it the same? Is the user sending the
> > value in mHz (milihertz)?
> 
> Yes, it is the same with the FILTER. You need to add
> a IIO_CHAN_INFO_SAMP_FREQ case in your write_raw() function. From user
> space, the input value should be in Hz, something like this:
> root:/sys/bus/iio/devices/iio:device0> echo 10 >
> in_voltage_sampling_frequency
> 
> > 
> > Thank you
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 7/7] staging:iio:ad2s90: Move out of staging

[Ardelean, Alexandru]

On Fri, 2018-11-23 at 22:23 -0200, Matheus Tavares wrote:
> Move ad2s90 resolver driver out of staging to the main tree.
> 

Acked-by: Alexandru Ardelean 

> Signed-off-by: Matheus Tavares 
> Signed-off-by: Victor Colombo 
> ---
> Changes in v3:
>  - none
> 
> Changes in v2:
>  - Disabled git move detection, to see the whole code, as Jonathan
>  suggested
> 
>  drivers/iio/resolver/Kconfig  |  10 ++
>  drivers/iio/resolver/Makefile |   1 +
>  drivers/iio/resolver/ad2s90.c | 131 ++
>  drivers/staging/iio/resolver/Kconfig  |  10 --
>  drivers/staging/iio/resolver/Makefile |   1 -
>  drivers/staging/iio/resolver/ad2s90.c | 131 --
>  6 files changed, 142 insertions(+), 142 deletions(-)
>  create mode 100644 drivers/iio/resolver/ad2s90.c
>  delete mode 100644 drivers/staging/iio/resolver/ad2s90.c
> 
> diff --git a/drivers/iio/resolver/Kconfig b/drivers/iio/resolver/Kconfig
> index 2ced9f22aa70..786801be54f6 100644
> --- a/drivers/iio/resolver/Kconfig
> +++ b/drivers/iio/resolver/Kconfig
> @@ -3,6 +3,16 @@
>  #
>  menu "Resolver to digital converters"
>  
> +config AD2S90
> + tristate "Analog Devices ad2s90 driver"
> + depends on SPI
> + help
> +   Say yes here to build support for Analog Devices spi resolver
> +   to digital converters, ad2s90, provides direct access via sysfs.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called ad2s90.
> +
>  config AD2S1200
>   tristate "Analog Devices ad2s1200/ad2s1205 driver"
>   depends on SPI
> diff --git a/drivers/iio/resolver/Makefile
> b/drivers/iio/resolver/Makefile
> index 4e1dccae07e7..398d82d50028 100644
> --- a/drivers/iio/resolver/Makefile
> +++ b/drivers/iio/resolver/Makefile
> @@ -2,4 +2,5 @@
>  # Makefile for Resolver/Synchro drivers
>  #
>  
> +obj-$(CONFIG_AD2S90) += ad2s90.o
>  obj-$(CONFIG_AD2S1200) += ad2s1200.o
> diff --git a/drivers/iio/resolver/ad2s90.c
> b/drivers/iio/resolver/ad2s90.c
> new file mode 100644
> index ..a41f5cb10da5
> --- /dev/null
> +++ b/drivers/iio/resolver/ad2s90.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ad2s90.c simple support for the ADI Resolver to Digital Converters:
> AD2S90
> + *
> + * Copyright (c) 2010-2010 Analog Devices Inc.
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +/*
> + * Although chip's max frequency is 2Mhz, it needs 600ns between CS and
> the
> + * first falling edge of SCLK, so frequency should be at most 1 / (2 *
> 6e-7)
> + */
> +#define AD2S90_MAX_SPI_FREQ_HZ  83
> +
> +struct ad2s90_state {
> + struct mutex lock; /* lock to protect rx buffer */
> + struct spi_device *sdev;
> + u8 rx[2] cacheline_aligned;
> +};
> +
> +static int ad2s90_read_raw(struct iio_dev *indio_dev,
> +struct iio_chan_spec const *chan,
> +int *val,
> +int *val2,
> +long m)
> +{
> + int ret;
> + struct ad2s90_state *st = iio_priv(indio_dev);
> +
> + if (chan->type != IIO_ANGL)
> + return -EINVAL;
> +
> + switch (m) {
> + case IIO_CHAN_INFO_SCALE:
> + /* 2 * Pi / 2^12 */
> + *val = 6283; /* mV */
> + *val2 = 12;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(>lock);
> + ret = spi_read(st->sdev, st->rx, 2);
> + if (ret < 0) {
> + mutex_unlock(>lock);
> + return ret;
> + }
> + *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >>
> 4);
> +
> + mutex_unlock(>lock);
> +
> + return IIO_VAL_INT;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct iio_info ad2s90_info = {
> + .read_raw = ad2s90_read_raw,
> +};
> +
> +static const struct iio_chan_spec ad2s90_chan = {
> + .type = IIO_ANGL,
> + .indexed = 1,
> + .channel = 0,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE),
> +};
> +
> +static int ad2s90_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct ad2s90_state *st;
> +
> + if (spi->max_speed_hz > AD2S90_MAX_SPI_FREQ_HZ) {
> + dev_err(>dev, "SPI CLK, %d Hz exceeds %d Hz\n",
> + spi->max_speed_hz, AD2S90_MAX_SPI_FREQ_HZ);
> + return -EINVAL;
> + }
> +
> + indio_dev = devm_iio_device_alloc(>dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> + st = iio_priv(indio_dev);
> + spi_set_drvdata(spi, indio_dev);
> +
> + mutex_init(>lock);
> + st->sdev = spi;
> + indio_dev->dev.parent = >dev;
> + indio_dev->info = _info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + 

Re: [PATCH v2 5/5] Staging: iio: adt7316: Use device tree data to assign irq_type

[Ardelean, Alexandru]

On Tue, 2018-11-20 at 22:30 +0530, Shreeya Patel wrote:
> ADT7316 driver no more uses platform data and hence use device tree
> data instead of platform data for assigning irq_type field.
> Switch case figures out the type of irq and if it's the default case
> then assign the default value to the irq_type i.e.
> irq_type = IRQF_TRIGGER_LOW
> 

1 comment inline

> Signed-off-by: Shreeya Patel 
> ---
>  drivers/staging/iio/addac/adt7316.c | 21 +
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/addac/adt7316.c
> b/drivers/staging/iio/addac/adt7316.c
> index 9c72538baf9e..c647875a64f5 100644
> --- a/drivers/staging/iio/addac/adt7316.c
> +++ b/drivers/staging/iio/addac/adt7316.c
> @@ -2101,8 +2101,7 @@ int adt7316_probe(struct device *dev, struct
> adt7316_bus *bus,
>  {
>   struct adt7316_chip_info *chip;
>   struct iio_dev *indio_dev;
> - unsigned short *adt7316_platform_data = dev->platform_data;
> - int irq_type = IRQF_TRIGGER_LOW;
> + int irq_type;
>   int ret = 0;
>  
>   indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
> @@ -2146,8 +2145,22 @@ int adt7316_probe(struct device *dev, struct
> adt7316_bus *bus,
>   indio_dev->modes = INDIO_DIRECT_MODE;
>  
>   if (chip->bus.irq > 0) {
> - if (adt7316_platform_data[0])
> - irq_type = adt7316_platform_data[0];
> + irq_type =
> + irqd_get_trigger_type(irq_get_irq_data(chip-
> >bus.irq));
> +
> + switch (irq_type) {
> + case IRQF_TRIGGER_HIGH:
> + case IRQF_TRIGGER_RISING:
> + break;
> + case IRQF_TRIGGER_LOW:
> + case IRQF_TRIGGER_FALLING:
> + break;
> + default:
> + dev_info(dev, "mode %d unsupported, using
> IRQF_TRIGGER_LOW\n",
> +  irq_type);
> + irq_type = IRQF_TRIGGER_LOW;
> + break;
> + }

It would be an idea to move this part [together with
devm_request_threaded_irq()] into a "adt7316_setup_irq()" function. To un-
clutter the code in the adt7316_probe() function.

>  
>   ret = devm_request_threaded_irq(dev, chip->bus.irq,
>   NULL,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 3/7] staging:iio:ad2s90: Add max frequency check at probe

[Ardelean, Alexandru]

On Sun, 2018-11-18 at 02:25 -0200, Matheus Tavares wrote:
> From: Alexandru Ardelean 
> 
> This patch adds a max frequency check at the beginning of ad2s90_probe
> function so that when it is set to a value above 0.83Mhz, dev_err is
> called with an appropriate message and -EINVAL is returned.
> 
> The defined limit is 0.83Mhz instead of 2Mhz, which is the chip's max
> frequency as specified in the datasheet, because, as also specified in
> the datasheet, a 600ns delay is expected between the application of a
> logic LO to CS and the application of SCLK. Since the delay is not
> implemented in the spi code, to satisfy it, SCLK's period should be at
> most 2 * 600ns, so the max frequency should be 1 / (2 * 6e-7), which
> gives roughly 83Hz.
> 
> Signed-off-by: Alexandru Ardelean 

I think you can use "Suggested-by:" instead.
But this is also fine.

> Signed-off-by: Matheus Tavares 
> ---
> Changes in v2:
>  - Correctly credit Alexandru as the patch's author
> 
>  drivers/staging/iio/resolver/ad2s90.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s90.c
> b/drivers/staging/iio/resolver/ad2s90.c
> index 913d6fad2d4d..fe90f2056bff 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -19,6 +19,12 @@
>  #include 
>  #include 
>  
> +/*
> + * Although chip's max frequency is 2Mhz, it needs 600ns between CS and
> the
> + * first falling edge of SCLK, so frequency should be at most 1 / (2 *
> 6e-7)
> + */
> +#define AD2S90_MAX_SPI_FREQ_HZ  83
> +
>  struct ad2s90_state {
>   struct mutex lock;
>   struct spi_device *sdev;
> @@ -78,6 +84,12 @@ static int ad2s90_probe(struct spi_device *spi)
>   struct iio_dev *indio_dev;
>   struct ad2s90_state *st;
>  
> + if (spi->max_speed_hz > AD2S90_MAX_SPI_FREQ_HZ) {
> + dev_err(>dev, "SPI CLK, %d Hz exceeds %d Hz\n",
> + spi->max_speed_hz, AD2S90_MAX_SPI_FREQ_HZ);
> + return -EINVAL;
> + }
> +
>   indio_dev = devm_iio_device_alloc(>dev, sizeof(*st));
>   if (!indio_dev)
>   return -ENOMEM;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/7] staging:iio:ad2s90: Add device tree support

[Ardelean, Alexandru]

On Sun, 2018-11-18 at 02:25 -0200, Matheus Tavares wrote:
> This patch adds device tree support to ad2s90 with standard
> device tree id table.
> 

Hey,

Comment inline

> Signed-off-by: Matheus Tavares 
> ---
> Changes in v2:
>  - none
> 
>  drivers/staging/iio/resolver/ad2s90.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s90.c
> b/drivers/staging/iio/resolver/ad2s90.c
> index 3e257ac46f7a..6ffbac66b837 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -107,6 +107,12 @@ static int ad2s90_probe(struct spi_device *spi)
>   return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>  }
>  
> +static const struct of_device_id ad2s90_of_match[] = {
> + { .compatible = "adi,ad2s90", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ad2s90_of_match);
> +
>  static const struct spi_device_id ad2s90_id[] = {
>   { "ad2s90" },
>   {}
> @@ -116,6 +122,7 @@ MODULE_DEVICE_TABLE(spi, ad2s90_id);
>  static struct spi_driver ad2s90_driver = {
>   .driver = {
>   .name = "ad2s90",
> + .of_match_table = of_match_ptr(ad2s90_of_match),

I think you need to remove the of_match_ptr().
There was a comment from Jonathan on another thread about this.
See:
   https://patchwork.kernel.org/patch/10682963/

So, 
+   .of_match_table = of_match_ptr(ad2s90_of_match),

becomes
> + .of_match_table = ad2s90_of_match,

>   },
>   .probe = ad2s90_probe,
>   .id_table = ad2s90_id,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 4/7] dt-bindings:iio:resolver: Add docs for ad2s90

[Ardelean, Alexandru]

On Sun, 2018-11-18 at 02:25 -0200, Matheus Tavares wrote:
> This patch adds the device tree binding documentation for the ad2s90
> resolver-to-digital converter.
> 

One minor comment inline.

> Signed-off-by: Matheus Tavares 
> ---
> Changes in v2:
>  - Rewritten 'spi-cpol and spi-cpha' item to say that the device can
>  work in either mode (0,0) or (1,1) and explain how they should be
>  specified in DT.
> 
>  .../bindings/iio/resolver/ad2s90.txt  | 28 +++
>  1 file changed, 28 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> new file mode 100644
> index ..594417539938
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> @@ -0,0 +1,28 @@
> +Analog Devices AD2S90 Resolver-to-Digital Converter
> +
> +https://www.analog.com/en/products/ad2s90.html
> +
> +Required properties:
> +  - compatible: should be "adi,ad2s90"
> +  - reg: SPI chip select number for the device
> +  - spi-max-frequency: set maximum clock frequency, must be 83
> +  - spi-cpol and spi-cpha:
> +Either SPI mode (0,0) or (1,1) must be used, so specify none or
> both of
> +spi-cpha, spi-cpol.
For SPI properties it's a good idea to also reference the document for SPI
bindings.
Something like:
See for more details:
 Documentation/devicetree/bindings/spi/spi-bus.txt

> +
> +Note about max frequency:
> +Chip's max frequency, as specified in its datasheet, is 2Mhz. But a
> 600ns
> +delay is expected between the application of a logic LO to CS and
> the
> +application of SCLK, as also specified. And since the delay is not
> +implemented in the spi code, to satisfy it, SCLK's period should be
> at most
> +2 * 600ns, so the max frequency should be 1 / (2 * 6e-7), which
> gives
> +roughly 83Hz.
> +
> +Example:
> +resolver@0 {
> + compatible = "adi,ad2s90";
> + reg = <0>;
> + spi-max-frequency = <83>;
> + spi-cpol;
> + spi-cpha;
> +};
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 4/4] staging: iio: ad7816: Add device tree table.

[Ardelean, Alexandru]

On Wed, 2018-11-14 at 23:16 +0530, Nishad Kamdar wrote:
> Add device tree table for matching vendor ID.
> 

This could have been just one patch.
Something like 
   [PATCH v4] staging: iio: ad7816: Add device tree table.

It's no longer a series, because the other patches were applied already.

I don't think you need to re-spin this [it's up to Jonathan though].

It's just something to keep in mind for later when re-spinning patches or
series of patches.

Alex

> Signed-off-by: Nishad Kamdar 
> ---
>  drivers/staging/iio/adc/ad7816.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c
> b/drivers/staging/iio/adc/ad7816.c
> index a2fead85cd46..925f7086bc07 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -422,6 +422,14 @@ static int ad7816_probe(struct spi_device *spi_dev)
>   return 0;
>  }
>  
> +static const struct of_device_id ad7816_of_match[] = {
> + { .compatible = "adi,ad7816", },
> + { .compatible = "adi,ad7817", },
> + { .compatible = "adi,ad7818", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad7816_of_match);
> +
>  static const struct spi_device_id ad7816_id[] = {
>   { "ad7816", ID_AD7816 },
>   { "ad7817", ID_AD7817 },
> @@ -434,6 +442,7 @@ MODULE_DEVICE_TABLE(spi, ad7816_id);
>  static struct spi_driver ad7816_driver = {
>   .driver = {
>   .name = "ad7816",
> + .of_match_table = of_match_ptr(ad7816_of_match),
>   },
>   .probe = ad7816_probe,
>   .id_table = ad7816_id,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update

[Ardelean, Alexandru]

On Sun, 2018-11-11 at 12:58 +, Jonathan Cameron wrote:
> On Thu, 8 Nov 2018 13:44:17 +
> "Ardelean, Alexandru"  wrote:
> 
> > On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> > > Only the ad778x have the 'gain' status bit. Check it before updating
> > > through a new variable is_ad778x in chip_info.
> > >   
> > 
> > Looks good.
> 
> Alex, formal tags definitely preferred!  It's not as though a
> looks good is any less of a review than an Ack, it's just better
> hidden as people need to look at mailing list archives...
> 
> Jonathan
> 

Acked-by: Alexandru Ardelean 

// Will remember that next time :)

Thanks
Alex

> > 
> > Alex
> > 
> > > Signed-off-by: Giuliano Belinassi 
> > > ---
> > > Changes in v2:
> > >   - Squashed is_ad778x declaration commit with the ad778x checkage
> > >   - Changed is_ad778x type to bool
> > >  
> > >  drivers/staging/iio/adc/ad7780.c | 15 +++
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > b/drivers/staging/iio/adc/ad7780.c
> > > index 91e016d534ed..9ec2b002539e 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -35,6 +35,7 @@ struct ad7780_chip_info {
> > >   struct iio_chan_specchannel;
> > >   unsigned intpattern_mask;
> > >   unsigned intpattern;
> > > + boolis_ad778x;
> > >  };
> > >  
> > >  struct ad7780_state {
> > > @@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct
> > > ad_sigma_delta *sigma_delta,
> > >   ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
> > >   return -EIO;
> > >  
> > > - if (raw_sample & AD7780_GAIN)
> > > - st->gain = 1;
> > > - else
> > > - st->gain = 128;
> > > + if (chip_info->is_ad778x) {
> > > + if (raw_sample & AD7780_GAIN)
> > > + st->gain = 1;
> > > + else
> > > + st->gain = 128;
> > > + }
> > >  
> > >   return 0;
> > >  }
> > > @@ -135,21 +138,25 @@ static const struct ad7780_chip_info
> > > ad7780_chip_info_tbl[] = {
> > >   .channel = AD7780_CHANNEL(12, 24),
> > >   .pattern = 0x5,
> > >   .pattern_mask = 0x7,
> > > + .is_ad778x = false,
> > >   },
> > >   [ID_AD7171] = {
> > >   .channel = AD7780_CHANNEL(16, 24),
> > >   .pattern = 0x5,
> > >   .pattern_mask = 0x7,
> > > + .is_ad778x = false,
> > >   },
> > >   [ID_AD7780] = {
> > >   .channel = AD7780_CHANNEL(24, 32),
> > >   .pattern = 0x1,
> > >   .pattern_mask = 0x3,
> > > + .is_ad778x = true,
> > >   },
> > >   [ID_AD7781] = {
> > >   .channel = AD7780_CHANNEL(20, 32),
> > >   .pattern = 0x1,
> > >   .pattern_mask = 0x3,
> > > + .is_ad778x = true,
> > >   },
> > >  };
> > >
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 1/2] staging: iio: ad7780: update voltage on read

[Ardelean, Alexandru]

On Sun, 2018-11-11 at 14:30 +, Jonathan Cameron wrote:
> On Tue, 6 Nov 2018 09:24:44 +
> "Ardelean, Alexandru"  wrote:
> 
> > On Mon, 2018-11-05 at 17:14 -0200, Renato Lui Geh wrote:
> > > The ad7780 driver previously did not read the correct device output,
> > > as
> > > it read an outdated value set at initialization. It now updates its
> > > voltage on read.
> > >   
> 
> Applied to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to check it.  I'm not that fussed about pushing this
> one as a fix, as most of the time these things are on fixed regulators
> anyway, but it is nice to do it right.
> 
> > Looks good from my side.
> 
> Ack?

Acked-by: Alexandru Ardelean 

> 
> Much preferred if you are willing to give a formal acknowledgement.
> 
> Thanks,
> 
> Jonathan
> 
> 
> > 
> > Alex
> > 
> > > Signed-off-by: Renato Lui Geh 
> > > ---
> > > Changes in v3:
> > >   - removed initialization (int voltage_uv = 0)
> > >   - returns error when voltage_uv is null
> > > Changes in v4:
> > >   - returns error when voltage_uv is negative
> > > 
> > >  drivers/staging/iio/adc/ad7780.c | 6 +-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > b/drivers/staging/iio/adc/ad7780.c
> > > index b67412db0318..c7cb05cedbbc 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev
> > > *indio_dev,
> > >  long m)
> > >  {
> > >   struct ad7780_state *st = iio_priv(indio_dev);
> > > + int voltage_uv;
> > >  
> > >   switch (m) {
> > >   case IIO_CHAN_INFO_RAW:
> > >   return ad_sigma_delta_single_conversion(indio_dev, chan,
> > > val);
> > >   case IIO_CHAN_INFO_SCALE:
> > > - *val = st->int_vref_mv * st->gain;
> > > + voltage_uv = regulator_get_voltage(st->reg);
> > > + if (voltage_uv < 0)
> > > + return voltage_uv;
> > > + *val = (voltage_uv / 1000) * st->gain;
> > >   *val2 = chan->scan_type.realbits - 1;
> > >   return IIO_VAL_FRACTIONAL_LOG2;
> > >   case IIO_CHAN_INFO_OFFSET:  
> 
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface

[Ardelean, Alexandru]

On Fri, 2018-11-09 at 13:05 +0530, Nishad Kamdar wrote:
> Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> instead of the deprecated old non-descriptor interface.
> 

Patch looks good.
I do have some thoughts about it. See inline.


> Signed-off-by: Nishad Kamdar 
> ---
>  drivers/staging/iio/adc/ad7816.c | 80 ++--
>  1 file changed, 34 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c
> b/drivers/staging/iio/adc/ad7816.c
> index bf76a8620bdb..12c4e0ab4713 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -7,7 +7,7 @@
>   */
>  
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -44,9 +44,9 @@
>  
>  struct ad7816_chip_info {
>   struct spi_device *spi_dev;
> - u16 rdwr_pin;
> - u16 convert_pin;
> - u16 busy_pin;
> + struct gpio_desc *rdwr_pin;
> + struct gpio_desc *convert_pin;
> + struct gpio_desc *busy_pin;

Only if you want to do a re-spin, here's an idea.
If you don't want to, feel free to disregard, since your patch is good.

You could compact things a bit; I know this wasn't what the initial code
was doing, but it's an option.

Something like:

enum {
GPIO_RWDR,
GPIO_CONVERT,
GPIO_BUSY,
__GPIO_MAX,
}

Then, what you end up having is:

 struct ad7816_chip_info {
struct spi_device *spi_dev;
 -  u16 rdwr_pin;
 -  u16 convert_pin;
 -  u16 busy_pin;
 +  struct gpio_desc *gpios[__GPIO_MAX];
 

// Continued below


>   u8  oti_data[AD7816_CS_MAX + 1];
>   u8  channel_id; /* 0 always be temperature */
>   u8  mode;
> @@ -61,28 +61,28 @@ static int ad7816_spi_read(struct ad7816_chip_info
> *chip, u16 *data)
>   int ret = 0;
>   __be16 buf;
>  
> - gpio_set_value(chip->rdwr_pin, 1);
> - gpio_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 0);

Obviously, in the above context, this becomes:
 +  gpiod_set_value(chip->gpios[GPIO_RWDR], 1);
 +  gpiod_set_value(chip->gpios[GPIO_RWDR], 0);

>   ret = spi_write(spi_dev, >channel_id, sizeof(chip-
> >channel_id));
>   if (ret < 0) {
>   dev_err(_dev->dev, "SPI channel setting error\n");
>   return ret;
>   }
> - gpio_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 1);
>  
>   if (chip->mode == AD7816_PD) { /* operating mode 2 */
> - gpio_set_value(chip->convert_pin, 1);
> - gpio_set_value(chip->convert_pin, 0);
> + gpiod_set_value(chip->convert_pin, 1);
> + gpiod_set_value(chip->convert_pin, 0);
>   } else { /* operating mode 1 */
> - gpio_set_value(chip->convert_pin, 0);
> - gpio_set_value(chip->convert_pin, 1);
> + gpiod_set_value(chip->convert_pin, 0);
> + gpiod_set_value(chip->convert_pin, 1);
>   }
>  
> - while (gpio_get_value(chip->busy_pin))
> + while (gpiod_get_value(chip->busy_pin))
>   cpu_relax();
>  
> - gpio_set_value(chip->rdwr_pin, 0);
> - gpio_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 1);
>   ret = spi_read(spi_dev, , sizeof(*data));
>   if (ret < 0) {
>   dev_err(_dev->dev, "SPI data read error\n");
> @@ -99,8 +99,8 @@ static int ad7816_spi_write(struct ad7816_chip_info
> *chip, u8 data)
>   struct spi_device *spi_dev = chip->spi_dev;
>   int ret = 0;
>  
> - gpio_set_value(chip->rdwr_pin, 1);
> - gpio_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 0);
>   ret = spi_write(spi_dev, , sizeof(data));
>   if (ret < 0)
>   dev_err(_dev->dev, "SPI oti data write error\n");
> @@ -129,10 +129,10 @@ static ssize_t ad7816_store_mode(struct device
> *dev,
>   struct ad7816_chip_info *chip = iio_priv(indio_dev);
>  
>   if (strcmp(buf, "full")) {
> - gpio_set_value(chip->rdwr_pin, 1);
> + gpiod_set_value(chip->rdwr_pin, 1);
>   chip->mode = AD7816_FULL;
>   } else {
> - gpio_set_value(chip->rdwr_pin, 0);
> + gpiod_set_value(chip->rdwr_pin, 0);
>   chip->mode = AD7816_PD;
>   }
>  
> @@ -345,15 +345,9 @@ static int ad7816_probe(struct spi_device *spi_dev)
>  {
>   struct ad7816_chip_info *chip;
>   struct iio_dev *indio_dev;
> - unsigned short *pins = dev_get_platdata(_dev->dev);
>   int ret = 0;
>   int i;
>  
> - if (!pins) {
> - dev_err(_dev->dev, "No necessary GPIO platform
> data.\n");
> - return -EINVAL;
> - }
> -
>   indio_dev = devm_iio_device_alloc(_dev->dev, sizeof(*chip));
>   if (!indio_dev)
>   return -ENOMEM;
> @@ -364,34 +358,28 @@ static int ad7816_probe(struct spi_device 

Re: [PATCH v3 4/4] staging: iio: ad7816: Add device tree table.

[Ardelean, Alexandru]

On Fri, 2018-11-09 at 13:08 +0530, Nishad Kamdar wrote:
> Add device tree table for matching vendor ID.

One comment inline for this.

Thanks
Alex

> 
> Signed-off-by: Nishad Kamdar 
> ---
>  drivers/staging/iio/adc/ad7816.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c
> b/drivers/staging/iio/adc/ad7816.c
> index a2fead85cd46..b8a9149fbac1 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -422,6 +422,12 @@ static int ad7816_probe(struct spi_device *spi_dev)
>   return 0;
>  }
>  
> +static const struct of_device_id ad7816_of_match[] = {
> + { .compatible = "adi,ad7816", },

I think you also need to add:
+   { .compatible = "adi,ad7817", },
+   { .compatible = "adi,ad7818", },

> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ad7816_of_match);
> +
>  static const struct spi_device_id ad7816_id[] = {
>   { "ad7816", ID_AD7816 },
>   { "ad7817", ID_AD7817 },
> @@ -434,6 +440,7 @@ MODULE_DEVICE_TABLE(spi, ad7816_id);
>  static struct spi_driver ad7816_driver = {
>   .driver = {
>   .name = "ad7816",
> + .of_match_table = of_match_ptr(ad7816_of_match),
>   },
>   .probe = ad7816_probe,
>   .id_table = ad7816_id,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 1/4] staging: iio: ad7816: Switch to the gpio descriptor interface

[Ardelean, Alexandru]

On Fri, 2018-11-09 at 08:05 +, Ardelean, Alexandru wrote:
> On Fri, 2018-11-09 at 13:05 +0530, Nishad Kamdar wrote:
> > Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> > instead of the deprecated old non-descriptor interface.
> > 
> 
> Patch looks good.
> I do have some thoughts about it. See inline.
> 

Nevermind what I just said here; I just saw patch3: `iio: ad7816: Set RD/WR
pin and CONVST pin as outputs.`

This looks good as is in the context of the patch series.

> 
> > Signed-off-by: Nishad Kamdar 
> > ---
> >  drivers/staging/iio/adc/ad7816.c | 80 ++--
> >  1 file changed, 34 insertions(+), 46 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7816.c
> > b/drivers/staging/iio/adc/ad7816.c
> > index bf76a8620bdb..12c4e0ab4713 100644
> > --- a/drivers/staging/iio/adc/ad7816.c
> > +++ b/drivers/staging/iio/adc/ad7816.c
> > @@ -7,7 +7,7 @@
> >   */
> >  
> >  #include 
> > -#include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -44,9 +44,9 @@
> >  
> >  struct ad7816_chip_info {
> > struct spi_device *spi_dev;
> > -   u16 rdwr_pin;
> > -   u16 convert_pin;
> > -   u16 busy_pin;
> > +   struct gpio_desc *rdwr_pin;
> > +   struct gpio_desc *convert_pin;
> > +   struct gpio_desc *busy_pin;
> 
> Only if you want to do a re-spin, here's an idea.
> If you don't want to, feel free to disregard, since your patch is good.
> 
> You could compact things a bit; I know this wasn't what the initial code
> was doing, but it's an option.
> 
> Something like:
> 
> enum {
>   GPIO_RWDR,
>   GPIO_CONVERT,
>   GPIO_BUSY,
>   __GPIO_MAX,
> }
> 
> Then, what you end up having is:
> 
>  struct ad7816_chip_info {
>   struct spi_device *spi_dev;
>  -u16 rdwr_pin;
>  -u16 convert_pin;
>  -u16 busy_pin;
>  +struct gpio_desc *gpios[__GPIO_MAX];
>  
> 
> // Continued below
> 
> 
> > u8  oti_data[AD7816_CS_MAX + 1];
> > u8  channel_id; /* 0 always be temperature */
> > u8  mode;
> > @@ -61,28 +61,28 @@ static int ad7816_spi_read(struct ad7816_chip_info
> > *chip, u16 *data)
> > int ret = 0;
> > __be16 buf;
> >  
> > -   gpio_set_value(chip->rdwr_pin, 1);
> > -   gpio_set_value(chip->rdwr_pin, 0);
> > +   gpiod_set_value(chip->rdwr_pin, 1);
> > +   gpiod_set_value(chip->rdwr_pin, 0);
> 
> Obviously, in the above context, this becomes:
>  +gpiod_set_value(chip->gpios[GPIO_RWDR], 1);
>  +gpiod_set_value(chip->gpios[GPIO_RWDR], 0);
> 
> > ret = spi_write(spi_dev, >channel_id, sizeof(chip-
> > > channel_id));
> > 
> > if (ret < 0) {
> > dev_err(_dev->dev, "SPI channel setting error\n");
> > return ret;
> > }
> > -   gpio_set_value(chip->rdwr_pin, 1);
> > +   gpiod_set_value(chip->rdwr_pin, 1);
> >  
> > if (chip->mode == AD7816_PD) { /* operating mode 2 */
> > -   gpio_set_value(chip->convert_pin, 1);
> > -   gpio_set_value(chip->convert_pin, 0);
> > +   gpiod_set_value(chip->convert_pin, 1);
> > +   gpiod_set_value(chip->convert_pin, 0);
> > } else { /* operating mode 1 */
> > -   gpio_set_value(chip->convert_pin, 0);
> > -   gpio_set_value(chip->convert_pin, 1);
> > +   gpiod_set_value(chip->convert_pin, 0);
> > +   gpiod_set_value(chip->convert_pin, 1);
> > }
> >  
> > -   while (gpio_get_value(chip->busy_pin))
> > +   while (gpiod_get_value(chip->busy_pin))
> > cpu_relax();
> >  
> > -   gpio_set_value(chip->rdwr_pin, 0);
> > -   gpio_set_value(chip->rdwr_pin, 1);
> > +   gpiod_set_value(chip->rdwr_pin, 0);
> > +   gpiod_set_value(chip->rdwr_pin, 1);
> > ret = spi_read(spi_dev, , sizeof(*data));
> > if (ret < 0) {
> > dev_err(_dev->dev, "SPI data read error\n");
> > @@ -99,8 +99,8 @@ static int ad7816_spi_write(struct ad7816_chip_info
> > *chip, u8 data)
> > struct spi_device *spi_dev = chip->spi_dev;
> > int ret = 0;
> >  
> > -   gpio_set_value(chip->rdwr_pin, 1);
> > -   gpio_set_value(chip->rdwr_pin, 0);
> > +   gpiod_set_value(chip->rdwr_pin, 1);
> > +   gpiod_set_value(chip->rdwr_pin, 0);
> > ret = spi_write(spi_dev, , sizeof(data));
> > if (ret < 0)
> >

Re: [PATCH v2 2/2] staging: iio: ad7780: generates pattern_mask from PAT bits

[Ardelean, Alexandru]

On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> Previously, all pattern_masks and patterns in the chip_info table were
> hardcoded. Now they are generated using the PAT macros, as described in
> the datasheets.

One comment about indentation/whitespace.

Rest looks good.

Alex

> 
> Signed-off-by: Giuliano Belinassi 
> ---
> Changes in v2:
>   - Added the PATTERN and PATTERN_MASK macros
>   - Update BIT macros alignment to match with PATTERN
>   - Generate pattern mask with PATTERN_MASK macros.
> 
> drivers/staging/iio/adc/ad7780.c | 40 +++-
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 9ec2b002539e..ff8e3b2d0efc 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -22,14 +22,22 @@
>  #include 
>  #include 
>  
> -#define AD7780_RDY   BIT(7)
> -#define AD7780_FILTERBIT(6)
> -#define AD7780_ERR   BIT(5)
> -#define AD7780_ID1   BIT(4)
> -#define AD7780_ID0   BIT(3)
> -#define AD7780_GAIN  BIT(2)
> -#define AD7780_PAT1  BIT(1)
> -#define AD7780_PAT0  BIT(0)
> +#define AD7780_RDY   BIT(7)
> +#define AD7780_FILTERBIT(6)
> +#define AD7780_ERR   BIT(5)
> +#define AD7780_ID1   BIT(4)
> +#define AD7780_ID0   BIT(3)
> +#define AD7780_GAIN  BIT(2)
> +#define AD7780_PAT1  BIT(1)
> +#define AD7780_PAT0  BIT(0)

Changing indentation here doesn't add much value; it's mostly
patch/whitespace noise.

While I agree that it looks nicer to indent all these to the same level,
you also need to think about the fact that the kernel git repo is already
pretty big as-is, so it's a good idea if a patch adds as much code/semantic
value [as possible] with as little change [as possible] and with as good of
readability [as possible].
[ Kind of sounds like a balance act between the 3 things ].


> +
> +#define AD7780_PATTERN   (AD7780_PAT0)
> +#define AD7780_PATTERN_MASK  (AD7780_PAT0 | AD7780_PAT1)
> +
> +#define AD7170_PAT2  BIT(2)
> +
> +#define AD7170_PATTERN   (AD7780_PAT0 | AD7170_PAT2)
> +#define AD7170_PATTERN_MASK  (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
>  
>  struct ad7780_chip_info {
>   struct iio_chan_specchannel;
> @@ -136,26 +144,26 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
>  static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
>   [ID_AD7170] = {
>   .channel = AD7780_CHANNEL(12, 24),
> - .pattern = 0x5,
> - .pattern_mask = 0x7,
> + .pattern = AD7170_PATTERN,
> + .pattern_mask = AD7170_PATTERN_MASK,
>   .is_ad778x = false,
>   },
>   [ID_AD7171] = {
>   .channel = AD7780_CHANNEL(16, 24),
> - .pattern = 0x5,
> - .pattern_mask = 0x7,
> + .pattern = AD7170_PATTERN,
> + .pattern_mask = AD7170_PATTERN_MASK,
>   .is_ad778x = false,
>   },
>   [ID_AD7780] = {
>   .channel = AD7780_CHANNEL(24, 32),
> - .pattern = 0x1,
> - .pattern_mask = 0x3,
> + .pattern = AD7780_PATTERN,
> + .pattern_mask = AD7780_PATTERN_MASK,
>   .is_ad778x = true,
>   },
>   [ID_AD7781] = {
>   .channel = AD7780_CHANNEL(20, 32),
> - .pattern = 0x1,
> - .pattern_mask = 0x3,
> + .pattern = AD7780_PATTERN,
> + .pattern_mask = AD7780_PATTERN_MASK,
>   .is_ad778x = true,
>   },
>  };
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] staging: iio: ad7780: check if ad778x before gain update

[Ardelean, Alexandru]

On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> Only the ad778x have the 'gain' status bit. Check it before updating
> through a new variable is_ad778x in chip_info.
> 

Looks good.

Alex

> Signed-off-by: Giuliano Belinassi 
> ---
> Changes in v2:
>   - Squashed is_ad778x declaration commit with the ad778x checkage
>   - Changed is_ad778x type to bool
>  
>  drivers/staging/iio/adc/ad7780.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 91e016d534ed..9ec2b002539e 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -35,6 +35,7 @@ struct ad7780_chip_info {
>   struct iio_chan_specchannel;
>   unsigned intpattern_mask;
>   unsigned intpattern;
> + boolis_ad778x;
>  };
>  
>  struct ad7780_state {
> @@ -113,10 +114,12 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
>   ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
>   return -EIO;
>  
> - if (raw_sample & AD7780_GAIN)
> - st->gain = 1;
> - else
> - st->gain = 128;
> + if (chip_info->is_ad778x) {
> + if (raw_sample & AD7780_GAIN)
> + st->gain = 1;
> + else
> + st->gain = 128;
> + }
>  
>   return 0;
>  }
> @@ -135,21 +138,25 @@ static const struct ad7780_chip_info
> ad7780_chip_info_tbl[] = {
>   .channel = AD7780_CHANNEL(12, 24),
>   .pattern = 0x5,
>   .pattern_mask = 0x7,
> + .is_ad778x = false,
>   },
>   [ID_AD7171] = {
>   .channel = AD7780_CHANNEL(16, 24),
>   .pattern = 0x5,
>   .pattern_mask = 0x7,
> + .is_ad778x = false,
>   },
>   [ID_AD7780] = {
>   .channel = AD7780_CHANNEL(24, 32),
>   .pattern = 0x1,
>   .pattern_mask = 0x3,
> + .is_ad778x = true,
>   },
>   [ID_AD7781] = {
>   .channel = AD7780_CHANNEL(20, 32),
>   .pattern = 0x1,
>   .pattern_mask = 0x3,
> + .is_ad778x = true,
>   },
>  };
>  
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 3/3] staging: iio: ad7780: generates pattern_mask from PAT bits

[Ardelean, Alexandru]

On Wed, 2018-11-07 at 16:50 -0200, Giuliano Belinassi wrote:
> Previously, all pattern_masks in the chip_info table were hardcoded. Now
> they
> are generated using the PAT macros, as described in the datasheets.
> 

I like this change :)
I only have nitpicks.
See inline.


> Signed-off-by: Giuliano Belinassi 
> ---
>  drivers/staging/iio/adc/ad7780.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 0a473aae52f2..fa9e047b5191 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -31,6 +31,8 @@
>  #define AD7780_PAT1  BIT(1)
>  #define AD7780_PAT0  BIT(0)
>  
> +#define AD7170_PAT2  BIT(2)
> +
>  struct ad7780_chip_info {
>   struct iio_chan_specchannel;
>   unsigned intpattern_mask;
> @@ -137,25 +139,25 @@ static const struct ad7780_chip_info
> ad7780_chip_info_tbl[] = {
>   [ID_AD7170] = {
>   .channel = AD7780_CHANNEL(12, 24),
>   .pattern = 0x5,
> - .pattern_mask = 0x7,
> + .pattern_mask = AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2,

If you are updating these pattern masks, you should update the default
patterns as well with the macros to be consistent.

And to be a bit more compact, you could define:

#define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
#d
efine AD7780_PATTERN_MASK   (AD7780_PAT0 | AD7780_PAT1)

#define AD7170_PATTERN  (AD7780_PAT1 | AD7170_PAT2)
#define AD7780_PATTERN  (AD7780_PAT0)

Then you can assign AD7170_PATTERN[_MASK] to AD7170/AD7171 & 
AD7780_PATTERN[_MASK] to AD7780/AD7781.

>   .is_ad778x = false,
>   },
>   [ID_AD7171] = {
>   .channel = AD7780_CHANNEL(16, 24),
>   .pattern = 0x5,
> - .pattern_mask = 0x7,
> + .pattern_mask = AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2,
>   .is_ad778x = false,
>   },
>   [ID_AD7780] = {
>   .channel = AD7780_CHANNEL(24, 32),
>   .pattern = 0x1,
> - .pattern_mask = 0x3,
> + .pattern_mask = AD7780_PAT0 | AD7780_PAT1,
>   .is_ad778x = true,
>   },
>   [ID_AD7781] = {
>   .channel = AD7780_CHANNEL(20, 32),
>   .pattern = 0x1,
> - .pattern_mask = 0x3,
> + .pattern_mask = AD7780_PAT0 | AD7780_PAT1,
>   .is_ad778x = true,
>   },
>  };
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/3] staging: iio: ad7780: check if ad778x before gain update

[Ardelean, Alexandru]

On Wed, 2018-11-07 at 16:50 -0200, Giuliano Belinassi wrote:
> Only the ad778x have the 'gain' status bit. Check it before updating.
> 

This looks good.
The only note is that it can be squashed with the 1st patch (which I noted
on the 1st patch).

> Signed-off-by: Giuliano Belinassi 
> ---
>  drivers/staging/iio/adc/ad7780.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 6e51bfdb076a..0a473aae52f2 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -114,10 +114,12 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
>   ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
>   return -EIO;
>  
> - if (raw_sample & AD7780_GAIN)
> - st->gain = 1;
> - else
> - st->gain = 128;
> + if (chip_info->is_ad778x) {
> + if (raw_sample & AD7780_GAIN)
> + st->gain = 1;
> + else
> + st->gain = 128;
> + }
>  
>   return 0;
>  }
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] staging: iio: ad7780: Add is_ad778x flag chip info

[Ardelean, Alexandru]

On Wed, 2018-11-07 at 16:49 -0200, Giuliano Belinassi wrote:
> This patch allows further checking of whatever the chip is (ad778x or
> ad717x).

Hey,

The patch looks good overall.
I only have one nitpick for this patch. See inline.

And you can squash this patch with patch `[PATCH 2/3] staging: iio: ad7780:
check if ad778x before gain update`.
In fact, the title of the squashed patch can just be `staging: iio: ad7780:
check if ad778x before gain update` ; because the code in this patch
implies that it's used to check if the device is an ad778x chip.

This patch doesn't have much value on it's own without the 2nd patch, and
you can do them in a single go.

/*
 * Note: yes, I know that these subtle semantics between patch 
 * splitting & squashing can be a bit annoying ; I don't have a general
 * recommendation for them, other than just to keep sending patches
 */

Thanks
Alex

> 
> Signed-off-by: Giuliano Belinassi 
> ---
>  drivers/staging/iio/adc/ad7780.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 91e016d534ed..6e51bfdb076a 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -35,6 +35,7 @@ struct ad7780_chip_info {
>   struct iio_chan_specchannel;
>   unsigned intpattern_mask;
>   unsigned intpattern;
> + u8  is_ad778x;
You could make this `bool` type since you are assigning `true/false` values
to this field. It's generally good to be consistent between type names &
type values when using them [even though in C, these are pretty much the
same].

>  };
>  
>  struct ad7780_state {
> @@ -135,21 +136,25 @@ static const struct ad7780_chip_info
> ad7780_chip_info_tbl[] = {
>   .channel = AD7780_CHANNEL(12, 24),
>   .pattern = 0x5,
>   .pattern_mask = 0x7,
> + .is_ad778x = false,
>   },
>   [ID_AD7171] = {
>   .channel = AD7780_CHANNEL(16, 24),
>   .pattern = 0x5,
>   .pattern_mask = 0x7,
> + .is_ad778x = false,
>   },
>   [ID_AD7780] = {
>   .channel = AD7780_CHANNEL(24, 32),
>   .pattern = 0x1,
>   .pattern_mask = 0x3,
> + .is_ad778x = true,
>   },
>   [ID_AD7781] = {
>   .channel = AD7780_CHANNEL(20, 32),
>   .pattern = 0x1,
>   .pattern_mask = 0x3,
> + .is_ad778x = true,
>   },
>  };
>  
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 2/2] staging: iio: ad7780: remove unnecessary stashed voltage value

[Ardelean, Alexandru]

On Mon, 2018-11-05 at 17:16 -0200, Renato Lui Geh wrote:
> This patch removes the unnecessary field int_vref_mv in ad7780_state
> referring to the device's voltage.
> 

Looks good from my side.

Alex

> Signed-off-by: Renato Lui Geh 
> ---
> Changes in v3:
>   - removed unnecessary int_vref_mv from ad7780_state
> Changes in v4:
>   - removed voltage reading on probe
> 
>  drivers/staging/iio/adc/ad7780.c | 9 +
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index c7cb05cedbbc..52a914360574 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -42,7 +42,6 @@ struct ad7780_state {
>   struct regulator*reg;
>   struct gpio_desc*powerdown_gpio;
>   unsigned intgain;
> - u16 int_vref_mv;
>  
>   struct ad_sigma_delta sd;
>  };
> @@ -165,7 +164,7 @@ static int ad7780_probe(struct spi_device *spi)
>  {
>   struct ad7780_state *st;
>   struct iio_dev *indio_dev;
> - int ret, voltage_uv = 0;
> + int ret;
>  
>   indio_dev = devm_iio_device_alloc(>dev, sizeof(*st));
>   if (!indio_dev)
> @@ -185,16 +184,10 @@ static int ad7780_probe(struct spi_device *spi)
>   dev_err(>dev, "Failed to enable specified AVdd
> supply\n");
>   return ret;
>   }
> - voltage_uv = regulator_get_voltage(st->reg);
>  
>   st->chip_info =
>   _chip_info_tbl[spi_get_device_id(spi)->driver_data];
>  
> - if (voltage_uv)
> - st->int_vref_mv = voltage_uv / 1000;
> - else
> - dev_warn(>dev, "Reference voltage unspecified\n");
> -
>   spi_set_drvdata(spi, indio_dev);
>  
>   indio_dev->dev.parent = >dev;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 1/2] staging: iio: ad7780: update voltage on read

[Ardelean, Alexandru]

On Mon, 2018-11-05 at 17:14 -0200, Renato Lui Geh wrote:
> The ad7780 driver previously did not read the correct device output, as
> it read an outdated value set at initialization. It now updates its
> voltage on read.
> 

Looks good from my side.

Alex

> Signed-off-by: Renato Lui Geh 
> ---
> Changes in v3:
>   - removed initialization (int voltage_uv = 0)
>   - returns error when voltage_uv is null
> Changes in v4:
>   - returns error when voltage_uv is negative
> 
>  drivers/staging/iio/adc/ad7780.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index b67412db0318..c7cb05cedbbc 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>  long m)
>  {
>   struct ad7780_state *st = iio_priv(indio_dev);
> + int voltage_uv;
>  
>   switch (m) {
>   case IIO_CHAN_INFO_RAW:
>   return ad_sigma_delta_single_conversion(indio_dev, chan,
> val);
>   case IIO_CHAN_INFO_SCALE:
> - *val = st->int_vref_mv * st->gain;
> + voltage_uv = regulator_get_voltage(st->reg);
> + if (voltage_uv < 0)
> + return voltage_uv;
> + *val = (voltage_uv / 1000) * st->gain;
>   *val2 = chan->scan_type.realbits - 1;
>   return IIO_VAL_FRACTIONAL_LOG2;
>   case IIO_CHAN_INFO_OFFSET:
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 1/3] staging: iio: ad7780: fix offset read value

[Ardelean, Alexandru]

Good catch.

Acked-by: Alexandru Ardelean 

On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> Variable val subtracted an uninitialized value on IIO_CHAN_INFO_OFFSET.
> This was fixed by assigning the correct value instead.
> 
> Signed-off-by: Renato Lui Geh 
> ---
>  drivers/staging/iio/adc/ad7780.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index b67412db0318..91e016d534ed 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -96,7 +96,7 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>   *val2 = chan->scan_type.realbits - 1;
>   return IIO_VAL_FRACTIONAL_LOG2;
>   case IIO_CHAN_INFO_OFFSET:
> - *val -= (1 << (chan->scan_type.realbits - 1));
> + *val = -(1 << (chan->scan_type.realbits - 1));
>   return IIO_VAL_INT;
>   }
>  
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 2/3] staging: iio: ad7780: update voltage on read

[Ardelean, Alexandru]

On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> The ad7780 driver previously did not read the correct device output, as
> it read an outdated value set at initialization. It now updates its
> voltage on read.
> 
> Signed-off-by: Renato Lui Geh 
> ---
> Changes in v3:
>   - removed initialization (int voltage_uv = 0)
>   - returns error when voltage_uv is null
> 
>  drivers/staging/iio/adc/ad7780.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 91e016d534ed..f2a11e9424cd 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -87,12 +87,16 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>  long m)
>  {
>   struct ad7780_state *st = iio_priv(indio_dev);
> + int voltage_uv;
>  
>   switch (m) {
>   case IIO_CHAN_INFO_RAW:
>   return ad_sigma_delta_single_conversion(indio_dev, chan,
> val);
>   case IIO_CHAN_INFO_SCALE:
> - *val = st->int_vref_mv * st->gain;
> + voltage_uv = regulator_get_voltage(st->reg);
> + if (!voltage_uv)

This looks wrong.
I admit this was done in the same way in the probe function, but that looks
a bit wrong as well.

Typically, the return value of `regulator_get_voltage()` would get checked
with:

ret = regulator_get_voltage(st->reg);
if (ret < 0)
   return ret;
*val = ret / 1000;

So, negative values are errors and zero & positive values are valid voltage
values.

> + return -EINVAL;
> + *val = (voltage_uv / 1000) * st->gain;
>   *val2 = chan->scan_type.realbits - 1;
>   return IIO_VAL_FRACTIONAL_LOG2;
>   case IIO_CHAN_INFO_OFFSET:
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 3/3] staging: iio: ad7780: remove unnecessary stashed voltage value

[Ardelean, Alexandru]

On Thu, 2018-11-01 at 11:43 -0300, Renato Lui Geh wrote:
> This patch removes the unnecessary field int_vref_mv in ad7780_state
> referring to the device's voltage.
> 
> Signed-off-by: Renato Lui Geh 
> ---
> Changes in v3:
>   - removed unnecessary int_vref_mv from ad7780_state
> 
>  drivers/staging/iio/adc/ad7780.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index f2a11e9424cd..f250370efcf7 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -42,7 +42,6 @@ struct ad7780_state {
>   struct regulator*reg;
>   struct gpio_desc*powerdown_gpio;
>   unsigned intgain;
> - u16 int_vref_mv;
>  
>   struct ad_sigma_delta sd;
>  };
> @@ -190,9 +189,7 @@ static int ad7780_probe(struct spi_device *spi)
>   st->chip_info =
>   _chip_info_tbl[spi_get_device_id(spi)->driver_data];
>  
> - if (voltage_uv)
> - st->int_vref_mv = voltage_uv / 1000;
> - else
> + if (!voltage_uv)
>   dev_warn(>dev, "Reference voltage unspecified\n");

I think you could remove this altogether, and also remove the entire
`voltage_uv = regulator_get_voltage(st->reg);` assignment.

It doesn't make much sense to read the voltage here, since it won't be used
in the probe part anymore.

>  
>   spi_set_drvdata(spi, indio_dev);
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] Staging: iio: ade7758: Remove iio_dev mlock

[Ardelean, Alexandru]

On Tue, 2018-02-06 at 01:08 +0530, Shreeya Patel wrote:
> In the ade7758 file, read raw does not require iio_dev->mlock for
> reads. It can run concurrently as resource protection is handled
> by buf_lock in read register.

Removed linux-kernel list and Greg from reply list.

> 
> Signed-off-by: Shreeya Patel 
> ---
>  drivers/staging/iio/meter/ade7758_core.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7758_core.c 
> b/drivers/staging/iio/meter/ade7758_core.c
> index 7b7ffe5..227dbfc 100644
> --- a/drivers/staging/iio/meter/ade7758_core.c
> +++ b/drivers/staging/iio/meter/ade7758_core.c
> @@ -526,9 +526,7 @@ static int ade7758_read_raw(struct iio_dev *indio_dev,
>  
>   switch (mask) {
>   case IIO_CHAN_INFO_SAMP_FREQ:
> - mutex_lock(_dev->mlock);
>   ret = ade7758_read_samp_freq(_dev->dev, val);
> - mutex_unlock(_dev->mlock);

this patch can be squashed into the second one
semantically they are the same

>   return ret;
>   default:
>   return -EINVAL;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 2/2] Staging: iio: ade7758: Expand buf_lock to cover both buffer and state protection

[Ardelean, Alexandru]

On Tue, 2018-02-06 at 01:10 +0530, Shreeya Patel wrote:
> iio_dev->mlock is to be used only by the IIO core for protecting
> device mode changes between INDIO_DIRECT and INDIO_BUFFER.
> 
> This patch replaces the use of mlock with the already established
> buf_lock mutex.
> 
> Introducing 'unlocked' forms of read and write registers. The
> read/write frequency functions now require buf_lock to be held.
> That's not obvious so avoid this but moving the locking inside
> the functions where it is then clear that they are taking the
> unlocked forms of the register read/write.
> 
> It isn't readily apparent that write frequency function requires
> the locks to be taken, so move it inside the function to where it
> is required to protect.

Removed linux-kernel list & Greg from reply list.

> 
> Signed-off-by: Shreeya Patel 
> ---
> 
> Changes in v2
>   -Add static keyword to newly introduced functions and remove some
> added comments which are not required.
> 
> Changes in v3
>   -Remove some useless mlocks and send it as another patch.
> Also make the necessary change in the current patch associated with 
> the new patch with commit id 88eba33. Make commit message more 
> appropriate.
> 
> Changes in v4
>   -Write frequency function do not require lock so move it inside
> the function to where it is required to protect.
> 
> Changes in v5
>   -Remove goto statement and make the code to return -EINVAL there
> itself. 
> 
>  drivers/staging/iio/meter/ade7758.h  |  2 +-
>  drivers/staging/iio/meter/ade7758_core.c | 52 +++--
> ---
>  2 files changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7758.h
> b/drivers/staging/iio/meter/ade7758.h
> index 6ae78d8..2de81b5 100644
> --- a/drivers/staging/iio/meter/ade7758.h
> +++ b/drivers/staging/iio/meter/ade7758.h
> @@ -111,7 +111,7 @@
>   * @trig:data ready trigger registered with iio
>   * @tx:  transmit buffer
>   * @rx:  receive buffer
> - * @buf_lock:mutex to protect tx and rx
> + * @buf_lock:mutex to protect tx, rx, read and write
> frequency
>   **/
>  struct ade7758_state {
>   struct spi_device   *us;
> diff --git a/drivers/staging/iio/meter/ade7758_core.c
> b/drivers/staging/iio/meter/ade7758_core.c
> index 227dbfc..dfe8e97 100644
> --- a/drivers/staging/iio/meter/ade7758_core.c
> +++ b/drivers/staging/iio/meter/ade7758_core.c
> @@ -24,17 +24,25 @@
>  #include "meter.h"
>  #include "ade7758.h"
>  
> -int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
> +static int __ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8
> val)
>  {
> - int ret;
>   struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>   struct ade7758_state *st = iio_priv(indio_dev);
>  
> - mutex_lock(>buf_lock);
>   st->tx[0] = ADE7758_WRITE_REG(reg_address);
>   st->tx[1] = val;
>  
> - ret = spi_write(st->us, st->tx, 2);
> + return spi_write(st->us, st->tx, 2);
> +}
> +
> +int ade7758_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
> +{
> + int ret;
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct ade7758_state *st = iio_priv(indio_dev);
> +
> + mutex_lock(>buf_lock);
> + ret = __ade7758_spi_write_reg_8(dev, reg_address, val);
>   mutex_unlock(>buf_lock);
>  
>   return ret;
> @@ -91,7 +99,7 @@ static int ade7758_spi_write_reg_24(struct device *dev, u8
> reg_address,
>   return ret;
>  }
>  
> -int ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8 *val)
> +static int __ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8
> *val)
>  {
>   struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>   struct ade7758_state *st = iio_priv(indio_dev);
> @@ -111,7 +119,6 @@ int ade7758_spi_read_reg_8(struct device *dev, u8
> reg_address, u8 *val)
>   },
>   };
>  
> - mutex_lock(>buf_lock);
>   st->tx[0] = ADE7758_READ_REG(reg_address);
>   st->tx[1] = 0;
>  
> @@ -124,7 +131,19 @@ int ade7758_spi_read_reg_8(struct device *dev, u8
> reg_address, u8 *val)
>   *val = st->rx[0];
>  
>  error_ret:
> + return ret;
> +}
> +
> +int ade7758_spi_read_reg_8(struct device *dev, u8 reg_address, u8 *val)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct ade7758_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(>buf_lock);
> + ret = __ade7758_spi_read_reg_8(dev, reg_address, val);
>   mutex_unlock(>buf_lock);
> +
>   return ret;
>  }
>  
> @@ -480,10 +499,12 @@ static int ade7758_read_samp_freq(struct device *dev,
> int *val)
>   return 0;
>  }
>  
> -static int ade7758_write_samp_freq(struct device *dev, int val)
> +static int ade7758_write_samp_freq(struct iio_dev *indio_dev, int val)

Maybe keep the declaration the same.
The same construct for accessing indio_dev could be used as above.