Hello Ludovic, Thank you very much for your feedback! On Tue, Oct 31, 2017 at 04:22:50PM +0100, Ludovic Desroches wrote: > On Fri, Oct 27, 2017 at 05:12:17PM +0200, Juergen Fitschen wrote: > > Some AT91 hardware has no slave mode included or only limited features > > (i.e. no fifos). > > > > I am wondering if it won't be better to squash this patch into the > previous one: > Without it, it seems that we can set slave_detected for the RM9200 even > if it doesn't support the slave mode. Good point. I will squash both patches into one in the next version. In the first place I wanted to support the review process by splitting the changes in two patches. > > diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h > > index bb502c1..4a4fa67 100644 > > --- a/drivers/i2c/busses/i2c-at91.h > > +++ b/drivers/i2c/busses/i2c-at91.h > > @@ -107,9 +107,14 @@ > > > > #define AT91_TWI_VER 0x00fc /* Version Register */ > > > > +#define AT91_TWI_SM_AVAILABLE BIT(0) /* Slave mode supported */ > > +#define AT91_TWI_SM_CAN_NACK BIT(1) /* Can send NACKs in slave mode */ > > +#define AT91_TWI_SM_HAS_FIFO BIT(2) /* Has FIFO for slave mode */ > > + > > I would not add AT91_TWI_SM_CAN_NACK, AT91_TWI_SM_HAS_FIFO since there > is no code relying on them. Maybe you have some plans for the future? Wolfram mentioned that supporting NACKs would be a welcome feature [1]. But I haven't implemented it, yet. The same goes for FIFO support. ATM I am not sure if my application will need this, since I am observing quite a lot clock stretching without FIFOs due to the occupied receive holding registered (RHR). BTW: Both implementations would be kind of controversal. Without using FIFOs the desired NACK would be delayed by 1 byte (cf. my "artistic" ASCII graphic [2]). If FIFOs are enabled the delay would be even larger. So the options are: * No NACKs at all * NACKs delayed by 1 byte, no FIFOs * NACKs delayed by n byte, with FIFOs Non of these abovementioned options is optimal and fulfill the desired behaviour (cf. section I2C_SLAVE_WRITE_RECEIVED of [3]). Furthermore, AFAIK NACKs and FIFOs are just supported by SAMA5D2x MPUs. These are the main reasons why I haven't implented anything related to AT91_TWI_SM_CAN_NACK and AT91_TWI_SM_HAS_FIFO. The designware driver ignores the NACK problem, as well. Do you have an opinion on this topic? In the next version of this patchset I will remove this. I think readding these flags if needed shouldn't be a big deal. Best regards Juergen [1] https://marc.info/?l=linux-i2c&m=150831224824540&w=2 [2] https://marc.info/?l=linux-i2c&m=150833171430595&w=2 [3] https://www.kernel.org/doc/Documentation/i2c/slave-interface
