Re: [PATCH v2 2/2] i3c/master: add the mipi-i3c-hci driver

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Nicolas,

On Mon, Oct 05, 2020 at 06:15:59PM -0400, Nicolas Pitre wrote:
> On Thu, 1 Oct 2020, Sakari Ailus wrote:
> 
> > Hi Nicolas,
> 
> Hello Sakari,
> 
> Thanks for your review.
> 
> > Thanks for the patchset. It's a big patch... it might be easier to get
> > reviews by splitting it. Feel free to do that if it's not too hard.
> 
> This is hard.  The only thing that I could split out without too much 
> pain that still can be tested is the DMA support. And there is no 
> bisectability advantage in doing so either.

Note that it does not need to be functional nor even compiled. Up to you
entirely.

> 
> > Please see my comments below.
> > 
> > On Tue, Aug 18, 2020 at 11:17:23PM -0400, Nicolas Pitre wrote:
> > > From: Nicolas Pitre <npitre@baylibre.com>
> > > +/* Data Transfer Speed and Mode */
> > > +enum hci_cmd_mode {
> > > +	MODE_I3C_SDR0		= 0x0,
> > > +	MODE_I3C_SDR1		= 0x1,
> > > +	MODE_I3C_SDR2		= 0x2,
> > > +	MODE_I3C_SDR3		= 0x3,
> > > +	MODE_I3C_SDR4		= 0x4,
> > > +	MODE_I3C_HDR_TSx	= 0x5,
> > > +	MODE_I3C_HDR_DDR	= 0x6,
> > > +	MODE_I3C_HDR_BT		= 0x7,
> > > +	MODE_I3C_Fm_FmP		= 0x8,
> > > +	MODE_I2C_Fm		= 0x0,
> > > +	MODE_I2C_FmP		= 0x1,
> > > +	MODE_I2C_UD1		= 0x2,
> > > +	MODE_I2C_UD2		= 0x3,
> > > +	MODE_I2C_UD3		= 0x4,
> > 
> > I wonder if there's something else that separates I3C and I²C modes. Should
> > the two be in a different enum?
> 
> There is only that I2C bit in the Device Address Table. Still, those 
> mode values are stored in the same register field so there is no real 
> advantage from splitting them.

Ok, fair enough.

> 
> > > +/* TID generation */
> > > +#define hci_get_tid(bits) \
> > > +	(atomic_inc_return_relaxed(&hci->next_cmd_tid) % (1 << (bits)))
> > 
> > 1U. And you don't need a modulo here, simple bitwise and is more efficient.
> 
> Good point about 1U. However the compiler is smart enough to convert the 
> modulus into a bitwise "and" in the generated assembly.

I guess it depends on the compiler. Still the result of shifting 1 to the
signed bit is not defined. It might not happen in this driver but using
unsigned value there is a good practice.

> 
> > > +/* Our various instances */
> > > +extern const struct hci_cmd_ops i3c_hci_cmd_v1;
> > > +extern const struct hci_cmd_ops i3c_hci_cmd_v2;
> > 
> > As these are global symbols, I'd add a prefix such as "mipi_" to these.
> 
> Won't make a difference in the modular case... but fair enough.

The driver may be linked directly to the kernel, too.

> 
> > > +// SPDX-License-Identifier: BSD-3-Clause
> > 
> > Please read Documentation/process/license-rules.rst . IOW, BSD 3-clause
> > license alone is not one of the acceptable licenses. I also don't see a
> > need for dual licensing.
> 
> Really?
> 
> I did read Documentation/process/license-rules.rst obviously.
> 
> Let's have a look again. From that document:
> 
> |The licenses currently used, as well as the licenses for code added to the
> |kernel, can be broken down into:
> |
> |1. _`Preferred licenses`:
> |
> |   Whenever possible these licenses should be used as they are known to be
> |   fully compatible and widely used.  These licenses are available from the
> |   directory::
> |
> |      LICENSES/preferred/
> |
> |   in the kernel source tree.
> 
> Incidentally, the file LICENSES/preferred/BSD-3-Clause can be found 
> there. And it contains:
> 
> |  To use the BSD 3-clause "New" or "Revised" License put the following SPDX
> |  tag/value pair into a comment according to the placement guidelines in
> |  the licensing rules documentation:
> |    SPDX-License-Identifier: BSD-3-Clause
> 
> There is indeed a mention in license-rules.rst that suggests: 
> "individual files can be provided under a dual license, e.g. one of the 
> compatible GPL variants and alternatively under a permissive license 
> like BSD, MIT etc." It says *can* not *must*. In fact, there is a 
> section explicitly for licenses that may only be used in a dual-license 
> setup:
> 
> |3. Dual Licensing Only
> |
> |   These licenses should only be used to dual license code with another
> |   license in addition to a preferred license.  These licenses are available
> |   from the directory::
> |
> |      LICENSES/dual/
> |
> |   in the kernel source tree.
> 
> And no BSD license is to be found there.
> 
> If still in doubt, let's see what exists in practice:
> 
> $ git grep "SPDX-License-Identifier: BSD-3-Clause\($\| \*/\)" drivers/
> drivers/crypto/talitos.h:/* SPDX-License-Identifier: BSD-3-Clause */
> drivers/firmware/ti_sci.h:/* SPDX-License-Identifier: BSD-3-Clause */
> drivers/net/dsa/sja1105/sja1105_clocking.c:// SPDX-License-Identifier: BSD-3-Clause
> drivers/net/dsa/sja1105/sja1105_sgmii.h:/* SPDX-License-Identifier: BSD-3-Clause */
> drivers/net/dsa/sja1105/sja1105_spi.c:// SPDX-License-Identifier: BSD-3-Clause
> drivers/net/dsa/sja1105/sja1105_static_config.c:// SPDX-License-Identifier: BSD-3-Clause
> drivers/net/dsa/sja1105/sja1105_static_config.h:/* SPDX-License-Identifier: BSD-3-Clause */
> drivers/remoteproc/omap_remoteproc.h:/* SPDX-License-Identifier: BSD-3-Clause */
> drivers/staging/greybus/audio_apbridgea.h:/* SPDX-License-Identifier: BSD-3-Clause */
> drivers/staging/greybus/tools/lbtest:# SPDX-License-Identifier: BSD-3-Clause
> drivers/staging/greybus/tools/loopback_test.c:// SPDX-License-Identifier: BSD-3-Clause
> drivers/usb/serial/keyspan_usa26msg.h:/* SPDX-License-Identifier: BSD-3-Clause */
> drivers/usb/serial/keyspan_usa28msg.h:/* SPDX-License-Identifier: BSD-3-Clause */
> drivers/usb/serial/keyspan_usa49msg.h:/* SPDX-License-Identifier: BSD-3-Clause */
> drivers/usb/serial/keyspan_usa67msg.h:/* SPDX-License-Identifier: BSD-3-Clause */
> drivers/usb/serial/keyspan_usa90msg.h:/* SPDX-License-Identifier: BSD-3-Clause */
> 
> So there are couple precedents already.

These look more like accidents rather than informed decisions to merge
BSD-only licensed code. I'd still say no.

> > > +#define CMD_I1_DATA_BYTE_4(v)		FIELD_PREP(W1_MASK(63, 56), v)
> > 
> > I'm not sure W*_MASK() macros are helpful here. I'd just use plain
> > FIELD_PREP() and BIT(). The digit after I in the macro name already signals
> > the position.
> 
> Those macros are very helpful. The spec is written in terms of bit 
> numbers from 0 through 127. Manually converting those into the 32-bit 
> word that the hardware interface uses is error prone.

Ah, if it's in the spec, then it indeed helps to keep them the same. Feel
free to keep as-is.

> > > +static int hci_cmd_v1_prep_ccc(struct i3c_hci *hci,
> > > +			       struct hci_xfer *xfer,
> > > +			       u8 ccc_addr, u8 ccc_cmd, bool raw)
> > > +{
> > > +	u_int dat_idx = 0;
> > 
> > Please use unsigned int instead. Same elsewhere.
> 
> Why?

Because nobody else uses it and it expands to a standard type anyway. The
comment in types.h suggests it comes from BSDs. So there's no reason to use
it in new kernel code.

> > > +	xfer->cmd_tid = hci_get_tid(4);
> > 
> > Why 4 and e.g. not 7? A macro with a descriptive name would be nice.
> 
> The macro is:
> 
> #define hci_get_tid(bits)
> 
> but yeah... it could be in the macro name as well.
> 
> In fact, there is no longer 3-bits TID fields. They're all 4-bits wide 
> now. So I simply removed the macro argument instead.

Ok.

> > > +		DBG("next_addr = 0x%02x, DAA using DAT %d", next_addr, dat_idx);
> > 
> > dev_dbg() perhaps? Same elsewhere.
> 
> Nah... Given all the needed arguments and the function name prefix I 
> want, the dev_dbg() ended up spanning 3 lines whereas the DBG() wrapper 
> takes only one in most cases.

Possibly so, but creating your own debug infrastructure where it already
exists does not look like a great idea. For instance, the DBG macro does
not use the device whereas the rest assume that hci is your host controller
struct pointer. If nothing else, it's simply ugly.

> 
> > > +	}
> > > +
> > > +	if (dat_idx >= 0)
> > > +		i3c_hci_dat_free_entry(hci, dat_idx);
> > > +	hci_free_xfer(xfer, 1);
> > 
> > A newline before a lone return would be nice.
> > 
> > > +	return ret;
> 
> What do you mean? It is part of the cleanup and exit block which doesn't 
> make it lone anyway.

It just looks better that way. But feel free to keep it as-is.

> > > +#if 0
> > > +	if (ccc->rnw) {
> > > +		HEXDUMP("got: ", ccc->dests[0].payload.data,
> > > +				 ccc->dests[0].payload.len);
> > > +	}
> > 
> > This should probably be removed.
> 
> Well, after Boris's suggestion, I turned it into this:
> 
> 	if (ccc->rnw)
> 		DBG("got: %*ph",
> 			ccc->dests[0].payload.len, ccc->dests[0].payload.data);
> 
> But yes, this should go eventually. Not yet though.

That looks better.

> > > +		return -EPROTONOSUPPORT;
> > 
> > I'd just use -EINVAL.
> 
> -EINVAL is widely used already. Let's try to report more precise errors 
> when we can.

Ok.

-- 
Kind regards,

Sakari Ailus

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c