linux-kernel - Re: [PATCH v2 1/4] iio: adc: ad7887: convert dual-channel mode to DT/ACPI

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 21 Nov 2020 15:36:53 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Alexandru Ardelean <alexandru.ardelean@...log.com>
Cc:     <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-iio@...r.kernel.org>, <robh+dt@...nel.org>,
        <andy.shevchenko@...il.com>
Subject: Re: [PATCH v2 1/4] iio: adc: ad7887: convert dual-channel mode to
 DT/ACPI

On Thu, 19 Nov 2020 12:07:45 +0200
Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:

> This change converts the configuration of the dual-channel mode from the
> old platform-data, to the device_property_present() function, which
> supports both device-tree and ACPI configuration setups.
> 
> With this change the old platform_data include of the driver can be
> removed.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>

Hi Alexandru,

The set looks good in general, but there is a an oddity in the driver.
If we don't provide the vref regulator, then the scale is set to reflect
the internal 2.5V reference.  Fiven the external reference only
works by overdriving the 2.5V (must be higher than that to have
an effect) I guess we could in theory clamp it to a minimum of
2.5V but anyone wiring up less than that would have built a crazy board
so we can probably ignore it.

However, as I read the datasheet in dual channel mode it should be set
to VDD not 2.5V.  Right now you could make it work in a DT file
by setting VREF==VDD regulator but that's inelegant.

If you agree with my logic, perhaps a follow up patch?

Jonathan


> ---
> 
> I'm wondering if this changeset is what was in mind here:
>  https://lore.kernel.org/linux-iio/CA+U=DsqF5tu8Be9KXeyCWD2uHvV688Nc3n=z_Xi2J6H6DFJPRQ@mail.gmail.com/T/#mbe72e4da3acea3899d0d35402ea81e52a9bc34e6
> This driver could have been simplified/reduced a whole lot more, but I'm
> not sure about it. It's a bit of patch-noise, and later
> 
> Changelog v1 -> v2:
> * dropped patch 'iio: adc: ad7887: convert driver to full DT probing'
>   not adding the device_get_match_data() logic anymore
> * added patch 'iio: adc: ad7887: remove matching code from driver'
>   hooking the chip info directly to AD7887
> * added patch 'iio: adc: ad7887: add OF match table'
>   this just adds an OF table for DT and ACPI
> 
>  drivers/iio/adc/ad7887.c             | 10 +++++-----
>  include/linux/platform_data/ad7887.h | 21 ---------------------
>  2 files changed, 5 insertions(+), 26 deletions(-)
>  delete mode 100644 include/linux/platform_data/ad7887.h
> 
> diff --git a/drivers/iio/adc/ad7887.c b/drivers/iio/adc/ad7887.c
> index 4f6f0e0e03ee..06f684c053a0 100644
> --- a/drivers/iio/adc/ad7887.c
> +++ b/drivers/iio/adc/ad7887.c
> @@ -23,8 +23,6 @@
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>  
> -#include <linux/platform_data/ad7887.h>
> -
>  #define AD7887_REF_DIS		BIT(5)	/* on-chip reference disable */
>  #define AD7887_DUAL		BIT(4)	/* dual-channel mode */
>  #define AD7887_CH_AIN1		BIT(3)	/* convert on channel 1, DUAL=1 */
> @@ -241,9 +239,9 @@ static void ad7887_reg_disable(void *data)
>  
>  static int ad7887_probe(struct spi_device *spi)
>  {
> -	struct ad7887_platform_data *pdata = spi->dev.platform_data;
>  	struct ad7887_state *st;
>  	struct iio_dev *indio_dev;
> +	bool dual_mode;
>  	uint8_t mode;
>  	int ret;
>  
> @@ -286,7 +284,9 @@ static int ad7887_probe(struct spi_device *spi)
>  	mode = AD7887_PM_MODE4;
>  	if (!st->reg)
>  		mode |= AD7887_REF_DIS;
> -	if (pdata && pdata->en_dual)
> +
> +	dual_mode = device_property_present(&spi->dev, "adi,dual-channel-mode");
> +	if (dual_mode)
>  		mode |= AD7887_DUAL;
>  
>  	st->tx_cmd_buf[0] = AD7887_CH_AIN0 | mode;
> @@ -298,7 +298,7 @@ static int ad7887_probe(struct spi_device *spi)
>  	spi_message_init(&st->msg[AD7887_CH0]);
>  	spi_message_add_tail(&st->xfer[0], &st->msg[AD7887_CH0]);
>  
> -	if (pdata && pdata->en_dual) {
> +	if (dual_mode) {
>  		st->tx_cmd_buf[2] = AD7887_CH_AIN1 | mode;
>  
>  		st->xfer[1].rx_buf = &st->data[0];
> diff --git a/include/linux/platform_data/ad7887.h b/include/linux/platform_data/ad7887.h
> deleted file mode 100644
> index 9b4dca6ae70b..000000000000
> --- a/include/linux/platform_data/ad7887.h
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * AD7887 SPI ADC driver
> - *
> - * Copyright 2010 Analog Devices Inc.
> - */
> -#ifndef IIO_ADC_AD7887_H_
> -#define IIO_ADC_AD7887_H_
> -
> -/**
> - * struct ad7887_platform_data - AD7887 ADC driver platform data
> - * @en_dual: Whether to use dual channel mode. If set to true AIN1 becomes the
> - *	second input channel, and Vref is internally connected to Vdd. If set to
> - *	false the device is used in single channel mode and AIN1/Vref is used as
> - *	VREF input.
> - */
> -struct ad7887_platform_data {
> -	bool en_dual;
> -};
> -
> -#endif /* IIO_ADC_AD7887_H_ */