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

[Andy Shevchenko]

On Mon, Mar 22, 2021 at 12:30 PM Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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.

> > > +       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.

...

> > > +       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@xxxxxxxxxxxxxx/
>
> 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).

> > > +       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.

...

> > > +       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@xxxxxxxxxxxx>
Date:   Tue Mar 9 11:36:13 2021 -0800

   iio: set default trig->dev.parent

-- 
With Best Regards,
Andy Shevchenko