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] [day] [month] [year] [list]

Message-ID: <f48947f4-ec86-6877-9c88-414cb0f4dca9@axentia.se>
Date:   Thu, 21 Feb 2019 16:06:36 +0000
From:   Peter Rosin <peda@...ntia.se>
To:     Pankaj Bansal <pankaj.bansal@....com>, 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

On 2019-02-21 15:17, Pankaj Bansal wrote:
> 
> 
>> -----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 ?

I discarded that since you then have to specialize the error handling
to not just do "ret = PTR_ERR(regmap);"

But if you'd like to do something other than my minimal fix, then feel free
to make a proposal...

Cheers,
Peter

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