linux-kernel - Re: [PATCH v3 5/5] iio: dac: ltc2664: Add driver for LTC2664 and LTC2672

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: <afd89c16-d9d7-43e3-b40a-a88588fd7346@baylibre.com>
Date: Fri, 7 Jun 2024 14:13:36 -0500
From: David Lechner <dlechner@...libre.com>
To: "Paller, Kim Seer" <KimSeer.Paller@...log.com>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Cc: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen
 <lars@...afoo.de>, Liam Girdwood <lgirdwood@...il.com>,
 Mark Brown <broonie@...nel.org>, Dimitri Fedrau <dima.fedrau@...il.com>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Rob Herring <robh@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>,
 "Hennerich, Michael" <Michael.Hennerich@...log.com>,
 Nuno Sá <noname.nuno@...il.com>,
 kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v3 5/5] iio: dac: ltc2664: Add driver for LTC2664 and
 LTC2672

On 6/6/24 10:49 AM, Paller, Kim Seer wrote:


>>> +#define LTC2664_CHANNEL(_chan) {					\
>>> +	.indexed = 1,							\
>>> +	.output = 1,							\
>>> +	.channel = (_chan),						\
>>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_SCALE) |		\
>>> +		BIT(IIO_CHAN_INFO_OFFSET) | BIT(IIO_CHAN_INFO_RAW),
>> 	\
>>> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW),
>> 	\
>>> +	.ext_info = ltc2664_ext_info,					\
>>> +}
>>> +
>>> +static const struct iio_chan_spec ltc2664_channels[] = {
>>> +	LTC2664_CHANNEL(0),
>>> +	LTC2664_CHANNEL(1),
>>> +	LTC2664_CHANNEL(2),
>>> +	LTC2664_CHANNEL(3),
>>> +};
>>> +
>>> +static const struct iio_chan_spec ltc2672_channels[] = {
>>> +	LTC2664_CHANNEL(0),
>>> +	LTC2664_CHANNEL(1),
>>> +	LTC2664_CHANNEL(2),
>>> +	LTC2664_CHANNEL(3),
>>> +	LTC2664_CHANNEL(4),
>>> +};
>>
>> Do we really need these since they are only used as a template anyway?
>> We could just have a single template for one channel and copy it as
>> manay times as needed.
> 
> Yes, from what I can see we need separate channel specs for both devices
> since they have a differing number of channels. As for your suggestion about
> having a single template for one channel and copying it as many times as
> needed, I'm not entirely sure how to implement it in this context. Could you
> provide something like a code snippet to illustrate this?
> 

Instead of the #define and arrays above, just have a single static struct:


static const struct iio_chan_spec ltc2664_channel_template = {
	.indexed = 1,
	.output = 1,
	.info_mask_separate = BIT(IIO_CHAN_INFO_SCALE) |
			      BIT(IIO_CHAN_INFO_OFFSET) |
			      BIT(IIO_CHAN_INFO_RAW),
	.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW),
	.ext_info = ltc2664_ext_info,
};


>>> +static int ltc2664_setup(struct ltc2664_state *st, struct regulator *vref)
>>> +{
>>> +	const struct ltc2664_chip_info *chip_info = st->chip_info;
>>> +	struct gpio_desc *gpio;
>>> +	int ret;
>>> +
>>> +	/* If we have a clr/reset pin, use that to reset the chip. */
>>> +	gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", GPIOD_OUT_HIGH);
>>> +	if (IS_ERR(gpio))
>>> +		return dev_err_probe(&st->spi->dev, PTR_ERR(gpio),
>>> +				     "Failed to get reset gpio");
>>> +	if (gpio) {
>>> +		usleep_range(1000, 1200);
>>> +		gpiod_set_value_cansleep(gpio, 0);
>>> +	}
>>> +
>>> +	/*
>>> +	 * Duplicate the default channel configuration as it can change during
>>> +	 * @ltc2664_channel_config()
>>> +	 */
>>> +	st->iio_channels = devm_kmemdup(&st->spi->dev, chip_info->iio_chan,
>>> +					(chip_info->num_channels + 1) *
>>> +					sizeof(*chip_info->iio_chan),
>>> +					GFP_KERNEL);

Then here, instead of devm_kmemdup():

	st->iio_channels = devm_kcalloc(&st->spi->dev,
					chip_info->num_channels,
					sizeof(struct iio_chan_spec),
					GFP_KERNEL);
	if (!st->iio_channels)
		return -ENOMEM;

	for (i = 0; i < chip_info->num_channels; i++) {
		st->iio_channels[i] = ltc2664_channel_template;
		st->iio_channels[i].type = chip_info->measurement_type;
		st->iio_channels[i].channel = i;
	}

Note: the original code was missing the error check and I think
num_channels + 1 was 1 too many, so I fixed both of those in the
example as well.

This also replaces:

	st->iio_channels[chan].type = chip_info->measurement_type;

from ltc2664_set_span() as it seems a bit out of place there.

>>> +
>>> +	ret = ltc2664_channel_config(st);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (!vref)
>>> +		return 0;
>>> +
>>> +	return regmap_set_bits(st->regmap, LTC2664_CMD_CONFIG, LTC2664_REF_DISABLE);
>>> +}