Re: [PATCH v2] Adding i2c-cp2615: i2c support for Silicon Labs' CP2615 Digital Audio Bridge

[Bence Csókás]

Thanks for the lightning quick response!

Wolfram Sang <wsa@xxxxxxxxxx> ezt írta (időpont: 2021. márc. 17., Sze, 13:34):
>
> On Wed, Mar 17, 2021 at 10:30:21AM +0000, Bence Csókás wrote:
> > Signed-off-by: Bence Csókás <bence98@xxxxxxxxxx>
>
> Thanks, this looks good now and I think we are very close.
>
> > ---
>
> Next, time please provide a small summary of changes since last version.
> I get enough patches that it becomes confusing otherwise.
>

You are right, sorry, I am still familiarizing myself with `git send-email`

> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-cp2615.c
> > @@ -0,0 +1,282 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
> If you want GPL v2 only, then you need to say in MODULE_LICENSE also
> "GPL v2".
>

GPLv2 or later is fine by me. If I change this to "//
SPDX-License-Identifier: GPL-2.0-or-later", is that OK?

> > +enum cp2615_iop_msg_type {
> > +     iop_GetAccessoryInfo = 0xD100,
> > +     iop_AccessoryInfo = 0xA100,
> > +     iop_GetPortConfiguration = 0xD203,
> > +     iop_PortConfiguration = 0xA203,
> > +     // ...
>
> This comment can go?

Sorry, this slipped in from before... I shall remove it.


> > +/*
> > + * This chip has some limitations: one is that the USB endpoint
> > + * can only receive 64 bytes/transfer, that leaves 54 bytes for
> > + * the I2C transfer. On top of that, EITHER read_len OR write_len
> > + * may be zero, but not both. If both are non-zero, the adapter
> > + * issues a write followed by a read. And the chip does not
> > + * support repeated START between the write and read phases.
>
> Good and useful paragraph!

Thank you!

>
> > + * FIXME: There in no quirk flag for specifying that the adapter
> > + * does not support empty transfers, or that it cannot emit a
>
> Can't we use I2C_AQ_NO_ZERO_LEN here?

I thought that meant the adapter cannot handle NEITHER zero-length
reads NOR writes, but the CP2615 can do a zero read combined with a
non-zero write or the other way around, just both cannot be zero. If
both are zero, the chip just ignores the request, as I've learned from
a very confusing situation with `i2cdetect`.

>
> > + * START condition between the combined phases.
>
> True! But it makes sense, so we can fix that. We just need to add
> I2C_AQ_NO_REP_START and a short explanation to i2c.h. If you want, you
> can do it in a seperate patch. I can do it, too, if you prefer.

Sure! I should just define it as BIT(7) or something, right? Should I
do it in a completely different patchset, or is it OK if I submit it
as the 2/2 of PATCH v3? Are there maybe other adapters that would be
affected?

> Maybe skip the defines for VID and PID and use the values directly?
> I am not a USB expert, not really sure what the consistent way is.

I think this is how they usually do it, or at least from what I've seen.

>
> So, this and the checkpatch issues and I think we are done.
>
> Thanks,
>
>    Wolfram
>

Thanks,
Bence