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

[Victor Chernyakin]

Anything I don't reply to I've addressed for v3.

> > +/*
> > + * 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.

> > +		/* 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. 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.)

> > +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

    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])));