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

[Nicolas Pitre <nico@fluxnic.net>]

On Wed, 7 Oct 2020, Sakari Ailus wrote:

> Hi Nicolas,
> 
> On Mon, Oct 05, 2020 at 06:15:59PM -0400, Nicolas Pitre wrote:
> > On Thu, 1 Oct 2020, Sakari Ailus wrote:
> > > On Tue, Aug 18, 2020 at 11:17:23PM -0400, Nicolas Pitre wrote:
> > > > +	(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.

Not really. All gcc versions in the last 20 years did it. llvm always 
did it too. That's a trivial optimization that all serious compilers 
implement.

> 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.

Agreed.

> > > > +// 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.

BSD-only licensed code _is_ OK per the rules clearly and 
unambiguously quoted above. It's not a matter of opinion.

That is the license to be used per the policy from the organisation 
sponsoring this work. This even had to go through legal approval before 
I was allowed to post this code here.

So if you don't like BSD being OK then please submit a patch changing 
the rules. If accepted upstream then I'll see for this code to be 
dual-licensed or even made GPL only so to conform to the new rules. 
Otherwise BSD-licensed it stays.

> > > 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.

OK then. I'll grant you that strike against BSD.  ;-)

> > > > +		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.

It's not a debug _infrastructure_. Not even a new one. It is merely a 
convenience wrapper on top of the existing infrastructure that I can 
redefine to whatever suits me best when working on this code, which 
incidentally means _not_ using dev_dbg().

> For instance, the DBG macro does not use the device whereas the rest 
> assume that hci is your host controller struct pointer.

And that's very much on purpose to keep my debug lines shorter.

> If nothing else, it's simply ugly.

Your opinion. Obviously I disagree.

As a compromise I'll remove the other wrappers whose definition is 
unlikely to change.


Nicolas

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