Re: [PATCH] iio: at91-sama5d2_adc: remove usage of iio_priv_to_dev() helper

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 18 Jun 2020 13:34:13 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Thu, 2020-06-18 at 12:47 +0000, Eugen.Hristev@microchip.com wrote:
> > [External]
> > 
> > On 17.06.2020 17:02, Ardelean, Alexandru wrote:  
> > > On Wed, 2020-06-17 at 13:25 +0000, Eugen.Hristev@microchip.com wrote:  
> > > > On 31.05.2020 17:39, Jonathan Cameron wrote:
> > > >   
> > > > > On Mon, 25 May 2020 13:53:41 +0300
> > > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > > > >   
> > > > > > We may want to get rid of the iio_priv_to_dev() helper. The
> > > > > > reason is that
> > > > > > we will hide some of the members of the iio_dev structure (to
> > > > > > prevent
> > > > > > drivers from accessing them directly), and that will also mean
> > > > > > hiding the
> > > > > > implementation of the iio_priv_to_dev() helper inside the IIO
> > > > > > core.
> > > > > > 
> > > > > > Hiding the implementation of iio_priv_to_dev() implies that some
> > > > > > fast-
> > > > > > paths
> > > > > > may not be fast anymore, so a general idea is to try to get rid
> > > > > > of the
> > > > > > iio_priv_to_dev() altogether.
> > > > > > The iio_priv() helper won't be affected by the rework, as the
> > > > > > iio_dev
> > > > > > struct will keep a reference to the private information.
> > > > > > 
> > > > > > For this driver, not using iio_priv_to_dev(), means reworking
> > > > > > some paths
> > > > > > to
> > > > > > pass the iio device and using iio_priv() to access the private
> > > > > > information,
> > > > > > and also keeping a reference to the iio device for some quirky
> > > > > > paths.
> > > > > > 
> > > > > > One [quirky] path is the at91_adc_workq_handler() which requires
> > > > > > the IIO
> > > > > > device & the state struct to push to buffers.
> > > > > > Since this requires the back-ref to the IIO device, the
> > > > > > at91_adc_touch_pos() also uses it. This simplifies the patch a
> > > > > > bit. The
> > > > > > information required in this function is mostly for debugging
> > > > > > purposes.
> > > > > > Replacing it with a reference to the IIO device would have been a
> > > > > > slightly
> > > > > > bigger change, which may not be worth it (for just the debugging
> > > > > > purpose
> > > > > > and given that we need the back-ref to the IIO device anyway).  
> > > > > 
> > > > > That workq is indeed quirky.  This looks fine to me in general.
> > > > > I'll
> > > > > want an appropriate ack from the at91 side of things if possible so
> > > > > let's leave this on the list for a while longer.  
> > > > 
> > > > Hi,
> > > > 
> > > > I am available to test this patch,
> > > > Can you tell me on which branch to apply it. On 5.8-rc1 it fails for
> > > > me
> > > > (or maybe it needs rebasing ?)
> > > >   
> > > 
> > > Hmm, weird.
> > > I rebased the patches on Jonathan's iio/testing.
> > > It seemed to work.
> > > https://urldefense.com/v3/__https://github.com/commodo/linux/commits/iio-priv-to-dev__;!!A3Ni8CS0y2Y!oEVHsA6gf9ydBvAAjlhRV5QO1bMTZN2U_OXbor0gei7mWk14m64rilJ2WTAXvtWmGaisXQ$ 
> > > 
> > > As for which branch to test/apply. Not sure.
> > > Maybe Jonathan's iio/testing as base?
> > > Looks like it's based on 5.8.
> > >   
> > 
> > Tested-by: Eugen Hristev <eugen.hristev@microchip.com>
> > 
> > I have tested the major features of the driver (including the resistive 
> > touch) and it looks to be working fine.
> > 
> > I did not fully understand the quirkyness of the workq . Something you 
> > want to change to that ?  
> 
> Umm, not really.
> I did not follow that code too in-depth.
> And I would like not to.
> Mostly to avoid scaling myself in too many directions.
> 
> There may be a slightly better approach to it, but ¯\_(ツ)_/¯

I'm assuming no one minds me taking this as is.

Applied to the togreg branch of iio.git first pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> 
> >   
> > > > Eugen
> > > >   
> > > > > Poke me if no action in a few weeks.
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > Jonathan
> > > > >   
> > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > > > > ---
> > > > > >    drivers/iio/adc/at91-sama5d2_adc.c | 30 +++++++++++++++++-----
> > > > > > --------
> > > > > >    1 file changed, 17 insertions(+), 13 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c
> > > > > > b/drivers/iio/adc/at91-
> > > > > > sama5d2_adc.c
> > > > > > index 9abbbdcc7420..7bce18444430 100644
> > > > > > --- a/drivers/iio/adc/at91-sama5d2_adc.c
> > > > > > +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> > > > > > @@ -402,6 +402,7 @@ struct at91_adc_state {
> > > > > >         wait_queue_head_t               wq_data_available;
> > > > > >         struct at91_adc_dma             dma_st;
> > > > > >         struct at91_adc_touch           touch_st;
> > > > > > +     struct iio_dev                  *indio_dev;
> > > > > >         u16                             buffer[AT91_BUFFER_MAX_HW
> > > > > > ORDS];
> > > > > >         /*
> > > > > >          * lock to prevent concurrent 'single conversion'
> > > > > > requests through
> > > > > > @@ -642,13 +643,13 @@ static u16 at91_adc_touch_pos(struct
> > > > > > at91_adc_state
> > > > > > *st, int reg)
> > > > > >         /* first half of register is the x or y, second half is
> > > > > > the scale
> > > > > > */
> > > > > >         val = at91_adc_readl(st, reg);
> > > > > >         if (!val)
> > > > > > -             dev_dbg(&iio_priv_to_dev(st)->dev, "pos is 0\n");
> > > > > > +             dev_dbg(&st->indio_dev->dev, "pos is 0\n");
> > > > > > 
> > > > > >         pos = val & AT91_SAMA5D2_XYZ_MASK;
> > > > > >         result = (pos << AT91_SAMA5D2_MAX_POS_BITS) - pos;
> > > > > >         scale = (val >> 16) & AT91_SAMA5D2_XYZ_MASK;
> > > > > >         if (scale == 0) {
> > > > > > -             dev_err(&iio_priv_to_dev(st)->dev, "scale is 0\n");
> > > > > > +             dev_err(&st->indio_dev->dev, "scale is 0\n");
> > > > > >                 return 0;
> > > > > >         }
> > > > > >         result /= scale;
> > > > > > @@ -1204,9 +1205,9 @@ static unsigned
> > > > > > at91_adc_startup_time(unsigned
> > > > > > startup_time_min,
> > > > > >         return i;
> > > > > >    }
> > > > > > 
> > > > > > -static void at91_adc_setup_samp_freq(struct at91_adc_state *st,
> > > > > > unsigned
> > > > > > freq)
> > > > > > +static void at91_adc_setup_samp_freq(struct iio_dev *indio_dev,
> > > > > > unsigned
> > > > > > freq)
> > > > > >    {
> > > > > > -     struct iio_dev *indio_dev = iio_priv_to_dev(st);
> > > > > > +     struct at91_adc_state *st = iio_priv(indio_dev);
> > > > > >         unsigned f_per, prescal, startup, mr;
> > > > > > 
> > > > > >         f_per = clk_get_rate(st->per_clk);
> > > > > > @@ -1275,9 +1276,9 @@ static void
> > > > > > at91_adc_pen_detect_interrupt(struct
> > > > > > at91_adc_state *st)
> > > > > >         st->touch_st.touching = true;
> > > > > >    }
> > > > > > 
> > > > > > -static void at91_adc_no_pen_detect_interrupt(struct
> > > > > > at91_adc_state *st)
> > > > > > +static void at91_adc_no_pen_detect_interrupt(struct iio_dev
> > > > > > *indio_dev)
> > > > > >    {
> > > > > > -     struct iio_dev *indio_dev = iio_priv_to_dev(st);
> > > > > > +     struct at91_adc_state *st = iio_priv(indio_dev);
> > > > > > 
> > > > > >         at91_adc_writel(st, AT91_SAMA5D2_TRGR,
> > > > > >                         AT91_SAMA5D2_TRGR_TRGMOD_NO_TRIGGER);
> > > > > > @@ -1297,7 +1298,7 @@ static void at91_adc_workq_handler(struct
> > > > > > work_struct *workq)
> > > > > >                                         struct at91_adc_touch,
> > > > > > workq);
> > > > > >         struct at91_adc_state *st = container_of(touch_st,
> > > > > >                                         struct at91_adc_state,
> > > > > > touch_st);
> > > > > > -     struct iio_dev *indio_dev = iio_priv_to_dev(st);
> > > > > > +     struct iio_dev *indio_dev = st->indio_dev;
> > > > > > 
> > > > > >         iio_push_to_buffers(indio_dev, st->buffer);
> > > > > >    }
> > > > > > @@ -1318,7 +1319,7 @@ static irqreturn_t at91_adc_interrupt(int
> > > > > > irq, void
> > > > > > *private)
> > > > > >                 at91_adc_pen_detect_interrupt(st);
> > > > > >         } else if ((status & AT91_SAMA5D2_IER_NOPEN)) {
> > > > > >                 /* nopen detected IRQ */
> > > > > > -             at91_adc_no_pen_detect_interrupt(st);
> > > > > > +             at91_adc_no_pen_detect_interrupt(indio);
> > > > > >         } else if ((status & AT91_SAMA5D2_ISR_PENS) &&
> > > > > >                    ((status & rdy_mask) == rdy_mask)) {
> > > > > >                 /* periodic trigger IRQ - during pen sense */
> > > > > > @@ -1486,7 +1487,7 @@ static int at91_adc_write_raw(struct
> > > > > > iio_dev
> > > > > > *indio_dev,
> > > > > >                     val > st->soc_info.max_sample_rate)
> > > > > >                         return -EINVAL;
> > > > > > 
> > > > > > -             at91_adc_setup_samp_freq(st, val);
> > > > > > +             at91_adc_setup_samp_freq(indio_dev, val);
> > > > > >                 return 0;
> > > > > >         default:
> > > > > >                 return -EINVAL;
> > > > > > @@ -1624,8 +1625,10 @@ static int
> > > > > > at91_adc_update_scan_mode(struct iio_dev
> > > > > > *indio_dev,
> > > > > >         return 0;
> > > > > >    }
> > > > > > 
> > > > > > -static void at91_adc_hw_init(struct at91_adc_state *st)
> > > > > > +static void at91_adc_hw_init(struct iio_dev *indio_dev)
> > > > > >    {
> > > > > > +     struct at91_adc_state *st = iio_priv(indio_dev);
> > > > > > +
> > > > > >         at91_adc_writel(st, AT91_SAMA5D2_CR,
> > > > > > AT91_SAMA5D2_CR_SWRST);
> > > > > >         at91_adc_writel(st, AT91_SAMA5D2_IDR, 0xffffffff);
> > > > > >         /*
> > > > > > @@ -1635,7 +1638,7 @@ static void at91_adc_hw_init(struct
> > > > > > at91_adc_state
> > > > > > *st)
> > > > > >         at91_adc_writel(st, AT91_SAMA5D2_MR,
> > > > > >                         AT91_SAMA5D2_MR_TRANSFER(2) |
> > > > > > AT91_SAMA5D2_MR_ANACH);
> > > > > > 
> > > > > > -     at91_adc_setup_samp_freq(st, st->soc_info.min_sample_rate);
> > > > > > +     at91_adc_setup_samp_freq(indio_dev, st-  
> > > > > > >soc_info.min_sample_rate);  
> > > > > > 
> > > > > >         /* configure extended mode register */
> > > > > >         at91_adc_config_emr(st);
> > > > > > @@ -1718,6 +1721,7 @@ static int at91_adc_probe(struct
> > > > > > platform_device
> > > > > > *pdev)
> > > > > >         indio_dev->num_channels = ARRAY_SIZE(at91_adc_channels);
> > > > > > 
> > > > > >         st = iio_priv(indio_dev);
> > > > > > +     st->indio_dev = indio_dev;
> > > > > > 
> > > > > >         bitmap_set(&st->touch_st.channels_bitmask,
> > > > > >                    AT91_SAMA5D2_TOUCH_X_CHAN_IDX, 1);
> > > > > > @@ -1829,7 +1833,7 @@ static int at91_adc_probe(struct
> > > > > > platform_device
> > > > > > *pdev)
> > > > > >                 goto vref_disable;
> > > > > >         }
> > > > > > 
> > > > > > -     at91_adc_hw_init(st);
> > > > > > +     at91_adc_hw_init(indio_dev);
> > > > > > 
> > > > > >         ret = clk_prepare_enable(st->per_clk);
> > > > > >         if (ret)
> > > > > > @@ -1945,7 +1949,7 @@ static __maybe_unused int
> > > > > > at91_adc_resume(struct
> > > > > > device *dev)
> > > > > >         if (ret)
> > > > > >                 goto vref_disable_resume;
> > > > > > 
> > > > > > -     at91_adc_hw_init(st);
> > > > > > +     at91_adc_hw_init(indio_dev);
> > > > > > 
> > > > > >         /* reconfiguring trigger hardware state */
> > > > > >         if (!iio_buffer_enabled(indio_dev))