Hi Joel,
On Mon, Oct 30, 2017 at 8:22 AM, Joel Stanley <joel@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> .../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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
