Re: [PATCH v2 3/3] iio: cdc: add Texas Instruments FDC1004 driver

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

On Sun, Oct 25, 2026 at 05:26:37PM -0500, Victor Chernyakin wrote:

...

> > > +/*
> > > + * OFFSET_CAL_CINn: 16 bits, signed. Format: IIO_VAL_DECIMAL64_FEMTO.
> > > + * 1 step = 1/(2^11) pF = 1/(1000 * 2^11) nF = 0.000000488281250 nF.
> > > + */
> > > +static const s64 fdc1004_calibbias[] = {
> > > +	-0x8000 * 488281250UL, /* min */
> > > +	+0x0001 * 488281250UL, /* step */
> > > +	+0x7FFF * 488281250UL, /* max */
> > > +};
> > 
> > Can you explain the negative capacitance meaning? Does it mean some vector
> > (direction) in the physical world?
> 
> If a channel has a parasitic capacitance C, we can set a bias of -C to
> get rid of it.

I see, please add it into the comment above, so nobody will be surprised by
the values which we learnt should never be negative.

...

> > > +		/* The datasheet says to read MSB first, LSB second. */
> > > +		ret = regmap_bulk_read(data->regmap, FDC1004_REG_MEASn_MSB(0),
> > > +				       reg_vals, ARRAY_SIZE(reg_vals));
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		*val = (sign_extend32(reg_vals[0], 15) << 8) | (reg_vals[1] >> 8);
> > 
> > So, why the value is not marked as __be32 with the respective conversion?
> 
> regmap gives us the two 16-bit registers in native CPU endianness.

It's only if you specify endianess for the whole communication of regmap.
I'm too lazy now to check the original patch, but if it's not specified,
it means the device native endianess, whatever it is on per register basis.

> If the CPU is little endian, that means the overall 32 bits aren't in
> __be32 format (because the two bytes in each of the halves are swapped).
> 
> (Hopefully that makes sense? I'm not sure if I explained it poorly.)

But yes, even if you do it in the device native endianess, it will still be
a pair of 16-bit values with the halves being swapped in case of treating
them as a single 32-bit value. This is unfortunate... So, basically this
needs to be elaborated in the comment above (where you refer datasheet).

...

> > > +static const int fdc1004_calibbias_decomposed[] = {
> > > +	lower_32_bits(fdc1004_calibbias[0]), upper_32_bits(fdc1004_calibbias[0]),
> > > +	lower_32_bits(fdc1004_calibbias[1]), upper_32_bits(fdc1004_calibbias[1]),
> > > +	lower_32_bits(fdc1004_calibbias[2]), upper_32_bits(fdc1004_calibbias[2]),
> > > +};
> > > +
> > > +static const int fdc1004_calibscale_decomposed[] = {
> > > +	lower_32_bits(fdc1004_calibscale[0]), upper_32_bits(fdc1004_calibscale[0]),
> > > +	lower_32_bits(fdc1004_calibscale[1]), upper_32_bits(fdc1004_calibscale[1]),
> > > +	lower_32_bits(fdc1004_calibscale[2]), upper_32_bits(fdc1004_calibscale[2]),
> > > +};
> > 
> > Why do we need these tables while already having them just in different
> > format? Perhaps you can always keep the decomposed variant and otherwise
> > join them at run-time?
> 
> If we only have decomposed variants, the IIO_CHAN_INFO_CALIBBIAS
> and IIO_CHAN_INFO_CALIBSCALE branches in fdc1004_write_raw would become
> ugly and hard to read, e.g.:
> 
>     if (val64 < fdc1004_calibscale[0] || val64 > fdc1004_calibscale[2])
>         return -EINVAL;
> 
>     return regmap_write(data->regmap,
>                         FDC1004_REG_GAIN_CAL_CINn(chan->channel),
>                         DIV_ROUND_CLOSEST(val64, fdc1004_calibscale[1]));
> 
> versus

What I meant is to compare decomposed val64 as well.

>     if (val64 < iio_val_s64_compose(fdc1004_calibscale[0], fdc1004_calibscale[1]) ||
>         val64 > iio_val_s64_compose(fdc1004_calibscale[4], fdc1004_calibscale[5]))
>         return -EINVAL;
> 
>     return regmap_write(data->regmap, FDC1004_REG_GAIN_CAL_CINn(chan->channel),
>                         DIV_ROUND_CLOSEST(val64,
>                                           iio_val_s64_compose(fdc1004_calibscale[2],
>                                                               fdc1004_calibscale[3])));

You also need to make decomposed arrays to be double dimension arrays.
Otherwise this [2]/[3] is confusing in comparison to [1][0]/[1][1].

-- 
With Best Regards,
Andy Shevchenko