[PATCH v2 2/2] i3c/master: add the mipi-i3c-hci driver
Sakari Ailus
sakari.ailus at iki.fi
Wed Oct 7 06:17:18 EDT 2020
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 at 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
More information about the linux-i3c
mailing list