diff mbox series

iio: adc: aspeed: Deassert reset in probe

Message ID 20171030072218.2094-1-joel@jms.id.au
State Accepted
Commit edf7550a1f93a88be2bf743b5d352b278d1b789c
Headers show
Series iio: adc: aspeed: Deassert reset in probe | expand

Commit Message

Joel Stanley Oct. 30, 2017, 7:22 a.m. UTC
The ASPEED SoC must deassert a reset in order to use the ADC peripheral.

The device tree bindings are updated to document the resets phandle, and
the example is updated to match what is expected for both the reset and
clock phandle.

Signed-off-by: Joel Stanley <joel@jms.id.au>

---
 .../devicetree/bindings/iio/adc/aspeed_adc.txt      |  4 +++-
 drivers/iio/adc/aspeed_adc.c                        | 21 ++++++++++++++++-----
 2 files changed, 19 insertions(+), 6 deletions(-)

-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Philipp Zabel Oct. 30, 2017, 4:21 p.m. UTC | #1
Hi Joel,

On Mon, Oct 30, 2017 at 8:22 AM, Joel Stanley <joel@jms.id.au> wrote:
> The ASPEED SoC must deassert a reset in order to use the ADC peripheral.

>

> The device tree bindings are updated to document the resets phandle, and

> the example is updated to match what is expected for both the reset and

> clock phandle.

>

> Signed-off-by: Joel Stanley <joel@jms.id.au>

> ---

>  .../devicetree/bindings/iio/adc/aspeed_adc.txt      |  4 +++-

>  drivers/iio/adc/aspeed_adc.c                        | 21 ++++++++++++++++-----

>  2 files changed, 19 insertions(+), 6 deletions(-)

>

> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt

> index 674e133b7cd7..034fc2ba100e 100644

> --- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt

> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt

> @@ -8,6 +8,7 @@ Required properties:

>  - reg: memory window mapping address and length

>  - clocks: Input clock used to derive the sample clock. Expected to be the

>            SoC's APB clock.

> +- resets: Reset controller phandle


Adding new required properties to existing bindings would break backwards
compatibility. In this case, the reset is optional anyway.

>  - #io-channel-cells: Must be set to <1> to indicate channels are selected

>                       by index.

>

> @@ -15,6 +16,7 @@ Example:

>         adc@1e6e9000 {

>                 compatible = "aspeed,ast2400-adc";

>                 reg = <0x1e6e9000 0xb0>;

> -               clocks = <&clk_apb>;

> +               clocks = <&syscon ASPEED_CLK_APB>;

> +               resets = <&syscon ASPEED_RESET_ADC>;

>                 #io-channel-cells = <1>;

>         };

> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c

> index 8a958d5f1905..0fc9712cdde4 100644

> --- a/drivers/iio/adc/aspeed_adc.c

> +++ b/drivers/iio/adc/aspeed_adc.c

> @@ -17,6 +17,7 @@

>  #include <linux/module.h>

>  #include <linux/of_platform.h>

>  #include <linux/platform_device.h>

> +#include <linux/reset.h>

>  #include <linux/spinlock.h>

>  #include <linux/types.h>

>

> @@ -53,11 +54,12 @@ struct aspeed_adc_model_data {

>  };

>

>  struct aspeed_adc_data {

> -       struct device   *dev;

> -       void __iomem    *base;

> -       spinlock_t      clk_lock;

> -       struct clk_hw   *clk_prescaler;

> -       struct clk_hw   *clk_scaler;

> +       struct device           *dev;

> +       void __iomem            *base;

> +       spinlock_t              clk_lock;

> +       struct clk_hw           *clk_prescaler;

> +       struct clk_hw           *clk_scaler;

> +       struct reset_control    *rst;

>  };

>

>  #define ASPEED_CHAN(_idx, _data_reg_addr) {                    \

> @@ -217,6 +219,13 @@ static int aspeed_adc_probe(struct platform_device *pdev)

>                 goto scaler_error;


Since data->rst is initialized to NULL, this does work.
It would be nicer though to add a new label for errors after
reset_control_deassert below.

>         }

>

> +       data->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);

> +       if (IS_ERR(data->rst)) {

> +               dev_err(&pdev->dev, "invalid reset controller in device tree");

> +               data->rst = NULL;

> +       } else

> +               reset_control_deassert(data->rst);


I'd return an error if devm_reset_control_get_optional_exclusive fails.
Then reset_control_deassert can be called unconditionally:

        data->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
        if (IS_ERR(data->rst)) {
                dev_err(&pdev->dev, "invalid reset controller in device tree");
                ret = PTR_ERR(data->rst);
                goto reset_error;
        }
        reset_control_deassert(data->rst);

> +

>         model_data = of_device_get_match_data(&pdev->dev);

>

>         if (model_data->wait_init_sequence) {

> @@ -268,6 +277,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)

>

>  scaler_error:

>         clk_hw_unregister_divider(data->clk_prescaler);

> +       reset_control_assert(data->rst);


This should be done in reverse order, before
clk_hw_unregister_divider, and get its own label.

>         return ret;

>  }

>

> @@ -282,6 +292,7 @@ static int aspeed_adc_remove(struct platform_device *pdev)

>         clk_disable_unprepare(data->clk_scaler->clk);

>         clk_hw_unregister_divider(data->clk_scaler);

>         clk_hw_unregister_divider(data->clk_prescaler);

> +       reset_control_assert(data->rst);


Same here, if the reset is deasserted after registering clk_scaler,
it would be cleaner to assert it before unregistering the clock.

>

>         return 0;

>  }

> --

> 2.14.1

>


regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joel Stanley Oct. 31, 2017, 12:32 a.m. UTC | #2
On Tue, Oct 31, 2017 at 2:51 AM, Philipp Zabel <philipp.zabel@gmail.com> wrote:
> Hi Joel,

>

> On Mon, Oct 30, 2017 at 8:22 AM, Joel Stanley <joel@jms.id.au> wrote:

>> The ASPEED SoC must deassert a reset in order to use the ADC peripheral.

>>

>> The device tree bindings are updated to document the resets phandle, and

>> the example is updated to match what is expected for both the reset and

>> clock phandle.

>>

>> Signed-off-by: Joel Stanley <joel@jms.id.au>

>> ---

>>  .../devicetree/bindings/iio/adc/aspeed_adc.txt      |  4 +++-

>>  drivers/iio/adc/aspeed_adc.c                        | 21 ++++++++++++++++-----

>>  2 files changed, 19 insertions(+), 6 deletions(-)

>>

>> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt

>> index 674e133b7cd7..034fc2ba100e 100644

>> --- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt

>> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt

>> @@ -8,6 +8,7 @@ Required properties:

>>  - reg: memory window mapping address and length

>>  - clocks: Input clock used to derive the sample clock. Expected to be the

>>            SoC's APB clock.

>> +- resets: Reset controller phandle

>

> Adding new required properties to existing bindings would break backwards

> compatibility. In this case, the reset is optional anyway.


As far as the hardware is concerned (and therefore the bindings), the
reset is required.

To date the only reason the driver worked was an out of tree hack in
mach-aspeed. I have written a clk/reset driver and are now working to
ensure the drivers work correctly.

The driver is written to make the reset optional as it's unlikely the
clk/reset driver and device tree updates will land this merge window.
As we say; the bindings should describe the hardware, not the Linux
implementation of the driver.

>

>>  - #io-channel-cells: Must be set to <1> to indicate channels are selected

>>                       by index.

>>

>> @@ -15,6 +16,7 @@ Example:

>>         adc@1e6e9000 {

>>                 compatible = "aspeed,ast2400-adc";

>>                 reg = <0x1e6e9000 0xb0>;

>> -               clocks = <&clk_apb>;

>> +               clocks = <&syscon ASPEED_CLK_APB>;

>> +               resets = <&syscon ASPEED_RESET_ADC>;

>>                 #io-channel-cells = <1>;

>>         };

>> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c

>> index 8a958d5f1905..0fc9712cdde4 100644

>> --- a/drivers/iio/adc/aspeed_adc.c

>> +++ b/drivers/iio/adc/aspeed_adc.c

>> @@ -17,6 +17,7 @@

>>  #include <linux/module.h>

>>  #include <linux/of_platform.h>

>>  #include <linux/platform_device.h>

>> +#include <linux/reset.h>

>>  #include <linux/spinlock.h>

>>  #include <linux/types.h>

>>

>> @@ -53,11 +54,12 @@ struct aspeed_adc_model_data {

>>  };

>>

>>  struct aspeed_adc_data {

>> -       struct device   *dev;

>> -       void __iomem    *base;

>> -       spinlock_t      clk_lock;

>> -       struct clk_hw   *clk_prescaler;

>> -       struct clk_hw   *clk_scaler;

>> +       struct device           *dev;

>> +       void __iomem            *base;

>> +       spinlock_t              clk_lock;

>> +       struct clk_hw           *clk_prescaler;

>> +       struct clk_hw           *clk_scaler;

>> +       struct reset_control    *rst;

>>  };

>>

>>  #define ASPEED_CHAN(_idx, _data_reg_addr) {                    \

>> @@ -217,6 +219,13 @@ static int aspeed_adc_probe(struct platform_device *pdev)

>>                 goto scaler_error;

>

> Since data->rst is initialized to NULL, this does work.

> It would be nicer though to add a new label for errors after

> reset_control_deassert below.

>

>>         }

>>

>> +       data->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);

>> +       if (IS_ERR(data->rst)) {

>> +               dev_err(&pdev->dev, "invalid reset controller in device tree");

>> +               data->rst = NULL;

>> +       } else

>> +               reset_control_deassert(data->rst);

>

> I'd return an error if devm_reset_control_get_optional_exclusive fails.

> Then reset_control_deassert can be called unconditionally:


Ok.

>

>         data->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);

>         if (IS_ERR(data->rst)) {

>                 dev_err(&pdev->dev, "invalid reset controller in device tree");

>                 ret = PTR_ERR(data->rst);

>                 goto reset_error;

>         }

>         reset_control_deassert(data->rst);

>

>> +

>>         model_data = of_device_get_match_data(&pdev->dev);

>>

>>         if (model_data->wait_init_sequence) {

>> @@ -268,6 +277,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)

>>

>>  scaler_error:

>>         clk_hw_unregister_divider(data->clk_prescaler);

>> +       reset_control_assert(data->rst);

>

> This should be done in reverse order, before

> clk_hw_unregister_divider, and get its own label.


Okay, fixed in v2.

Thanks for the review.

Cheers,

Joel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
index 674e133b7cd7..034fc2ba100e 100644
--- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
@@ -8,6 +8,7 @@  Required properties:
 - reg: memory window mapping address and length
 - clocks: Input clock used to derive the sample clock. Expected to be the
           SoC's APB clock.
+- resets: Reset controller phandle
 - #io-channel-cells: Must be set to <1> to indicate channels are selected
                      by index.
 
@@ -15,6 +16,7 @@  Example:
 	adc@1e6e9000 {
 		compatible = "aspeed,ast2400-adc";
 		reg = <0x1e6e9000 0xb0>;
-		clocks = <&clk_apb>;
+		clocks = <&syscon ASPEED_CLK_APB>;
+		resets = <&syscon ASPEED_RESET_ADC>;
 		#io-channel-cells = <1>;
 	};
diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
index 8a958d5f1905..0fc9712cdde4 100644
--- a/drivers/iio/adc/aspeed_adc.c
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -17,6 +17,7 @@ 
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
 
@@ -53,11 +54,12 @@  struct aspeed_adc_model_data {
 };
 
 struct aspeed_adc_data {
-	struct device	*dev;
-	void __iomem	*base;
-	spinlock_t	clk_lock;
-	struct clk_hw	*clk_prescaler;
-	struct clk_hw	*clk_scaler;
+	struct device		*dev;
+	void __iomem		*base;
+	spinlock_t		clk_lock;
+	struct clk_hw		*clk_prescaler;
+	struct clk_hw		*clk_scaler;
+	struct reset_control	*rst;
 };
 
 #define ASPEED_CHAN(_idx, _data_reg_addr) {			\
@@ -217,6 +219,13 @@  static int aspeed_adc_probe(struct platform_device *pdev)
 		goto scaler_error;
 	}
 
+	data->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
+	if (IS_ERR(data->rst)) {
+		dev_err(&pdev->dev, "invalid reset controller in device tree");
+		data->rst = NULL;
+	} else
+		reset_control_deassert(data->rst);
+
 	model_data = of_device_get_match_data(&pdev->dev);
 
 	if (model_data->wait_init_sequence) {
@@ -268,6 +277,7 @@  static int aspeed_adc_probe(struct platform_device *pdev)
 
 scaler_error:
 	clk_hw_unregister_divider(data->clk_prescaler);
+	reset_control_assert(data->rst);
 	return ret;
 }
 
@@ -282,6 +292,7 @@  static int aspeed_adc_remove(struct platform_device *pdev)
 	clk_disable_unprepare(data->clk_scaler->clk);
 	clk_hw_unregister_divider(data->clk_scaler);
 	clk_hw_unregister_divider(data->clk_prescaler);
+	reset_control_assert(data->rst);
 
 	return 0;
 }