RE: [PATCH] drivers: mux: Generic register bitfield-based multiplexer driver

[Leo Li]

> -----Original Message-----
> From: Peter Rosin <peda@xxxxxxxxxx>
> Sent: Monday, February 18, 2019 5:39 PM
> To: Leo Li <leoyang.li@xxxxxxx>; Pankaj Bansal <pankaj.bansal@xxxxxxx>;
> linux-kernel@xxxxxxxxxxxxxxx; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH] drivers: mux: Generic register bitfield-based
> multiplexer driver
> 
> On 2019-02-18 22:07, Leo Li wrote:
> > From: Peter Rosin <peda@xxxxxxxxxx>
> >> On 2019-02-18 11:20, Pankaj Bansal wrote:
> >>> From: Peter Rosin [mailto:peda@xxxxxxxxxx]
> >>>> 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 agree that we should avoid the duplication.

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

I don't think that it is hard to maintain the backward compatibility with the rename.  The updated driver can keep handling the "mmio-mux" device tree compatible string.  And we can also have MUX_MMIO selects the new MUX_REGMAP if we want to keep the compatibility with old kernel config file.

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