linux-kernel - RE: [PATCH v3 2/2] 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: <VI1PR0401MB24963EE1460E0FF47CDF066CF17B0@VI1PR0401MB2496.eurprd04.prod.outlook.com>
Date:   Tue, 26 Feb 2019 06:08:42 +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 v3 2/2] drivers: mux: Add Generic regmap bitfield-based
 multiplexer in mmio-mux

Hi Peter,

> -----Original Message-----
> From: Peter Rosin [mailto:peda@...ntia.se]
> Sent: Monday, 25 February, 2019 08:14 PM
> To: Pankaj Bansal <pankaj.bansal@....com>; Leo Li <leoyang.li@....com>
> Cc: linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v3 2/2] drivers: mux: Add Generic regmap bitfield-based
> multiplexer in mmio-mux
> 
> On 2019-02-24 09:27, 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:
> >     V3:
> >     - Added the patch in series with device tree binding patch
> >     - Added the NULL return handling for regmap
> >     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  | 10
> > +++++++---
> >  2 files changed, 13 insertions(+), 9 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..cc02155e4644 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,9 +44,12 @@
> static
> > int mux_mmio_probe(struct platform_device *pdev)
> >  	int ret;
> >  	int i;
> >
> > -	regmap = syscon_node_to_regmap(np->parent);
> > -	if (IS_ERR(regmap)) {
> > -		ret = PTR_ERR(regmap);
> > +	if (of_device_is_compatible(np, "mmio-mux"))
> > +		regmap = syscon_node_to_regmap(np->parent);
> > +	else
> > +		regmap = dev_get_regmap(dev->parent, NULL);
> > +	if (IS_ERR_OR_NULL(regmap)) {
> > +		ret = PTR_ERR_OR_ZERO(regmap) ? PTR_ERR(regmap) : -
> ENODEV;
> 
> The above is not correct, this should be better (untested):
> 
> 		ret = PTR_ERR(regmap) ?: -ENODEV;

Omitting the second operand in ternary operator is not standard. https://stackoverflow.com/questions/34559705/ternary-operator-without-the-middle-expression

Although, it *has been* used in kernel in many places
https://livegrep.com/search/linux?q=file%3A%5C.c%24%20%5C%3F%5C%3A&fold_case=auto&regex=true&context=true


> 
> Cheers,
> Peter
> 
> >  		dev_err(dev, "failed to get regmap: %d\n", ret);
> >  		return ret;
> >  	}
> >