i2c-axxia: support slave mode
PATCH 1/1 i2c-axxia: support slave mode
This device contains both master and slave controllers which can be enabled simultaneously. Both controllers share the same SDA/SCL lines and interrupt source but has separate control and status registers. Controllers also works in loopback mode - slave device can communicate with its own master controller internally. The controller can handle up to two addresses, both of which may be 10 bit. Most of the logic (sending (N)ACK, handling repeated start or switching between write/read) is handled automatically which makes working with this controller quite easy.
For simplicity, this patch adds basic support, limiting to only one slave address. Support for the 2nd device may be added in the future.
Note that synchronize_irq() is used to ensure any running slave interrupt is finished to make sure slave i2c_client structure can be safely used by i2c_slave_event.
---
Note that checkpatch shows warnings about "line over 80 characters" for some of those register definitions added but I personally think splitting those comments would decrease readability, not increase it. I can do that, however, if you think otherwise.
--- drivers/i2c/busses/Kconfig | 1 + drivers/i2c/busses/i2c-axxia.c | 160 +++++++++++++++++++++++++++++++-- 2 files changed, 153 insertions(+), 8 deletions(-)
drivers/i2c/busses/Kconfig +1 -0
@@ -429,6 +429,7 @@ config I2C_AXXIA tristate "Axxia I2C controller" depends on ARCH_AXXIA || COMPILE_TEST default ARCH_AXXIA + select I2C_SLAVE help Say yes if you want to support the I2C bus on Axxia platforms.
drivers/i2c/busses/i2c-axxia.c +152 -8
213 lines not on this page · raw message
Signature · 1 line
2.21.0
Hi Krzysztof,
Quoted 5 lines
This device contains both master and slave controllers which can be enabled simultaneously. Both controllers share the same SDA/SCL lines and interrupt source but has separate control and status registers. Controllers also works in loopback mode - slave device can communicate with its own master controller internally. The controller can handle up
Cool, I never got this to work with my hardware. I always had to wire two controllers together,
Quoted 4 lines
to two addresses, both of which may be 10 bit. Most of the logic (sending (N)ACK, handling repeated start or switching between write/read) is handled automatically which makes working with this controller quite easy.
Yes, looks pretty straightforward. Nice!
Quoted 2 lines
For simplicity, this patch adds basic support, limiting to only one slave address. Support for the 2nd device may be added in the future.
Fine with me. Incremental additions are easier to review.
Quoted 4 lines
Note that checkpatch shows warnings about "line over 80 characters" for some of those register definitions added but I personally think splitting those comments would decrease readability, not increase it. I can do that, however, if you think otherwise.
I am fine with that, too.
drivers/i2c/busses/i2c-axxia.c static int axxia_i2c_fill_tx_fifo(struct axxia_i2c_dev *idev) · line 323
Quoted 3 lines
drivers/i2c/busses/i2c-axxia.c +3
-0+if (fifo_status &SLV_FIFO_DV1 ) { +if (fifo_status &SLV_FIFO_STRC ) { +dev_dbg (dev,"First data byte sent\n" );
I think, however, these debug messages could go. They were surely helpful during development but assuming things work now, they will not help backend authors. Can you agree?
Rest looks good from what I can tell without knowing the hardware.
Thanks,
Wolfram
Hi Wolfram,
Quoted 9 lines
Hi Krzysztof,
drivers/i2c/busses/i2c-axxia.c static int axxia_i2c_fill_tx_fifo(struct axxia_i2c_dev *idev) · line 323
Quoted 3 lines
drivers/i2c/busses/i2c-axxia.c +3
-0+if (fifo_status &SLV_FIFO_DV1 ) { +if (fifo_status &SLV_FIFO_STRC ) { +dev_dbg (dev,"First data byte sent\n" );I think, however, these debug messages could go. They were surely helpful during development but assuming things work now, they will not help backend authors. Can you agree?
Good point. I'll remove those verbose messages and maybe leave one or two debug messages with just a summary of the status which will hopefully be a good compromise. Will that be ok?
Quoted 2 lines
Rest looks good from what I can tell without knowing the hardware.
It also seems to work correctly and I tried to do everything in a way that nothing is changed if slave mode is not used to eliminate the risk of regressions.
BTW, I have added this synchronize_irq() in unreg_slave callback just to make sure it is save to set idev->slave to NULL already. Most of the controllers do not have such a guard and I'm wondering why that wouldn't be a problem for them. Like the i2c-rcar.c - isn't there a small race condition if some slave interrupt triggers just before ICSIER is cleared and somehow does not finish before priv->slave is set to NULL? This is the situation I was afraid of and tried to solve by using this synchronize_irq().
Krzysztof
Hi Krzysztof,
Quoted 3 lines
Good point. I'll remove those verbose messages and maybe leave one or two debug messages with just a summary of the status which will hopefully be a good compromise. Will that be ok?
Very likely :)
Quoted 8 lines
BTW, I have added this
synchronize_irq()inunreg_slavecallback just to make sure it is save to setidev->slaveto NULL already. Most of the controllers do not have such a guard and I'm wondering why that wouldn't be a problem for them. Like thei2c-rcar.c- isn't there a small race condition if some slave interrupt triggers just before ICSIER is cleared and somehow does not finish beforepriv->slaveis set to NULL? This is the situation I was afraid of and tried to solve by using thissynchronize_irq().
You have an important point there. unreg_slave is protected by the adapter lock but this won't help if another master is requesting something, thus, causing interrupts.
Phew, nearly all slave implementation need to fix this!
Thanks a lot,
Wolfram