| Message ID | 1e3e7e50e25afd3ebe14daf89d1f194c45ca6cbb.1509112910.git.me@jue.yt |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: at91: slave mode support | expand |
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. > Signed-off-by: Juergen Fitschen <me@jue.yt> > --- > drivers/i2c/busses/i2c-at91-core.c | 14 ++++++++++++-- > drivers/i2c/busses/i2c-at91.h | 5 +++++ > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c > index 3d7287c..9bf1e9d 100644 > --- a/drivers/i2c/busses/i2c-at91-core.c > +++ b/drivers/i2c/busses/i2c-at91-core.c > @@ -69,6 +69,7 @@ void at91_init_twi_bus(struct at91_twi_dev *dev) > static struct at91_twi_pdata at91rm9200_config = { > .clk_max_div = 5, > .clk_offset = 3, > + .slave_mode_features = 0, > .has_unre_flag = true, > .has_alt_cmd = false, > .has_hold_field = false, > @@ -77,6 +78,7 @@ static struct at91_twi_pdata at91rm9200_config = { > static struct at91_twi_pdata at91sam9261_config = { > .clk_max_div = 5, > .clk_offset = 4, > + .slave_mode_features = AT91_TWI_SM_AVAILABLE, > .has_unre_flag = false, > .has_alt_cmd = false, > .has_hold_field = false, > @@ -85,6 +87,7 @@ static struct at91_twi_pdata at91sam9261_config = { > static struct at91_twi_pdata at91sam9260_config = { > .clk_max_div = 7, > .clk_offset = 4, > + .slave_mode_features = AT91_TWI_SM_AVAILABLE, > .has_unre_flag = false, > .has_alt_cmd = false, > .has_hold_field = false, > @@ -93,6 +96,7 @@ static struct at91_twi_pdata at91sam9260_config = { > static struct at91_twi_pdata at91sam9g20_config = { > .clk_max_div = 7, > .clk_offset = 4, > + .slave_mode_features = AT91_TWI_SM_AVAILABLE, > .has_unre_flag = false, > .has_alt_cmd = false, > .has_hold_field = false, > @@ -101,6 +105,7 @@ static struct at91_twi_pdata at91sam9g20_config = { > static struct at91_twi_pdata at91sam9g10_config = { > .clk_max_div = 7, > .clk_offset = 4, > + .slave_mode_features = AT91_TWI_SM_AVAILABLE, > .has_unre_flag = false, > .has_alt_cmd = false, > .has_hold_field = false, > @@ -131,6 +136,7 @@ static const struct platform_device_id at91_twi_devtypes[] = { > static struct at91_twi_pdata at91sam9x5_config = { > .clk_max_div = 7, > .clk_offset = 4, > + .slave_mode_features = AT91_TWI_SM_AVAILABLE, > .has_unre_flag = false, > .has_alt_cmd = false, > .has_hold_field = false, > @@ -139,6 +145,7 @@ static struct at91_twi_pdata at91sam9x5_config = { > static struct at91_twi_pdata sama5d4_config = { > .clk_max_div = 7, > .clk_offset = 4, > + .slave_mode_features = AT91_TWI_SM_AVAILABLE, > .has_unre_flag = false, > .has_alt_cmd = false, > .has_hold_field = true, > @@ -147,6 +154,7 @@ static struct at91_twi_pdata sama5d4_config = { > static struct at91_twi_pdata sama5d2_config = { > .clk_max_div = 7, > .clk_offset = 4, > + .slave_mode_features = AT91_TWI_SM_AVAILABLE | AT91_TWI_SM_HAS_FIFO | AT91_TWI_SM_CAN_NACK, > .has_unre_flag = true, > .has_alt_cmd = true, > .has_hold_field = true, > @@ -219,6 +227,10 @@ static int at91_twi_probe(struct platform_device *pdev) > if (!dev->pdata) > return -ENODEV; > > + dev->slave_detected = i2c_detect_slave_mode(&pdev->dev); > + if (dev->slave_detected && dev->pdata->slave_mode_features == 0) > + return -EPFNOSUPPORT; > + > dev->base = devm_ioremap_resource(&pdev->dev, mem); > if (IS_ERR(dev->base)) > return PTR_ERR(dev->base); > @@ -245,8 +257,6 @@ static int at91_twi_probe(struct platform_device *pdev) > dev->adapter.timeout = AT91_I2C_TIMEOUT; > dev->adapter.dev.of_node = pdev->dev.of_node; > > - dev->slave_detected = i2c_detect_slave_mode(&pdev->dev); > - > if (dev->slave_detected) > rc = at91_twi_probe_slave(pdev, phy_addr, dev); > else > 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? > struct at91_twi_pdata { > unsigned clk_max_div; > unsigned clk_offset; > + unsigned slave_mode_features; > bool has_unre_flag; > bool has_alt_cmd; > bool has_hold_field; > -- > 2.7.4 >
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
On Wed, Nov 01, 2017 at 12:16:36PM +0100, Juergen Fitschen wrote: > 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? > Not really, I'll discuss with the hardware guy to see how we can manage NACKs or at least how we can improve it for next versions of the controller. Regards Ludovic > 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
Hi Juergen, On Wed, Nov 01, 2017 at 12:16:36PM +0100, Juergen Fitschen wrote: > 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? > After discussing with the hardware guy, I confirm that we can't NACK the byte we have just received. From his point of view and according to the i2c specification it's not something that can be handled: "On the byte level, a device may be able to receive bytes of data at a fast rate, but needs more time to store a received byte or prepare another byte to be transmitted. Slaves can then hold the SCL line LOW after reception and acknowledgment of a byte to force the master into a wait state until the slave is ready for the next byte transfer in a type of handshake procedure (see Figure 7)." From the Clock stretching section. Since the clock stretching is only allowed after the acknowledgment, we won't have time to change the ACK value for the byte we have just received. I think using NACKs delayed by 1 byte is the only solution. Using FIFOs should not be recommended for slave mode since it will delay the NACKs in an unpredictable way, it'll depend on FIFOs content. Regards Ludovic > 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
Hello Ludovic, On Fri, Nov 03, 2017 at 09:46:02AM +0100, Ludovic Desroches wrote: > > > > 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? > > > > After discussing with the hardware guy, I confirm that we can't NACK the > byte we have just received. From his point of view and according to the > i2c specification it's not something that can be handled: > > "On the byte level, a device may be able to receive bytes of data at a fast rate, but needs > more time to store a received byte or prepare another byte to be transmitted. Slaves can > then hold the SCL line LOW after reception and acknowledgment of a byte to force the > master into a wait state until the slave is ready for the next byte transfer in a type of > handshake procedure (see Figure 7)." From the Clock stretching section. > > Since the clock stretching is only allowed after the acknowledgment, we > won't have time to change the ACK value for the byte we have just > received. Thank you very much for being this investigative! I haven't known that clock stretching isn't allowed everywhere in a transmission. Maybe that's a little flaw in the slave mode interface. It forces hardware to violate the I2C standard due to possible waiting time until the software tells the I2C interface whether to ACK or to NACK. > > I think using NACKs delayed by 1 byte is the only solution. Using FIFOs > should not be recommended for slave mode since it will delay the NACKs > in an unpredictable way, it'll depend on FIFOs content. > I'm going to implement this in the next version of the patch. I already found a bug in the IRQ handler. So please wait with further testing until I released the next version of the patchset. I really do not want to waste your time with a buggy driver. Further explanation of the bug will follow next week, Thanks again and best regards Juergen
On Fri, Nov 03, 2017 at 03:07:32PM +0100, Juergen Fitschen wrote: > Hello Ludovic, > > On Fri, Nov 03, 2017 at 09:46:02AM +0100, Ludovic Desroches wrote: > > > > > 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? > > > > > > > After discussing with the hardware guy, I confirm that we can't NACK the > > byte we have just received. From his point of view and according to the > > i2c specification it's not something that can be handled: > > > > "On the byte level, a device may be able to receive bytes of data at a fast rate, but needs > > more time to store a received byte or prepare another byte to be transmitted. Slaves can > > then hold the SCL line LOW after reception and acknowledgment of a byte to force the > > master into a wait state until the slave is ready for the next byte transfer in a type of > > handshake procedure (see Figure 7)." From the Clock stretching section. > > > > Since the clock stretching is only allowed after the acknowledgment, we > > won't have time to change the ACK value for the byte we have just > > received. > > Thank you very much for being this investigative! I haven't known that clock > stretching isn't allowed everywhere in a transmission. Maybe that's a little > flaw in the slave mode interface. It forces hardware to violate the I2C standard > due to possible waiting time until the software tells the I2C interface whether > to ACK or to NACK. > It seems but I am surprised that the slave framework may rely on a violation of the I2C standard. Maybe it depends on the way the specification is interpreted. It says that slaves can hold the SCL line low after recepetion and acknowledgment but it doesn't says that it can't before... Wolfram, what's your mind about that? > > > > I think using NACKs delayed by 1 byte is the only solution. Using FIFOs > > should not be recommended for slave mode since it will delay the NACKs > > in an unpredictable way, it'll depend on FIFOs content. > > > > I'm going to implement this in the next version of the patch. I already found a > bug in the IRQ handler. So please wait with further testing until I released the > next version of the patchset. I really do not want to waste your time with a > buggy driver. Further explanation of the bug will follow next week, > Ok, thanks for the work you're doing. Regards Ludovic
diff --git a/drivers/i2c/busses/i2c-at91-core.c b/drivers/i2c/busses/i2c-at91-core.c index 3d7287c..9bf1e9d 100644 --- a/drivers/i2c/busses/i2c-at91-core.c +++ b/drivers/i2c/busses/i2c-at91-core.c @@ -69,6 +69,7 @@ void at91_init_twi_bus(struct at91_twi_dev *dev) static struct at91_twi_pdata at91rm9200_config = { .clk_max_div = 5, .clk_offset = 3, + .slave_mode_features = 0, .has_unre_flag = true, .has_alt_cmd = false, .has_hold_field = false, @@ -77,6 +78,7 @@ static struct at91_twi_pdata at91rm9200_config = { static struct at91_twi_pdata at91sam9261_config = { .clk_max_div = 5, .clk_offset = 4, + .slave_mode_features = AT91_TWI_SM_AVAILABLE, .has_unre_flag = false, .has_alt_cmd = false, .has_hold_field = false, @@ -85,6 +87,7 @@ static struct at91_twi_pdata at91sam9261_config = { static struct at91_twi_pdata at91sam9260_config = { .clk_max_div = 7, .clk_offset = 4, + .slave_mode_features = AT91_TWI_SM_AVAILABLE, .has_unre_flag = false, .has_alt_cmd = false, .has_hold_field = false, @@ -93,6 +96,7 @@ static struct at91_twi_pdata at91sam9260_config = { static struct at91_twi_pdata at91sam9g20_config = { .clk_max_div = 7, .clk_offset = 4, + .slave_mode_features = AT91_TWI_SM_AVAILABLE, .has_unre_flag = false, .has_alt_cmd = false, .has_hold_field = false, @@ -101,6 +105,7 @@ static struct at91_twi_pdata at91sam9g20_config = { static struct at91_twi_pdata at91sam9g10_config = { .clk_max_div = 7, .clk_offset = 4, + .slave_mode_features = AT91_TWI_SM_AVAILABLE, .has_unre_flag = false, .has_alt_cmd = false, .has_hold_field = false, @@ -131,6 +136,7 @@ static const struct platform_device_id at91_twi_devtypes[] = { static struct at91_twi_pdata at91sam9x5_config = { .clk_max_div = 7, .clk_offset = 4, + .slave_mode_features = AT91_TWI_SM_AVAILABLE, .has_unre_flag = false, .has_alt_cmd = false, .has_hold_field = false, @@ -139,6 +145,7 @@ static struct at91_twi_pdata at91sam9x5_config = { static struct at91_twi_pdata sama5d4_config = { .clk_max_div = 7, .clk_offset = 4, + .slave_mode_features = AT91_TWI_SM_AVAILABLE, .has_unre_flag = false, .has_alt_cmd = false, .has_hold_field = true, @@ -147,6 +154,7 @@ static struct at91_twi_pdata sama5d4_config = { static struct at91_twi_pdata sama5d2_config = { .clk_max_div = 7, .clk_offset = 4, + .slave_mode_features = AT91_TWI_SM_AVAILABLE | AT91_TWI_SM_HAS_FIFO | AT91_TWI_SM_CAN_NACK, .has_unre_flag = true, .has_alt_cmd = true, .has_hold_field = true, @@ -219,6 +227,10 @@ static int at91_twi_probe(struct platform_device *pdev) if (!dev->pdata) return -ENODEV; + dev->slave_detected = i2c_detect_slave_mode(&pdev->dev); + if (dev->slave_detected && dev->pdata->slave_mode_features == 0) + return -EPFNOSUPPORT; + dev->base = devm_ioremap_resource(&pdev->dev, mem); if (IS_ERR(dev->base)) return PTR_ERR(dev->base); @@ -245,8 +257,6 @@ static int at91_twi_probe(struct platform_device *pdev) dev->adapter.timeout = AT91_I2C_TIMEOUT; dev->adapter.dev.of_node = pdev->dev.of_node; - dev->slave_detected = i2c_detect_slave_mode(&pdev->dev); - if (dev->slave_detected) rc = at91_twi_probe_slave(pdev, phy_addr, dev); else 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 */ + struct at91_twi_pdata { unsigned clk_max_div; unsigned clk_offset; + unsigned slave_mode_features; bool has_unre_flag; bool has_alt_cmd; bool has_hold_field;
Some AT91 hardware has no slave mode included or only limited features (i.e. no fifos). Signed-off-by: Juergen Fitschen <me@jue.yt> --- drivers/i2c/busses/i2c-at91-core.c | 14 ++++++++++++-- drivers/i2c/busses/i2c-at91.h | 5 +++++ 2 files changed, 17 insertions(+), 2 deletions(-)