* [PATCH 0/2] i2c: designware: Add support for a bus clock @ 2018-07-16 8:59 ` Phil Edworthy 0 siblings, 0 replies; 27+ messages in thread From: Phil Edworthy @ 2018-07-16 8:59 UTC (permalink / raw) Cc: Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Phil Edworthy The Synopsys I2C Controller has a bus clock, but typically SoCs hide this away. However, on some SoCs you need to explicity enable the bus clock in order to access the registers. Phil Edworthy (2): dt: snps,designware-i2c: Add clock bindings documentation i2c: designware: Add support for a bus clock Documentation/devicetree/bindings/i2c/i2c-designware.txt | 9 +++++++-- drivers/i2c/busses/i2c-designware-common.c | 14 +++++++++++++- drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++ 4 files changed, 23 insertions(+), 3 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 0/2] i2c: designware: Add support for a bus clock @ 2018-07-16 8:59 ` Phil Edworthy 0 siblings, 0 replies; 27+ messages in thread From: Phil Edworthy @ 2018-07-16 8:59 UTC (permalink / raw) Cc: Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Phil Edworthy The Synopsys I2C Controller has a bus clock, but typically SoCs hide this away. However, on some SoCs you need to explicity enable the bus clock in order to access the registers. Phil Edworthy (2): dt: snps,designware-i2c: Add clock bindings documentation i2c: designware: Add support for a bus clock Documentation/devicetree/bindings/i2c/i2c-designware.txt | 9 +++++++-- drivers/i2c/busses/i2c-designware-common.c | 14 +++++++++++++- drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++ 4 files changed, 23 insertions(+), 3 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/2] dt: snps,designware-i2c: Add clock bindings documentation 2018-07-16 8:59 ` Phil Edworthy (?) @ 2018-07-16 8:59 ` Phil Edworthy 2018-07-20 16:19 ` Rob Herring -1 siblings, 1 reply; 27+ messages in thread From: Phil Edworthy @ 2018-07-16 8:59 UTC (permalink / raw) To: Rob Herring, Mark Rutland Cc: Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Phil Edworthy, devicetree The driver requires an undocumented clock property, so detail it. Add documentation for a separate bus clock. Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- Documentation/devicetree/bindings/i2c/i2c-designware.txt | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt index fbb0a6d..e9f4387 100644 --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt @@ -1,16 +1,21 @@ * Synopsys DesignWare I2C Required properties : - - compatible : should be "snps,designware-i2c" - reg : Offset and length of the register set for the device - interrupts : <IRQ> where IRQ is the interrupt number. + - clocks : phandles for the clocks, see the description of clock-names below. + The phandle for the "ic_clk" clock is required. The phandle for the "bus" + clock is optional. If a single clock is specified but no clock-name, it is + the "ic_clk" clock. If both clocks are listed, the "ic_clk" must be first. Recommended properties : - - clock-frequency : desired I2C bus clock frequency in Hz. Optional properties : + - clock-names : Contains the names of the clocks: + "ic_clk", for the core clock used to generate the external I2C clock. + "bus", the bus clock, sometimes described as pclk, for register accesses. - i2c-sda-hold-time-ns : should contain the SDA hold time in nanoseconds. This option is only supported in hardware blocks version 1.11a or newer. -- 2.7.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 1/2] dt: snps,designware-i2c: Add clock bindings documentation 2018-07-16 8:59 ` [PATCH 1/2] dt: snps,designware-i2c: Add clock bindings documentation Phil Edworthy @ 2018-07-20 16:19 ` Rob Herring 0 siblings, 0 replies; 27+ messages in thread From: Rob Herring @ 2018-07-20 16:19 UTC (permalink / raw) To: Phil Edworthy Cc: Mark Rutland, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, devicetree On Mon, Jul 16, 2018 at 09:59:12AM +0100, Phil Edworthy wrote: > The driver requires an undocumented clock property, so detail it. > Add documentation for a separate bus clock. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > Documentation/devicetree/bindings/i2c/i2c-designware.txt | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-16 8:59 ` Phil Edworthy (?) (?) @ 2018-07-16 8:59 ` Phil Edworthy 2018-07-17 12:07 ` Simon Horman -1 siblings, 1 reply; 27+ messages in thread From: Phil Edworthy @ 2018-07-16 8:59 UTC (permalink / raw) To: Jarkko Nikula Cc: Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Phil Edworthy, Andy Shevchenko, Mika Westerberg The Synopsys I2C Controller has a bus clock, but typically SoCs hide this away. However, on some SoCs you need to explicity enable the bus clock in order to access the registers. Therefore, enable an optional bus clock specified by DT. Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- drivers/i2c/busses/i2c-designware-common.c | 14 +++++++++++++- drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index 48914df..4fa67d6 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -186,13 +186,25 @@ unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev) int i2c_dw_prepare_clk(struct dw_i2c_dev *dev, bool prepare) { + int ret; + if (IS_ERR(dev->clk)) return PTR_ERR(dev->clk); - if (prepare) + if (prepare) { + /* Optional bus clock */ + if (!IS_ERR(dev->busclk)) { + ret = clk_prepare_enable(dev->busclk); + if (ret) + return ret; + } + return clk_prepare_enable(dev->clk); + } clk_disable_unprepare(dev->clk); + clk_disable_unprepare(dev->busclk); + return 0; } EXPORT_SYMBOL_GPL(i2c_dw_prepare_clk); diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index d690e64..10f905d 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -239,6 +239,7 @@ struct dw_i2c_dev { void __iomem *base; struct completion cmd_complete; struct clk *clk; + struct clk *busclk; struct reset_control *rst; struct i2c_client *slave; u32 (*get_clk_rate_khz) (struct dw_i2c_dev *dev); diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 5660daf..64389fe 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -332,6 +332,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) else i2c_dw_configure_master(dev); + /* Optional bus clock */ + dev->busclk = devm_clk_get(&pdev->dev, "bus"); dev->clk = devm_clk_get(&pdev->dev, NULL); if (!i2c_dw_prepare_clk(dev, true)) { dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz; -- 2.7.4 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-16 8:59 ` [PATCH 2/2] i2c: designware: Add support for a bus clock Phil Edworthy @ 2018-07-17 12:07 ` Simon Horman 2018-07-17 12:23 ` Andy Shevchenko 0 siblings, 1 reply; 27+ messages in thread From: Simon Horman @ 2018-07-17 12:07 UTC (permalink / raw) To: Phil Edworthy Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Andy Shevchenko, Mika Westerberg On Mon, Jul 16, 2018 at 09:59:13AM +0100, Phil Edworthy wrote: > The Synopsys I2C Controller has a bus clock, but typically SoCs hide this away. > However, on some SoCs you need to explicity enable the bus clock in order to > access the registers. > Therefore, enable an optional bus clock specified by DT. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > --- > drivers/i2c/busses/i2c-designware-common.c | 14 +++++++++++++- > drivers/i2c/busses/i2c-designware-core.h | 1 + > drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++ > 3 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c > index 48914df..4fa67d6 100644 > --- a/drivers/i2c/busses/i2c-designware-common.c > +++ b/drivers/i2c/busses/i2c-designware-common.c > @@ -186,13 +186,25 @@ unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev) > > int i2c_dw_prepare_clk(struct dw_i2c_dev *dev, bool prepare) > { > + int ret; > + > if (IS_ERR(dev->clk)) > return PTR_ERR(dev->clk); > > - if (prepare) > + if (prepare) { > + /* Optional bus clock */ > + if (!IS_ERR(dev->busclk)) { I suspect that error values stored in dev->busclk, other than -ENOENT, should be treated as errors. > + ret = clk_prepare_enable(dev->busclk); > + if (ret) > + return ret; > + } > + > return clk_prepare_enable(dev->clk); > + } > > clk_disable_unprepare(dev->clk); > + clk_disable_unprepare(dev->busclk); > + > return 0; > } > EXPORT_SYMBOL_GPL(i2c_dw_prepare_clk); > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h > index d690e64..10f905d 100644 > --- a/drivers/i2c/busses/i2c-designware-core.h > +++ b/drivers/i2c/busses/i2c-designware-core.h > @@ -239,6 +239,7 @@ struct dw_i2c_dev { > void __iomem *base; > struct completion cmd_complete; > struct clk *clk; > + struct clk *busclk; > struct reset_control *rst; > struct i2c_client *slave; > u32 (*get_clk_rate_khz) (struct dw_i2c_dev *dev); > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index 5660daf..64389fe 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -332,6 +332,8 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) > else > i2c_dw_configure_master(dev); > > + /* Optional bus clock */ > + dev->busclk = devm_clk_get(&pdev->dev, "bus"); > dev->clk = devm_clk_get(&pdev->dev, NULL); > if (!i2c_dw_prepare_clk(dev, true)) { > dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-17 12:07 ` Simon Horman @ 2018-07-17 12:23 ` Andy Shevchenko 2018-07-17 12:42 ` Phil Edworthy 0 siblings, 1 reply; 27+ messages in thread From: Andy Shevchenko @ 2018-07-17 12:23 UTC (permalink / raw) To: Simon Horman, Phil Edworthy Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg On Tue, 2018-07-17 at 14:07 +0200, Simon Horman wrote: > On Mon, Jul 16, 2018 at 09:59:13AM +0100, Phil Edworthy wrote: > > The Synopsys I2C Controller has a bus clock, but typically SoCs hide > > this away. > > However, on some SoCs you need to explicity enable the bus clock in > > order to > > access the registers. > > Therefore, enable an optional bus clock specified by DT. > > + /* Optional bus clock */ > > + if (!IS_ERR(dev->busclk)) { > > I suspect that error values stored in dev->busclk, other than > -ENOENT, > should be treated as errors. While your point sounds valid (don't remember how clk_get() is implemented), NULL is also OK to have. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-17 12:23 ` Andy Shevchenko @ 2018-07-17 12:42 ` Phil Edworthy 0 siblings, 0 replies; 27+ messages in thread From: Phil Edworthy @ 2018-07-17 12:42 UTC (permalink / raw) To: Andy Shevchenko, Simon Horman Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg Hi Simon, Andy, On 17 July 2018 13:24, Andy Shevchenko wrote: > On Tue, 2018-07-17 at 14:07 +0200, Simon Horman wrote: > > On Mon, Jul 16, 2018 at 09:59:13AM +0100, Phil Edworthy wrote: > > > The Synopsys I2C Controller has a bus clock, but typically SoCs hide > > > this away. > > > However, on some SoCs you need to explicity enable the bus clock in > > > order to access the registers. > > > Therefore, enable an optional bus clock specified by DT. > > > > + /* Optional bus clock */ > > > + if (!IS_ERR(dev->busclk)) { > > > > I suspect that error values stored in dev->busclk, other than > > -ENOENT, should be treated as errors. IS_ERR catches all errors and is the correct way to check the value returned by devm_clk_get. > While your point sounds valid (don't remember how clk_get() is > implemented), NULL is also OK to have. Ok as in there is no bus clock, right? So it should be: if (!IS_ERR_OR_NULL (dev->busclk)) { Thanks Phil ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] i2c: designware: Add support for a bus clock @ 2018-07-17 12:42 ` Phil Edworthy 0 siblings, 0 replies; 27+ messages in thread From: Phil Edworthy @ 2018-07-17 12:42 UTC (permalink / raw) To: Andy Shevchenko, Simon Horman Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg Hi Simon, Andy, On 17 July 2018 13:24, Andy Shevchenko wrote: > On Tue, 2018-07-17 at 14:07 +0200, Simon Horman wrote: > > On Mon, Jul 16, 2018 at 09:59:13AM +0100, Phil Edworthy wrote: > > > The Synopsys I2C Controller has a bus clock, but typically SoCs hide > > > this away. > > > However, on some SoCs you need to explicity enable the bus clock in > > > order to access the registers. > > > Therefore, enable an optional bus clock specified by DT. > > > > + /* Optional bus clock */ > > > + if (!IS_ERR(dev->busclk)) { > > > > I suspect that error values stored in dev->busclk, other than > > -ENOENT, should be treated as errors. IS_ERR catches all errors and is the correct way to check the value returned by devm_clk_get. > While your point sounds valid (don't remember how clk_get() is > implemented), NULL is also OK to have. Ok as in there is no bus clock, right? So it should be: if (!IS_ERR_OR_NULL (dev->busclk)) { Thanks Phil ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-17 12:42 ` Phil Edworthy (?) @ 2018-07-17 13:01 ` Geert Uytterhoeven 2018-07-17 13:12 ` Phil Edworthy -1 siblings, 1 reply; 27+ messages in thread From: Geert Uytterhoeven @ 2018-07-17 13:01 UTC (permalink / raw) To: Phil Edworthy Cc: Andy Shevchenko, Simon Horman, Jarkko Nikula, Linux I2C, Linux Kernel Mailing List, Linux-Renesas, Mika Westerberg Hi Phil, On Tue, Jul 17, 2018 at 2:42 PM Phil Edworthy <phil.edworthy@renesas.com> wrote: > On 17 July 2018 13:24, Andy Shevchenko wrote: > > On Tue, 2018-07-17 at 14:07 +0200, Simon Horman wrote: > > > On Mon, Jul 16, 2018 at 09:59:13AM +0100, Phil Edworthy wrote: > > > > The Synopsys I2C Controller has a bus clock, but typically SoCs hide > > > > this away. > > > > However, on some SoCs you need to explicity enable the bus clock in > > > > order to access the registers. > > > > Therefore, enable an optional bus clock specified by DT. > > > > > > + /* Optional bus clock */ > > > > + if (!IS_ERR(dev->busclk)) { > > > > > > I suspect that error values stored in dev->busclk, other than > > > -ENOENT, should be treated as errors. > IS_ERR catches all errors and is the correct way to check the value > returned by devm_clk_get. What if it returns -EPROBE_DEFER, which means the clock is referenced, and thus not optional, but not yet ready? For optional clocks, all errors but -ENOENT should be propagated up. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-17 13:01 ` Geert Uytterhoeven @ 2018-07-17 13:12 ` Phil Edworthy 0 siblings, 0 replies; 27+ messages in thread From: Phil Edworthy @ 2018-07-17 13:12 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Andy Shevchenko, Simon Horman, Jarkko Nikula, Linux I2C, Linux Kernel Mailing List, Linux-Renesas, Mika Westerberg Hi Geert, On 17 July 2018 14:02, Geert Uytterhoeven wrote: > On Tue, Jul 17, 2018 at 2:42 PM Phil Edworthy wrote: > > On 17 July 2018 13:24, Andy Shevchenko wrote: > > > On Tue, 2018-07-17 at 14:07 +0200, Simon Horman wrote: > > > > On Mon, Jul 16, 2018 at 09:59:13AM +0100, Phil Edworthy wrote: > > > > > The Synopsys I2C Controller has a bus clock, but typically SoCs > > > > > hide this away. > > > > > However, on some SoCs you need to explicity enable the bus clock > > > > > in order to access the registers. > > > > > Therefore, enable an optional bus clock specified by DT. > > > > > > > > + /* Optional bus clock */ > > > > > + if (!IS_ERR(dev->busclk)) { > > > > > > > > I suspect that error values stored in dev->busclk, other than > > > > -ENOENT, should be treated as errors. > > IS_ERR catches all errors and is the correct way to check the value > > returned by devm_clk_get. > > What if it returns -EPROBE_DEFER, which means the clock is referenced, and > thus not optional, but not yet ready? > For optional clocks, all errors but -ENOENT should be propagated up. Good point! Thanks Phil ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-17 12:42 ` Phil Edworthy @ 2018-07-17 14:18 ` Andy Shevchenko -1 siblings, 0 replies; 27+ messages in thread From: Andy Shevchenko @ 2018-07-17 14:18 UTC (permalink / raw) To: Phil Edworthy, Simon Horman Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > While your point sounds valid (don't remember how clk_get() is > > implemented), NULL is also OK to have. > > Ok as in there is no bus clock, right? > So it should be: > if (!IS_ERR_OR_NULL (dev->busclk)) Nope, NULL is no error case for optional clock. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock @ 2018-07-17 14:18 ` Andy Shevchenko 0 siblings, 0 replies; 27+ messages in thread From: Andy Shevchenko @ 2018-07-17 14:18 UTC (permalink / raw) To: Phil Edworthy, Simon Horman Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > While your point sounds valid (don't remember how clk_get() is > > implemented), NULL is also OK to have. > > Ok as in there is no bus clock, right? > So it should be: > if (!IS_ERR_OR_NULL (dev->busclk)) Nope, NULL is no error case for optional clock. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-17 14:18 ` Andy Shevchenko @ 2018-07-17 14:40 ` Phil Edworthy -1 siblings, 0 replies; 27+ messages in thread From: Phil Edworthy @ 2018-07-17 14:40 UTC (permalink / raw) To: Andy Shevchenko, Simon Horman Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg Hi Andy, On 17 July 2018 15:19, Andy Shevchenko wrote: > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > While your point sounds valid (don't remember how clk_get() is > > > implemented), NULL is also OK to have. > > > > Ok as in there is no bus clock, right? > > So it should be: > > if (!IS_ERR_OR_NULL (dev->busclk)) > > Nope, NULL is no error case for optional clock. I must be missing something here... I agree that NULL for an optional clock is not an error. However, the code above is now: + if (prepare) { + /* Optional bus clock */ + if (!IS_ERR_OR_NULL(dev->busclk)) { + ret = clk_prepare_enable(dev->busclk); + if (ret) + return ret; + } + return clk_prepare_enable(dev->clk); + } So, if you have a valid busclk, it gets enabled, otherwise it is left alone. Thanks Phil ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] i2c: designware: Add support for a bus clock @ 2018-07-17 14:40 ` Phil Edworthy 0 siblings, 0 replies; 27+ messages in thread From: Phil Edworthy @ 2018-07-17 14:40 UTC (permalink / raw) To: Andy Shevchenko, Simon Horman Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg Hi Andy, On 17 July 2018 15:19, Andy Shevchenko wrote: > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > While your point sounds valid (don't remember how clk_get() is > > > implemented), NULL is also OK to have. > > > > Ok as in there is no bus clock, right? > > So it should be: > > if (!IS_ERR_OR_NULL (dev->busclk)) > > Nope, NULL is no error case for optional clock. I must be missing something here... I agree that NULL for an optional clock is not an error. However, the code above is now: + if (prepare) { + /* Optional bus clock */ + if (!IS_ERR_OR_NULL(dev->busclk)) { + ret = clk_prepare_enable(dev->busclk); + if (ret) + return ret; + } + return clk_prepare_enable(dev->clk); + } So, if you have a valid busclk, it gets enabled, otherwise it is left alone. Thanks Phil ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-17 14:40 ` Phil Edworthy @ 2018-07-17 14:47 ` Andy Shevchenko -1 siblings, 0 replies; 27+ messages in thread From: Andy Shevchenko @ 2018-07-17 14:47 UTC (permalink / raw) To: Phil Edworthy, Simon Horman Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg On Tue, 2018-07-17 at 14:40 +0000, Phil Edworthy wrote: > Hi Andy, > > On 17 July 2018 15:19, Andy Shevchenko wrote: > > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > > > While your point sounds valid (don't remember how clk_get() is > > > > implemented), NULL is also OK to have. > > > > > > Ok as in there is no bus clock, right? > > > So it should be: > > > if (!IS_ERR_OR_NULL (dev->busclk)) > > > > Nope, NULL is no error case for optional clock. > > I must be missing something here... See how clk_prepare_enable() is implemented. > I agree that NULL for an optional clock is not an error. However, the > code above is now: > + if (prepare) { > + /* Optional bus clock */ > + if (!IS_ERR_OR_NULL(dev->busclk)) { Check for NULL is redundant. > + ret = clk_prepare_enable(dev->busclk); > + if (ret) > + return ret; > + } > + > return clk_prepare_enable(dev->clk); > + } > > So, if you have a valid busclk, it gets enabled, otherwise it is > left alone. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock @ 2018-07-17 14:47 ` Andy Shevchenko 0 siblings, 0 replies; 27+ messages in thread From: Andy Shevchenko @ 2018-07-17 14:47 UTC (permalink / raw) To: Phil Edworthy, Simon Horman Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg On Tue, 2018-07-17 at 14:40 +0000, Phil Edworthy wrote: > Hi Andy, > > On 17 July 2018 15:19, Andy Shevchenko wrote: > > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > > > While your point sounds valid (don't remember how clk_get() is > > > > implemented), NULL is also OK to have. > > > > > > Ok as in there is no bus clock, right? > > > So it should be: > > > if (!IS_ERR_OR_NULL (dev->busclk)) > > > > Nope, NULL is no error case for optional clock. > > I must be missing something here... See how clk_prepare_enable() is implemented. > I agree that NULL for an optional clock is not an error. However, the > code above is now: > + if (prepare) { > + /* Optional bus clock */ > + if (!IS_ERR_OR_NULL(dev->busclk)) { Check for NULL is redundant. > + ret = clk_prepare_enable(dev->busclk); > + if (ret) > + return ret; > + } > + > return clk_prepare_enable(dev->clk); > + } > > So, if you have a valid busclk, it gets enabled, otherwise it is > left alone. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-17 14:47 ` Andy Shevchenko @ 2018-07-17 14:57 ` Phil Edworthy -1 siblings, 0 replies; 27+ messages in thread From: Phil Edworthy @ 2018-07-17 14:57 UTC (permalink / raw) To: Andy Shevchenko, Simon Horman Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg Hi Andy, On 17 July 2018 15:47, Andy Shevchenko wrote: > On Tue, 2018-07-17 at 14:40 +0000, Phil Edworthy wrote: > > On 17 July 2018 15:19, Andy Shevchenko wrote: > > > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > > > > > While your point sounds valid (don't remember how clk_get() is > > > > > implemented), NULL is also OK to have. > > > > > > > > Ok as in there is no bus clock, right? > > > > So it should be: > > > > if (!IS_ERR_OR_NULL (dev->busclk)) > > > > > > Nope, NULL is no error case for optional clock. > > > > I must be missing something here... > > See how clk_prepare_enable() is implemented. Ok, if busclk is NULL the code can safely call clk_prepare_enable() > > I agree that NULL for an optional clock is not an error. However, the > > code above is now: > > + if (prepare) { > > + /* Optional bus clock */ > > > + if (!IS_ERR_OR_NULL(dev->busclk)) { > > Check for NULL is redundant. > > > + ret = clk_prepare_enable(dev->busclk); > > + if (ret) > > + return ret; > > + } > > + > > return clk_prepare_enable(dev->clk); > > + } > > > > So, if you have a valid busclk, it gets enabled, otherwise it is left > > alone. > So the code as sent in the original email is correct (aside from Geert's comments about EPROBE_DEFER handling). Maybe I need some coffee :\ Thanks Phil ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] i2c: designware: Add support for a bus clock @ 2018-07-17 14:57 ` Phil Edworthy 0 siblings, 0 replies; 27+ messages in thread From: Phil Edworthy @ 2018-07-17 14:57 UTC (permalink / raw) To: Andy Shevchenko, Simon Horman Cc: Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg Hi Andy, On 17 July 2018 15:47, Andy Shevchenko wrote: > On Tue, 2018-07-17 at 14:40 +0000, Phil Edworthy wrote: > > On 17 July 2018 15:19, Andy Shevchenko wrote: > > > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > > > > > While your point sounds valid (don't remember how clk_get() is > > > > > implemented), NULL is also OK to have. > > > > > > > > Ok as in there is no bus clock, right? > > > > So it should be: > > > > if (!IS_ERR_OR_NULL (dev->busclk)) > > > > > > Nope, NULL is no error case for optional clock. > > > > I must be missing something here... > > See how clk_prepare_enable() is implemented. Ok, if busclk is NULL the code can safely call clk_prepare_enable() > > I agree that NULL for an optional clock is not an error. However, the > > code above is now: > > + if (prepare) { > > + /* Optional bus clock */ > > > + if (!IS_ERR_OR_NULL(dev->busclk)) { > > Check for NULL is redundant. > > > + ret = clk_prepare_enable(dev->busclk); > > + if (ret) > > + return ret; > > + } > > + > > return clk_prepare_enable(dev->clk); > > + } > > > > So, if you have a valid busclk, it gets enabled, otherwise it is left > > alone. > So the code as sent in the original email is correct (aside from Geert's comments about EPROBE_DEFER handling). Maybe I need some coffee :\ Thanks Phil ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-17 14:57 ` Phil Edworthy @ 2018-07-18 9:14 ` Simon Horman -1 siblings, 0 replies; 27+ messages in thread From: Simon Horman @ 2018-07-18 9:14 UTC (permalink / raw) To: Phil Edworthy Cc: Andy Shevchenko, Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg On Tue, Jul 17, 2018 at 02:57:27PM +0000, Phil Edworthy wrote: > Hi Andy, > > On 17 July 2018 15:47, Andy Shevchenko wrote: > > On Tue, 2018-07-17 at 14:40 +0000, Phil Edworthy wrote: > > > On 17 July 2018 15:19, Andy Shevchenko wrote: > > > > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > > > > > > > While your point sounds valid (don't remember how clk_get() is > > > > > > implemented), NULL is also OK to have. > > > > > > > > > > Ok as in there is no bus clock, right? > > > > > So it should be: > > > > > if (!IS_ERR_OR_NULL (dev->busclk)) > > > > > > > > Nope, NULL is no error case for optional clock. > > > > > > I must be missing something here... > > > > See how clk_prepare_enable() is implemented. > Ok, if busclk is NULL the code can safely call clk_prepare_enable() > > > > I agree that NULL for an optional clock is not an error. However, the > > > code above is now: > > > + if (prepare) { > > > + /* Optional bus clock */ > > > > > + if (!IS_ERR_OR_NULL(dev->busclk)) { > > > > Check for NULL is redundant. > > > > > + ret = clk_prepare_enable(dev->busclk); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > return clk_prepare_enable(dev->clk); > > > + } > > > > > > So, if you have a valid busclk, it gets enabled, otherwise it is left > > > alone. > > > So the code as sent in the original email is correct (aside from Geert's > comments about EPROBE_DEFER handling). > > Maybe I need some coffee :\ > Thanks > Phil My point is that errors should be treated as errors. In i2c_dw_prepare_clk() the following appears: if (IS_ERR(dev->clk)) return PTR_ERR(dev->clk); So dev->clk being an error value is treated as an error that is passed up to the caller. But in your patch (and the snippet below) dev->busclk is treated as the optional clock not being present. Even if the error stored nothing to do with the clock not being present - f.e. ENOMEM or as Geert mentioned elsewhere, EPROBE_DEFER. Assuming the absense of the optional clock is indicated by ENOENT, in my view correct code would include something like: ... if (IS_ERR(dev->clk)) return PTR_ERR(dev->clk); if (IS_ERR(dev->buslck) && PTR_ERR(dev->busclk) != -ENOENT) return PTR_ERR(dev->busclk); ... ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock @ 2018-07-18 9:14 ` Simon Horman 0 siblings, 0 replies; 27+ messages in thread From: Simon Horman @ 2018-07-18 9:14 UTC (permalink / raw) To: Phil Edworthy Cc: Andy Shevchenko, Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg On Tue, Jul 17, 2018 at 02:57:27PM +0000, Phil Edworthy wrote: > Hi Andy, > > On 17 July 2018 15:47, Andy Shevchenko wrote: > > On Tue, 2018-07-17 at 14:40 +0000, Phil Edworthy wrote: > > > On 17 July 2018 15:19, Andy Shevchenko wrote: > > > > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > > > > > > > While your point sounds valid (don't remember how clk_get() is > > > > > > implemented), NULL is also OK to have. > > > > > > > > > > Ok as in there is no bus clock, right? > > > > > So it should be: > > > > > if (!IS_ERR_OR_NULL (dev->busclk)) > > > > > > > > Nope, NULL is no error case for optional clock. > > > > > > I must be missing something here... > > > > See how clk_prepare_enable() is implemented. > Ok, if busclk is NULL the code can safely call clk_prepare_enable() > > > > I agree that NULL for an optional clock is not an error. However, the > > > code above is now: > > > + if (prepare) { > > > + /* Optional bus clock */ > > > > > + if (!IS_ERR_OR_NULL(dev->busclk)) { > > > > Check for NULL is redundant. > > > > > + ret = clk_prepare_enable(dev->busclk); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > return clk_prepare_enable(dev->clk); > > > + } > > > > > > So, if you have a valid busclk, it gets enabled, otherwise it is left > > > alone. > > > So the code as sent in the original email is correct (aside from Geert's > comments about EPROBE_DEFER handling). > > Maybe I need some coffee :\ > Thanks > Phil My point is that errors should be treated as errors. In i2c_dw_prepare_clk() the following appears: if (IS_ERR(dev->clk)) return PTR_ERR(dev->clk); So dev->clk being an error value is treated as an error that is passed up to the caller. But in your patch (and the snippet below) dev->busclk is treated as the optional clock not being present. Even if the error stored nothing to do with the clock not being present - f.e. ENOMEM or as Geert mentioned elsewhere, EPROBE_DEFER. Assuming the absense of the optional clock is indicated by ENOENT, in my view correct code would include something like: ... if (IS_ERR(dev->clk)) return PTR_ERR(dev->clk); if (IS_ERR(dev->buslck) && PTR_ERR(dev->busclk) != -ENOENT) return PTR_ERR(dev->busclk); ... ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-18 9:14 ` Simon Horman @ 2018-07-18 9:21 ` Phil Edworthy -1 siblings, 0 replies; 27+ messages in thread From: Phil Edworthy @ 2018-07-18 9:21 UTC (permalink / raw) To: Simon Horman Cc: Andy Shevchenko, Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg Hi Simon, On 18 July 2018 10:15 Simon Horman wrote: > On Tue, Jul 17, 2018 at 02:57:27PM +0000, Phil Edworthy wrote: > > On 17 July 2018 15:47, Andy Shevchenko wrote: > > > On Tue, 2018-07-17 at 14:40 +0000, Phil Edworthy wrote: > > > > On 17 July 2018 15:19, Andy Shevchenko wrote: > > > > > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > > > > > > > > > While your point sounds valid (don't remember how clk_get() > > > > > > > is implemented), NULL is also OK to have. > > > > > > > > > > > > Ok as in there is no bus clock, right? > > > > > > So it should be: > > > > > > if (!IS_ERR_OR_NULL (dev->busclk)) > > > > > > > > > > Nope, NULL is no error case for optional clock. > > > > > > > > I must be missing something here... > > > > > > See how clk_prepare_enable() is implemented. > > Ok, if busclk is NULL the code can safely call clk_prepare_enable() > > > > > > I agree that NULL for an optional clock is not an error. However, > > > > the code above is now: > > > > + if (prepare) { > > > > + /* Optional bus clock */ > > > > > > > + if (!IS_ERR_OR_NULL(dev->busclk)) { > > > > > > Check for NULL is redundant. > > > > > > > + ret = clk_prepare_enable(dev->busclk); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > return clk_prepare_enable(dev->clk); > > > > + } > > > > > > > > So, if you have a valid busclk, it gets enabled, otherwise it is > > > > left alone. > > > > > So the code as sent in the original email is correct (aside from > > Geert's comments about EPROBE_DEFER handling). > > > > Maybe I need some coffee :\ > > Thanks > > Phil > > My point is that errors should be treated as errors. > > In i2c_dw_prepare_clk() the following appears: > > if (IS_ERR(dev->clk)) > return PTR_ERR(dev->clk); > > So dev->clk being an error value is treated as an error that is passed up to the > caller. > > But in your patch (and the snippet below) dev->busclk is treated as the > optional clock not being present. Even if the error stored nothing to do with > the clock not being present - f.e. ENOMEM or as Geert mentioned > elsewhere, EPROBE_DEFER. > > Assuming the absense of the optional clock is indicated by ENOENT, in my > view correct code would include something like: > > ... > > if (IS_ERR(dev->clk)) > return PTR_ERR(dev->clk); > > if (IS_ERR(dev->buslck) && PTR_ERR(dev->busclk) != -ENOENT) > return PTR_ERR(dev->busclk); > > ... Yes, I completely agree! Thanks Phil ^ permalink raw reply [flat|nested] 27+ messages in thread
* RE: [PATCH 2/2] i2c: designware: Add support for a bus clock @ 2018-07-18 9:21 ` Phil Edworthy 0 siblings, 0 replies; 27+ messages in thread From: Phil Edworthy @ 2018-07-18 9:21 UTC (permalink / raw) To: Simon Horman Cc: Andy Shevchenko, Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg Hi Simon, On 18 July 2018 10:15 Simon Horman wrote: > On Tue, Jul 17, 2018 at 02:57:27PM +0000, Phil Edworthy wrote: > > On 17 July 2018 15:47, Andy Shevchenko wrote: > > > On Tue, 2018-07-17 at 14:40 +0000, Phil Edworthy wrote: > > > > On 17 July 2018 15:19, Andy Shevchenko wrote: > > > > > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > > > > > > > > > While your point sounds valid (don't remember how clk_get() > > > > > > > is implemented), NULL is also OK to have. > > > > > > > > > > > > Ok as in there is no bus clock, right? > > > > > > So it should be: > > > > > > if (!IS_ERR_OR_NULL (dev->busclk)) > > > > > > > > > > Nope, NULL is no error case for optional clock. > > > > > > > > I must be missing something here... > > > > > > See how clk_prepare_enable() is implemented. > > Ok, if busclk is NULL the code can safely call clk_prepare_enable() > > > > > > I agree that NULL for an optional clock is not an error. However, > > > > the code above is now: > > > > + if (prepare) { > > > > + /* Optional bus clock */ > > > > > > > + if (!IS_ERR_OR_NULL(dev->busclk)) { > > > > > > Check for NULL is redundant. > > > > > > > + ret = clk_prepare_enable(dev->busclk); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > return clk_prepare_enable(dev->clk); > > > > + } > > > > > > > > So, if you have a valid busclk, it gets enabled, otherwise it is > > > > left alone. > > > > > So the code as sent in the original email is correct (aside from > > Geert's comments about EPROBE_DEFER handling). > > > > Maybe I need some coffee :\ > > Thanks > > Phil > > My point is that errors should be treated as errors. > > In i2c_dw_prepare_clk() the following appears: > > if (IS_ERR(dev->clk)) > return PTR_ERR(dev->clk); > > So dev->clk being an error value is treated as an error that is passed up to the > caller. > > But in your patch (and the snippet below) dev->busclk is treated as the > optional clock not being present. Even if the error stored nothing to do with > the clock not being present - f.e. ENOMEM or as Geert mentioned > elsewhere, EPROBE_DEFER. > > Assuming the absense of the optional clock is indicated by ENOENT, in my > view correct code would include something like: > > ... > > if (IS_ERR(dev->clk)) > return PTR_ERR(dev->clk); > > if (IS_ERR(dev->buslck) && PTR_ERR(dev->busclk) != -ENOENT) > return PTR_ERR(dev->busclk); > > ... Yes, I completely agree! Thanks Phil ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-18 9:21 ` Phil Edworthy @ 2018-07-19 7:42 ` Simon Horman -1 siblings, 0 replies; 27+ messages in thread From: Simon Horman @ 2018-07-19 7:42 UTC (permalink / raw) To: Phil Edworthy Cc: Andy Shevchenko, Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg On Wed, Jul 18, 2018 at 09:21:22AM +0000, Phil Edworthy wrote: > Hi Simon, > > On 18 July 2018 10:15 Simon Horman wrote: > > On Tue, Jul 17, 2018 at 02:57:27PM +0000, Phil Edworthy wrote: > > > On 17 July 2018 15:47, Andy Shevchenko wrote: > > > > On Tue, 2018-07-17 at 14:40 +0000, Phil Edworthy wrote: > > > > > On 17 July 2018 15:19, Andy Shevchenko wrote: > > > > > > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > > > > > > > > > > > While your point sounds valid (don't remember how clk_get() > > > > > > > > is implemented), NULL is also OK to have. > > > > > > > > > > > > > > Ok as in there is no bus clock, right? > > > > > > > So it should be: > > > > > > > if (!IS_ERR_OR_NULL (dev->busclk)) > > > > > > > > > > > > Nope, NULL is no error case for optional clock. > > > > > > > > > > I must be missing something here... > > > > > > > > See how clk_prepare_enable() is implemented. > > > Ok, if busclk is NULL the code can safely call clk_prepare_enable() > > > > > > > > I agree that NULL for an optional clock is not an error. However, > > > > > the code above is now: > > > > > + if (prepare) { > > > > > + /* Optional bus clock */ > > > > > > > > > + if (!IS_ERR_OR_NULL(dev->busclk)) { > > > > > > > > Check for NULL is redundant. > > > > > > > > > + ret = clk_prepare_enable(dev->busclk); > > > > > + if (ret) > > > > > + return ret; > > > > > + } > > > > > + > > > > > return clk_prepare_enable(dev->clk); > > > > > + } > > > > > > > > > > So, if you have a valid busclk, it gets enabled, otherwise it is > > > > > left alone. > > > > > > > So the code as sent in the original email is correct (aside from > > > Geert's comments about EPROBE_DEFER handling). > > > > > > Maybe I need some coffee :\ > > > Thanks > > > Phil > > > > My point is that errors should be treated as errors. > > > > In i2c_dw_prepare_clk() the following appears: > > > > if (IS_ERR(dev->clk)) > > return PTR_ERR(dev->clk); > > > > So dev->clk being an error value is treated as an error that is passed up to the > > caller. > > > > But in your patch (and the snippet below) dev->busclk is treated as the > > optional clock not being present. Even if the error stored nothing to do with > > the clock not being present - f.e. ENOMEM or as Geert mentioned > > elsewhere, EPROBE_DEFER. > > > > Assuming the absense of the optional clock is indicated by ENOENT, in my > > view correct code would include something like: > > > > ... > > > > if (IS_ERR(dev->clk)) > > return PTR_ERR(dev->clk); > > > > if (IS_ERR(dev->buslck) && PTR_ERR(dev->busclk) != -ENOENT) > > return PTR_ERR(dev->busclk); > > > > ... > > Yes, I completely agree! Great, sorry if I elaborated excessively. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock @ 2018-07-19 7:42 ` Simon Horman 0 siblings, 0 replies; 27+ messages in thread From: Simon Horman @ 2018-07-19 7:42 UTC (permalink / raw) To: Phil Edworthy Cc: Andy Shevchenko, Jarkko Nikula, Geert Uytterhoeven, linux-i2c, linux-kernel, linux-renesas-soc, Mika Westerberg On Wed, Jul 18, 2018 at 09:21:22AM +0000, Phil Edworthy wrote: > Hi Simon, > > On 18 July 2018 10:15 Simon Horman wrote: > > On Tue, Jul 17, 2018 at 02:57:27PM +0000, Phil Edworthy wrote: > > > On 17 July 2018 15:47, Andy Shevchenko wrote: > > > > On Tue, 2018-07-17 at 14:40 +0000, Phil Edworthy wrote: > > > > > On 17 July 2018 15:19, Andy Shevchenko wrote: > > > > > > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > > > > > > > > > > > While your point sounds valid (don't remember how clk_get() > > > > > > > > is implemented), NULL is also OK to have. > > > > > > > > > > > > > > Ok as in there is no bus clock, right? > > > > > > > So it should be: > > > > > > > if (!IS_ERR_OR_NULL (dev->busclk)) > > > > > > > > > > > > Nope, NULL is no error case for optional clock. > > > > > > > > > > I must be missing something here... > > > > > > > > See how clk_prepare_enable() is implemented. > > > Ok, if busclk is NULL the code can safely call clk_prepare_enable() > > > > > > > > I agree that NULL for an optional clock is not an error. However, > > > > > the code above is now: > > > > > + if (prepare) { > > > > > + /* Optional bus clock */ > > > > > > > > > + if (!IS_ERR_OR_NULL(dev->busclk)) { > > > > > > > > Check for NULL is redundant. > > > > > > > > > + ret = clk_prepare_enable(dev->busclk); > > > > > + if (ret) > > > > > + return ret; > > > > > + } > > > > > + > > > > > return clk_prepare_enable(dev->clk); > > > > > + } > > > > > > > > > > So, if you have a valid busclk, it gets enabled, otherwise it is > > > > > left alone. > > > > > > > So the code as sent in the original email is correct (aside from > > > Geert's comments about EPROBE_DEFER handling). > > > > > > Maybe I need some coffee :\ > > > Thanks > > > Phil > > > > My point is that errors should be treated as errors. > > > > In i2c_dw_prepare_clk() the following appears: > > > > if (IS_ERR(dev->clk)) > > return PTR_ERR(dev->clk); > > > > So dev->clk being an error value is treated as an error that is passed up to the > > caller. > > > > But in your patch (and the snippet below) dev->busclk is treated as the > > optional clock not being present. Even if the error stored nothing to do with > > the clock not being present - f.e. ENOMEM or as Geert mentioned > > elsewhere, EPROBE_DEFER. > > > > Assuming the absense of the optional clock is indicated by ENOENT, in my > > view correct code would include something like: > > > > ... > > > > if (IS_ERR(dev->clk)) > > return PTR_ERR(dev->clk); > > > > if (IS_ERR(dev->buslck) && PTR_ERR(dev->busclk) != -ENOENT) > > return PTR_ERR(dev->busclk); > > > > ... > > Yes, I completely agree! Great, sorry if I elaborated excessively. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-18 9:14 ` Simon Horman (?) (?) @ 2018-07-18 11:06 ` Geert Uytterhoeven 2018-07-18 12:52 ` Andy Shevchenko -1 siblings, 1 reply; 27+ messages in thread From: Geert Uytterhoeven @ 2018-07-18 11:06 UTC (permalink / raw) To: Simon Horman Cc: Phil Edworthy, Andy Shevchenko, Jarkko Nikula, Linux I2C, Linux Kernel Mailing List, Linux-Renesas, Mika Westerberg On Wed, Jul 18, 2018 at 11:14 AM Simon Horman <horms@verge.net.au> wrote: > On Tue, Jul 17, 2018 at 02:57:27PM +0000, Phil Edworthy wrote: > > On 17 July 2018 15:47, Andy Shevchenko wrote: > > > On Tue, 2018-07-17 at 14:40 +0000, Phil Edworthy wrote: > > > > On 17 July 2018 15:19, Andy Shevchenko wrote: > > > > > On Tue, 2018-07-17 at 12:42 +0000, Phil Edworthy wrote: > > > > > > > > > > > > While your point sounds valid (don't remember how clk_get() is > > > > > > > implemented), NULL is also OK to have. > > > > > > > > > > > > Ok as in there is no bus clock, right? > > > > > > So it should be: > > > > > > if (!IS_ERR_OR_NULL (dev->busclk)) > > > > > > > > > > Nope, NULL is no error case for optional clock. > > > > > > > > I must be missing something here... > > > > > > See how clk_prepare_enable() is implemented. > > Ok, if busclk is NULL the code can safely call clk_prepare_enable() > > > > > > I agree that NULL for an optional clock is not an error. However, the > > > > code above is now: > > > > + if (prepare) { > > > > + /* Optional bus clock */ > > > > > > > + if (!IS_ERR_OR_NULL(dev->busclk)) { > > > > > > Check for NULL is redundant. > > > > > > > + ret = clk_prepare_enable(dev->busclk); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > return clk_prepare_enable(dev->clk); > > > > + } > > > > > > > > So, if you have a valid busclk, it gets enabled, otherwise it is left > > > > alone. > > > > > So the code as sent in the original email is correct (aside from Geert's > > comments about EPROBE_DEFER handling). > > > > Maybe I need some coffee :\ > > Thanks > > Phil > > My point is that errors should be treated as errors. > > In i2c_dw_prepare_clk() the following appears: > > if (IS_ERR(dev->clk)) > return PTR_ERR(dev->clk); > > So dev->clk being an error value is treated as an error that > is passed up to the caller. > > But in your patch (and the snippet below) dev->busclk is treated > as the optional clock not being present. Even if the error stored > nothing to do with the clock not being present - f.e. ENOMEM or > as Geert mentioned elsewhere, EPROBE_DEFER. > > Assuming the absense of the optional clock is indicated by ENOENT, > in my view correct code would include something like: > > ... > > if (IS_ERR(dev->clk)) > return PTR_ERR(dev->clk); > > if (IS_ERR(dev->buslck) && PTR_ERR(dev->busclk) != -ENOENT) > return PTR_ERR(dev->busclk); > > ... As this is a commonly-used construct, perhaps it would be good to introduce clk_get_optional() (cfr. gpiod_get_optional()) and devm_clk_get_optional(), which would return NULL instead of -ENOENT? Then it becomes a simple check for IS_ERR() in the driver. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/2] i2c: designware: Add support for a bus clock 2018-07-18 11:06 ` Geert Uytterhoeven @ 2018-07-18 12:52 ` Andy Shevchenko 0 siblings, 0 replies; 27+ messages in thread From: Andy Shevchenko @ 2018-07-18 12:52 UTC (permalink / raw) To: Geert Uytterhoeven, Simon Horman Cc: Phil Edworthy, Jarkko Nikula, Linux I2C, Linux Kernel Mailing List, Linux-Renesas, Mika Westerberg On Wed, 2018-07-18 at 13:06 +0200, Geert Uytterhoeven wrote: > On Wed, Jul 18, 2018 at 11:14 AM Simon Horman <horms@verge.net.au> > wrote: > > if (IS_ERR(dev->buslck) && PTR_ERR(dev->busclk) != -ENOENT) > > return PTR_ERR(dev->busclk); > > > > ... > > As this is a commonly-used construct, perhaps it would be good to > introduce > clk_get_optional() (cfr. gpiod_get_optional()) and > devm_clk_get_optional(), > which would return NULL instead of -ENOENT? > Then it becomes a simple check for IS_ERR() in the driver. I'm puzzled why CCF still lack of such API. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2018-07-20 16:20 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-16 8:59 [PATCH 0/2] i2c: designware: Add support for a bus clock Phil Edworthy 2018-07-16 8:59 ` Phil Edworthy 2018-07-16 8:59 ` [PATCH 1/2] dt: snps,designware-i2c: Add clock bindings documentation Phil Edworthy 2018-07-20 16:19 ` Rob Herring 2018-07-16 8:59 ` [PATCH 2/2] i2c: designware: Add support for a bus clock Phil Edworthy 2018-07-17 12:07 ` Simon Horman 2018-07-17 12:23 ` Andy Shevchenko 2018-07-17 12:42 ` Phil Edworthy 2018-07-17 12:42 ` Phil Edworthy 2018-07-17 13:01 ` Geert Uytterhoeven 2018-07-17 13:12 ` Phil Edworthy 2018-07-17 14:18 ` Andy Shevchenko 2018-07-17 14:18 ` Andy Shevchenko 2018-07-17 14:40 ` Phil Edworthy 2018-07-17 14:40 ` Phil Edworthy 2018-07-17 14:47 ` Andy Shevchenko 2018-07-17 14:47 ` Andy Shevchenko 2018-07-17 14:57 ` Phil Edworthy 2018-07-17 14:57 ` Phil Edworthy 2018-07-18 9:14 ` Simon Horman 2018-07-18 9:14 ` Simon Horman 2018-07-18 9:21 ` Phil Edworthy 2018-07-18 9:21 ` Phil Edworthy 2018-07-19 7:42 ` Simon Horman 2018-07-19 7:42 ` Simon Horman 2018-07-18 11:06 ` Geert Uytterhoeven 2018-07-18 12:52 ` Andy Shevchenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.