Re: [PATCH v4 03/10] iio: adc: add the ti-ads1262 driver

[Jonathan Cameron <jic23@kernel.org>]

A couple of comments on comments.

Thanks,

J
> > +	},
> > +	{
> > +		.type = IIO_VOLTAGE,
> > +		.channel = ADS1262_INPMUX_AVDD,
> > +		.channel2 = ADS1262_INPMUX_AVDD,
> > +		.indexed = 1,
> > +		.address = ADS1262_MONITOR_ADDR_OFFSET + 1,
> > +		.scan_type = {
> > +			.format = IIO_SCAN_FORMAT_SIGNED_INT,
> > +			.realbits = ADS1262_ADC1_RESOLUTION,
> > +			.storagebits = 32,
> > +			.endianness = IIO_BE,
> > +		},
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +	},
> > +	{
> > +		.type = IIO_VOLTAGE,
> > +		.channel = ADS1262_INPMUX_DVDD,
> > +		.channel2 = ADS1262_INPMUX_DVDD,
> > +		.indexed = 1,
> > +		.address = ADS1262_MONITOR_ADDR_OFFSET + 2,
> > +		.scan_type = {
> > +			.format = IIO_SCAN_FORMAT_SIGNED_INT,
> > +			.realbits = ADS1262_ADC1_RESOLUTION,
> > +			.storagebits = 32,
> > +			.endianness = IIO_BE,
> > +		},
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +	},
> > +	{
> > +		.type = IIO_VOLTAGE,
> > +		.channel = ADS1262_INPMUX_TDAC,
> > +		.channel2 = ADS1262_INPMUX_TDAC,  
> 
> Hmm... a differential where channel == channel2 usually means a shorted
> input. TDACP and TDACN can be controlled indepedantly, so really are two
> separate channels.

This came up recently.  We do have a history of doing this for
fixed purpose pairs as well.  The ambiguity vs shorted inputs
was one of the negatives, but I decided it was nicer that making
numbers up for IN1+ IN1- type setups.

If they are separately controllable then indeed makes no sense
to do this. Given them separate numbers unless intent is a shorted
channel.

> 
> > +		.indexed = 1,
> > +		.differential = 1,
> > +		.address = ADS1262_MONITOR_ADDR_OFFSET + 3,
> > +		.scan_type = {
> > +			.format = IIO_SCAN_FORMAT_SIGNED_INT,
> > +			.realbits = ADS1262_ADC1_RESOLUTION,
> > +			.storagebits = 32,
> > +			.endianness = IIO_BE,
> > +		},
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +	},
> > +};
> > +  
> > +
> > +	rate = clk_get_rate(clk);
> > +	if (clk && !rate)
> > +		return dev_err_probe(dev, -EINVAL, "failed to get clock rate\n");
> > +	st->clk_rate = rate ? rate : ADS1262_NOMINAL_CLK_RATE;
> > +
> > +	st->start_gpiod = devm_gpiod_get_optional(dev, "start", GPIOD_OUT_LOW);
> > +	if (IS_ERR(st->start_gpiod))
> > +		return dev_err_probe(dev, PTR_ERR(st->start_gpiod),
> > +				     "failed to get start GPIO\n");
> > +
> > +	st->regmap = devm_regmap_init(dev, &ads1262_regmap_bus, st,
> > +				      &ads1262_regmap_config);
> > +	if (IS_ERR(st->regmap))
> > +		return PTR_ERR(st->regmap);
> > +
> > +	ret = ads1262_dev_configure(st);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to configure device\n");
> > +
> > +	indio_dev->name = ads1262_device_id_to_name[st->dev_id];  
> 
> Not so sure about this. Almost always, this is coming from the compatible
> match data. So unless we plan on trusting the device ID returned by the
> chip over the devicetree when we add more to the device id tables and looking
> up per-chip behavior from there instead of the compatible, I would go with
> the traditional approach. That way the name userpace sees match the driver
> behavior that goes with the other chip-specific match data that is likely
> to be added in the future.

We have done this detection path in the past (with fallback to the
dt compat where we don't know better). Normally we do this because
we know there are boards in the wild with the wrong description and
want to be nice.  Here it indeed seems perhaps too complex.

Jonathan