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

From: Peter Rosin
Date: Thu Feb 21 2019 - 08:42:30 EST


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@xxxxxxx>
> ---
>
> 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?

Cheers,
Peter

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