Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[De Marchi, Lucas]

On Wed, 2016-08-17 at 11:05 +0300, Jarkko Nikula wrote:
> On 08/16/2016 05:07 PM, De Marchi, Lucas wrote:
> > 
> > On Tue, 2016-08-16 at 17:00 +0300, Jarkko Nikula wrote:
> > > 
> > > > 
> > > > +    */
> > > > +   reg = dw_readl(dev, DW_IC_CON);
> > > > +   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER,
> > > > DW_IC_CON);
> > > > +
> > > > +   if ((dw_readl(dev, DW_IC_CON) &
> > > > DW_IC_CON_10BITADDR_MASTER) ==
> > > > +   (reg & DW_IC_CON_10BITADDR_MASTER)) {
> > > > +   dev->dynamic_tar_update_enabled = true;
> > > > +   dev_dbg(dev->dev, "Dynamic TAR update
> > > > enabled");
> > > > +   }
> > > 
> > > Is this possible to move to i2c_dw_probe()? I guess the enabled
> > > status
> > > doesn't change runtime?
> > 
> > It was actually useful at this place during development of this
> > patch
> > because we could check any unexpected change in behavior when
> > resuming.
> > We did catch a bug because of this and fixed.
> > I'm not sure if now it makes more sense to move to probe method.
> > I'd
> > leave it where it is, but I'm open to move it there.
> > 
> Can you do a quick re-test that case to see does it change runtime?
> If 
> it does then this needs a comment why there is need to do this check 
> each time when HW is reinitialized. Otherwise there is chance
> someone 
> may move this code to probe time in the future.

I already tested and it doesn't change. I'll move it to i2c_dw_probe()
then.

thanks

Lucas De Marchi

Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[De Marchi, Lucas]

On Wed, 2016-08-17 at 11:05 +0300, Jarkko Nikula wrote:
> On 08/16/2016 05:07 PM, De Marchi, Lucas wrote:
> > 
> > On Tue, 2016-08-16 at 17:00 +0300, Jarkko Nikula wrote:
> > > 
> > > > 
> > > > +    */
> > > > +   reg = dw_readl(dev, DW_IC_CON);
> > > > +   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER,
> > > > DW_IC_CON);
> > > > +
> > > > +   if ((dw_readl(dev, DW_IC_CON) &
> > > > DW_IC_CON_10BITADDR_MASTER) ==
> > > > +   (reg & DW_IC_CON_10BITADDR_MASTER)) {
> > > > +   dev->dynamic_tar_update_enabled = true;
> > > > +   dev_dbg(dev->dev, "Dynamic TAR update
> > > > enabled");
> > > > +   }
> > > 
> > > Is this possible to move to i2c_dw_probe()? I guess the enabled
> > > status
> > > doesn't change runtime?
> > 
> > It was actually useful at this place during development of this
> > patch
> > because we could check any unexpected change in behavior when
> > resuming.
> > We did catch a bug because of this and fixed.
> > I'm not sure if now it makes more sense to move to probe method.
> > I'd
> > leave it where it is, but I'm open to move it there.
> > 
> Can you do a quick re-test that case to see does it change runtime?
> If 
> it does then this needs a comment why there is need to do this check 
> each time when HW is reinitialized. Otherwise there is chance
> someone 
> may move this code to probe time in the future.

I already tested and it doesn't change. I'll move it to i2c_dw_probe()
then.

thanks

Lucas De Marchi

Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[Jarkko Nikula]

On 08/16/2016 05:07 PM, De Marchi, Lucas wrote:

On Tue, 2016-08-16 at 17:00 +0300, Jarkko Nikula wrote:

+*/
+   reg = dw_readl(dev, DW_IC_CON);
+   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER,
DW_IC_CON);
+
+   if ((dw_readl(dev, DW_IC_CON) &
DW_IC_CON_10BITADDR_MASTER) ==
+   (reg & DW_IC_CON_10BITADDR_MASTER)) {
+   dev->dynamic_tar_update_enabled = true;
+   dev_dbg(dev->dev, "Dynamic TAR update enabled");
+   }


Is this possible to move to i2c_dw_probe()? I guess the enabled
status
doesn't change runtime?


It was actually useful at this place during development of this patch
because we could check any unexpected change in behavior when resuming.
We did catch a bug because of this and fixed.
I'm not sure if now it makes more sense to move to probe method. I'd
leave it where it is, but I'm open to move it there.

Can you do a quick re-test that case to see does it change runtime? If 
it does then this needs a comment why there is need to do this check 
each time when HW is reinitialized. Otherwise there is chance someone 
may move this code to probe time in the future.


--
Jarkko

Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[Jarkko Nikula]

On 08/16/2016 05:07 PM, De Marchi, Lucas wrote:

On Tue, 2016-08-16 at 17:00 +0300, Jarkko Nikula wrote:

+*/
+   reg = dw_readl(dev, DW_IC_CON);
+   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER,
DW_IC_CON);
+
+   if ((dw_readl(dev, DW_IC_CON) &
DW_IC_CON_10BITADDR_MASTER) ==
+   (reg & DW_IC_CON_10BITADDR_MASTER)) {
+   dev->dynamic_tar_update_enabled = true;
+   dev_dbg(dev->dev, "Dynamic TAR update enabled");
+   }


Is this possible to move to i2c_dw_probe()? I guess the enabled
status
doesn't change runtime?


It was actually useful at this place during development of this patch
because we could check any unexpected change in behavior when resuming.
We did catch a bug because of this and fixed.
I'm not sure if now it makes more sense to move to probe method. I'd
leave it where it is, but I'm open to move it there.

Can you do a quick re-test that case to see does it change runtime? If 
it does then this needs a comment why there is need to do this check 
each time when HW is reinitialized. Otherwise there is chance someone 
may move this code to probe time in the future.


--
Jarkko

Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[De Marchi, Lucas]

On Tue, 2016-08-16 at 17:00 +0300, Jarkko Nikula wrote:
> Hi, + Wolfram
> 
> On 07/29/2016 01:03 AM, Lucas De Marchi wrote:
> > 
> > This adapter can be synthesized with dynamic tar update enabled or
> > disabled.
> > When enabled it is not necessary to disable the adapter to change
> > the slave
> > address in some situations, which saves some time per transaction.
> > 
> > There is no direct register to know if this feature is enabled but
> > we can do it
> > indirectly by writing to the 10BIT_ADDR field in IC_CON: this field
> > is
> > read only when dynamic tar update is enabled.
> > 
> > Signed-off-by: Lucas De Marchi 
> > Signed-off-by: José Roberto de Souza 
> > ---
> >  drivers/i2c/busses/i2c-designware-core.c | 37
> > ++--
> >  drivers/i2c/busses/i2c-designware-core.h |  1 +
> >  2 files changed, 27 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c
> > b/drivers/i2c/busses/i2c-designware-core.c
> > index 2c61585..a8408db 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -388,6 +388,20 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> >     /* configure the i2c master */
> >     dw_writel(dev, dev->master_cfg , DW_IC_CON);
> > 
> > +   /*
> > +    * Test if dynamic TAR update is enabled in this
> > controller by writing to
> 
> Over 80 characters in this line.

I'll fix this and wait for more comments (or a few days) before sending
a new version.

> > +    */
> > +   reg = dw_readl(dev, DW_IC_CON);
> > +   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER,
> > DW_IC_CON);
> > +
> > +   if ((dw_readl(dev, DW_IC_CON) &
> > DW_IC_CON_10BITADDR_MASTER) ==
> > +   (reg & DW_IC_CON_10BITADDR_MASTER)) {
> > +   dev->dynamic_tar_update_enabled = true;
> > +   dev_dbg(dev->dev, "Dynamic TAR update enabled");
> > +   }
> 
> Is this possible to move to i2c_dw_probe()? I guess the enabled
> status 
> doesn't change runtime?

It was actually useful at this place during development of this patch
because we could check any unexpected change in behavior when resuming.
We did catch a bug because of this and fixed.
I'm not sure if now it makes more sense to move to probe method. I'd
leave it where it is, but I'm open to move it there.

thanks

Lucas De Marchi

Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[De Marchi, Lucas]

On Tue, 2016-08-16 at 17:00 +0300, Jarkko Nikula wrote:
> Hi, + Wolfram
> 
> On 07/29/2016 01:03 AM, Lucas De Marchi wrote:
> > 
> > This adapter can be synthesized with dynamic tar update enabled or
> > disabled.
> > When enabled it is not necessary to disable the adapter to change
> > the slave
> > address in some situations, which saves some time per transaction.
> > 
> > There is no direct register to know if this feature is enabled but
> > we can do it
> > indirectly by writing to the 10BIT_ADDR field in IC_CON: this field
> > is
> > read only when dynamic tar update is enabled.
> > 
> > Signed-off-by: Lucas De Marchi 
> > Signed-off-by: José Roberto de Souza 
> > ---
> >  drivers/i2c/busses/i2c-designware-core.c | 37
> > ++--
> >  drivers/i2c/busses/i2c-designware-core.h |  1 +
> >  2 files changed, 27 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c
> > b/drivers/i2c/busses/i2c-designware-core.c
> > index 2c61585..a8408db 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -388,6 +388,20 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> >     /* configure the i2c master */
> >     dw_writel(dev, dev->master_cfg , DW_IC_CON);
> > 
> > +   /*
> > +    * Test if dynamic TAR update is enabled in this
> > controller by writing to
> 
> Over 80 characters in this line.

I'll fix this and wait for more comments (or a few days) before sending
a new version.

> > +    */
> > +   reg = dw_readl(dev, DW_IC_CON);
> > +   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER,
> > DW_IC_CON);
> > +
> > +   if ((dw_readl(dev, DW_IC_CON) &
> > DW_IC_CON_10BITADDR_MASTER) ==
> > +   (reg & DW_IC_CON_10BITADDR_MASTER)) {
> > +   dev->dynamic_tar_update_enabled = true;
> > +   dev_dbg(dev->dev, "Dynamic TAR update enabled");
> > +   }
> 
> Is this possible to move to i2c_dw_probe()? I guess the enabled
> status 
> doesn't change runtime?

It was actually useful at this place during development of this patch
because we could check any unexpected change in behavior when resuming.
We did catch a bug because of this and fixed.
I'm not sure if now it makes more sense to move to probe method. I'd
leave it where it is, but I'm open to move it there.

thanks

Lucas De Marchi

Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[Jarkko Nikula]

Hi, + Wolfram

On 07/29/2016 01:03 AM, Lucas De Marchi wrote:

This adapter can be synthesized with dynamic tar update enabled or disabled.
When enabled it is not necessary to disable the adapter to change the slave
address in some situations, which saves some time per transaction.

There is no direct register to know if this feature is enabled but we can do it
indirectly by writing to the 10BIT_ADDR field in IC_CON: this field is
read only when dynamic tar update is enabled.

Signed-off-by: Lucas De Marchi 
Signed-off-by: José Roberto de Souza 
---
 drivers/i2c/busses/i2c-designware-core.c | 37 ++--
 drivers/i2c/busses/i2c-designware-core.h |  1 +
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c 
b/drivers/i2c/busses/i2c-designware-core.c
index 2c61585..a8408db 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -388,6 +388,20 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
/* configure the i2c master */
dw_writel(dev, dev->master_cfg , DW_IC_CON);

+   /*
+* Test if dynamic TAR update is enabled in this controller by writing 
to


Over 80 characters in this line.


+*/
+   reg = dw_readl(dev, DW_IC_CON);
+   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER, DW_IC_CON);
+
+   if ((dw_readl(dev, DW_IC_CON) & DW_IC_CON_10BITADDR_MASTER) ==
+   (reg & DW_IC_CON_10BITADDR_MASTER)) {
+   dev->dynamic_tar_update_enabled = true;
+   dev_dbg(dev->dev, "Dynamic TAR update enabled");
+   }


Is this possible to move to i2c_dw_probe()? I guess the enabled status 
doesn't change runtime?


--
Jarkko

Re: [PATCH v3 2/3] i2c: designware: detect when dynamic tar update is possible

[Jarkko Nikula]

Hi, + Wolfram

On 07/29/2016 01:03 AM, Lucas De Marchi wrote:

This adapter can be synthesized with dynamic tar update enabled or disabled.
When enabled it is not necessary to disable the adapter to change the slave
address in some situations, which saves some time per transaction.

There is no direct register to know if this feature is enabled but we can do it
indirectly by writing to the 10BIT_ADDR field in IC_CON: this field is
read only when dynamic tar update is enabled.

Signed-off-by: Lucas De Marchi 
Signed-off-by: José Roberto de Souza 
---
 drivers/i2c/busses/i2c-designware-core.c | 37 ++--
 drivers/i2c/busses/i2c-designware-core.h |  1 +
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c 
b/drivers/i2c/busses/i2c-designware-core.c
index 2c61585..a8408db 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -388,6 +388,20 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
/* configure the i2c master */
dw_writel(dev, dev->master_cfg , DW_IC_CON);

+   /*
+* Test if dynamic TAR update is enabled in this controller by writing 
to


Over 80 characters in this line.


+*/
+   reg = dw_readl(dev, DW_IC_CON);
+   dw_writel(dev, reg ^ DW_IC_CON_10BITADDR_MASTER, DW_IC_CON);
+
+   if ((dw_readl(dev, DW_IC_CON) & DW_IC_CON_10BITADDR_MASTER) ==
+   (reg & DW_IC_CON_10BITADDR_MASTER)) {
+   dev->dynamic_tar_update_enabled = true;
+   dev_dbg(dev->dev, "Dynamic TAR update enabled");
+   }


Is this possible to move to i2c_dw_probe()? I guess the enabled status 
doesn't change runtime?


--
Jarkko