| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VepMJKuePOgqhgfJzpvLXAizoKsPyJ99wtzHod_xxYLYA@mail.gmail.com>
Date: Tue, 9 Mar 2021 15:21:15 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Rob Herring <robh+dt@...nel.org>,
Jonathan Cameron <jic23@...nel.org>,
devicetree <devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
David Jander <david@...tonic.nl>,
Robin van der Gracht <robin@...tonic.nl>,
linux-iio <linux-iio@...r.kernel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>,
Dmitry Torokhov <dmitry.torokhov@...il.com>
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@...gutronix.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
Powered by blists - more mailing lists