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

Basically the TI TSC2046 touchscreen controller is 8 channel ADC optimized for
the touchscreen use case. By implementing it as an IIO ADC device, we can
make use of resistive-adc-touch and iio-hwmon drivers.

So far, this driver was tested with a custom version of resistive-adc-touch driver,
since it needs to be extended to make use of Z1 and Z2 channels. The X/Y
are working without additional changes.

Since kbuild bot found some issues and it will be v4, some additional
comments from me below.

...

+#define        TI_TSC2046_SAMPLE_BITS \
+       (sizeof(struct tsc2046_adc_atom) * BITS_PER_BYTE)

Isn't it something like BITS_PER_TYPE(struct ...) ?

...

+struct tsc2046_adc_atom {
+       /*
+        * Command transmitted to the controller. This filed is empty on the RX
+        * buffer.
+        */
+       u8 cmd;
+       /*
+        * Data received from the controller. This filed is empty for the TX
+        * buffer
+        */
+       __be16 data;
+} __packed;

filed -> field in both cases above.

...

+       /*
+        * Lock to protect the layout and the spi transfer buffer.

SPI

+        * tsc2046_adc_group_layout can be changed within update_scan_mode(),
+        * in this case the l[] and tx/rx buffer will be out of sync to each
+        * other.
+        */

...

+static unsigned int tsc2046_adc_time_to_count(struct tsc2046_adc_priv *priv,
+                                             unsigned long time)
+{
+       unsigned int bit_count, sample_count;
+
+       bit_count = DIV_ROUND_UP(time * NSEC_PER_USEC, priv->time_per_bit_ns);

Does it survive 32-bit builds?

+       sample_count = DIV_ROUND_UP(bit_count, TI_TSC2046_SAMPLE_BITS);
+
+       dev_dbg(&priv->spi->dev, "Effective speed %u, time per bit: %u, count bits: %u, count samples: %u\n",
+               priv->effective_speed_hz, priv->time_per_bit_ns,
+               bit_count, sample_count);
+
+       return sample_count;
+}

...

+       /*
+        * if PD bits are 0, controller will automatically disable ADC, VREF and
+        * enable IRQ.
+        */
+       if (keep_power)
+               pd = TI_TSC2046_PD0_ADC_ON;
+       else
+               pd = 0;

Can be ternary on one line, but it's up to you.

...

+static u16 tsc2046_adc_get_value(struct tsc2046_adc_atom *buf)
+{
+       /* Last 3 bits on the wire are empty */

Last?! You meant Least significant?
Also, don't we lose precision if a new compatible chip appears that
does fill those bits?

Perhaps define the constant and put a comment why it's like this.

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

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

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

+}

...

+static int tsc2046_adc_scan(struct iio_dev *indio_dev)
+{
+       struct tsc2046_adc_priv *priv = iio_priv(indio_dev);
+       struct device *dev = &priv->spi->dev;
+       int group;
+       int ret;
+
+       ret = spi_sync(priv->spi, &priv->msg);
+       if (ret < 0) {

+               dev_err_ratelimited(dev, "SPI transfer filed: %pe\n",
+                                   ERR_PTR(ret));

One line?

+               return ret;
+       }

+       ret = iio_push_to_buffers_with_timestamp(indio_dev, &priv->scan_buf,
+                                                iio_get_time_ns(indio_dev));
+       /* If consumer is kfifo, we may get a EBUSY here - ignore it. */

the consumer

+       if (ret < 0 && ret != -EBUSY) {
+               dev_err_ratelimited(dev, "Failed to push scan buffer %pe\n",
+                                   ERR_PTR(ret));
+
+               return ret;
+       }
+
+       return 0;
+}

...

+static enum hrtimer_restart tsc2046_adc_trig_more(struct hrtimer *hrtimer)
+{
+       struct tsc2046_adc_priv *priv = container_of(hrtimer,
+                                                    struct tsc2046_adc_priv,
+                                                    trig_timer);
+       unsigned long flags;
+
+       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?

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

...

+       size_t size = 0;

Move the assignment closer to the actual use of the variable.

...

+       /*
+        * In case SPI controller do not report effective_speed_hz, use
+        * configure value and hope it will match

Missed period.

+        */
+       if (!priv->effective_speed_hz)
+               priv->effective_speed_hz = priv->spi->max_speed_hz;

Also can be ternary on one line, but it's up to you.

...

+       name = devm_kasprintf(dev, GFP_KERNEL, "%s-%s",
+                             TI_TSC2046_NAME, dev_name(dev));

No NULL check?
Should be added or justified.

...

+       trig->dev.parent = indio_dev->dev.parent;

Don't we have this done by core (some recent patches in upstream)?

...

+       ret = devm_iio_device_register(dev, indio_dev);
+       if (ret) {
+               dev_err(dev, "Failed to register iio device\n");

+               return ret;
+       }
+
+       return 0;

  return ret;
or even
  return devm_iio_device_register(dev, indio_dev);

-- 
With Best Regards,
Andy Shevchenko