Re: [PATCH v3 3/3] iio: adc: add ADC driver for the TI TSC2046 controller

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Mon, Mar 22, 2021 at 03:41:22PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 22, 2021 at 12:30 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > On Fri, Mar 19, 2021 at 07:42:41PM +0200, Andy Shevchenko wrote:
> > > On Fri, Mar 19, 2021 at 4:45 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> ...
> 
> > > > +static u16 tsc2046_adc_get_value(struct tsc2046_adc_atom *buf)
> > > > +{
> > > > +       /* Last 3 bits on the wire are empty */
> > >
> > > Last?! You meant Least significant?
> >
> > ACK. LSB
> >
> > > Also, don't we lose precision if a new compatible chip appears that
> > > does fill those bits?
> >
> > ACK. All of controllers supported by this driver:
> > drivers/input/touchscreen/ads7846.c:
> > - ti,tsc2046
> > - ti,ads7843
> > - ti,ads7845
> > - ti,ads7846
> > - ti,ads7873 (hm, there is no ti,ads7873, is it actually analog devices AD7873?)
> >
> > support 8- or 12-bit resolution. Only 12 bit mode is supported by this
> > driver. It is possible that some one can produce a resistive touchscreen
> > controller based on X > 12bit ADC, but this will probably not increase precision
> > of this construction (there is a lot of noise any ways...). With other
> > words, it is possible, but not probably that some one will really do it.
> >
> > > Perhaps define the constant and put a comment why it's like this.
> 
> Okay, and what happens here is something like cutting LSBs, but it
> sounds strange to me. If you get 16 bit values, the MSBs should not be
> used?
> 
> So, a good comment is required to explain the logic behind.
> 
> > > > +       return get_unaligned_be16(&buf->data) >> 3;
> > > > +}
> 
> ...
> 
> > > > +static size_t tsc2046_adc_group_set_layout(struct tsc2046_adc_priv *priv,
> > > > +                                          unsigned int group,
> > > > +                                          unsigned int ch_idx)
> > > > +{
> > > > +       struct tsc2046_adc_ch_cfg *ch = &priv->ch_cfg[ch_idx];
> > > > +       struct tsc2046_adc_group_layout *prev, *cur;
> > > > +       unsigned int max_count, count_skip;
> > > > +       unsigned int offset = 0;
> > > > +
> > > > +       if (group) {
> > > > +               prev = &priv->l[group - 1];
> > > > +               offset = prev->offset + prev->count;
> > > > +       }
> > >
> > > I guess you may easily refactor this by supplying a pointer to the
> > > current layout + current size.
> >
> > Sure, but this will not make code more readable and it will not affect
> > the performance. Are there any other reason to do it? Just to make one
> > line instead of two?
> 
> It's still N - 1 unneeded checks and code is slightly harder to read.

fixed

> > > > +       cur = &priv->l[group];
> > >
> > > Also, can you move it down closer to the (single?) caller.
> > >
> > > > +}
> 
> ...
> 
> > > > +               dev_err_ratelimited(dev, "SPI transfer filed: %pe\n",
> > > > +                                   ERR_PTR(ret));
> > >
> > > One line?
> >
> > it will exceed the 80 char rule
> 
> It's fine here.

fixed

> ...
> 
> > > > +       spin_lock_irqsave(&priv->trig_lock, flags);
> > > > +
> > > > +       disable_irq_nosync(priv->spi->irq);
> > >
> > > > +       atomic_inc(&priv->trig_more_count);
> > >
> > > You already have a spin lock, do you need to use the atomic API?
> >
> > I can only pass review comment from my other driver:
> > Memory locations that are concurrently accessed needs to be
> > marked as such, otherwise the compiler is allowed to funky stuff:
> > https://lore.kernel.org/lkml/CAGzjT4ez+gWr3BFQsEr-wma+vs6UZNJ+mRARx_BWoAKEJSsN=w@mail.gmail.com/
> >
> > And here is one more link:
> > https://lwn.net/Articles/793253/#How%20Real%20Is%20All%20This?
> >
> > Starting with commit 62e8a3258bda atomic API is using READ/WRITE_ONCE,
> > so I assume, I do nothing wrong by using it. Correct?
> 
> Hmm... What I don't understand here is why you need a second level of
> atomicity. spin lock already makes this access atomic (at least I have
> checked couple of places and in both the variable is being accessed
> under spin lock).

fixed

> > > > +       iio_trigger_poll(priv->trig);
> > > > +
> > > > +       spin_unlock_irqrestore(&priv->trig_lock, flags);
> 
> ...
> 
> > > > +       name = devm_kasprintf(dev, GFP_KERNEL, "%s-%s",
> > > > +                             TI_TSC2046_NAME, dev_name(dev));
> > >
> > > No NULL check?
> > > Should be added or justified.
> >
> > name is set not optionally  by the spi_add_device()->spi_dev_set_name()
> 
> I didn't get it.
> You allocate memory and haven't checked against NULL. Why?

> If the name field is optional and having it's NULL is okay (non-fatal
> error), put a comment.

ah... I missed the point. You was talking about name == NULL, i was
thinking about dev_name(dev) == NULL.

fixed

> ...
> 
> > > > +       trig->dev.parent = indio_dev->dev.parent;
> > >
> > > Don't we have this done by core (some recent patches in upstream)?
> >
> > can you please point to the code which is doing it?
> 
> I believe it's this one:
> 
> commit 970108185a0be29d1dbb25202d8c12d798e1c3a5
> Author: Gwendal Grignou <gwendal@chromium.org>
> Date:   Tue Mar 9 11:36:13 2021 -0800
> 
>    iio: set default trig->dev.parent

ok, found it on iio/testing and rebased against it..

fixed

Thank you,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |