Re: [PATCH v3 3/3] iio: adc: add ADC driver for the TI TSC2046 controller
From: Andy Shevchenko
Date: Fri Mar 19 2021 - 13:43:34 EST
On Fri, Mar 19, 2021 at 4:45 PM Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> 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