| Message ID | 20240609-ti-eqep-enable-clock-v1-1-1e9e7626467e@baylibre.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | counter: ti-eqep: enable clock at probe | expand |
Hi David, On 6/9/24 3:27 PM, David Lechner wrote: > The TI eQEP clock is both a functional and interface clock. Since it is > required for the device to function, we should be enabling it at probe. > > Up to now, we've just been lucky that the clock was enabled by something > else on the system already. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > drivers/counter/ti-eqep.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c > index 072b11fd6b32..825ae22c3ebc 100644 > --- a/drivers/counter/ti-eqep.c > +++ b/drivers/counter/ti-eqep.c > @@ -6,6 +6,7 @@ > */ > > #include <linux/bitops.h> > +#include <linux/clk.h> > #include <linux/counter.h> > #include <linux/kernel.h> > #include <linux/mod_devicetable.h> > @@ -376,6 +377,7 @@ static int ti_eqep_probe(struct platform_device *pdev) > struct counter_device *counter; > struct ti_eqep_cnt *priv; > void __iomem *base; > + struct clk *clk; > int err; > > counter = devm_counter_alloc(dev, sizeof(*priv)); > @@ -415,6 +417,10 @@ static int ti_eqep_probe(struct platform_device *pdev) > pm_runtime_enable(dev); > pm_runtime_get_sync(dev); > > + clk = devm_clk_get_enabled(dev, NULL); > + if (IS_ERR(clk)) I think it would be nice to have print here in case the we fail to get the clock. ~ Judith > + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable clock\n"); > + > err = counter_add(counter); > if (err < 0) { > pm_runtime_put_sync(dev); > > --- > base-commit: bb3f1c5fc434b0b177449f7f73ea9b112b397dd1 > change-id: 20240609-ti-eqep-enable-clock-91697095ca81
On 6/10/24 8:44 AM, Judith Mendez wrote: > Hi David, > > On 6/9/24 3:27 PM, David Lechner wrote: >> The TI eQEP clock is both a functional and interface clock. Since it is >> required for the device to function, we should be enabling it at probe. >> >> Up to now, we've just been lucky that the clock was enabled by something >> else on the system already. >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> >> --- >> drivers/counter/ti-eqep.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c >> index 072b11fd6b32..825ae22c3ebc 100644 >> --- a/drivers/counter/ti-eqep.c >> +++ b/drivers/counter/ti-eqep.c >> @@ -6,6 +6,7 @@ >> */ >> #include <linux/bitops.h> >> +#include <linux/clk.h> >> #include <linux/counter.h> >> #include <linux/kernel.h> >> #include <linux/mod_devicetable.h> >> @@ -376,6 +377,7 @@ static int ti_eqep_probe(struct platform_device *pdev) >> struct counter_device *counter; >> struct ti_eqep_cnt *priv; >> void __iomem *base; >> + struct clk *clk; >> int err; >> counter = devm_counter_alloc(dev, sizeof(*priv)); >> @@ -415,6 +417,10 @@ static int ti_eqep_probe(struct platform_device *pdev) >> pm_runtime_enable(dev); >> pm_runtime_get_sync(dev); >> + clk = devm_clk_get_enabled(dev, NULL); >> + if (IS_ERR(clk)) > > I think it would be nice to have print here in case the we fail to get > the clock. Do you mean that we should call devm_clk_get() and clk_prepare_enable() separately so that we get two different error messages? The dev_err_probe() below will already print a message on any error (other that EPROBE_DEFER). > > ~ Judith > >> + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable clock\n"); >> + >> err = counter_add(counter); >> if (err < 0) { >> pm_runtime_put_sync(dev); >> >> --- >> base-commit: bb3f1c5fc434b0b177449f7f73ea9b112b397dd1 >> change-id: 20240609-ti-eqep-enable-clock-91697095ca81 >
Hi David, On 6/10/24 8:53 AM, David Lechner wrote: > On 6/10/24 8:44 AM, Judith Mendez wrote: >> Hi David, >> >> On 6/9/24 3:27 PM, David Lechner wrote: >>> The TI eQEP clock is both a functional and interface clock. Since it is >>> required for the device to function, we should be enabling it at probe. >>> >>> Up to now, we've just been lucky that the clock was enabled by something >>> else on the system already. >>> >>> Signed-off-by: David Lechner <dlechner@baylibre.com> >>> --- >>> drivers/counter/ti-eqep.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c >>> index 072b11fd6b32..825ae22c3ebc 100644 >>> --- a/drivers/counter/ti-eqep.c >>> +++ b/drivers/counter/ti-eqep.c >>> @@ -6,6 +6,7 @@ >>> */ >>> #include <linux/bitops.h> >>> +#include <linux/clk.h> >>> #include <linux/counter.h> >>> #include <linux/kernel.h> >>> #include <linux/mod_devicetable.h> >>> @@ -376,6 +377,7 @@ static int ti_eqep_probe(struct platform_device *pdev) >>> struct counter_device *counter; >>> struct ti_eqep_cnt *priv; >>> void __iomem *base; >>> + struct clk *clk; >>> int err; >>> counter = devm_counter_alloc(dev, sizeof(*priv)); >>> @@ -415,6 +417,10 @@ static int ti_eqep_probe(struct platform_device *pdev) >>> pm_runtime_enable(dev); >>> pm_runtime_get_sync(dev); >>> + clk = devm_clk_get_enabled(dev, NULL); >>> + if (IS_ERR(clk)) >> >> I think it would be nice to have print here in case the we fail to get >> the clock. > > Do you mean that we should call devm_clk_get() and clk_prepare_enable() > separately so that we get two different error messages? > > The dev_err_probe() below will already print a message on any error > (other that EPROBE_DEFER). Apologies, I misread. The way you have it is good, thanks. Reviewed-by: Judith Mendez <jm@ti.com> > >> >> ~ Judith >> >>> + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable clock\n"); >>> + >>> err = counter_add(counter); >>> if (err < 0) { >>> pm_runtime_put_sync(dev); >>> >>> --- >>> base-commit: bb3f1c5fc434b0b177449f7f73ea9b112b397dd1 >>> change-id: 20240609-ti-eqep-enable-clock-91697095ca81 >> >
On Sun, Jun 09, 2024 at 03:27:11PM -0500, David Lechner wrote: > The TI eQEP clock is both a functional and interface clock. Since it is > required for the device to function, we should be enabling it at probe. > > Up to now, we've just been lucky that the clock was enabled by something > else on the system already. > > Signed-off-by: David Lechner <dlechner@baylibre.com> It sounds like a potential bug is being fixed here. Should this have a Fixes tag so we can get this merged in the stable trees too? William Breathitt Gray
On 6/16/24 4:15 AM, William Breathitt Gray wrote: > On Sun, Jun 09, 2024 at 03:27:11PM -0500, David Lechner wrote: >> The TI eQEP clock is both a functional and interface clock. Since it is >> required for the device to function, we should be enabling it at probe. >> >> Up to now, we've just been lucky that the clock was enabled by something >> else on the system already. >> >> Signed-off-by: David Lechner <dlechner@baylibre.com> > > It sounds like a potential bug is being fixed here. Should this have a > Fixes tag so we can get this merged in the stable trees too? > > William Breathitt Gray I suppose. Will respin.
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c index 072b11fd6b32..825ae22c3ebc 100644 --- a/drivers/counter/ti-eqep.c +++ b/drivers/counter/ti-eqep.c @@ -6,6 +6,7 @@ */ #include <linux/bitops.h> +#include <linux/clk.h> #include <linux/counter.h> #include <linux/kernel.h> #include <linux/mod_devicetable.h> @@ -376,6 +377,7 @@ static int ti_eqep_probe(struct platform_device *pdev) struct counter_device *counter; struct ti_eqep_cnt *priv; void __iomem *base; + struct clk *clk; int err; counter = devm_counter_alloc(dev, sizeof(*priv)); @@ -415,6 +417,10 @@ static int ti_eqep_probe(struct platform_device *pdev) pm_runtime_enable(dev); pm_runtime_get_sync(dev); + clk = devm_clk_get_enabled(dev, NULL); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), "failed to enable clock\n"); + err = counter_add(counter); if (err < 0) { pm_runtime_put_sync(dev);
The TI eQEP clock is both a functional and interface clock. Since it is required for the device to function, we should be enabling it at probe. Up to now, we've just been lucky that the clock was enabled by something else on the system already. Signed-off-by: David Lechner <dlechner@baylibre.com> --- drivers/counter/ti-eqep.c | 6 ++++++ 1 file changed, 6 insertions(+) --- base-commit: bb3f1c5fc434b0b177449f7f73ea9b112b397dd1 change-id: 20240609-ti-eqep-enable-clock-91697095ca81