diff mbox

[v2,1/4] iio: adc: rockchip_saradc: reset saradc controller before programming it

Message ID 1469535195-5227-1-git-send-email-wxt@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Caesar Wang July 26, 2016, 12:13 p.m. UTC
SARADC controller needs to be reset before programming it, otherwise
it will not function properly.

Signed-off-by: Caesar Wang <wxt@rock-chips.com>
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-iio@vger.kernel.org
Cc: linux-rockchip@lists.infradead.org

---

Changes in v2:
- Make the reset as an optional property, since it should work
with old devicetrees as well.

 .../bindings/iio/adc/rockchip-saradc.txt           |  7 +++++
 drivers/iio/adc/Kconfig                            |  1 +
 drivers/iio/adc/rockchip_saradc.c                  | 31 ++++++++++++++++++++++
 3 files changed, 39 insertions(+)

Comments

Guenter Roeck July 26, 2016, 1:39 p.m. UTC | #1
On 07/26/2016 05:13 AM, Caesar Wang wrote:
> SARADC controller needs to be reset before programming it, otherwise
> it will not function properly.
>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-iio@vger.kernel.org
> Cc: linux-rockchip@lists.infradead.org
>
> ---
>
> Changes in v2:
> - Make the reset as an optional property, since it should work
> with old devicetrees as well.
>
>   .../bindings/iio/adc/rockchip-saradc.txt           |  7 +++++
>   drivers/iio/adc/Kconfig                            |  1 +
>   drivers/iio/adc/rockchip_saradc.c                  | 31 ++++++++++++++++++++++
>   3 files changed, 39 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt b/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt
> index bf99e2f..205593f 100644
> --- a/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt
> @@ -16,6 +16,11 @@ Required properties:
>   - vref-supply: The regulator supply ADC reference voltage.
>   - #io-channel-cells: Should be 1, see ../iio-bindings.txt
>
> +Optional properties:
> +- resets: Must contain an entry for each entry in reset-names if need support
> +	  this option. See ../reset/reset.txt for details.
> +- reset-names: Must include the name "saradc-apb".
> +
>   Example:
>   	saradc: saradc@2006c000 {
>   		compatible = "rockchip,saradc";
> @@ -23,6 +28,8 @@ Example:
>   		interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>   		clocks = <&cru SCLK_SARADC>, <&cru PCLK_SARADC>;
>   		clock-names = "saradc", "apb_pclk";
> +		resets = <&cru SRST_SARADC>;
> +		reset-names = "saradc-apb";
>   		#io-channel-cells = <1>;
>   		vref-supply = <&vcc18>;
>   	};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 1de31bd..7675772 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -389,6 +389,7 @@ config QCOM_SPMI_VADC
>   config ROCKCHIP_SARADC
>   	tristate "Rockchip SARADC driver"
>   	depends on ARCH_ROCKCHIP || (ARM && COMPILE_TEST)
> +	depends on RESET_CONTROLLER
>   	help
>   	  Say yes here to build support for the SARADC found in SoCs from
>   	  Rockchip.
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index f9ad6c2..dc1379f 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -21,6 +21,8 @@
>   #include <linux/of_device.h>
>   #include <linux/clk.h>
>   #include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/reset.h>
>   #include <linux/regulator/consumer.h>
>   #include <linux/iio/iio.h>
>
> @@ -53,6 +55,7 @@ struct rockchip_saradc {
>   	struct clk		*clk;
>   	struct completion	completion;
>   	struct regulator	*vref;
> +	struct reset_control	*reset;
>   	const struct rockchip_saradc_data *data;
>   	u16			last_val;
>   };
> @@ -190,6 +193,16 @@ static const struct of_device_id rockchip_saradc_match[] = {
>   };
>   MODULE_DEVICE_TABLE(of, rockchip_saradc_match);
>
> +/**
> + * Reset SARADC Controller.
> + */
> +static void rockchip_saradc_reset_controller(struct reset_control *reset)
> +{
> +	reset_control_assert(reset);
> +	usleep_range(10, 20);
> +	reset_control_deassert(reset);
> +}
> +
>   static int rockchip_saradc_probe(struct platform_device *pdev)
>   {
>   	struct rockchip_saradc *info = NULL;
> @@ -218,6 +231,21 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>   	if (IS_ERR(info->regs))
>   		return PTR_ERR(info->regs);
>
> +	/*
> +	 * The reset should be an optional property, as it should work
> +	 * with old devicetrees as well
> +	 */
> +	info->reset = devm_reset_control_get_optional(&pdev->dev,
> +			"saradc-apb");

Does anyone know what the _optional API is for ? It seems to be exactly the same
as devm_reset_control_get().

Guenter

> +	if (IS_ERR(info->reset)) {
> +		ret = PTR_ERR(info->reset);
> +		if (ret != -ENOENT)
> +			return ret;
> +
> +		dev_dbg(&pdev->dev, "no reset control found\n");
> +		info->reset = NULL;
> +	}
> +
>   	init_completion(&info->completion);
>
>   	irq = platform_get_irq(pdev, 0);
> @@ -252,6 +280,9 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>   		return PTR_ERR(info->vref);
>   	}
>
> +	if (info->reset)
> +		rockchip_saradc_reset_controller(info->reset);
> +
>   	/*
>   	 * Use a default value for the converter clock.
>   	 * This may become user-configurable in the future.
>
Caesar Wang July 26, 2016, 2:47 p.m. UTC | #2
On 2016年07月26日 21:39, Guenter Roeck wrote:
>
>> static int rockchip_saradc_probe(struct platform_device *pdev)
>> {
>> struct rockchip_saradc *info = NULL;
>> @@ -218,6 +231,21 @@ static int rockchip_saradc_probe(struct 
>> platform_device *pdev)
>> if (IS_ERR(info->regs))
>> return PTR_ERR(info->regs);
>>
>> + /*
>> + * The reset should be an optional property, as it should work
>> + * with old devicetrees as well
>> + */
>> + info->reset = devm_reset_control_get_optional(&pdev->dev,
>> + "saradc-apb");
>
> Does anyone know what the _optional API is for ? It seems to be 
> exactly the same
> as devm_reset_control_get().

“
As far as I see, the difference is WARN_ON(1)
when CONFIG_RESET_CONTROLLER is not defined.


The _optional functions were introduced by the following commit:

----------------->8-----------------
commit b424080a9e086e683ad5fdc624a7cf3c024e0c0f
Author: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
Date: Fri Mar 7 15:18:47 2014 +0100

reset: Add optional resets and stubs

This patch adds device_reset_optional and (devm_)reset_control_get_optional
variants that drivers can use to indicate they can function without control
over the reset line. For those functions, stubs are added so the drivers 
can
be compiled with CONFIG_RESET_CONTROLLER disabled.
Also, device_reset is annotated with __must_check. Drivers
ignoring the return
value should use device_reset_optional instead.

Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
------------------8<-----------------------------
”

for more information on 
http://www.spinics.net/lists/kernel/msg2306677.html :-)

>
> Guenter
>
>> + if (IS_ERR(info->reset)) {
>> + ret = PTR_ERR(info->reset);
>> + if (ret != -ENOENT)
>> + return ret;
>> +
>> + dev_dbg(&pdev->dev, "no reset control found\n");
>> + info->reset = NULL;
>> + }
>> +
>> init_completion(&info->completion);
>>
>> irq = platform_get_irq(pdev, 0);
>> @@ -252,6 +280,9 @@ static int rockchip_saradc_probe(struct 
>> platform_device *pdev)
>> return PTR_ERR(info->vref);
>> }
>>
>> + if (info->reset)
>> + rockchip_saradc_reset_controller(info->reset);
>> +
>> /*
>> * Use a default value for the converter clock.
>> * This may become user-configurable in the future.
>>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Guenter Roeck July 26, 2016, 5 p.m. UTC | #3
On Tue, Jul 26, 2016 at 10:47:16PM +0800, Caesar Wang wrote:
> 
> On 2016年07月26日 21:39, Guenter Roeck wrote:
> >
> >>static int rockchip_saradc_probe(struct platform_device *pdev)
> >>{
> >>struct rockchip_saradc *info = NULL;
> >>@@ -218,6 +231,21 @@ static int rockchip_saradc_probe(struct
> >>platform_device *pdev)
> >>if (IS_ERR(info->regs))
> >>return PTR_ERR(info->regs);
> >>
> >>+ /*
> >>+ * The reset should be an optional property, as it should work
> >>+ * with old devicetrees as well
> >>+ */
> >>+ info->reset = devm_reset_control_get_optional(&pdev->dev,
> >>+ "saradc-apb");
> >
> >Does anyone know what the _optional API is for ? It seems to be exactly
> >the same
> >as devm_reset_control_get().
> 
> “
> As far as I see, the difference is WARN_ON(1)
> when CONFIG_RESET_CONTROLLER is not defined.
> 
> 
> The _optional functions were introduced by the following commit:
> 
> ----------------->8-----------------
> commit b424080a9e086e683ad5fdc624a7cf3c024e0c0f
> Author: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Date: Fri Mar 7 15:18:47 2014 +0100
> 
> reset: Add optional resets and stubs
> 
> This patch adds device_reset_optional and (devm_)reset_control_get_optional
> variants that drivers can use to indicate they can function without control
> over the reset line. For those functions, stubs are added so the drivers can
> be compiled with CONFIG_RESET_CONTROLLER disabled.
> Also, device_reset is annotated with __must_check. Drivers
> ignoring the return
> value should use device_reset_optional instead.
> 

Is that really what we are looking for here ? CONFIG_RESET_CONTROLLER
is required for other functions of rk3399, isn't it ?

Guenter

> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> ------------------8<-----------------------------
> ”
> 
> for more information on http://www.spinics.net/lists/kernel/msg2306677.html
> :-)
> 
> >
> >Guenter
> >
> >>+ if (IS_ERR(info->reset)) {
> >>+ ret = PTR_ERR(info->reset);
> >>+ if (ret != -ENOENT)
> >>+ return ret;
> >>+
> >>+ dev_dbg(&pdev->dev, "no reset control found\n");
> >>+ info->reset = NULL;
> >>+ }
> >>+
> >>init_completion(&info->completion);
> >>
> >>irq = platform_get_irq(pdev, 0);
> >>@@ -252,6 +280,9 @@ static int rockchip_saradc_probe(struct
> >>platform_device *pdev)
> >>return PTR_ERR(info->vref);
> >>}
> >>
> >>+ if (info->reset)
> >>+ rockchip_saradc_reset_controller(info->reset);
> >>+
> >>/*
> >>* Use a default value for the converter clock.
> >>* This may become user-configurable in the future.
> >>
> >
> >
> >_______________________________________________
> >Linux-rockchip mailing list
> >Linux-rockchip@lists.infradead.org
> >http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
> 
> -- 
> caesar wang | software engineer | wxt@rock-chip.com
> 
>
Guenter Roeck July 26, 2016, 10:41 p.m. UTC | #4
On Tue, Jul 26, 2016 at 08:13:12PM +0800, Caesar Wang wrote:
> SARADC controller needs to be reset before programming it, otherwise
> it will not function properly.
> 
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-iio@vger.kernel.org
> Cc: linux-rockchip@lists.infradead.org

Tested-by: Guenter Roeck <linux@roeck-us.net>

> 
> ---
> 
> Changes in v2:
> - Make the reset as an optional property, since it should work
> with old devicetrees as well.
> 
>  .../bindings/iio/adc/rockchip-saradc.txt           |  7 +++++
>  drivers/iio/adc/Kconfig                            |  1 +
>  drivers/iio/adc/rockchip_saradc.c                  | 31 ++++++++++++++++++++++
>  3 files changed, 39 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt b/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt
> index bf99e2f..205593f 100644
> --- a/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt
> @@ -16,6 +16,11 @@ Required properties:
>  - vref-supply: The regulator supply ADC reference voltage.
>  - #io-channel-cells: Should be 1, see ../iio-bindings.txt
>  
> +Optional properties:
> +- resets: Must contain an entry for each entry in reset-names if need support
> +	  this option. See ../reset/reset.txt for details.
> +- reset-names: Must include the name "saradc-apb".
> +
>  Example:
>  	saradc: saradc@2006c000 {
>  		compatible = "rockchip,saradc";
> @@ -23,6 +28,8 @@ Example:
>  		interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&cru SCLK_SARADC>, <&cru PCLK_SARADC>;
>  		clock-names = "saradc", "apb_pclk";
> +		resets = <&cru SRST_SARADC>;
> +		reset-names = "saradc-apb";
>  		#io-channel-cells = <1>;
>  		vref-supply = <&vcc18>;
>  	};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 1de31bd..7675772 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -389,6 +389,7 @@ config QCOM_SPMI_VADC
>  config ROCKCHIP_SARADC
>  	tristate "Rockchip SARADC driver"
>  	depends on ARCH_ROCKCHIP || (ARM && COMPILE_TEST)
> +	depends on RESET_CONTROLLER
>  	help
>  	  Say yes here to build support for the SARADC found in SoCs from
>  	  Rockchip.
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index f9ad6c2..dc1379f 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -21,6 +21,8 @@
>  #include <linux/of_device.h>
>  #include <linux/clk.h>
>  #include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/reset.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/iio/iio.h>
>  
> @@ -53,6 +55,7 @@ struct rockchip_saradc {
>  	struct clk		*clk;
>  	struct completion	completion;
>  	struct regulator	*vref;
> +	struct reset_control	*reset;
>  	const struct rockchip_saradc_data *data;
>  	u16			last_val;
>  };
> @@ -190,6 +193,16 @@ static const struct of_device_id rockchip_saradc_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, rockchip_saradc_match);
>  
> +/**
> + * Reset SARADC Controller.
> + */
> +static void rockchip_saradc_reset_controller(struct reset_control *reset)
> +{
> +	reset_control_assert(reset);
> +	usleep_range(10, 20);
> +	reset_control_deassert(reset);
> +}
> +
>  static int rockchip_saradc_probe(struct platform_device *pdev)
>  {
>  	struct rockchip_saradc *info = NULL;
> @@ -218,6 +231,21 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  	if (IS_ERR(info->regs))
>  		return PTR_ERR(info->regs);
>  
> +	/*
> +	 * The reset should be an optional property, as it should work
> +	 * with old devicetrees as well
> +	 */
> +	info->reset = devm_reset_control_get_optional(&pdev->dev,
> +			"saradc-apb");
> +	if (IS_ERR(info->reset)) {
> +		ret = PTR_ERR(info->reset);
> +		if (ret != -ENOENT)
> +			return ret;
> +
> +		dev_dbg(&pdev->dev, "no reset control found\n");
> +		info->reset = NULL;
> +	}
> +
>  	init_completion(&info->completion);
>  
>  	irq = platform_get_irq(pdev, 0);
> @@ -252,6 +280,9 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  		return PTR_ERR(info->vref);
>  	}
>  
> +	if (info->reset)
> +		rockchip_saradc_reset_controller(info->reset);
> +
>  	/*
>  	 * Use a default value for the converter clock.
>  	 * This may become user-configurable in the future.
> -- 
> 1.9.1
>
Caesar Wang July 27, 2016, 12:42 a.m. UTC | #5
On 2016年07月27日 01:00, Guenter Roeck wrote:
> On Tue, Jul 26, 2016 at 10:47:16PM +0800, Caesar Wang wrote:
>> On 2016年07月26日 21:39, Guenter Roeck wrote:
>>>> static int rockchip_saradc_probe(struct platform_device *pdev)
>>>> {
>>>> struct rockchip_saradc *info = NULL;
>>>> @@ -218,6 +231,21 @@ static int rockchip_saradc_probe(struct
>>>> platform_device *pdev)
>>>> if (IS_ERR(info->regs))
>>>> return PTR_ERR(info->regs);
>>>>
>>>> + /*
>>>> + * The reset should be an optional property, as it should work
>>>> + * with old devicetrees as well
>>>> + */
>>>> + info->reset = devm_reset_control_get_optional(&pdev->dev,
>>>> + "saradc-apb");
>>> Does anyone know what the _optional API is for ? It seems to be exactly
>>> the same
>>> as devm_reset_control_get().
>> “
>> As far as I see, the difference is WARN_ON(1)
>> when CONFIG_RESET_CONTROLLER is not defined.
>>
>>
>> The _optional functions were introduced by the following commit:
>>
>> ----------------->8-----------------
>> commit b424080a9e086e683ad5fdc624a7cf3c024e0c0f
>> Author: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>> Date: Fri Mar 7 15:18:47 2014 +0100
>>
>> reset: Add optional resets and stubs
>>
>> This patch adds device_reset_optional and (devm_)reset_control_get_optional
>> variants that drivers can use to indicate they can function without control
>> over the reset line. For those functions, stubs are added so the drivers can
>> be compiled with CONFIG_RESET_CONTROLLER disabled.
>> Also, device_reset is annotated with __must_check. Drivers
>> ignoring the return
>> value should use device_reset_optional instead.
>>
> Is that really what we are looking for here ? CONFIG_RESET_CONTROLLER
> is required for other functions of rk3399, isn't it ?

Right, as the DRM and thermal are depend on the CONFIG_RESET_CONTROLLER 
.....


>
> Guenter
>
>> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>> ------------------8<-----------------------------
>> ”
>>
>> for more information on http://www.spinics.net/lists/kernel/msg2306677.html
>> :-)
>>
>>> Guenter
>>>
>>>> + if (IS_ERR(info->reset)) {
>>>> + ret = PTR_ERR(info->reset);
>>>> + if (ret != -ENOENT)
>>>> + return ret;
>>>> +
>>>> + dev_dbg(&pdev->dev, "no reset control found\n");
>>>> + info->reset = NULL;
>>>> + }
>>>> +
>>>> init_completion(&info->completion);
>>>>
>>>> irq = platform_get_irq(pdev, 0);
>>>> @@ -252,6 +280,9 @@ static int rockchip_saradc_probe(struct
>>>> platform_device *pdev)
>>>> return PTR_ERR(info->vref);
>>>> }
>>>>
>>>> + if (info->reset)
>>>> + rockchip_saradc_reset_controller(info->reset);
>>>> +
>>>> /*
>>>> * Use a default value for the converter clock.
>>>> * This may become user-configurable in the future.
>>>>
>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
>> -- 
>> caesar wang | software engineer | wxt@rock-chip.com
>>
>>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Guenter Roeck July 27, 2016, 2 a.m. UTC | #6
On 07/26/2016 05:42 PM, Caesar Wang wrote:
>
> On 2016年07月27日 01:00, Guenter Roeck wrote:
>> On Tue, Jul 26, 2016 at 10:47:16PM +0800, Caesar Wang wrote:
>>> On 2016年07月26日 21:39, Guenter Roeck wrote:
>>>>> static int rockchip_saradc_probe(struct platform_device *pdev)
>>>>> {
>>>>> struct rockchip_saradc *info = NULL;
>>>>> @@ -218,6 +231,21 @@ static int rockchip_saradc_probe(struct
>>>>> platform_device *pdev)
>>>>> if (IS_ERR(info->regs))
>>>>> return PTR_ERR(info->regs);
>>>>>
>>>>> + /*
>>>>> + * The reset should be an optional property, as it should work
>>>>> + * with old devicetrees as well
>>>>> + */
>>>>> + info->reset = devm_reset_control_get_optional(&pdev->dev,
>>>>> + "saradc-apb");
>>>> Does anyone know what the _optional API is for ? It seems to be exactly
>>>> the same
>>>> as devm_reset_control_get().
>>> “
>>> As far as I see, the difference is WARN_ON(1)
>>> when CONFIG_RESET_CONTROLLER is not defined.
>>>
>>>
>>> The _optional functions were introduced by the following commit:
>>>
>>> ----------------->8-----------------
>>> commit b424080a9e086e683ad5fdc624a7cf3c024e0c0f
>>> Author: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>>> Date: Fri Mar 7 15:18:47 2014 +0100
>>>
>>> reset: Add optional resets and stubs
>>>
>>> This patch adds device_reset_optional and (devm_)reset_control_get_optional
>>> variants that drivers can use to indicate they can function without control
>>> over the reset line. For those functions, stubs are added so the drivers can
>>> be compiled with CONFIG_RESET_CONTROLLER disabled.
>>> Also, device_reset is annotated with __must_check. Drivers
>>> ignoring the return
>>> value should use device_reset_optional instead.
>>>
>> Is that really what we are looking for here ? CONFIG_RESET_CONTROLLER
>> is required for other functions of rk3399, isn't it ?
>
> Right, as the DRM and thermal are depend on the CONFIG_RESET_CONTROLLER .....
>

Since "optional" doesn't really mean "the reset property is optional"
but "CONFIG_RESET_CONTROLLER is optional", I would suggest to use
devm_reset_control_get().

Guenter
Caesar Wang July 27, 2016, 2:11 a.m. UTC | #7
On 2016年07月27日 10:00, Guenter Roeck wrote:
> On 07/26/2016 05:42 PM, Caesar Wang wrote:
>>
>> On 2016年07月27日 01:00, Guenter Roeck wrote:
>>> On Tue, Jul 26, 2016 at 10:47:16PM +0800, Caesar Wang wrote:
>>>> On 2016年07月26日 21:39, Guenter Roeck wrote:
>>>>>> static int rockchip_saradc_probe(struct platform_device *pdev)
>>>>>> {
>>>>>> struct rockchip_saradc *info = NULL;
>>>>>> @@ -218,6 +231,21 @@ static int rockchip_saradc_probe(struct
>>>>>> platform_device *pdev)
>>>>>> if (IS_ERR(info->regs))
>>>>>> return PTR_ERR(info->regs);
>>>>>>
>>>>>> + /*
>>>>>> + * The reset should be an optional property, as it should work
>>>>>> + * with old devicetrees as well
>>>>>> + */
>>>>>> + info->reset = devm_reset_control_get_optional(&pdev->dev,
>>>>>> + "saradc-apb");
>>>>> Does anyone know what the _optional API is for ? It seems to be 
>>>>> exactly
>>>>> the same
>>>>> as devm_reset_control_get().
>>>> “
>>>> As far as I see, the difference is WARN_ON(1)
>>>> when  is not defined.
>>>>
>>>>
>>>> The _optional functions were introduced by the following commit:
>>>>
>>>> ----------------->8-----------------
>>>> commit b424080a9e086e683ad5fdc624a7cf3c024e0c0f
>>>> Author: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
>>>> Date: Fri Mar 7 15:18:47 2014 +0100
>>>>
>>>> reset: Add optional resets and stubs
>>>>
>>>> This patch adds device_reset_optional and
>>>> variants that drivers can use to indicate they can function without 
>>>> control
>>>> over the reset line. For those functions, stubs are added so the 
>>>> drivers can
>>>> be compiled with CONFIG_RESET_CONTROLLER disabled.
>>>> Also, device_reset is annotated with __must_check. Drivers
>>>> ignoring the return
>>>> value should use device_reset_optional instead.
>>>>
>>> Is that really what we are looking for here ? CONFIG_RESET_CONTROLLER
>>> is required for other functions of rk3399, isn't it ?
>>
>> Right, as the DRM and thermal are depend on the 
>> CONFIG_RESET_CONTROLLER .....
>>
>
> Since "optional" doesn't really mean "the reset property is optional"
> but "CONFIG_RESET_CONTROLLER is optional", I would suggest to use
> devm_reset_control_get().

Agree, free riding.
Maybe the API (_optional) will be changed in later.
As someone make a point present an idea on 
http://www.spinics.net/lists/kernel/msg2306677.html

Okay,  devm_reset_control_get() will be better, anyway the driver has 
been depend on the CONFIG_RESET_CONTROLLER.
--

I will send a new patch for upstream if nobody object it on today.


Sorry for noisy!


>
> Guenter
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Rob Herring (Arm) July 27, 2016, 3:12 p.m. UTC | #8
On Tue, Jul 26, 2016 at 08:13:12PM +0800, Caesar Wang wrote:
> SARADC controller needs to be reset before programming it, otherwise
> it will not function properly.
> 
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-iio@vger.kernel.org
> Cc: linux-rockchip@lists.infradead.org
> 
> ---
> 
> Changes in v2:
> - Make the reset as an optional property, since it should work
> with old devicetrees as well.
> 
>  .../bindings/iio/adc/rockchip-saradc.txt           |  7 +++++

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/iio/adc/Kconfig                            |  1 +
>  drivers/iio/adc/rockchip_saradc.c                  | 31 ++++++++++++++++++++++
>  3 files changed, 39 insertions(+)
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt b/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt
index bf99e2f..205593f 100644
--- a/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/rockchip-saradc.txt
@@ -16,6 +16,11 @@  Required properties:
 - vref-supply: The regulator supply ADC reference voltage.
 - #io-channel-cells: Should be 1, see ../iio-bindings.txt
 
+Optional properties:
+- resets: Must contain an entry for each entry in reset-names if need support
+	  this option. See ../reset/reset.txt for details.
+- reset-names: Must include the name "saradc-apb".
+
 Example:
 	saradc: saradc@2006c000 {
 		compatible = "rockchip,saradc";
@@ -23,6 +28,8 @@  Example:
 		interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&cru SCLK_SARADC>, <&cru PCLK_SARADC>;
 		clock-names = "saradc", "apb_pclk";
+		resets = <&cru SRST_SARADC>;
+		reset-names = "saradc-apb";
 		#io-channel-cells = <1>;
 		vref-supply = <&vcc18>;
 	};
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 1de31bd..7675772 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -389,6 +389,7 @@  config QCOM_SPMI_VADC
 config ROCKCHIP_SARADC
 	tristate "Rockchip SARADC driver"
 	depends on ARCH_ROCKCHIP || (ARM && COMPILE_TEST)
+	depends on RESET_CONTROLLER
 	help
 	  Say yes here to build support for the SARADC found in SoCs from
 	  Rockchip.
diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
index f9ad6c2..dc1379f 100644
--- a/drivers/iio/adc/rockchip_saradc.c
+++ b/drivers/iio/adc/rockchip_saradc.c
@@ -21,6 +21,8 @@ 
 #include <linux/of_device.h>
 #include <linux/clk.h>
 #include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/reset.h>
 #include <linux/regulator/consumer.h>
 #include <linux/iio/iio.h>
 
@@ -53,6 +55,7 @@  struct rockchip_saradc {
 	struct clk		*clk;
 	struct completion	completion;
 	struct regulator	*vref;
+	struct reset_control	*reset;
 	const struct rockchip_saradc_data *data;
 	u16			last_val;
 };
@@ -190,6 +193,16 @@  static const struct of_device_id rockchip_saradc_match[] = {
 };
 MODULE_DEVICE_TABLE(of, rockchip_saradc_match);
 
+/**
+ * Reset SARADC Controller.
+ */
+static void rockchip_saradc_reset_controller(struct reset_control *reset)
+{
+	reset_control_assert(reset);
+	usleep_range(10, 20);
+	reset_control_deassert(reset);
+}
+
 static int rockchip_saradc_probe(struct platform_device *pdev)
 {
 	struct rockchip_saradc *info = NULL;
@@ -218,6 +231,21 @@  static int rockchip_saradc_probe(struct platform_device *pdev)
 	if (IS_ERR(info->regs))
 		return PTR_ERR(info->regs);
 
+	/*
+	 * The reset should be an optional property, as it should work
+	 * with old devicetrees as well
+	 */
+	info->reset = devm_reset_control_get_optional(&pdev->dev,
+			"saradc-apb");
+	if (IS_ERR(info->reset)) {
+		ret = PTR_ERR(info->reset);
+		if (ret != -ENOENT)
+			return ret;
+
+		dev_dbg(&pdev->dev, "no reset control found\n");
+		info->reset = NULL;
+	}
+
 	init_completion(&info->completion);
 
 	irq = platform_get_irq(pdev, 0);
@@ -252,6 +280,9 @@  static int rockchip_saradc_probe(struct platform_device *pdev)
 		return PTR_ERR(info->vref);
 	}
 
+	if (info->reset)
+		rockchip_saradc_reset_controller(info->reset);
+
 	/*
 	 * Use a default value for the converter clock.
 	 * This may become user-configurable in the future.