| 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: <CAKUZ0zKVrm8egyZ+3G7=JgcKwbgPe30UVT18j35RvR0OCr3V+g@mail.gmail.com>
Date: Fri, 18 Apr 2025 16:55:26 -0400
From: Gabriel Shahrouzi <gshahrouzi@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: gregkh@...uxfoundation.org, lars@...afoo.de, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev,
Michael.Hennerich@...log.com, sonic.zhang@...log.com, vapier@...too.org,
skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linux.dev,
stable@...r.kernel.org
Subject: Re: [PATCH v2] iio: adc: Include valid channel for channel selection
On Fri, Apr 18, 2025 at 11:35 AM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Thu, 17 Apr 2025 13:01:09 -0400
> Gabriel Shahrouzi <gshahrouzi@...il.com> wrote:
>
> > According to the datasheet on page 9 under the channel selection table,
> > all devices (AD7816/7/8) are able to use the channel marked as 7. This
> > channel is used for diagnostic purposes by routing the internal 1.23V
> > bandgap source through the MUX to the input of the ADC.
> >
> > Replace checking for string equality with checking for the same chip ID
> > to reduce time complexity.
> >
> > Group invalid channels for all devices together because they are
> > processed the same way.
> >
> > Fixes: 7924425db04a ("staging: iio: adc: new driver for AD7816 devices")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@...il.com>
>
> This is doing too many things for one patch. The fix wants be
> on it's own. Ideally before everything else but if that is tricky to do
> then I don't mind that much.
Got it - will split them up into modular changes.
>
> > ---
> > Changes since v2:
> > - Refactor by adding chip_info struct which simplifies
> > condtional logic.
> > ---
> > drivers/staging/iio/adc/ad7816.c | 68 ++++++++++++++++++--------------
> > 1 file changed, 38 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7816.c b/drivers/staging/iio/adc/ad7816.c
> > index 6c14d7bcdd675..ec955cbf06c17 100644
> > --- a/drivers/staging/iio/adc/ad7816.c
> > +++ b/drivers/staging/iio/adc/ad7816.c
> > @@ -41,8 +41,20 @@
> > * struct ad7816_chip_info - chip specific information
> > */
> >
> > +enum ad7816_type {
> > + ID_AD7816,
> > + ID_AD7817,
> > + ID_AD7818,
> > +};
> The aim of moving to a chip_info structure is typically
> to get rid of this sort of enum in favour of specific data
> in the chip_info structures. Anyhow see below for more on that.
>
> > +
> > struct ad7816_chip_info {
> > - kernel_ulong_t id;
> > + const char *name;
> > + enum ad7816_type type;
> > + u8 max_channels;
> > +};
> > +
> > +struct ad7816_state {
>
> This effective rename needs to be a patch on it's own before
> you introduce the new struct ad7816_chip_info. That lets
> us clearly see that all instances are renamed and is
> an easy patch to review as it's just a rename.
Got it.
>
> > + const struct ad7816_chip_info *chip_info;
> > struct spi_device *spi_dev;
> > struct gpio_desc *rdwr_pin;
> > struct gpio_desc *convert_pin;
> > @@ -52,16 +64,11 @@ struct ad7816_chip_info {
> > u8 mode;
> > };
> >
> > -enum ad7816_type {
> > - ID_AD7816,
> > - ID_AD7817,
> > - ID_AD7818,
> > -};
> >
> > @@ -215,7 +215,7 @@ static ssize_t ad7816_show_value(struct device *dev,
> > char *buf)
> > {
> > struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > - struct ad7816_chip_info *chip = iio_priv(indio_dev);
> > + struct ad7816_state *chip = iio_priv(indio_dev);
> > u16 data;
> > s8 value;
> > int ret;
> > @@ -271,7 +271,7 @@ static ssize_t ad7816_show_oti(struct device *dev,
> > char *buf)
> > {
> > struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > - struct ad7816_chip_info *chip = iio_priv(indio_dev);
> > + struct ad7816_state *chip = iio_priv(indio_dev);
> > int value;
> >
> > if (chip->channel_id > AD7816_CS_MAX) {
> > @@ -292,7 +292,7 @@ static inline ssize_t ad7816_set_oti(struct device *dev,
> > size_t len)
> > {
> > struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > - struct ad7816_chip_info *chip = iio_priv(indio_dev);
> > + struct ad7816_state *chip = iio_priv(indio_dev);
> > long value;
> > u8 data;
> > int ret;
> > @@ -345,14 +345,22 @@ static const struct iio_info ad7816_info = {
> > .event_attrs = &ad7816_event_attribute_group,
> > };
> >
> > +static const struct ad7816_chip_info ad7816_chip_infos[] = {
> > + [ID_AD7816] = { .name = "ad7816", .max_channels = 0, .type = ID_AD7816 },
> Use separate named structures.
>
> This approach with an enum and an array is something we used to do but it
> actually obscures what is going on when compared with separate names structure
> instances.
>
> static const struct ad7816_chip_info ad7816_chip_info = {..};
>
> static const struct ad7816_chip_info ad7817_chip_info = {..};
>
> etc.
Got it.
> > + [ID_AD7817] = { .name = "ad7817", .max_channels = 3, .type = ID_AD7817 },
> > + [ID_AD7818] = { .name = "ad7818", .max_channels = 1, .type = ID_AD7818 },
> > +};
> > +
> > /*
> > * device probe and remove
> > */
> >
> > static int ad7816_probe(struct spi_device *spi_dev)
> > {
> > - struct ad7816_chip_info *chip;
> > + struct ad7816_state *chip;
> > struct iio_dev *indio_dev;
> > + const struct spi_device_id *id = spi_get_device_id(spi_dev);
> > + enum ad7816_type chip_type = (enum ad7816_type)id->driver_data;
>
> Don't go via an enum. Put pointers directly in the driver_data field.
> + if the driver has on in the of_match_id table as well.
>
> Then use spi_get_device_match_data() to retrieve it in a firmware type
> independent way.
>
> Ideally by the end of the series you should no such enum
Sent the changes in v2 where no enum was used.
>
> > int i, ret;
> >
> > indio_dev = devm_iio_device_alloc(&spi_dev->dev, sizeof(*chip));
> > @@ -361,12 +369,12 @@ static int ad7816_probe(struct spi_device *spi_dev)
> > chip = iio_priv(indio_dev);
> > /* this is only used for device removal purposes */
> > dev_set_drvdata(&spi_dev->dev, indio_dev);
> > + chip->chip_info = &ad7816_chip_infos[chip_type];
> >
> > chip->spi_dev = spi_dev;
> > for (i = 0; i <= AD7816_CS_MAX; i++)
> > chip->oti_data[i] = 203;
> >
> > - chip->id = spi_get_device_id(spi_dev)->driver_data;
> > chip->rdwr_pin = devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_OUT_HIGH);
> > if (IS_ERR(chip->rdwr_pin)) {
> > ret = PTR_ERR(chip->rdwr_pin);
> > @@ -382,7 +390,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
> > ret);
> > return ret;
> > }
> > - if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
> > + if (chip->chip_info->type == ID_AD7816 || chip->chip_info->type == ID_AD7817) {
>
> Given you now have a chip_info structure make this data rather than code. That is
> it should be something ilke
>
> if (chip->chip_info->has_busy_pin)
>
> > chip->busy_pin = devm_gpiod_get(&spi_dev->dev, "busy",
> > GPIOD_IN);
> > if (IS_ERR(chip->busy_pin)) {
> > @@ -393,7 +401,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
> > }
> > }
> >
> > - indio_dev->name = spi_get_device_id(spi_dev)->name;
> > + indio_dev->name = chip->chip_info->name;
> > indio_dev->info = &ad7816_info;
> > indio_dev->modes = INDIO_DIRECT_MODE;
> >
>
Powered by blists - more mailing lists