Re: [PATCH v9 8/8] iio: adc: ad4851: add ad485x driver

[David Lechner <dlechner@baylibre.com>]

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