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@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.
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@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).
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@xxxxxxxxxxxx>
> 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 |
