On Mon, Mar 22, 2021 at 12:30 PM Oleksij Rempel [off-list ref] wrote:

On Fri, Mar 19, 2021 at 07:42:41PM +0200, Andy Shevchenko wrote:

quoted

On Fri, Mar 19, 2021 at 4:45 PM Oleksij Rempel [off-list ref] wrote:

...

quoted

quoted

+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

quoted

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.

quoted

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.

quoted

quoted

+       return get_unaligned_be16(&buf->data) >> 3;
+}

...

quoted

quoted

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

quoted

quoted

+       cur = &priv->l[group];

Also, can you move it down closer to the (single?) caller.

quoted

+}

...

quoted

quoted

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

...

quoted

quoted

+       spin_lock_irqsave(&priv->trig_lock, flags);
+
+       disable_irq_nosync(priv->spi->irq);

quoted

+       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/ (local)

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

quoted

quoted

+       iio_trigger_poll(priv->trig);
+
+       spin_unlock_irqrestore(&priv->trig_lock, flags);

...

quoted

quoted

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

...

quoted

quoted

+       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 [off-list ref]
Date:   Tue Mar 9 11:36:13 2021 -0800

   iio: set default trig->dev.parent

-- 
With Best Regards,
Andy Shevchenko