Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 9 Mar 2021 15:21:15 +0200 | Subject | Re: [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
| |