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

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

On 07/16, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The `LTC2378_DIFF_CHANNEL` macro defines an IIO channel that is intended to be differential but lacks the `.differential = 1`, `.indexed = 1`, `.channel = 0`, and `.channel2 = 1` attributes.
> --
> 
> --- Patch [2]: [PATCH v7 2/4] iio: adc: ltc2378: Add support for LTC2378-20 and similar ADCs ---
> commit 5710d5f25b45906977e2522567c1964b1d3219f8
> Author: Marcelo Schmitt <marcelo.schmitt@analog.com>
> 
> iio: adc: ltc2378: Add support for LTC2378-20 and similar ADCs
> 
> This commit adds a new driver for the Analog Devices LTC2378-20 and
> similar analog-to-digital converters.
> 
> > diff --git a/drivers/iio/adc/ltc2378.c b/drivers/iio/adc/ltc2378.c
> > new file mode 100644
> > index 0000000000000..5600c820c7f45
> > --- /dev/null
> > +++ b/drivers/iio/adc/ltc2378.c
> > @@ -0,0 +1,416 @@
> [ ... ]
> > +#define LTC2378_DIFF_CHANNEL(_sign, _real_bits, _storage_bits)			\
> > +{										\
> > +	.type = IIO_VOLTAGE,							\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |				\
> > +			      BIT(IIO_CHAN_INFO_SCALE),				\
> > +	.scan_index = 0,							\
> 
> [Severity: Medium]
> Does this macro need to set .differential = 1, .indexed = 1, .channel = 0,
> and .channel2 = 1? The name suggests these are differential channels, but
> without these attributes, the IIO ABI will expose them as single-ended
> (e.g., in_voltage_raw instead of in_voltage0-voltage1_raw).

For parts with multiple inputs and configurable differential channels (e.g.
AD4130-8, MCP3561/2/4R, NAFE13388 [1, 2, 3]), the voltageY-voltageZ ABI helps
clarifying what physical inputs make up a channel. For differential only parts,
the ABI seems to be flexible by only requiring a 'consistent labeling' (either
voltageY or voltageY-voltageZ IIUC). Will set the channel with the differential
ABI if maintainers believe that's the best way to have it. Might slightly tweak
the channel macros to keep .differential = 0 for pseudo-differential chips
(e.g.LTC2369-18).

[1]: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4130-8.pdf
[2]: https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3561_2_4R-Data-Sheet-DS200006391C.pdf
[3]: https://www.nxp.com/docs/en/data-sheet/NAFE13388.pdf

> 
> > +	.scan_type = {								\
> > +		.format = _sign ? IIO_SCAN_FORMAT_SIGNED_INT :			\
> > +				  IIO_SCAN_FORMAT_UNSIGNED_INT,			\
> > +		.realbits = _real_bits,						\
> > +		.storagebits = _storage_bits,					\
> > +		.shift = _storage_bits - _real_bits,				\
> > +		.endianness = IIO_BE,						\
> > +	},									\
> > +}
> > +
> > +#define LTC2378_BIPOLAR_DIFF_CHANNEL(_real_bits)				\
> > +	LTC2378_DIFF_CHANNEL(1, _real_bits, (((_real_bits) > 16) ? 32 : 16))
> > +
> > +#define LTC2378_UNIPOLAR_DIFF_CHANNEL(_real_bits)				\
> > +	LTC2378_DIFF_CHANNEL(0, _real_bits, (((_real_bits) > 16) ? 32 : 16))
> > +
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784235595.git.marcelo.schmitt@analog.com?part=2