[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20171101111636.GA17565@jfi-dev>
Date: Wed, 1 Nov 2017 12:16:36 +0100
From: Juergen Fitschen <me@....yt>
To: Ludovic Desroches <ludovic.desroches@...rochip.com>
Cc: Wolfram Sang <wsa@...-dreams.de>, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 4/4] i2c: at91: take slave mode capabilities of
hardware into account
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
Powered by blists - more mailing lists