| 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: <CAMknhBHU6k8J_PLCmGYF48S1q3uXByiCwzcd+B3q3Cd-02CUow@mail.gmail.com>
Date: Thu, 8 Feb 2024 16:27:07 -0600
From: David Lechner <dlechner@...libre.com>
To: Alisa-Dariana Roman <alisadariana@...il.com>
Cc: alexandru.tachici@...log.com, alisa.roman@...log.com, conor+dt@...nel.org,
devicetree@...r.kernel.org, jic23@...nel.org,
krzysztof.kozlowski+dt@...aro.org, krzysztof.kozlowski@...aro.org,
lars@...afoo.de, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
michael.hennerich@...log.com, robh+dt@...nel.org,
Nuno Sa <nuno.sa@...log.com>
Subject: Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support
On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman
<alisadariana@...il.com> wrote:
>
> Unlike the other AD719Xs, AD7194 has configurable differential
> channels. The default configuration for these channels can be changed
> from the devicetree.
>
> The default configuration is hardcoded in order to have a stable number
> of channels.
>
> Also modify config AD7192 description for better scaling.
>
> Moved ad7192_chip_info struct definition to allow use of callback
> function parse_channels().
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@...log.com>
> Reviewed-by: Nuno Sa <nuno.sa@...log.com>
> ---
> drivers/iio/adc/Kconfig | 11 ++-
> drivers/iio/adc/ad7192.c | 150 ++++++++++++++++++++++++++++++++++++---
> 2 files changed, 148 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 59ae1d17b50d..8062a4d1cbe7 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -71,12 +71,17 @@ config AD7124
> called ad7124.
>
> config AD7192
> - tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
> + tristate "Analog Devices AD7192 and similar ADC driver"
> depends on SPI
> select AD_SIGMA_DELTA
> help
> - Say yes here to build support for Analog Devices AD7190,
> - AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
> + Say yes here to build support for Analog Devices SPI analog to digital
> + converters (ADC):
> + - AD7190
> + - AD7192
> + - AD7193
> + - AD7194
> + - AD7195
> If unsure, say N (but it's safe to say "Y").
>
> To compile this driver as a module, choose M here: the
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index d8393ac048e7..a3ff60ed6f63 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
> + * AD7192 and similar SPI ADC driver
> *
> * Copyright 2011-2015 Analog Devices Inc.
> */
> @@ -125,10 +125,39 @@
> #define AD7193_CH_AIN8 0x480 /* AIN7 - AINCOM */
> #define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */
>
> +#define AD7194_CH_TEMP 0x100 /* Temp sensor */
> +#define AD7194_CH_AIN1 0x400 /* AIN1 - AINCOM */
> +#define AD7194_CH_AIN2 0x410 /* AIN2 - AINCOM */
> +#define AD7194_CH_AIN3 0x420 /* AIN3 - AINCOM */
> +#define AD7194_CH_AIN4 0x430 /* AIN4 - AINCOM */
> +#define AD7194_CH_AIN5 0x440 /* AIN5 - AINCOM */
> +#define AD7194_CH_AIN6 0x450 /* AIN6 - AINCOM */
> +#define AD7194_CH_AIN7 0x460 /* AIN7 - AINCOM */
> +#define AD7194_CH_AIN8 0x470 /* AIN8 - AINCOM */
> +#define AD7194_CH_AIN9 0x480 /* AIN9 - AINCOM */
> +#define AD7194_CH_AIN10 0x490 /* AIN10 - AINCOM */
> +#define AD7194_CH_AIN11 0x4A0 /* AIN11 - AINCOM */
> +#define AD7194_CH_AIN12 0x4B0 /* AIN12 - AINCOM */
> +#define AD7194_CH_AIN13 0x4C0 /* AIN13 - AINCOM */
> +#define AD7194_CH_AIN14 0x4D0 /* AIN14 - AINCOM */
> +#define AD7194_CH_AIN15 0x4E0 /* AIN15 - AINCOM */
> +#define AD7194_CH_AIN16 0x4F0 /* AIN16 - AINCOM */
> +#define AD7194_CH_POS_MASK GENMASK(7, 4)
> +#define AD7194_CH_POS(x) FIELD_PREP(AD7194_CH_POS_MASK, (x))
AD7194_CH_POS_MASK isn't used elsewhere, so maybe just this?
#define AD7194_CH_POS(x) FIELD_PREP(GENMASK(7, 4), (x))
> +#define AD7194_CH_NEG_MASK GENMASK(3, 0)
> +#define AD7194_CH_NEG(x) FIELD_PREP(AD7194_CH_NEG_MASK, (x))
> +#define AD7194_CH_DIFF(pos, neg) \
> + (AD7194_CH_POS(pos) | AD7194_CH_NEG(neg))
You could also add `((neg) == 0 ? BIT(10) : 0) | ...` and then use
this macro to define AD7194_CH_AINx.
#define AD7194_CH_AIN1 AD7194_CH_DIFF(1, 0)
> +#define AD7194_CH_DIFF_START 0
> +#define AD7194_CH_DIFF_NR 8
> +#define AD7194_CH_AIN_START 1
> +#define AD7194_CH_AIN_NR 16
> +
> /* ID Register Bit Designations (AD7192_REG_ID) */
> #define CHIPID_AD7190 0x4
> #define CHIPID_AD7192 0x0
> #define CHIPID_AD7193 0x2
> +#define CHIPID_AD7194 0x3
> #define CHIPID_AD7195 0x6
> #define AD7192_ID_MASK GENMASK(3, 0)
>
> @@ -166,17 +195,10 @@ enum {
> ID_AD7190,
> ID_AD7192,
> ID_AD7193,
> + ID_AD7194,
> ID_AD7195,
> };
>
> -struct ad7192_chip_info {
> - unsigned int chip_id;
> - const char *name;
> - const struct iio_chan_spec *channels;
> - u8 num_channels;
> - const struct iio_info *info;
> -};
> -
> struct ad7192_state {
> const struct ad7192_chip_info *chip_info;
> struct regulator *avdd;
> @@ -197,6 +219,15 @@ struct ad7192_state {
> struct ad_sigma_delta sd;
> };
>
> +struct ad7192_chip_info {
> + unsigned int chip_id;
> + const char *name;
> + const struct iio_chan_spec *channels;
> + u8 num_channels;
> + const struct iio_info *info;
> + int (*parse_channels)(struct ad7192_state *st);
> +};
> +
> static const char * const ad7192_syscalib_modes[] = {
> [AD7192_SYSCALIB_ZERO_SCALE] = "zero_scale",
> [AD7192_SYSCALIB_FULL_SCALE] = "full_scale",
> @@ -918,6 +949,15 @@ static const struct iio_info ad7192_info = {
> .update_scan_mode = ad7192_update_scan_mode,
> };
>
> +static const struct iio_info ad7194_info = {
> + .read_raw = ad7192_read_raw,
> + .write_raw = ad7192_write_raw,
> + .write_raw_get_fmt = ad7192_write_raw_get_fmt,
> + .read_avail = ad7192_read_avail,
> + .validate_trigger = ad_sd_validate_trigger,
> + .update_scan_mode = ad7192_update_scan_mode,
> +};
Isn't this identical to ad7192_info and ad7195_info now that .attrs is
removed? It seems like we could consolidate here.
> +
> static const struct iio_info ad7195_info = {
> .read_raw = ad7192_read_raw,
> .write_raw = ad7192_write_raw,
> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = {
> IIO_CHAN_SOFT_TIMESTAMP(14),
> };
>
> +static struct iio_chan_spec ad7194_channels[] = {
> + AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
> + AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
> + AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
> + AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
> + AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
> + AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
> + AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
> + AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
> + AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
> + AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
> + AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
> + AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
> + AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
> + AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
> + AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
> + AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
> + AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
> + AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
> + AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
> + AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
> + AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
> + AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
> + AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
> + AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
> + AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),
Shouldn't these be differential channels since they are
pseudo-differential inputs measuring the difference between AINx and
AINCOM?
> + IIO_CHAN_SOFT_TIMESTAMP(25),
> +};
i.e. like this (where AINCOM is voltage0 AINx is voltagex)
static struct iio_chan_spec ad7194_channels[] = {
AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1),
AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2),
AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3),
AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4),
AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5),
AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6),
AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7),
AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8),
AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9),
AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10),
AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11),
AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12),
AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13),
AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14),
AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15),
AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16),
AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP),
IIO_CHAN_SOFT_TIMESTAMP(17),
};
> +
> +static int ad7192_parse_channel(struct fwnode_handle *child)
> +{
> + u32 reg, ain[2];
> + int ret;
> +
> + ret = fwnode_property_read_u32(child, "reg", ®);
> + if (ret)
> + return ret;
> +
> + if (!in_range(reg, AD7194_CH_DIFF_START, AD7194_CH_DIFF_NR))
> + return -EINVAL;
> +
> + ret = fwnode_property_read_u32_array(child, "diff-channels", ain,
> + ARRAY_SIZE(ain));
> + if (ret)
> + return ret;
> +
> + if (!in_range(ain[0], AD7194_CH_AIN_START, AD7194_CH_AIN_NR) ||
> + !in_range(ain[1], AD7194_CH_AIN_START, AD7194_CH_AIN_NR))
> + return -EINVAL;
> +
> + ad7194_channels[reg].channel = ain[0];
> + ad7194_channels[reg].channel2 = ain[1];
> + ad7194_channels[reg].address = AD7194_CH_DIFF(ain[0], ain[1]);
.. then the suggested change to AD7194_CH_DIFF above also make this
work when ain[1] is zero which should be allowed in the range check
immediately before this.
> +
> + return 0;
> +}
> +
> +static int ad7192_parse_channels(struct ad7192_state *st)
> +{
> + struct device *dev = &st->sd.spi->dev;
> + struct fwnode_handle *child;
> + int ret;
> +
> + device_for_each_child_node(dev, child) {
> + ret = ad7192_parse_channel(child);
> + if (ret) {
> + fwnode_handle_put(child);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> [ID_AD7190] = {
> .chip_id = CHIPID_AD7190,
> @@ -1031,6 +1145,14 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
> .num_channels = ARRAY_SIZE(ad7193_channels),
> .info = &ad7192_info,
> },
> + [ID_AD7194] = {
> + .chip_id = CHIPID_AD7194,
> + .name = "ad7194",
> + .channels = ad7194_channels,
> + .num_channels = ARRAY_SIZE(ad7194_channels),
> + .info = &ad7194_info,
> + .parse_channels = ad7192_parse_channels,
> + },
> [ID_AD7195] = {
> .chip_id = CHIPID_AD7195,
> .name = "ad7195",
> @@ -1142,6 +1264,12 @@ static int ad7192_probe(struct spi_device *spi)
> }
> }
>
> + if (st->chip_info->parse_channels) {
> + ret = st->chip_info->parse_channels(st);
> + if (ret)
> + return ret;
> + }
> +
> ret = ad7192_setup(st);
> if (ret)
> return ret;
> @@ -1153,6 +1281,7 @@ static const struct of_device_id ad7192_of_match[] = {
> { .compatible = "adi,ad7190", .data = &ad7192_chip_info_tbl[ID_AD7190] },
> { .compatible = "adi,ad7192", .data = &ad7192_chip_info_tbl[ID_AD7192] },
> { .compatible = "adi,ad7193", .data = &ad7192_chip_info_tbl[ID_AD7193] },
> + { .compatible = "adi,ad7194", .data = &ad7192_chip_info_tbl[ID_AD7194] },
> { .compatible = "adi,ad7195", .data = &ad7192_chip_info_tbl[ID_AD7195] },
> {}
> };
> @@ -1162,6 +1291,7 @@ static const struct spi_device_id ad7192_ids[] = {
> { "ad7190", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7190] },
> { "ad7192", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7192] },
> { "ad7193", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7193] },
> + { "ad7194", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7194] },
> { "ad7195", (kernel_ulong_t)&ad7192_chip_info_tbl[ID_AD7195] },
> {}
> };
> @@ -1178,6 +1308,6 @@ static struct spi_driver ad7192_driver = {
> module_spi_driver(ad7192_driver);
>
> MODULE_AUTHOR("Michael Hennerich <michael.hennerich@...log.com>");
> -MODULE_DESCRIPTION("Analog Devices AD7190, AD7192, AD7193, AD7195 ADC");
> +MODULE_DESCRIPTION("Analog Devices AD7192 and similar ADC");
> MODULE_LICENSE("GPL v2");
> MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
> --
> 2.34.1
>
Powered by blists - more mailing lists