On Fri, Mar 5, 2021 at 3:40 PM Oleksij Rempel [off-list ref] wrote:

I have heard it will be a new version, so below a lot of nit-picks.

Basically the TI TSC2046 touchscreen controller is 8 channel ADC optimized for
the touchscreen use case. By implementing it as IIO ADC device, we can

an IIO

make use of resistive-adc-touch and iio-hwmon drivers.

So far, this driver was tested with custom version of resistive-adc-touch driver,
since it need to be extended to make use of Z1 and Z2 channels. The X/Y

needs

are working without additional changes.

...

+#include <asm/unaligned.h>

Usually asm/* goes after linux/*

+#include <linux/bitfield.h>
+#include <linux/delay.h>

+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger.h>

Can we move this group separately after all other includes?

+#include <linux/module.h>
+#include <linux/spi/spi.h>

...

+/* this driver doesn't aim at the peak continuous sample rate */

This

+/*
+ * Default settling time. This time depends on the:
+ * - PCB design
+ * - touch plates size, temperature, etc
+ * - initial power state of the ADC
+ *
+ * Since most values higher than 100us seems to be good, it make sense to

seem
makes

+ * have some default value. These values were measuring get by testing on a

were measured on a

+ * PLYM2M board at 2MHz SPI CLK rate.
+ *
+ * Sometimes there are extra signal filter capacitors on the touchscreen
+ * signals, which make it 10 or 100 times worse.
+ */

...

+#define TI_TSC2046_TIMESTAMP_SIZE              (sizeof(int64_t) / sizeof(int16_t))

Hmm... shouldn't we use a struct approach below?

...

+/* represents a HW sample */

Represents

Kernel doc with explanation on the fields?

...

+/* layout of atomic buffers within big buffer */

Ditto.

...

+       u16 scan_buf[TI_TSC2046_MAX_CHAN + 2 + TI_TSC2046_TIMESTAMP_SIZE];

Shouldn't we use a struct approach here?

...

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

...

+       dev_dbg(&priv->spi->dev, "%s effective speed %u, time per bit: %u, count bits: %u, count samples: %u\n",
+               __func__, priv->effective_speed_hz, priv->time_per_bit_ns,
+               bit_count, sample_count);

Drop all these __func__ from everywhere. For debug they may be enabled
via Dynamic Debug interface.

...

+       switch (ch_idx) {
+       case TI_TSC2046_ADDR_X:
+       case TI_TSC2046_ADDR_Y:
+       case TI_TSC2046_ADDR_Z1:
+       case TI_TSC2046_ADDR_Z2:
+               settle_time = TI_TSC2046_SETTLING_TIME_XYZ_DEF_US;
+               break;
+       default:
+               settle_time = 0;

break;

+       }

...

+static u8 tsc2046_adc_get_cmd(struct tsc2046_adc_priv *priv, int ch_idx,
+                             bool keep_power)
+{
+       u32 pd = 0; /* power down (pd) bits */
+
+       /*
+        * 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;

?

Otherwise comments are kinda not fully clear.

+       return TI_TSC2046_START | FIELD_PREP(TI_TSC2046_ADDR, ch_idx) | pd;
+}

...

+       /* last 3 bits on the wire are empty */

Last

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

Doesn't mean we will lose precision when the driver will be used as AD/C?

...

+static size_t tsc2046_adc_group_set_layout(struct tsc2046_adc_priv *priv,
+                                          unsigned int group,
+                                          unsigned int ch_idx)
+{
+       struct tsc2046_adc_group_layout *l = &priv->l[group];

Hmm...

+       unsigned int max_count, count_skip;
+       unsigned int offset = 0;
+
+       count_skip = tsc2046_adc_get_settle_count(priv, ch_idx);

+       if (group != 0) {
+               l = &priv->l[group - 1];
+               offset = l->offset + l->count;
+       }
+
+       l = &priv->l[group];

Why do you need to reassign this?

Wouldn't be simpler to rewrite it as

if (group)
  offset = ...;

?

+       max_count = count_skip + TI_TSC2046_SAMPLES_XYZ_DEF;
+
+       l->offset = offset;
+       l->count = max_count;
+       l->skip = count_skip;
+
+       return sizeof(*priv->tx) * max_count;
+}

...

+               switch (ch_idx) {
+               case TI_TSC2046_ADDR_Y:
+                       if (val == 0xfff)
+                               return -EAGAIN;
+                       break;
+               case TI_TSC2046_ADDR_X:
+                       if (!val)
+                               return -EAGAIN;
+                       break;

default?

+               }

...

+               if (valid) {
+                       /*
+                        * Validate data to stop sampling and reduce power
+                        * consumption.
+                        */
+                       ret = tsc2046_adc_get_valide_val(priv, group);
+                       if (ret < 0) {
+                               /* go back and invalidate all channels */
+                               group = 0;
+                               valid = false;
+                       }
+               } else {

+                       ret = 0xffff;

GENMASK() if it's a bitfield or U16_MAX if it's plain number?

+               }
+
+               priv->scan_buf[group] = ret;
+       }

...

+       unsigned int retry = 3;

Why this number? Comment?

...

+               /*
+                * We can sample it as fast as we can, but usually we do not
+                * need so many samples. Reduce the sample rate for default
+                * (touchscreen) use case.
+                * Currently we do not need highly precise sample rate. It

a highly

+                * is enough to have calculated numbers.
+                */

...

+       priv->time_per_scan_us = size * 8 *
+               priv->time_per_bit_ns / NSEC_PER_USEC;

One line?

...

+       /*
+        * calculate and allocate maximal size buffer if all channels are
+        * enabled

Calculate
enabled.

+        */

...

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

NULL check?

...

+       trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+                                     indio_dev->name, indio_dev->id);

One line?

+       if (!trig)
+               return -ENOMEM;

...

+static const struct of_device_id ads7950_of_table[] = {
+       { .compatible = "ti,tsc2046e-adc", .data = &tsc2046_adc_dcfg_tsc2046e },

+       { },

No comma needed for a terminator line.

+};

-- 
With Best Regards,
Andy Shevchenko