| Message ID | 1531731553-22979-3-git-send-email-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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 >
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.
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
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
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
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.
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
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.
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
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); ...
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
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
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.
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.
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;
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(-)