linux-kernel - RE: [PATCH v2] drivers: mux: Add Generic regmap bitfield-based multiplexer in mmio-mux

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: <VI1PR0401MB24962C5D61CB8B4BA08DA491F17E0@VI1PR0401MB2496.eurprd04.prod.outlook.com>
Date:   Thu, 21 Feb 2019 14:17:17 +0000
From:   Pankaj Bansal <pankaj.bansal@....com>
To:     Peter Rosin <peda@...ntia.se>, Leo Li <leoyang.li@....com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] drivers: mux: Add Generic regmap bitfield-based
 multiplexer in mmio-mux



> -----Original Message-----
> From: Peter Rosin [mailto:peda@...ntia.se]
> Sent: Thursday, 21 February, 2019 07:12 PM
> To: Pankaj Bansal <pankaj.bansal@....com>; Leo Li <leoyang.li@....com>
> Cc: linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v2] drivers: mux: Add Generic regmap bitfield-based
> multiplexer in mmio-mux
> 
> Hi!
> 
> Much better, thanks! One nit below:
> 
> On 2019-02-21 13:48, Pankaj Bansal wrote:
> > Generic register bitfield-based multiplexer that controls the
> > multiplexer producer defined under a parent node.
> > The driver corresponding to parent node provides register read/write
> > capabilities.
> >
> > Signed-off-by: Pankaj Bansal <pankaj.bansal@....com>
> > ---
> >
> > Notes:
> >     V2:
> >     - removed seperate driver regmap.c and added the regmap function in
> mmio.c
> >       based on compatible field, the syscon or regmap function would be called
> >     - Modified the KConfig as per Peter's comments
> >
> >  drivers/mux/Kconfig | 12 ++++++------  drivers/mux/mmio.c  |  6
> > +++++-
> >  2 files changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig index
> > 7659d6c5f718..e5c571fd232c 100644
> > --- a/drivers/mux/Kconfig
> > +++ b/drivers/mux/Kconfig
> > @@ -46,14 +46,14 @@ config MUX_GPIO
> >  	  be called mux-gpio.
> >
> >  config MUX_MMIO
> > -	tristate "MMIO register bitfield-controlled Multiplexer"
> > -	depends on (OF && MFD_SYSCON) || COMPILE_TEST
> > +	tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
> > +	depends on OF || COMPILE_TEST
> >  	help
> > -	  MMIO register bitfield-controlled Multiplexer controller.
> > +	  MMIO/Regmap register bitfield-controlled Multiplexer controller.
> >
> > -	  The driver builds multiplexer controllers for bitfields in a syscon
> > -	  register. For N bit wide bitfields, there will be 2^N possible
> > -	  multiplexer states.
> > +	  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.
> > diff --git a/drivers/mux/mmio.c b/drivers/mux/mmio.c index
> > 935ac44aa209..37fbcde7f1fc 100644
> > --- a/drivers/mux/mmio.c
> > +++ b/drivers/mux/mmio.c
> > @@ -28,6 +28,7 @@ static const struct mux_control_ops mux_mmio_ops = {
> >
> >  static const struct of_device_id mux_mmio_dt_ids[] = {
> >  	{ .compatible = "mmio-mux", },
> > +	{ .compatible = "reg-mux", },
> >  	{ /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, mux_mmio_dt_ids); @@ -43,7 +44,10 @@
> static
> > int mux_mmio_probe(struct platform_device *pdev)
> >  	int ret;
> >  	int i;
> >
> > -	regmap = syscon_node_to_regmap(np->parent);
> > +	if (of_device_is_compatible(np, "mmio-mux"))
> > +		regmap = syscon_node_to_regmap(np->parent);
> > +	else
> > +		regmap = dev_get_regmap(dev->parent, NULL);
> 
> dev_get_regmap() returns NULL on failure, so I think you need something like:
> 
> 		regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-
> EINVAL);
> 
> Or perhaps -ENODEV, -ENOTSUPP or something, not sure?

How about IS_ERR_OR_NULL just below this, where right now IS_ERR is being used ?

> 
> Cheers,
> Peter
> 
> >  	if (IS_ERR(regmap)) {
> >  		ret = PTR_ERR(regmap);
> >  		dev_err(dev, "failed to get regmap: %d\n", ret);
> >