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

Powered by blists - more mailing lists