On 1/14/25 7:20 AM, Jonathan Cameron wrote:
>
> Hi Antoniu
>
> For future replies please crop to only the bits you are reply to.
> Took me a couple of goes to find the reply, so in some cases
> the important parts can be completely missed by a reader if
> the rest isn't cropped down.
>
> ...
>
>>>> +static int ad4851_set_oversampling_ratio(struct iio_dev *indio_dev,
>>>> + const struct iio_chan_spec *chan,
>>>> + unsigned int osr)
>>>> +{
>>>> + struct ad4851_state *st = iio_priv(indio_dev);
>>>> + int val, ret;
>>>> +
>>>> + guard(mutex)(&st->lock);
>>>> +
>>>> + if (osr == 1) {
>>>> + ret = regmap_clear_bits(st->regmap,
>>> AD4851_REG_OVERSAMPLE,
>>>> + AD4851_OS_EN_MSK);
>>>> + if (ret)
>>>> + return ret;
>>>> + } else {
>>>> + val = ad4851_osr_to_regval(osr);
>>>> + if (val < 0)
>>>> + return -EINVAL;
>>>> +
>>>> + ret = regmap_update_bits(st->regmap,
>>> AD4851_REG_OVERSAMPLE,
>>>> + AD4851_OS_EN_MSK |
>>>> + AD4851_OS_RATIO_MSK,
>>>> + FIELD_PREP(AD4851_OS_EN_MSK,
>>> 1) |
>>>> +
>>> FIELD_PREP(AD4851_OS_RATIO_MSK, val));
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = iio_backend_oversampling_ratio_set(st->back, osr);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + switch (st->info->resolution) {
>>>> + case 20:
>>>> + switch (osr) {
>>>> + case 0:
>>>> + return -EINVAL;
>>>> + case 1:
>>>> + val = 20;
>>>> + break;
>>>> + default:
>>>> + val = 24;
>>>> + break;
>>>> + }
>>>> + break;
>>>> + case 16:
>>>> + val = 16;
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + ret = iio_backend_data_size_set(st->back, val);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + if (osr == 1 || st->info->resolution == 16) {
>>>> + ret = regmap_clear_bits(st->regmap, AD4851_REG_PACKET,
>>>> + AD4851_PACKET_FORMAT_MASK);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + st->resolution_boost_enabled = false;
>>>> + } else {
>>>> + ret = regmap_update_bits(st->regmap, AD4851_REG_PACKET,
>>>
>>> regmap_set_bits
>>
>> Why? Packet format is two bits wide according to the register map.
Oops, I was expecting it to be symmetric with regmap_clear_bits() above. But
of course you are correct.
>>
>>>> + AD4851_PACKET_FORMAT_MASK,
>>>> +
>>> FIELD_PREP(AD4851_PACKET_FORMAT_MASK, 1));
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + st->resolution_boost_enabled = true;
>>>> + }
>>>> +
>>>> + if (st->osr != osr) {
>>>> + ret = ad4851_scale_fill(indio_dev);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + st->osr = osr;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
> ...
>
>>>> +static int ad4851_setup(struct ad4851_state *st)
>>>> +{
>>>> + unsigned int product_id;
>>>> + int ret;
>>>> +
>>>> + if (st->pd_gpio) {
>>>> + /* To initiate a global reset, bring the PD pin high twice */
>>>> + gpiod_set_value(st->pd_gpio, 1);
>>>> + fsleep(1);
>>>> + gpiod_set_value(st->pd_gpio, 0);
>>>> + fsleep(1);
>>>> + gpiod_set_value(st->pd_gpio, 1);
>>>> + fsleep(1);
>>>> + gpiod_set_value(st->pd_gpio, 0);
>>>> + fsleep(1000);
>>>> + } else {
>>>> + ret = regmap_set_bits(st->regmap,
>>> AD4851_REG_INTERFACE_CONFIG_A,
>>>> + AD4851_SW_RESET);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> +
>>>> + if (st->vrefbuf_en) {
>>>> + ret = regmap_set_bits(st->regmap,
>>> AD4851_REG_DEVICE_CTRL,
>>>> + AD4851_REFBUF_PD);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>> +
>>>> + if (st->vrefio_en) {
>>>> + ret = regmap_set_bits(st->regmap,
>>> AD4851_REG_DEVICE_CTRL,
>>>> + AD4851_REFSEL_PD);
>>>> + if (ret)
>>>> + return ret;
>>>> + }
>>>
>>> PD stands for power down, so should we be powering down if not enabled?
>>> (i.e.
>>> if is missing !)
>> We power down the internal reference if the external one is used. Not sure what is wrong here.
I see. The macro name is wrong, which made me think it was doing something
different. It should be AD4851_REF_SEL rather than AD4851_REFSEL_PD.
>>>> +
>>>> + ret = regmap_write(st->regmap,
>>> AD4851_REG_INTERFACE_CONFIG_B,
>>>> + AD4851_SINGLE_INSTRUCTION);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = regmap_write(st->regmap,
>>> AD4851_REG_INTERFACE_CONFIG_A,
>>>> + AD4851_SDO_ENABLE);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>
> Thanks,
>
> Jonathan
