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

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

[Simon Xue]

From: David Wu <david.wu@rock-chips.com>

The referenced voltage is not changed after initiation, so just only
get referenced voltage once.

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");
-- 
2.25.1

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

[Simon Xue]

From: David Wu <david.wu@rock-chips.com>

The referenced voltage is not changed after initiation, so just only
get referenced voltage once.

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");
-- 
2.25.1




_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

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

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");

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

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

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");


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[David Wu]

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.

> 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");
> 
> 
> 
> 

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

[David Wu]

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.

> 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");
> 
> 
> 
> 



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Jonathan Cameron]

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");  
> > 
> > 
> > 
> >   
> 
> 

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

[Jonathan Cameron]

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");  
> > 
> > 
> > 
> >   
> 
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[David Wu]

Hi Jonathan,

在 2021/8/3 下午8:51, Jonathan Cameron 写道:
> 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.
> 

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.

> 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");
>>>
>>>
>>>
>>>    
>>
>>
> 
> 
> 
> 

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

[David Wu]

Hi Jonathan,

在 2021/8/3 下午8:51, Jonathan Cameron 写道:
> 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.
> 

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.

> 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");
>>>
>>>
>>>
>>>    
>>
>>
> 
> 
> 
> 



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[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@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.
> > 
> 
> 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@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");
> >>>
> >>>
> >>>
> >>>    
> >>
> >>
> > 
> > 
> > 
> > 
> 
> 
> 

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

[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@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.
> > 
> 
> 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@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");
> >>>
> >>>
> >>>
> >>>    
> >>
> >>
> > 
> > 
> > 
> > 
> 
> 
> 





_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [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@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.
>>>
>>
>> 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@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");
>>>>>
>>>>>
>>>>>
>>>>>     
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
> 
> 
> 
> 
> 
> 
> 

Re: [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@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.
>>>
>>
>> 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@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");
>>>>>
>>>>>
>>>>>
>>>>>     
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
> 
> 
> 
> 
> 
> 
> 



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip