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

[David Lechner]

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);
>>> +}