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: <ngcbj6p7vfakah5fqsxqjlmrcycpg5rxfrbh4s34fll2kb3zq2@eyesluawn5w2>
Date: Thu, 7 Aug 2025 14:10:01 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>, 
	Nuno Sá <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, linux-iio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 06/10] iio: adc: ad7476: Drop convstart chan_spec

On Thu, Aug 07, 2025 at 01:41:31PM +0100, Nuno Sá wrote:
> On Thu, Aug 07, 2025 at 12:34:52PM +0300, Matti Vaittinen wrote:
> > The ad7476 driver defines separate chan_spec structures for operation
> > with and without convstart GPIO. At quick glance this may seem as if the
> > driver did provide more than 1 data-channel to users - one for the
> > regular data, other for the data obtained with the convstart GPIO.
> > 
> > The only difference between the 'convstart' and 'non convstart'
> > -channels is presence / absence of the BIT(IIO_CHAN_INFO_RAW) in
> > channel's flags.
> > 
> > We can drop the convstart channel spec, and related convstart macro, by
> > allocating a mutable per driver instance channel spec an adding the flag
> > in probe if needed. This will simplify the driver with the cost of added
> > memory consumption.
> > 
> > Assuming there aren't systems with very many ADCs and very few
> > resources, this tradeoff seems worth making.
> > 
> > Simplify the driver by dropping the 'convstart' channel spec and
> > allocating the chan spec for each driver instance.
> 
> I do not agree with this one. Looking at the diff, code does not look
> simpler to me...

Ok, on a second thought I'm ok with this. It makes adding devices easier
and (IIUC) for the one you're adding later we only have "convst_channel"
channels.

On comment though...

> 
> - Nuno Sá
> 
> > 
> > Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
> > 
> > ---
> > Revision history:
> >  v1 => v2:
> >  - New patch
> > 
> > I considered squashing this change with the one limiting the chip_info
> > scope. Having this as a separate change should help reverting if someone
> > complains about the increased memory consumption though.
> > ---
> >  drivers/iio/adc/ad7476.c | 31 ++++++++++++++++++-------------
> >  1 file changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
> > index e97742912b8e..a30eb016c11c 100644
> > --- a/drivers/iio/adc/ad7476.c
> > +++ b/drivers/iio/adc/ad7476.c
> > @@ -29,8 +29,6 @@ struct ad7476_state;
> >  struct ad7476_chip_info {
> >  	unsigned int			int_vref_mv;
> >  	struct iio_chan_spec		channel[2];
> > -	/* channels used when convst gpio is defined */
> > -	struct iio_chan_spec		convst_channel[2];
> >  	void (*reset)(struct ad7476_state *);
> >  	bool				has_vref;
> >  	bool				has_vdrive;
> > @@ -41,6 +39,7 @@ struct ad7476_state {
> >  	struct gpio_desc		*convst_gpio;
> >  	struct spi_transfer		xfer;
> >  	struct spi_message		msg;
> > +	struct iio_chan_spec		channel[2];
> >  	int				scale_mv;
> >  	/*
> >  	 * DMA (thus cache coherency maintenance) may require the
> > @@ -153,24 +152,18 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
> >  #define AD7940_CHAN(bits) _AD7476_CHAN((bits), 15 - (bits), \
> >  		BIT(IIO_CHAN_INFO_RAW))
> >  #define AD7091R_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), 0)
> > -#define AD7091R_CONVST_CHAN(bits) _AD7476_CHAN((bits), 16 - (bits), \
> > -		BIT(IIO_CHAN_INFO_RAW))
> >  #define ADS786X_CHAN(bits) _AD7476_CHAN((bits), 12 - (bits), \
> >  		BIT(IIO_CHAN_INFO_RAW))
> >  
> >  static const struct ad7476_chip_info ad7091_chip_info = {
> >  	.channel[0] = AD7091R_CHAN(12),
> >  	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> > -	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
> > -	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> >  	.reset = ad7091_reset,
> >  };
> >  
> >  static const struct ad7476_chip_info ad7091r_chip_info = {
> >  	.channel[0] = AD7091R_CHAN(12),
> >  	.channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> > -	.convst_channel[0] = AD7091R_CONVST_CHAN(12),
> > -	.convst_channel[1] = IIO_CHAN_SOFT_TIMESTAMP(1),
> >  	.int_vref_mv = 2500,
> >  	.has_vref = true,
> >  	.reset = ad7091_reset,
> > @@ -282,7 +275,7 @@ static int ad7476_probe(struct spi_device *spi)
> >  	const struct ad7476_chip_info *chip_info;
> >  	struct ad7476_state *st;
> >  	struct iio_dev *indio_dev;
> > -	int ret;
> > +	int ret, i;
> >  
> >  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> >  	if (!indio_dev)
> > @@ -332,16 +325,28 @@ static int ad7476_probe(struct spi_device *spi)
> >  	if (IS_ERR(st->convst_gpio))
> >  		return PTR_ERR(st->convst_gpio);
> >  
> > +	/*
> > +	 * This will never realize. Unless someone changes the channel specs
> > +	 * in this driver. And if someone does, without changing the loop
> > +	 * below, then we'd better immediately produce a big fat error, before
> > +	 * the change proceeds from that developer's table.
> > +	 */
> > +	BUILD_BUG_ON(ARRAY_SIZE(st->channel) != ARRAY_SIZE(chip_info->channel));

I guess it make sense but still looks too fancy for this :)

> > +	for (i = 0; i < ARRAY_SIZE(st->channel); i++) {
> > +		st->channel[i] = chip_info->channel[i];
> > +		if (st->convst_gpio)

I would flip this an do:
	if (!st->convst_gpio)
		break;
		
> > +			st->channel[i].info_mask_separate |=
> > +				BIT(IIO_CHAN_INFO_RAW);
		
		__set_bit()...

- Nuno Sá

> > +	}
> > +
> >  	st->spi = spi;
> >  
> >  	indio_dev->name = spi_get_device_id(spi)->name;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> > -	indio_dev->channels = chip_info->channel;
> > -	indio_dev->num_channels = 2;
> > +	indio_dev->channels = st->channel;
> > +	indio_dev->num_channels = ARRAY_SIZE(st->channel);
> >  	indio_dev->info = &ad7476_info;
> >  
> > -	if (st->convst_gpio)
> > -		indio_dev->channels = chip_info->convst_channel;
> >  	/* Setup default message */
> >  
> >  	st->xfer.rx_buf = &st->data;
> > -- 
> > 2.50.1
> > 
> 
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ