linux-kernel - Re: [PATCH v2 1/6] iio: adc: ad4080: fix chip identification

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] [day] [month] [year] [list]

Message-ID: <20251004152249.18116842@jic23-huawei>
Date: Sat, 4 Oct 2025 15:22:49 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Antoniu Miclaus <antoniu.miclaus@...log.com>, robh@...nel.org,
 conor+dt@...nel.org, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/6] iio: adc: ad4080: fix chip identification

On Tue, 30 Sep 2025 13:35:46 +0100
Nuno Sá <noname.nuno@...il.com> wrote:

> Hi Antoniu,
> 
> I think that for a series like this you should include a cover letter...
Yup. Then I'd be replying to that rather than here!

> 
> On Tue, 2025-09-30 at 10:32 +0000, Antoniu Miclaus wrote:
> > Fix AD4080 chip identification by using the correct 16-bit product ID
> > (0x0050) instead of GENMASK(2, 0). Update the chip reading logic to
> > use regmap_bulk_read to read both PRODUCT_ID_L and PRODUCT_ID_H
> > registers and combine them into a 16-bit value.
> > 
> > The original implementation was incorrectly reading only 3 bits,
> > which would not correctly identify the AD4080 chip.
> > 
> > Fixes: 6b31ba1811b6 ("iio: adc: ad4080: add driver support")
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> > ---
> > changes in v2:
> >  - add the chip id handling into a separate commit.
> >  - use regmap_bulk_read.
> >  drivers/iio/adc/ad4080.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad4080.c b/drivers/iio/adc/ad4080.c
> > index 6e61787ed321..b80560aebe2d 100644
> > --- a/drivers/iio/adc/ad4080.c
> > +++ b/drivers/iio/adc/ad4080.c
> > @@ -125,7 +125,7 @@
> >  
> >  /* Miscellaneous Definitions */
> >  #define
> > AD4080_SPI_READ						BIT(7)
> > -#define AD4080_CHIP_ID						GENMASK(2, 0)
> > +#define AD4080_CHIP_ID						0x0050
> >  
> >  #define AD4080_LVDS_CNV_CLK_CNT_MAX				7
> >  
> > @@ -458,10 +458,11 @@ static int ad4080_setup(struct iio_dev *indio_dev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = regmap_read(st->regmap, AD4080_REG_CHIP_TYPE, &id);
> > +	ret = regmap_bulk_read(st->regmap, AD4080_REG_PRODUCT_ID_L, &id, 2);
> >  	if (ret)
> >  		return ret;
> >  
> > +	id = get_unaligned_le16(&id);  
> 
> Being id an 'unsigned int' I'm not really sure the above will work on big endian
> machines as we should only populate the 2 MSB, right? But independent of that,
> id is only being used in here so I would use proper __le16 (and u16) and
> le16_to_cpu().
> 
Spot on.  Types are a mess here and will trigger issues if sparse is pointed
at this code (and possibly other compiler related warnings).

The series otherwise looks good to me. You could probably have added all the DT
changes in one patch and all the devices support in another (rather than 2 of each)
but that's not important enough to change now.

So I'll pick the whole thing up on v3 once this type issue is resolved.

thanks,

Jonathan

> - Nuno Sá
> 
> >  	if (id != AD4080_CHIP_ID)
> >  		dev_info(dev, "Unrecognized CHIP_ID 0x%X\n", id);
> >