Re: [PATCH RFC 4/4] i2c: at91: take slave mode capabilities of hardware into account

From: Juergen Fitschen <me@jue.yt>
To: Ludovic Desroches <ludovic.desroches@microchip.com>
Cc: Wolfram Sang <wsa@the-dreams.de>,
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC 4/4] i2c: at91: take slave mode capabilities of hardware into account
Date: Wed, 1 Nov 2017 12:16:36 +0100	[thread overview]
Message-ID: <20171101111636.GA17565@jfi-dev> (raw)
In-Reply-To: <20171031152250.4oasjhfpya5rmzyv@rfolt0960.corp.atmel.com>

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