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

[David Wu]

Hi Heiko, Jonathan

we are adding the notifier for this patch, indeed for future consideration.

Thanks.

在 2021/8/5 下午8:07, Heiko Stübner 写道:
> Hi David,
> 
> Am Donnerstag, 5. August 2021, 13:56:18 CEST schrieb David Wu:
>> Hi Jonathan,
>>
>> 在 2021/8/3 下午8:51, Jonathan Cameron 写道:
>>> On Tue, 3 Aug 2021 11:09:47 +0800
>>> David Wu <david.wu at 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 at rock-chips.com> wrote:
>>>>>     
>>>>>> From: David Wu <david.wu at 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.
>>>
>>
>> For all current chips, the expected voltage is a fixed voltage, and
>> don't want to change it in any process.:-)
>>
>> So, if the voltage here does not change, then it can be obtained once in
>> probe(), which can save the time of each acquisition. For example, the
>> voltage of this regulator is obtained through i2c, which will increase
>> some consumption every time.
> 
> Jonathans request was to not think about "all current chips" but the
> general case, and as Jonathan said, adding that regulator notifier you
> already mentioned would be the nicest solution.
> 
> So I think the easiest way is to just add the voltage notifier to your patch
> to make everyone happy ;-) .
> 
> 
> Heiko
> 
>>
>>> Jonathan
>>>
>>>>
>>>>> Jonathan
>>>>>     
>>>>>>
>>>>>> Signed-off-by: Simon Xue <xxm at rock-chips.com>
>>>>>> Signed-off-by: David Wu <david.wu at 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");
>>>>>
>>>>>
>>>>>
>>>>>     
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
> 
> 
> 
> 
> 
> 
>