Re: [PATCH v3 2/2] drivers: mux: Add Generic regmap bitfield-based multiplexer in mmio-mux
On 2019-02-26 07:08, Pankaj Bansal wrote:
>>> + 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
Irrelevant, a great many non-standard features are used in the kernel.
> 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_case=auto=true=true
Yes, a local grep suggests that there are plenty of instances in the
kernel. In fact, there are multiple instances even in the kernel/
directory so it's not some fringe thing. My conclusion is that it
makes for readable code and is perfectly safe to use in the kernel,
which apparently does not care about compilers that do not understand
the construct.
$ git grep '?:' *.[ch] | wc
7734830 54975
I you still don't buy that and don't want to sign-off on it, then
write
ret = IS_ERR(regmap) ? PTR_ERR(regmap) : -ENODEV;
But that's just wordy and less readable, methinks.
However, the best thing to do, according to your maintainer, is still
the original suggestion to fixup the NULL as early as possible with
regmap = dev_get_regmap(dev->parent, NULL) ?: ERR_PTR(-ENODEV);
And leave the error handling block alone. Can you please just do that
and be done with it?
Cheers,
Peter
RE: [PATCH v3 2/2] drivers: mux: Add Generic regmap bitfield-based multiplexer in mmio-mux
Hi Peter,
> -Original Message-
> From: Peter Rosin [mailto:p...@axentia.se]
> Sent: Monday, 25 February, 2019 08:14 PM
> To: Pankaj Bansal ; Leo Li
> Cc: linux-kernel@vger.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
> > ---
> >
> > 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_case=auto=true=true
>
> Cheers,
> Peter
>
> > dev_err(dev, "failed to get regmap: %d\n", ret);
> > return ret;
> > }
> >
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
> ---
>
> 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;
Cheers,
Peter
> dev_err(dev, "failed to get regmap: %d\n", ret);
> return ret;
> }
>
