linux-i3c.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Nicolas Pitre <nico@fluxnic.net>
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: Laura Nixon <laura.nixon@team.mipi.org>,
	linux-i3c@lists.infradead.org,
	Boris Brezillon <boris.brezillon@collabora.com>,
	Matthew Schnoor <matthew.schnoor@intel.com>,
	Robert Gough <robert.gough@intel.com>
Subject: Re: [PATCH v2 2/2] i3c/master: add the mipi-i3c-hci driver
Date: Wed, 7 Oct 2020 12:30:59 -0400 (EDT)	[thread overview]
Message-ID: <nycvar.YSQ.7.78.906.2010071129120.2184@knanqh.ubzr> (raw)
In-Reply-To: <20201007101718.GA6413@valkosipuli.retiisi.org.uk>

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

  reply	other threads:[~2020-10-07 16:31 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-19  3:17 [PATCH v2 1/2] MIPI I3c HCI (Host Controller Interface) driver Nicolas Pitre
2020-08-19  3:17 ` [PATCH v2 1/2] dt-bindings: i3c: MIPI I3C Host Controller Interface Nicolas Pitre
2020-08-25 21:29   ` Rob Herring
2020-08-25 22:02     ` Nicolas Pitre
2020-08-25 23:06       ` Rob Herring
2020-08-26  0:40         ` Nicolas Pitre
2020-08-19  3:17 ` [PATCH v2 2/2] i3c/master: add the mipi-i3c-hci driver Nicolas Pitre
2020-10-01 12:31   ` Sakari Ailus
2020-10-05 22:15     ` Nicolas Pitre
2020-10-07 10:17       ` Sakari Ailus
2020-10-07 16:30         ` Nicolas Pitre [this message]
2020-10-09 12:01           ` Sakari Ailus
2020-08-19  3:21 ` [PATCH v2 1/2] MIPI I3c HCI (Host Controller Interface) driver Nicolas Pitre

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.YSQ.7.78.906.2010071129120.2184@knanqh.ubzr \
    --to=nico@fluxnic.net \
    --cc=boris.brezillon@collabora.com \
    --cc=laura.nixon@team.mipi.org \
    --cc=linux-i3c@lists.infradead.org \
    --cc=matthew.schnoor@intel.com \
    --cc=robert.gough@intel.com \
    --cc=sakari.ailus@iki.fi \
    --subject='Re: [PATCH v2 2/2] i3c/master: add the mipi-i3c-hci driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).