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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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