[PATCH 0/2] i2c: designware: Add support for a bus clock

[PATCH 0/2] i2c: designware: Add support for a bus clock

[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

[PATCH 0/2] i2c: designware: Add support for a bus clock

[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

[PATCH 1/2] dt: snps,designware-i2c: Add clock bindings documentation

[Phil Edworthy]

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

[PATCH 2/2] i2c: designware: Add support for a bus clock

[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.
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

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Simon Horman]

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
> 

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Andy Shevchenko]

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

RE: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Phil Edworthy]

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

RE: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Phil Edworthy]

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

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Geert Uytterhoeven]

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

RE: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Phil Edworthy]

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

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Andy Shevchenko]

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

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Andy Shevchenko]

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

RE: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Phil Edworthy]

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

RE: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Phil Edworthy]

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

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Andy Shevchenko]

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

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Andy Shevchenko]

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

RE: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Phil Edworthy]

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

RE: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Phil Edworthy]

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

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Simon Horman]

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);

	...

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Simon Horman]

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);

	...

RE: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Phil Edworthy]

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

RE: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Phil Edworthy]

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

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Geert Uytterhoeven]

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

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Andy Shevchenko]

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

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Simon Horman]

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.

Re: [PATCH 2/2] i2c: designware: Add support for a bus clock

[Simon Horman]

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.

Re: [PATCH 1/2] dt: snps,designware-i2c: Add clock bindings documentation

[Rob Herring]

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>