devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lee Jones <lee@kernel.org>
To: Rob Herring <robh@kernel.org>
Cc: Michal Schmidt <mschmidt@redhat.com>,
	Ivan Vecera <ivecera@redhat.com>,
	netdev@vger.kernel.org,
	Vadim Fedorenko <vadim.fedorenko@linux.dev>,
	Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>,
	Jiri Pirko <jiri@resnulli.us>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Prathosh Satish <Prathosh.Satish@microchip.com>,
	Kees Cook <kees@kernel.org>, Andy Shevchenko <andy@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2 00/14] Add Microchip ZL3073x support (part 1)
Date: Tue, 15 Apr 2025 11:28:51 +0100	[thread overview]
Message-ID: <20250415102851.GW372032@google.com> (raw)
In-Reply-To: <20250411155816.GA3300495-robh@kernel.org>

On Fri, 11 Apr 2025, Rob Herring wrote:

> On Fri, Apr 11, 2025 at 04:27:08PM +0200, Michal Schmidt wrote:
> > On Fri, Apr 11, 2025 at 9:26 AM Lee Jones <lee@kernel.org> wrote:
> > > On Wed, 09 Apr 2025, Ivan Vecera wrote:
> > > > Add support for Microchip Azurite DPLL/PTP/SyncE chip family that
> > > > provides DPLL and PTP functionality. This series bring first part
> > > > that adds the common MFD driver that provides an access to the bus
> > > > that can be either I2C or SPI.
> > > > [...]
> > >
> > > Not only are all of the added abstractions and ugly MACROs hard to read
> > > and troublesome to maintain, they're also completely unnecessary at this
> > > (driver) level.  Nicely authored, easy to read / maintain code wins over
> > > clever code 95% of the time.
> > 
> > Hello Lee,
> > 
> > IMHO defining the registers with the ZL3073X_REG*_DEF macros is both
> > clever and easy to read / maintain. On one line I can see the register
> > name, size and address. For the indexed registers also their count and
> > the stride. It's almost like looking at a datasheet. And the
> > type-checking for accessing the registers using the correct size is
> > nice. I even liked the paranoid WARN_ON for checking the index
> > overflows.
> 
> If this is much better, define (and upstream) something for everyone to 
> use rather than a one-off in some driver. It doesn't matter how great it 
> is if it is different from everyone else. The last thing I want to do is 
> figure out how register accesses work when looking at an unfamilar 
> driver.

Exactly right.  The issue isn't that defining register accesses using
MACROs is a bad idea generally.  I've seen it done before in downstream
BSPs and the like.  It's that this solution isn't following the
conventions already in carved for such things in the Mainline kernel.
To engineers already used to the current conventions, this is much
harder to follow and interact with.

As Rob says, if this is truly better, discuss the idea with a much
wider audience and have it applied broadly across all areas.  We shall
not be bucking the trend or trend setting here in MFD.

-- 
Lee Jones [李琼斯]

      reply	other threads:[~2025-04-15 10:28 UTC|newest]

Thread overview: 65+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-04-09 14:42 [PATCH v2 00/14] Add Microchip ZL3073x support (part 1) Ivan Vecera
2025-04-09 14:42 ` [PATCH v2 01/14] dt-bindings: dpll: Add device tree bindings for DPLL device and pin Ivan Vecera
2025-04-09 14:42 ` [PATCH v2 02/14] dt-bindings: dpll: Add support for Microchip Azurite chip family Ivan Vecera
2025-04-10  7:06   ` Krzysztof Kozlowski
2025-04-10  7:45     ` Ivan Vecera
2025-04-10 13:18       ` Conor Dooley
2025-04-10 13:35         ` Ivan Vecera
2025-04-10 17:07           ` Prathosh.Satish
2025-04-10 17:36             ` Ivan Vecera
2025-04-10 18:36               ` Prathosh.Satish
2025-04-10 17:36             ` Andrew Lunn
2025-04-10 18:33               ` Ivan Vecera
2025-04-10 21:12                 ` Andrew Lunn
2025-04-11  9:56                   ` Ivan Vecera
2025-04-14 17:19                     ` Conor Dooley
2025-04-09 14:42 ` [PATCH v2 03/14] mfd: Add Microchip ZL3073x support Ivan Vecera
2025-04-09 15:43   ` Andy Shevchenko
2025-04-10  7:19     ` Krzysztof Kozlowski
2025-04-10  7:52       ` Ivan Vecera
2025-04-10 17:50         ` Andrew Lunn
2025-04-10 18:36           ` Ivan Vecera
2025-04-09 14:42 ` [PATCH v2 04/14] mfd: zl3073x: Register itself as devlink device Ivan Vecera
2025-04-19 12:40   ` kernel test robot
2025-04-19 14:03   ` kernel test robot
2025-04-09 14:42 ` [PATCH v2 05/14] mfd: zl3073x: Add register access helpers Ivan Vecera
2025-04-09 14:42 ` [PATCH v2 06/14] mfd: zl3073x: Add macros for device registers access Ivan Vecera
2025-04-10  7:17   ` Krzysztof Kozlowski
2025-04-10  8:20     ` Ivan Vecera
2025-04-10 17:53       ` Andy Shevchenko
2025-04-13 10:18         ` Ivan Vecera
2025-04-09 14:42 ` [PATCH v2 07/14] mfd: zl3073x: Add components versions register defs Ivan Vecera
2025-04-10  7:13   ` Krzysztof Kozlowski
2025-04-10  8:26     ` Ivan Vecera
2025-04-10 17:41       ` Andrew Lunn
2025-04-10 18:44         ` Ivan Vecera
2025-04-10 21:54           ` Andrew Lunn
2025-04-15 10:01             ` Ivan Vecera
2025-04-15 11:16               ` Andy Shevchenko
2025-04-15 12:57               ` Andrew Lunn
2025-04-15 14:20                 ` Ivan Vecera
2025-04-10 17:50   ` Andy Shevchenko
2025-04-11 11:19     ` Ivan Vecera
2025-04-11 12:31       ` Andrew Lunn
2025-04-11 13:19         ` Ivan Vecera
2025-04-11 13:17       ` Ivan Vecera
2025-04-13 19:50         ` Andrew Lunn
2025-04-09 14:42 ` [PATCH v2 08/14] mfd: zl3073x: Implement devlink device info Ivan Vecera
2025-04-09 14:42 ` [PATCH v2 09/14] mfd: zl3073x: Add macro to wait for register value bits to be cleared Ivan Vecera
2025-04-09 14:42 ` [PATCH v2 10/14] mfd: zl3073x: Add functions to work with register mailboxes Ivan Vecera
2025-04-09 14:42 ` [PATCH v2 11/14] mfd: zl3073x: Add clock_id field Ivan Vecera
2025-04-09 14:42 ` [PATCH v2 12/14] lib: Allow modules to use strnchrnul Ivan Vecera
2025-04-09 14:42 ` [PATCH v2 13/14] mfd: zl3073x: Load mfg file into HW if it is present Ivan Vecera
2025-04-09 14:42 ` [PATCH v2 14/14] mfd: zl3073x: Fetch invariants during probe Ivan Vecera
2025-04-10  0:17 ` [PATCH v2 00/14] Add Microchip ZL3073x support (part 1) Jakub Kicinski
2025-04-10  9:18   ` Ivan Vecera
2025-04-10 17:26     ` Andrew Lunn
2025-04-10 22:57     ` Jakub Kicinski
2025-04-11  7:45       ` Ivan Vecera
2025-04-10  7:29 ` Lee Jones
2025-04-11  7:26 ` Lee Jones
2025-04-11  8:01   ` Ivan Vecera
2025-04-11 14:27   ` Michal Schmidt
2025-04-11 14:38     ` Michal Schmidt
2025-04-11 15:58     ` Rob Herring
2025-04-15 10:28       ` Lee Jones [this message]

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=20250415102851.GW372032@google.com \
    --to=lee@kernel.org \
    --cc=Prathosh.Satish@microchip.com \
    --cc=akpm@linux-foundation.org \
    --cc=andy@kernel.org \
    --cc=arkadiusz.kubalewski@intel.com \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=ivecera@redhat.com \
    --cc=jiri@resnulli.us \
    --cc=kees@kernel.org \
    --cc=krzk+dt@kernel.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mschmidt@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=robh@kernel.org \
    --cc=vadim.fedorenko@linux.dev \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).