Re: [PATCH v2 2/2] iio: adc: Add ti-ads1018 driver

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Nov 28, 2025 at 12:38:44PM -0500, Kurt Borja wrote:
> On Fri Nov 28, 2025 at 4:54 AM -05, Andy Shevchenko wrote:
> > On Thu, Nov 27, 2025 at 10:37:11PM -0500, Kurt Borja wrote:


Please, remove context you are agree with, no need to keep long message
which is harder to parse for what has been asked, answered, or discussed.

That said, I assume non-commented remarks will be addressed in the next
version as suggested.

...

> >> +static unsigned long ads1018_calc_delay(struct ads1018 *ads1018)
> >> +{
> >> +	const struct ads1018_chip_info *chip_info = ads1018->chip_info;
> >> +	unsigned long mode = chip_info->num_data_rate_mode_to_hz - 1;
> >> +	int hz = chip_info->data_rate_mode_to_hz[mode];
> >
> > Can frequency be negative?
> 
> I hope not 0.0
> 
> I return data_rate_mode_to_hz[] values directly in __ads1018_read_raw(),
> which uses `int *`. That's why I chose int, but I can just make it
> unsigned if that's preferred.

Keep it unsigned wherever it's possible.

...

> > static void ads1018_set_trigger_enable(struct ads1018 *ads1018)
> > {
> > 	spi_bus_lock(ads1018->spi->controller);
> > 	ads1018_read_locked(ads1018, NULL, true);
> > 	enable_irq(ads1018->drdy_irq);
> > }
> >
> > static void ads1018_set_trigger_disable(struct ads1018 *ads1018)
> > {
> > 	disable_irq(ads1018->drdy_irq);
> > 	ads1018_read_locked(ads1018, NULL, false);
> > 	spi_bus_unlock(ads1018->spi->controller);
> > }
> >
> > Or if you make _read_locked() to return the data, just ignoring it as in
> >
> > 	ads1018_read_locked(ads1018, ...);
> 
> Sure, I like this.

I noticed that it may return also an error, with that in mind the NULL pointer
is the easiest way to go.

...

> >> +static const int ads1018_gain_table[][2] = {
> >> +	ADS1018_FSR_TO_SCALE(6144, 11),
> >
> > This won't (hmm... might not? see below for the details) work on 32-bit.
> >
> >> +	ADS1018_FSR_TO_SCALE(4096, 11),
> >> +	ADS1018_FSR_TO_SCALE(2048, 11),
> >> +	ADS1018_FSR_TO_SCALE(1024, 11),
> >> +	ADS1018_FSR_TO_SCALE(512, 11),
> >> +	ADS1018_FSR_TO_SCALE(256, 11),
> >> +};
> >> +
> >> +static const int ads1118_gain_table[][2] = {
> >> +	ADS1018_FSR_TO_SCALE(6144, 15),
> >
> > Ditto.
> >
> >> +	ADS1018_FSR_TO_SCALE(4096, 15),
> >> +	ADS1018_FSR_TO_SCALE(2048, 15),
> >> +	ADS1018_FSR_TO_SCALE(1024, 15),
> >> +	ADS1018_FSR_TO_SCALE(512, 15),
> >> +	ADS1018_FSR_TO_SCALE(256, 15),
> >> +};
> >
> > To address that you need to divide MICRO at least by 2
> > (note, by 2⁶ is also possible without loosing in precision).
> >
> > // Assuming that you want to keep the initialisers as is now:
> > #define ADS1018_FSR_TO_SCALE(_fsr, _res) \
> > 	{ 0, ((_fsr) * (MICRO >> 6)) / BIT((_res) - 6) }
> >
> > OTOH, these all are constants, so the clever compiler (even 32-bit compiler)
> > may do it for you and won't complain as there is no overflow in the result.
> >
> > TL;DR: Try to compile this with 32-bit compiler on 32-bit machine before
> > changing as suggested.
> 
> I didn't consider 32-bit here... I'll test it for v3!

Every time you have a division in the code with (potentially) bit numbers
in the numerator and/or denominator always think about 32-bit case.

...

> Ack, for everything else. Thank you, Andy!

Okay, I assumed correctly in the top, but again, please remove the unneeded
context when replying.

-- 
With Best Regards,
Andy Shevchenko