diff mbox series

[2/6] pwm: sun4i: Add a quirk for reset line

Message ID 20190726184045.14669-3-jernej.skrabec@siol.net (mailing list archive)
State New, archived
Headers show
Series pwm: sun4i: Add support for Allwinner H6 | expand

Commit Message

Jernej Škrabec July 26, 2019, 6:40 p.m. UTC
H6 PWM core needs deasserted reset line in order to work.

Add a quirk for it.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/pwm/pwm-sun4i.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

Comments

Maxime Ripard July 27, 2019, 10:42 a.m. UTC | #1
On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> H6 PWM core needs deasserted reset line in order to work.
>
> Add a quirk for it.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Uwe Kleine-König July 29, 2019, 6:36 a.m. UTC | #2
Cc += reset framework maintainer

Hello Jernej,

On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> H6 PWM core needs deasserted reset line in order to work.
> 
> Add a quirk for it.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/pwm/pwm-sun4i.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index de78c824bbfd..1b7be8fbde86 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -16,6 +16,7 @@
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
> +#include <linux/reset.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/time.h>
> @@ -72,12 +73,14 @@ static const u32 prescaler_table[] = {
>  
>  struct sun4i_pwm_data {
>  	bool has_prescaler_bypass;
> +	bool has_reset;
>  	unsigned int npwm;
>  };
>  
>  struct sun4i_pwm_chip {
>  	struct pwm_chip chip;
>  	struct clk *clk;
> +	struct reset_control *rst;
>  	void __iomem *base;
>  	spinlock_t ctrl_lock;
>  	const struct sun4i_pwm_data *data;
> @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(pwm->clk))
>  		return PTR_ERR(pwm->clk);
>  
> +	if (pwm->data->has_reset) {
> +		pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
> +		if (IS_ERR(pwm->rst))
> +			return PTR_ERR(pwm->rst);
> +
> +		reset_control_deassert(pwm->rst);
> +	}
> +

I wonder why there is a need to track if a given chip needs a reset
line. I'd just use devm_reset_control_get_optional() and drop the
.has_reset member in struct sun4i_pwm_data.

>  	pwm->chip.dev = &pdev->dev;
>  	pwm->chip.ops = &sun4i_pwm_ops;
>  	pwm->chip.base = -1;
> @@ -383,19 +394,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
>  	ret = pwmchip_add(&pwm->chip);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
> -		return ret;
> +		goto err_pwm_add;
>  	}
>  
>  	platform_set_drvdata(pdev, pwm);
>  
>  	return 0;
> +
> +err_pwm_add:
> +	reset_control_assert(pwm->rst);
> +
> +	return ret;
>  }
>  
>  static int sun4i_pwm_remove(struct platform_device *pdev)
>  {
>  	struct sun4i_pwm_chip *pwm = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = pwmchip_remove(&pwm->chip);
> +	if (ret)
> +		return ret;
>  
> -	return pwmchip_remove(&pwm->chip);
> +	reset_control_assert(pwm->rst);
> +
> +	return 0;
>  }
>  
>  static struct platform_driver sun4i_pwm_driver = {
Chen-Yu Tsai July 29, 2019, 6:43 a.m. UTC | #3
On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Cc += reset framework maintainer
>
> Hello Jernej,
>
> On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> > H6 PWM core needs deasserted reset line in order to work.
> >
> > Add a quirk for it.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> >  drivers/pwm/pwm-sun4i.c | 27 +++++++++++++++++++++++++--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index de78c824bbfd..1b7be8fbde86 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pwm.h>
> > +#include <linux/reset.h>
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/time.h>
> > @@ -72,12 +73,14 @@ static const u32 prescaler_table[] = {
> >
> >  struct sun4i_pwm_data {
> >       bool has_prescaler_bypass;
> > +     bool has_reset;
> >       unsigned int npwm;
> >  };
> >
> >  struct sun4i_pwm_chip {
> >       struct pwm_chip chip;
> >       struct clk *clk;
> > +     struct reset_control *rst;
> >       void __iomem *base;
> >       spinlock_t ctrl_lock;
> >       const struct sun4i_pwm_data *data;
> > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> >       if (IS_ERR(pwm->clk))
> >               return PTR_ERR(pwm->clk);
> >
> > +     if (pwm->data->has_reset) {
> > +             pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
> > +             if (IS_ERR(pwm->rst))
> > +                     return PTR_ERR(pwm->rst);
> > +
> > +             reset_control_deassert(pwm->rst);
> > +     }
> > +
>
> I wonder why there is a need to track if a given chip needs a reset
> line. I'd just use devm_reset_control_get_optional() and drop the
> .has_reset member in struct sun4i_pwm_data.

Because it's not optional for this platform, i.e. it won't work if
the reset control (or clk, in the next patch) is somehow missing from
the device tree.

ChenYu

> >       pwm->chip.dev = &pdev->dev;
> >       pwm->chip.ops = &sun4i_pwm_ops;
> >       pwm->chip.base = -1;
> > @@ -383,19 +394,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> >       ret = pwmchip_add(&pwm->chip);
> >       if (ret < 0) {
> >               dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
> > -             return ret;
> > +             goto err_pwm_add;
> >       }
> >
> >       platform_set_drvdata(pdev, pwm);
> >
> >       return 0;
> > +
> > +err_pwm_add:
> > +     reset_control_assert(pwm->rst);
> > +
> > +     return ret;
> >  }
> >
> >  static int sun4i_pwm_remove(struct platform_device *pdev)
> >  {
> >       struct sun4i_pwm_chip *pwm = platform_get_drvdata(pdev);
> > +     int ret;
> > +
> > +     ret = pwmchip_remove(&pwm->chip);
> > +     if (ret)
> > +             return ret;
> >
> > -     return pwmchip_remove(&pwm->chip);
> > +     reset_control_assert(pwm->rst);
> > +
> > +     return 0;
> >  }
> >
> >  static struct platform_driver sun4i_pwm_driver = {
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Uwe Kleine-König July 29, 2019, 7:12 a.m. UTC | #4
Hello,

On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote:
> On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > >       if (IS_ERR(pwm->clk))
> > >               return PTR_ERR(pwm->clk);
> > >
> > > +     if (pwm->data->has_reset) {
> > > +             pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > +             if (IS_ERR(pwm->rst))
> > > +                     return PTR_ERR(pwm->rst);
> > > +
> > > +             reset_control_deassert(pwm->rst);
> > > +     }
> > > +
> >
> > I wonder why there is a need to track if a given chip needs a reset
> > line. I'd just use devm_reset_control_get_optional() and drop the
> > .has_reset member in struct sun4i_pwm_data.
> 
> Because it's not optional for this platform, i.e. it won't work if
> the reset control (or clk, in the next patch) is somehow missing from
> the device tree.

If the device tree is wrong it is considered ok that the driver doesn't
behave correctly. So this is not a problem you need (or should) care
about.

Best regards
Uwe
Philipp Zabel July 29, 2019, 10:18 a.m. UTC | #5
Hi,

On Mon, 2019-07-29 at 09:12 +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote:
> > On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> > > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > > >       if (IS_ERR(pwm->clk))
> > > >               return PTR_ERR(pwm->clk);
> > > > 
> > > > +     if (pwm->data->has_reset) {
> > > > +             pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > +             if (IS_ERR(pwm->rst))
> > > > +                     return PTR_ERR(pwm->rst);
> > > > +
> > > > +             reset_control_deassert(pwm->rst);
> > > > +     }
> > > > +
> > > 
> > > I wonder why there is a need to track if a given chip needs a reset
> > > line. I'd just use devm_reset_control_get_optional() and drop the
> > > .has_reset member in struct sun4i_pwm_data.
> > 
> > Because it's not optional for this platform, i.e. it won't work if
> > the reset control (or clk, in the next patch) is somehow missing from
> > the device tree.
> 
> If the device tree is wrong it is considered ok that the driver doesn't
> behave correctly. So this is not a problem you need (or should) care
> about.

I agree with this. Catching missing DT properties and other device tree
validation is not the job of device drivers. The _optional request
variants were introduced to simplify drivers that require the reset line
on some platforms and not on others.

I would ask to explicitly state whether the driver needs full control
over the moment of (de)assertion of the reset signal, or whether the
only requirement is that the reset signal stays deasserted while the PWM
driver is active, by using devm_reset_control_get_optional_exclusive or
devm_reset_control_get_optional_shared to request the reset control.

regards
Philipp
Maxime Ripard July 29, 2019, 4:37 p.m. UTC | #6
On Mon, Jul 29, 2019 at 09:12:18AM +0200, Uwe Kleine-König wrote:
> Hello,
>
> On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote:
> > On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> > > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > > >       if (IS_ERR(pwm->clk))
> > > >               return PTR_ERR(pwm->clk);
> > > >
> > > > +     if (pwm->data->has_reset) {
> > > > +             pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > +             if (IS_ERR(pwm->rst))
> > > > +                     return PTR_ERR(pwm->rst);
> > > > +
> > > > +             reset_control_deassert(pwm->rst);
> > > > +     }
> > > > +
> > >
> > > I wonder why there is a need to track if a given chip needs a reset
> > > line. I'd just use devm_reset_control_get_optional() and drop the
> > > .has_reset member in struct sun4i_pwm_data.
> >
> > Because it's not optional for this platform, i.e. it won't work if
> > the reset control (or clk, in the next patch) is somehow missing from
> > the device tree.
>
> If the device tree is wrong it is considered ok that the driver doesn't
> behave correctly. So this is not a problem you need (or should) care
> about.

To some extent that's true, but if we can make the life easier for
everyone by reporting an error and bailing out instead of silently
ignoring that, continuing to probe and just ending up with a
completely broken system for no apparent reason, then why not just do
that?

I mean, all it takes is three lines of code.

It's no different than just calling clk_get, and testing the return
code to see if it's there or not. I wouldn't call that check when you
depend on a clock "validating the DT". It's just making sure that all
the resources needed for you to operate properly are there, which is a
pretty common thing to do.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Uwe Kleine-König July 29, 2019, 6:20 p.m. UTC | #7
On Mon, Jul 29, 2019 at 06:37:15PM +0200, Maxime Ripard wrote:
> On Mon, Jul 29, 2019 at 09:12:18AM +0200, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote:
> > > On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> > > > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > > > >       if (IS_ERR(pwm->clk))
> > > > >               return PTR_ERR(pwm->clk);
> > > > >
> > > > > +     if (pwm->data->has_reset) {
> > > > > +             pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > > +             if (IS_ERR(pwm->rst))
> > > > > +                     return PTR_ERR(pwm->rst);
> > > > > +
> > > > > +             reset_control_deassert(pwm->rst);
> > > > > +     }
> > > > > +
> > > >
> > > > I wonder why there is a need to track if a given chip needs a reset
> > > > line. I'd just use devm_reset_control_get_optional() and drop the
> > > > .has_reset member in struct sun4i_pwm_data.
> > >
> > > Because it's not optional for this platform, i.e. it won't work if
> > > the reset control (or clk, in the next patch) is somehow missing from
> > > the device tree.
> >
> > If the device tree is wrong it is considered ok that the driver doesn't
> > behave correctly. So this is not a problem you need (or should) care
> > about.
> 
> To some extent that's true, but if we can make the life easier for
> everyone by reporting an error and bailing out instead of silently
> ignoring that, continuing to probe and just ending up with a
> completely broken system for no apparent reason, then why not just do
> that?
> 
> I mean, all it takes is three lines of code.

<pedantic>Actually it's more because you need to track for each variant
if it needs the clock (or reset stuff) or not.</pedantic>

It's a weighing between "simpler driver" and "catch unlikely errors".
(If the SoC's .dtsi includes the needed stuff, an error here is really
unlikely, isn't it?)

So to some degree it's subjective which one is better. I tend to prefer
"simpler driver". Also when catching this type of error you have to do
it right twice (in the .dtsi and the driver) while with my approach
there is only a single place that defines what is right, which is a good
design according to what I learned during my studies.

> It's no different than just calling clk_get, and testing the return
> code to see if it's there or not. I wouldn't call that check when you
> depend on a clock "validating the DT". It's just making sure that all
> the resources needed for you to operate properly are there, which is a
> pretty common thing to do.

This is different. As a driver author you are allowed (or even supposed)
to assume that the device tree (or ACPI or platform data ...) is right
and completely defines the stuff for the driven hardware to work
correctly. You must not assume that clk_get() succeeds unconditionally.

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index de78c824bbfd..1b7be8fbde86 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -16,6 +16,7 @@ 
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
+#include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/time.h>
@@ -72,12 +73,14 @@  static const u32 prescaler_table[] = {
 
 struct sun4i_pwm_data {
 	bool has_prescaler_bypass;
+	bool has_reset;
 	unsigned int npwm;
 };
 
 struct sun4i_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
+	struct reset_control *rst;
 	void __iomem *base;
 	spinlock_t ctrl_lock;
 	const struct sun4i_pwm_data *data;
@@ -371,6 +374,14 @@  static int sun4i_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(pwm->clk))
 		return PTR_ERR(pwm->clk);
 
+	if (pwm->data->has_reset) {
+		pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
+		if (IS_ERR(pwm->rst))
+			return PTR_ERR(pwm->rst);
+
+		reset_control_deassert(pwm->rst);
+	}
+
 	pwm->chip.dev = &pdev->dev;
 	pwm->chip.ops = &sun4i_pwm_ops;
 	pwm->chip.base = -1;
@@ -383,19 +394,31 @@  static int sun4i_pwm_probe(struct platform_device *pdev)
 	ret = pwmchip_add(&pwm->chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
-		return ret;
+		goto err_pwm_add;
 	}
 
 	platform_set_drvdata(pdev, pwm);
 
 	return 0;
+
+err_pwm_add:
+	reset_control_assert(pwm->rst);
+
+	return ret;
 }
 
 static int sun4i_pwm_remove(struct platform_device *pdev)
 {
 	struct sun4i_pwm_chip *pwm = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = pwmchip_remove(&pwm->chip);
+	if (ret)
+		return ret;
 
-	return pwmchip_remove(&pwm->chip);
+	reset_control_assert(pwm->rst);
+
+	return 0;
 }
 
 static struct platform_driver sun4i_pwm_driver = {