linux-kernel - Re: [PATCH] drivers: mux: Generic register bitfield-based multiplexer driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bfe3fd9c-7375-1c53-5692-7cdea3c9b670@axentia.se>
Date:   Mon, 18 Feb 2019 23:38:31 +0000
From:   Peter Rosin <peda@...ntia.se>
To:     Leo Li <leoyang.li@....com>, Pankaj Bansal <pankaj.bansal@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>
Subject: Re: [PATCH] drivers: mux: Generic register bitfield-based multiplexer
 driver

On 2019-02-18 22:07, Leo Li wrote:
> From: Peter Rosin <peda@...ntia.se>
>> On 2019-02-18 11:20, Pankaj Bansal wrote:
>>> From: Peter Rosin [mailto:peda@...ntia.se]
>>>> Anyway, I would prefer if you could extend drivers/mux/mmio.c to
>>>> support both compatibles, and using the compatible to select if
>>>>
>>>> 	regmap = syscon_node_to_regmap(np->parent);
>>>>
>>>> or
>>>>
>>>> 	regmap = dev_get_regmap(dev->parent, NULL);
>>>>
>>>> is called to get to the desired regmap.
>>>
>>> This can be done. The name mmio.c however suggests that mux is
>>> controlled by a Memory mapped device.
>>> IMO, if the generic regmap API is to be added to it, the name needs to
>>> changed. Any suggestions ?
>>
>> Just keep the driver name as is, there is no shortage of drivers supporting
>> more than one thing...
> 
> You are right that a lot of drivers support multiple functions. But
> the problem here is that the current name mmio is only a subset of
> what the updated driver will handle, which can create confusion.

I refuse the duplication. This new driver is doing the exact same
thing (-ish) as the old one. Having the same code in two places is just
a recipe for future divergence when everyone have forgotten that
there are two nearly identical drivers that both need patching. Stating
this in a comment somewhere in the drivers will not help all that much
in my experience. The comment will be missing from the context in some
seemingly trivial patch, and there you go. There *will* be weed down
the line, if duplication is allowed.

I can agree that mux-regmap.c and CONFIG_MUX_REGMAP would have been
better names, but mux-mmio is already there. So it is what it is. But
if you can convince me that changing the name will not cause any
trouble anywhere for any existing mux-mmio users, I suppose we can
do a rename. But I bet there will be some nasty corner cases and
odd use cases, so you will have to present strong arguments.

Just update the Kconfig to document the dual nature and remove the
MFD_SYSCON dependency. I suppose you also need to handle the possibly
missing syscon in the .c file, details, details. But something like
this perhaps:

config MUX_MMIO
	tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
	depends on OF || COMPILE_TEST
	help
	  MMIO/Regmap register bitfield-controlled Multiplexer controller.

	  The driver builds multiplexer controllers for bitfields in either
	  a syscon register or a driver regmap register. For N bit wide
	  bitfields, there will be 2^N possible multiplexer states.

	  To compile the driver as a module, choose M here: the module will
	  be called mux-mmio.

Cheers,
Peter