linux-kernel - Re: [PATCH] iio: adc: ad7192: Fix ac excitation feature

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]
Message-ID: <b8693c52df5cf520d6994b872bac0768901a0a6d.camel@gmail.com>
Date:   Thu, 15 Jun 2023 13:46:44 +0200
From:   Nuno Sá <noname.nuno@...il.com>
To:     Alisa Roman <alisa.roman@...log.com>
Cc:     stable@...r.kernel.org,
        Alexandru Tachici <alexandru.tachici@...log.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: adc: ad7192: Fix ac excitation feature

On Wed, 2023-06-14 at 18:52 +0300, Alisa Roman wrote:
> AC excitation enable feature exposed to user on AD7192, allowing a bit
> which should be 0 to be set. This feature is specific only to AD7195. AC
> excitation attribute moved accordingly.
> 
> In the AD7195 documentation, the AC excitation enable bit is on position
> 22 in the Configuration register. ACX macro changed to match correct
> register and bit.
> 
> Note that the fix tag is for the commit that moved the driver out of
> staging.
> 
> Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
> Signed-off-by: Alisa Roman <alisa.roman@...log.com>
> Cc: stable@...r.kernel.org
> ---

Hi Alisa,

I see you improved the commit message to explain what's going on but you should
have versioned your patches accordingly. Anyways, don't forget to do it next
time :). You could also mention the name change AD7192_MODE_ACX ->
AD7192_CONFIG_ACX even though it's a bit obvious. Anyways:

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

>  drivers/iio/adc/ad7192.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 8685e0b58a83..7bc3ebfe8081 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -62,7 +62,6 @@
>  #define AD7192_MODE_STA_MASK   BIT(20) /* Status Register transmission Mask
> */
>  #define AD7192_MODE_CLKSRC(x)  (((x) & 0x3) << 18) /* Clock Source Select */
>  #define AD7192_MODE_SINC3      BIT(15) /* SINC3 Filter Select */
> -#define AD7192_MODE_ACX                BIT(14) /* AC excitation enable(AD7195
> only)*/
>  #define AD7192_MODE_ENPAR      BIT(13) /* Parity Enable */
>  #define AD7192_MODE_CLKDIV     BIT(12) /* Clock divide by 2 (AD7190/2 only)*/
>  #define AD7192_MODE_SCYCLE     BIT(11) /* Single cycle conversion */
> @@ -91,6 +90,7 @@
>  /* Configuration Register Bit Designations (AD7192_REG_CONF) */
>  
>  #define AD7192_CONF_CHOP       BIT(23) /* CHOP enable */
> +#define AD7192_CONF_ACX                BIT(22) /* AC excitation enable(AD7195
> only) */
>  #define AD7192_CONF_REFSEL     BIT(20) /* REFIN1/REFIN2 Reference Select */
>  #define AD7192_CONF_CHAN(x)    ((x) << 8) /* Channel select */
>  #define AD7192_CONF_CHAN_MASK  (0x7FF << 8) /* Channel select mask */
> @@ -472,7 +472,7 @@ static ssize_t ad7192_show_ac_excitation(struct device
> *dev,
>         struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>         struct ad7192_state *st = iio_priv(indio_dev);
>  
> -       return sysfs_emit(buf, "%d\n", !!(st->mode & AD7192_MODE_ACX));
> +       return sysfs_emit(buf, "%d\n", !!(st->conf & AD7192_CONF_ACX));
>  }
>  
>  static ssize_t ad7192_show_bridge_switch(struct device *dev,
> @@ -513,13 +513,13 @@ static ssize_t ad7192_set(struct device *dev,
>  
>                 ad_sd_write_reg(&st->sd, AD7192_REG_GPOCON, 1, st->gpocon);
>                 break;
> -       case AD7192_REG_MODE:
> +       case AD7192_REG_CONF:
>                 if (val)
> -                       st->mode |= AD7192_MODE_ACX;
> +                       st->conf |= AD7192_CONF_ACX;
>                 else
> -                       st->mode &= ~AD7192_MODE_ACX;
> +                       st->conf &= ~AD7192_CONF_ACX;
>  
> -               ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> +               ad_sd_write_reg(&st->sd, AD7192_REG_CONF, 3, st->conf);
>                 break;
>         default:
>                 ret = -EINVAL;
> @@ -579,12 +579,11 @@ static IIO_DEVICE_ATTR(bridge_switch_en, 0644,
>  
>  static IIO_DEVICE_ATTR(ac_excitation_en, 0644,
>                        ad7192_show_ac_excitation, ad7192_set,
> -                      AD7192_REG_MODE);
> +                      AD7192_REG_CONF);
>  
>  static struct attribute *ad7192_attributes[] = {
>         &iio_dev_attr_filter_low_pass_3db_frequency_available.dev_attr.attr,
>         &iio_dev_attr_bridge_switch_en.dev_attr.attr,
> -       &iio_dev_attr_ac_excitation_en.dev_attr.attr,
>         NULL
>  };
>  
> @@ -595,6 +594,7 @@ static const struct attribute_group ad7192_attribute_group
> = {
>  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,
> +       &iio_dev_attr_ac_excitation_en.dev_attr.attr,
>         NULL
>  };
>