[PATCH v2 2/2] i3c/master: add the mipi-i3c-hci driver
Nicolas Pitre
nico at fluxnic.net
Wed Oct 7 12:30:59 EDT 2020
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
More information about the linux-i3c
mailing list