lkml.org 
[lkml]   [2021]   [Mar]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
On Fri, Mar 5, 2021 at 3:40 PM Oleksij Rempel <o.rempel@pengutronix.de> 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

\
 
 \ /
  Last update: 2021-03-09 14:23    [W:0.144 / U:0.680 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site