Re: [PATCH v4 2/4] iio: adc: ltc2378: Add support for LTC2378-20 and similar ADCs

[Marcelo Schmitt <marcelo.schmitt1@gmail.com>]

> > +{										\
> > +	.type = IIO_VOLTAGE,							\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
> > +			      BIT(IIO_CHAN_INFO_SCALE) |			\
> > +			      (_offl ? BIT(IIO_CHAN_INFO_SAMP_FREQ) : 0),	\
> > +	.info_mask_separate_available = _offl ? BIT(IIO_CHAN_INFO_SAMP_FREQ) : 0,\
> > +	.scan_index = 0,							\
> > +	.scan_type = {								\
> > +		.format = _sign ? IIO_SCAN_FORMAT_SIGNED_INT :			\
> > +				  IIO_SCAN_FORMAT_UNSIGNED_INT,			\
> > +		.realbits = _real_bits,						\
> > +		.storagebits = _storage_bits,					\
> > +		.shift = (_offl ? 0 : _storage_bits - _real_bits),		\
> > +		.endianness = _offl ? IIO_CPU : IIO_BE				\
> > +	},									\
> > +}
> > +
> > +#define LTC2378_BIPOLAR_DIFF_CHANNEL(_real_bits)				\
> > +	__LTC2378_DIFF_CHANNEL(1, _real_bits, (((_real_bits) > 16) ? 32 : 16), 0)
> > +
> > +#define LTC2378_UNIPOLAR_DIFF_CHANNEL(_real_bits)				\
> > +	__LTC2378_DIFF_CHANNEL(0, _real_bits, (((_real_bits) > 16) ? 32 : 16), 0)
> 
> Why not move the (((_real_bits) > 16) ? 32 : 16) into the __LTC2378_DIFF_CHANNEL()
> macro to avoid repeating it?
> 
Because that would go wrong for LTC2378_OFFLOAD_BIPOLAR_DIFF_CHANNEL() in patch 3.

> > +
> > +struct ltc2378_chip_info {
> > +	const char *name;
> > +	unsigned int internal_ref_uv;
...
> 
> > +static int ltc2378_regulator_setup(struct device *dev, struct ltc2378_state *st)
> > +{
> > +	int ret;
> > +
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "refin");
> > +	if (ret < 0 && ret != -ENODEV) {
> > +		return dev_err_probe(dev, ret, "failed to read refin regulator\n");
> > +	} else if (ret > 0) {
> 
> Else is not needed here.
Why not? Intendend flow/logic is to get and use refin for devices that have it
(currently only one chip has). Except for that chip that has 'refin', other
chips have only 'ref' and no internal reference. Also, the chip that has 'refin'
doesn't have 'ref' so it uses an internal reference when 'refin' is not found.
Anyways, the driver can use 'refin' if there is one available, no?

P.S. Will separate regulator logic according to chip type.

> 
> > +		st->ref_uV = ret;
> > +		return 0;
> > +	}
> > +
> > +	if (st->info->internal_ref_uv) {
> > +		st->ref_uV = st->info->internal_ref_uv;
> > +		return 0;
> > +	}
> 
> I would be tempted to have two separate functions here and only call one depending
> on the chip. Otherwise, it allows incorrect devicetree.

Hmm, I see. Drat, I was hoping to avoid checking for specific chip parts to
handle regulators. dtbs_check should error if a chip other than ltc2338 has 'refin'.
Anyways, I see maintainers will probably prefer the chip specific init flow.

> 
> > +
> > +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "failed to read ref regulator\n");
> > +
> > +	st->ref_uV = ret;
> > +
> > +	return 0;
> > +}
> > +

Will adjust according to other comments as well.

Thanks,
Marcelo