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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <48da2012171cfa06c55042678bbec5f8aad66108.camel@gmail.com>
Date:   Wed, 15 Nov 2023 15:20:49 +0100
From:   Nuno Sá <noname.nuno@...il.com>
To:     Alisa-Dariana Roman <alisadariana@...il.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Alisa-Dariana Roman <alisa.roman@...log.com>
Cc:     linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Alexandru Tachici <alexandru.tachici@...log.com>
Subject: Re: [PATCH v2 4/4] iio: adc: ad7192: Add AD7194 support

On Tue, 2023-11-14 at 22:05 +0200, Alisa-Dariana Roman 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.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@...log.com>
> ---

Well, just some minor stuff from my side. Even if you don't address the, feel
still free to:

Reviewed-by: Nuno Sa <nuno.sa@...log.com>

>  drivers/iio/adc/Kconfig  |  11 ++-
>  drivers/iio/adc/ad7192.c | 144 ++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 150 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 1e2b7a2c67c6..05344054b88e 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -55,12 +55,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 48e0357564af..0532678ad665 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
> + * AD7190 AD7192 AD7193 AD7194 AD7195 SPI ADC driver

Maybe this should also be "Analog Devices AD7192 and similar 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))
> +#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))
> +#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,6 +195,7 @@ enum {
>         ID_AD7190,
>         ID_AD7192,
>         ID_AD7193,
> +       ID_AD7194,
>         ID_AD7195,
>  };
>  
> @@ -644,6 +674,15 @@ static const struct attribute_group ad7192_attribute_group = {
>         .attrs = ad7192_attributes,
>  };
>  
> +static struct attribute *ad7194_attributes[] = {
> +       &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
> +       NULL
> +};
> +
> +static const struct attribute_group ad7194_attribute_group = {
> +       .attrs = ad7194_attributes,
> +};
> +
>  static struct attribute *ad7195_attributes[] = {
>         &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
>         &iio_dev_attr_bridge_switch_en.dev_attr.attr,
> @@ -928,6 +967,16 @@ 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,
> +       .attrs = &ad7194_attribute_group,
> +       .validate_trigger = ad_sd_validate_trigger,
> +       .update_scan_mode = ad7192_update_scan_mode,
> +};
> +
>  static const struct iio_info ad7195_info = {
>         .read_raw = ad7192_read_raw,
>         .write_raw = ad7192_write_raw,
> @@ -1017,6 +1066,35 @@ 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),
> +       IIO_CHAN_SOFT_TIMESTAMP(25),
> +};
> +
>  static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
>         [ID_AD7190] = {
>                 .chip_id = CHIPID_AD7190,
> @@ -1039,6 +1117,13 @@ 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,
> +       },
>         [ID_AD7195] = {
>                 .chip_id = CHIPID_AD7195,
>                 .name = "ad7195",
> @@ -1053,6 +1138,53 @@ static void ad7192_reg_disable(void *reg)
>         regulator_disable(reg);
>  }
>  
> +static int ad7192_parse_channel(struct iio_dev *indio_dev,
> +                               struct fwnode_handle *child)
> +{
> +       u32 reg, ain[2];
> +       int ret;
> +
> +       ret = fwnode_property_read_u32(child, "reg", &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]);
> +
> +       return 0;
> +}
> +
> +static int ad7192_parse_channels(struct iio_dev *indio_dev)
> +{
> +       struct ad7192_state *st = iio_priv(indio_dev);

Don't really like this much... We should just pass the 'ad7192_state' but I see
you're being consistent with the rest of the probe(). If you need to re-spin the
series, I would like to see a precursor patch changing this and the other relevant
places.

> +       struct device *dev = &st->sd.spi->dev;
> +       struct fwnode_handle *child;
> +       int ret;
> +
> +       device_for_each_child_node(dev, child) {
> +               ret = ad7192_parse_channel(indio_dev, child);
> +               if (ret) {
> +                       fwnode_handle_put(child);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static int ad7192_probe(struct spi_device *spi)
>  {
>         struct ad7192_state *st;
> @@ -1150,6 +1282,12 @@ static int ad7192_probe(struct spi_device *spi)
>                 }
>         }
>  
> +       if (st->chip_info->chip_id == CHIPID_AD7194) {
> +               ret = ad7192_parse_channels(indio_dev);
> +               if (ret)
> +                       return ret;
> +       }
> +
>         ret = ad7192_setup(indio_dev);
>         if (ret)
>                 return ret;
> @@ -1161,6 +1299,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] },
>         {}
>  };
> @@ -1170,6 +1309,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] },
>         {}
>  };
> @@ -1186,6 +1326,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 AD7190, AD7192, AD7193, AD7194, AD7195 ADC");

Ditto...

- Nuno Sá

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ