Re: [PATCH] iio: adc: rockchip_saradc: just get referenced voltage once at probe

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

On Tue, 3 Aug 2021 11:09:47 +0800
David Wu <david.wu@rock-chips.com> wrote:

> Hi Jonathan,
> 
> 在 2021/8/2 下午6:42, Jonathan Cameron 写道:
> > On Mon, 2 Aug 2021 17:09:29 +0800
> > Simon Xue <xxm@rock-chips.com> wrote:
> >   
> >> From: David Wu <david.wu@rock-chips.com>
> >>
> >> The referenced voltage is not changed after initiation, so just only
> >> get referenced voltage once.  
> > Hi David,
> > 
> > Isn't this an external reference voltage?  If so how do you know
> > it is not changed at runtime?  It might be unlikely and not happen
> > on particular platforms, but that's not he same as saying it can never
> > happen.  Clearly it's racey anyway if that does happen, but we definitely
> > don't expect frequent voltage changes.
> >   
> 
> The current regulator is not changed and not subject to external 
> changes, this can reduce the getting voltage. Assuming that there will 
> be changes in the future, we then add the notify of the regulator, so 
> that the voltage change can be obtained.

If this patch added the notifier that would be a nice solution, but
right now it potentially introduced a regression. You have made me a little curious...
Are you seeing a significant cost to querying that regulator voltage?
If so, I'd imagine it's a lack of caching in the regulator driver or similar.
Scale readback via sysfs shouldn't be in a fast path anyway.

You can't depend on what boards today do, because someone with a board
built tomorrow may well use an old kernel which supports the voltage
changing, and then see a regression when they upgrade to the kernel
containing this patch.

Jonathan

> 
> > Jonathan
> >   
> >>
> >> Signed-off-by: Simon Xue <xxm@rock-chips.com>
> >> Signed-off-by: David Wu <david.wu@rock-chips.com>
> >> ---
> >>   drivers/iio/adc/rockchip_saradc.c | 16 +++++++++-------
> >>   1 file changed, 9 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> >> index f3eb8d2e50dc..cd33c0b9d3eb 100644
> >> --- a/drivers/iio/adc/rockchip_saradc.c
> >> +++ b/drivers/iio/adc/rockchip_saradc.c
> >> @@ -49,6 +49,7 @@ struct rockchip_saradc {
> >>   	struct clk		*clk;
> >>   	struct completion	completion;
> >>   	struct regulator	*vref;
> >> +	int			uv_vref;
> >>   	struct reset_control	*reset;
> >>   	const struct rockchip_saradc_data *data;
> >>   	u16			last_val;
> >> @@ -105,13 +106,7 @@ static int rockchip_saradc_read_raw(struct iio_dev *indio_dev,
> >>   		mutex_unlock(&indio_dev->mlock);
> >>   		return IIO_VAL_INT;
> >>   	case IIO_CHAN_INFO_SCALE:
> >> -		ret = regulator_get_voltage(info->vref);
> >> -		if (ret < 0) {
> >> -			dev_err(&indio_dev->dev, "failed to get voltage\n");
> >> -			return ret;
> >> -		}
> >> -
> >> -		*val = ret / 1000;
> >> +		*val = info->uv_vref / 1000;
> >>   		*val2 = chan->scan_type.realbits;
> >>   		return IIO_VAL_FRACTIONAL_LOG2;
> >>   	default:
> >> @@ -410,6 +405,13 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
> >>   		return ret;
> >>   	}
> >>   
> >> +	info->uv_vref = regulator_get_voltage(info->vref);
> >> +	if (info->uv_vref < 0) {
> >> +		dev_err(&pdev->dev, "failed to get voltage\n");
> >> +		ret = info->uv_vref;
> >> +		return ret;
> >> +	}
> >> +
> >>   	ret = clk_prepare_enable(info->pclk);
> >>   	if (ret < 0) {
> >>   		dev_err(&pdev->dev, "failed to enable pclk\n");  
> > 
> > 
> > 
> >   
> 
>