Re: [PATCH v7 4/4] iio: cdc: add Texas Instruments FDC1004 driver

[Victor Chernyakin]

> > +	case IIO_CHAN_INFO_OFFSET: {
> > +		if (val < fdc1004_offset[0] || val > fdc1004_offset[2] ||
> > +		    val % fdc1004_offset[1])
> > +			return -EINVAL;
> > +
> > +		guard(mutex)(&data->lock);
> > +
> > +		data->offsets[chan->channel] = val / fdc1004_offset[1];
> 
> Make a temporary variables to have division and modulo operations coupled
> together. The rationale is explained here:
> 9b3cd5c7099f ("regmap: place foo / 8 and foo % 8 closer to each other")

Now that I think about it, maybe the modulo check is unnecessarily
strict and I should just remove it. Each offset step is ~10% of
the measurement full scale range, so if the driver rounds the value
to the closest step, the maximum difference between what the user
wanted and what they got is 5%. That shouldn't cause any surprises.


> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		val64 = iio_val_s64_compose(val, val2);
> > +		if (val64 < fdc1004_calibbias[0] || val64 > fdc1004_calibbias[2])
> > +			return -EINVAL;
> > +
> > +		return regmap_write(map, FDC1004_REG_OFFSET_CAL_CINn(chan->channel),
> > +				    (u16)DIV_S64_ROUND_CLOSEST(val64, fdc1004_calibbias[1]));
> 
> Why casting? Shouldn't it be handled on the regmap_write() stage based on
> the register / field description?

This was prompted by this sashiko review:

https://lore.kernel.org/SN1PR12MB252888B43D53AB1723952D21A5742@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

TLDR I'm not certain the cast is necessary, but I'm also not certain
it's not necessary (if regmap does handle it, I couldn't figure out
where), so I've added it out of caution.