| Message ID | 20190726184045.14669-3-jernej.skrabec@siol.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: sun4i: Add support for Allwinner H6 | expand |
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
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 = {
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/ |
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
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
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
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 --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 = {
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(-)