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

[Peter Rosin]

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);
>