Re: [PATCH v12 1/2] i2c: designware: add SLAVE mode functions

[Wolfram Sang]

On Thu, Jun 22, 2017 at 11:17:32AM +0100, Luis Oliveira wrote:
> - Changes in Kconfig to enable I2C_DESIGNWARE_SLAVE support
> - Slave functions added to core library file
> - Slave abort sources added to common source file
> - New driver: i2c-designware-slave added
> - Changes in the Makefile to compile the I2C_DESIGNWARE_SLAVE module
>   when supported by the architecture.
> 
> All the SLAVE flow is added but it is not enabled via platform
> driver.
> 
> Signed-off-by: Luis Oliveira 

I fixed this checkpatch warning for you:

WARNING: quoted string split across lines
#391: FILE: drivers/i2c/busses/i2c-designware-slave.c:281:
+   "%#x STAUTS SLAVE_ACTTVITY=%#x : RAW_INTR_STAT=%#x"
+   " : INTR_STAT=%#x\n",

Nice, you fooled two code checkers by having code in comments :) No
need to fix, of course:

SMATCH
drivers/i2c/busses/i2c-designware-slave.c:318 i2c_dw_irq_handler_slave warn: 
unused return: stat = i2c_dw_read_clear_intrbits_slave()
drivers/i2c/busses/i2c-designware-slave.c:329 i2c_dw_irq_handler_slave warn: 
unused return: stat = i2c_dw_read_clear_intrbits_slave()
CPPCHECK
drivers/i2c/busses/i2c-designware-slave.c:329: style: Variable 'stat' is 
assigned a value that is never used.

Build warning:

  CC  drivers/i2c/busses/i2c-designware-slave.o
drivers/i2c/busses/i2c-designware-slave.c:173:5: warning: no previous prototype 
for ‘i2c_dw_reg_slave’ [-Wmissing-prototypes]
 int i2c_dw_reg_slave(struct i2c_client *slave)

I declared the function static to fix it.

Applied to for-next, thanks!



signature.asc
Description: PGP signature

Re: [PATCH v12 1/2] i2c: designware: add SLAVE mode functions

[Wolfram Sang]

On Thu, Jun 22, 2017 at 11:17:32AM +0100, Luis Oliveira wrote:
> - Changes in Kconfig to enable I2C_DESIGNWARE_SLAVE support
> - Slave functions added to core library file
> - Slave abort sources added to common source file
> - New driver: i2c-designware-slave added
> - Changes in the Makefile to compile the I2C_DESIGNWARE_SLAVE module
>   when supported by the architecture.
> 
> All the SLAVE flow is added but it is not enabled via platform
> driver.
> 
> Signed-off-by: Luis Oliveira 

I fixed this checkpatch warning for you:

WARNING: quoted string split across lines
#391: FILE: drivers/i2c/busses/i2c-designware-slave.c:281:
+   "%#x STAUTS SLAVE_ACTTVITY=%#x : RAW_INTR_STAT=%#x"
+   " : INTR_STAT=%#x\n",

Nice, you fooled two code checkers by having code in comments :) No
need to fix, of course:

SMATCH
drivers/i2c/busses/i2c-designware-slave.c:318 i2c_dw_irq_handler_slave warn: 
unused return: stat = i2c_dw_read_clear_intrbits_slave()
drivers/i2c/busses/i2c-designware-slave.c:329 i2c_dw_irq_handler_slave warn: 
unused return: stat = i2c_dw_read_clear_intrbits_slave()
CPPCHECK
drivers/i2c/busses/i2c-designware-slave.c:329: style: Variable 'stat' is 
assigned a value that is never used.

Build warning:

  CC  drivers/i2c/busses/i2c-designware-slave.o
drivers/i2c/busses/i2c-designware-slave.c:173:5: warning: no previous prototype 
for ‘i2c_dw_reg_slave’ [-Wmissing-prototypes]
 int i2c_dw_reg_slave(struct i2c_client *slave)

I declared the function static to fix it.

Applied to for-next, thanks!



signature.asc
Description: PGP signature

Re: [PATCH v12 1/2] i2c: designware: add SLAVE mode functions

[Jarkko Nikula]

On 06/22/2017 01:17 PM, Luis Oliveira wrote:

- Changes in Kconfig to enable I2C_DESIGNWARE_SLAVE support
- Slave functions added to core library file
- Slave abort sources added to common source file
- New driver: i2c-designware-slave added
- Changes in the Makefile to compile the I2C_DESIGNWARE_SLAVE module
  when supported by the architecture.

All the SLAVE flow is added but it is not enabled via platform
driver.

Signed-off-by: Luis Oliveira 
---
V11-v12
- Changed ifdef condition in the Makefile fixed Kbuild error

 drivers/i2c/busses/Kconfig |  14 +-
 drivers/i2c/busses/Makefile|   3 +
 drivers/i2c/busses/i2c-designware-common.c |   6 +
 drivers/i2c/busses/i2c-designware-core.h   |   2 +
 drivers/i2c/busses/i2c-designware-slave.c  | 395 +
 5 files changed, 419 insertions(+), 1 deletion(-)
 create mode 100644 drivers/i2c/busses/i2c-designware-slave.c


...

+static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
+{
+   u32 raw_stat, stat, enabled;
+   u8 val, slave_activity;
+
+   stat = dw_readl(dev, DW_IC_INTR_STAT);
+   enabled = dw_readl(dev, DW_IC_ENABLE);
+   raw_stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
+   slave_activity = ((dw_readl(dev, DW_IC_STATUS) &
+   DW_IC_STATUS_SLAVE_ACTIVITY) >> 6);
+
+   if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY))
+   return 0;
+
+   dev_dbg(dev->dev,
+   "%#x STAUTS SLAVE_ACTTVITY=%#x : RAW_INTR_STAT=%#x"
+   " : INTR_STAT=%#x\n",
+   enabled, slave_activity, raw_stat, stat);
+


Minor notes:
- Strings that are printed should fit in single line for making easier 
to grep them from sources. Maybe not all of those variables are not 
necessary in this debug print (e.g. are we interested other than bit 0 
in DW_IC_ENABLE and line above already returns if it reads zero).

- Typos "STAUTS SLAVE_ACTTVITY".

You may add my acked by if you want to send another version or I'm fine 
with incremental patch also.


Acked-by: Jarkko Nikula 

Re: [PATCH v12 1/2] i2c: designware: add SLAVE mode functions

[Jarkko Nikula]

On 06/22/2017 01:17 PM, Luis Oliveira wrote:

- Changes in Kconfig to enable I2C_DESIGNWARE_SLAVE support
- Slave functions added to core library file
- Slave abort sources added to common source file
- New driver: i2c-designware-slave added
- Changes in the Makefile to compile the I2C_DESIGNWARE_SLAVE module
  when supported by the architecture.

All the SLAVE flow is added but it is not enabled via platform
driver.

Signed-off-by: Luis Oliveira 
---
V11-v12
- Changed ifdef condition in the Makefile fixed Kbuild error

 drivers/i2c/busses/Kconfig |  14 +-
 drivers/i2c/busses/Makefile|   3 +
 drivers/i2c/busses/i2c-designware-common.c |   6 +
 drivers/i2c/busses/i2c-designware-core.h   |   2 +
 drivers/i2c/busses/i2c-designware-slave.c  | 395 +
 5 files changed, 419 insertions(+), 1 deletion(-)
 create mode 100644 drivers/i2c/busses/i2c-designware-slave.c


...

+static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
+{
+   u32 raw_stat, stat, enabled;
+   u8 val, slave_activity;
+
+   stat = dw_readl(dev, DW_IC_INTR_STAT);
+   enabled = dw_readl(dev, DW_IC_ENABLE);
+   raw_stat = dw_readl(dev, DW_IC_RAW_INTR_STAT);
+   slave_activity = ((dw_readl(dev, DW_IC_STATUS) &
+   DW_IC_STATUS_SLAVE_ACTIVITY) >> 6);
+
+   if (!enabled || !(raw_stat & ~DW_IC_INTR_ACTIVITY))
+   return 0;
+
+   dev_dbg(dev->dev,
+   "%#x STAUTS SLAVE_ACTTVITY=%#x : RAW_INTR_STAT=%#x"
+   " : INTR_STAT=%#x\n",
+   enabled, slave_activity, raw_stat, stat);
+


Minor notes:
- Strings that are printed should fit in single line for making easier 
to grep them from sources. Maybe not all of those variables are not 
necessary in this debug print (e.g. are we interested other than bit 0 
in DW_IC_ENABLE and line above already returns if it reads zero).

- Typos "STAUTS SLAVE_ACTTVITY".

You may add my acked by if you want to send another version or I'm fine 
with incremental patch also.


Acked-by: Jarkko Nikula 

Re: [PATCH v12 1/2] i2c: designware: add SLAVE mode functions

[Andy Shevchenko]

On Thu, 2017-06-22 at 11:17 +0100, Luis Oliveira wrote:
> - Changes in Kconfig to enable I2C_DESIGNWARE_SLAVE support
> - Slave functions added to core library file
> - Slave abort sources added to common source file
> - New driver: i2c-designware-slave added
> - Changes in the Makefile to compile the I2C_DESIGNWARE_SLAVE module
>   when supported by the architecture.
> 
> All the SLAVE flow is added but it is not enabled via platform
> driver.

There are couple of nits, otherwise I'm okay with the patch.

Reviewed-by: Andy Shevchenko 

> V11-v12
> - Changed ifdef condition in the Makefile fixed Kbuild error

+ rebased on top of latest i2c-next (drop applied patches effectively)

> +static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
> +{
> 

> + dev_dbg(dev->dev,

> + "%#x STAUTS SLAVE_ACTTVITY=%#x : RAW_INTR_STAT=%#x"
> + " : INTR_STAT=%#x\n",

Don't split string literals.

> + enabled, slave_activity, raw_stat, stat);


> + dev_vdbg(dev->dev, "Byte %X
> acked!",
> +  val);

I'm not sure it's any helpful.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v12 1/2] i2c: designware: add SLAVE mode functions

[Andy Shevchenko]

On Thu, 2017-06-22 at 11:17 +0100, Luis Oliveira wrote:
> - Changes in Kconfig to enable I2C_DESIGNWARE_SLAVE support
> - Slave functions added to core library file
> - Slave abort sources added to common source file
> - New driver: i2c-designware-slave added
> - Changes in the Makefile to compile the I2C_DESIGNWARE_SLAVE module
>   when supported by the architecture.
> 
> All the SLAVE flow is added but it is not enabled via platform
> driver.

There are couple of nits, otherwise I'm okay with the patch.

Reviewed-by: Andy Shevchenko 

> V11-v12
> - Changed ifdef condition in the Makefile fixed Kbuild error

+ rebased on top of latest i2c-next (drop applied patches effectively)

> +static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
> +{
> 

> + dev_dbg(dev->dev,

> + "%#x STAUTS SLAVE_ACTTVITY=%#x : RAW_INTR_STAT=%#x"
> + " : INTR_STAT=%#x\n",

Don't split string literals.

> + enabled, slave_activity, raw_stat, stat);


> + dev_vdbg(dev->dev, "Byte %X
> acked!",
> +  val);

I'm not sure it's any helpful.

-- 
Andy Shevchenko 
Intel Finland Oy