Message-ID: <175a0eda-fca3-018c-2b97-f9634f1600fa@microchip.com>
Date:   Wed, 11 May 2022 09:06:36 +0000
From:   <Claudiu.Beznea@...rochip.com>
To:     <Kavyasree.Kotagiri@...rochip.com>,
        <krzysztof.kozlowski+dt@...aro.org>, <Nicolas.Ferre@...rochip.com>,
        <alexandre.belloni@...tlin.com>, <peda@...ntia.se>
CC:     <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <lee.jones@...aro.org>,
        <linux@...linux.org.uk>, <Manohar.Puri@...rochip.com>,
        <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH v2 4/4] mux: lan966: Add support for flexcom mux
 controller

Hi, Kavyasree,

On 09.05.2022 11:49, Kavyasree Kotagiri wrote:
> LAN966 SoC have 5 flexcoms. Each flexcom has 2 chip-selects.
> For each chip select of each flexcom there is a configuration
> register FLEXCOM_SHARED[0-4]:SS_MASK[0-1]. The width of
> configuration register is 21 because there are 21 shared pins
> on each of which the chip select can be mapped. Each bit of the
> register represents a different FLEXCOM_SHARED pin.
> 
> Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@...rochip.com>
> ---
>  arch/arm/mach-at91/Kconfig  |   2 +
>  drivers/mfd/atmel-flexcom.c |  55 +++++++++++++++-
>  drivers/mux/Kconfig         |  12 ++++
>  drivers/mux/Makefile        |   2 +
>  drivers/mux/lan966-flx.c    | 121 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 191 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mux/lan966-flx.c
> 
> diff --git a/arch/arm/mach-at91/Kconfig b/arch/arm/mach-at91/Kconfig
> index 279810381256..26fb0f4e1b79 100644
> --- a/arch/arm/mach-at91/Kconfig
> +++ b/arch/arm/mach-at91/Kconfig
> @@ -74,6 +74,8 @@ config SOC_LAN966
>  	select DW_APB_TIMER_OF
>  	select ARM_GIC
>  	select MEMORY
> +	select MULTIPLEXER
> +	select MUX_LAN966
>  	help
>  	  This enables support for ARMv7 based Microchip LAN966 SoC family.
>  
> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> index 559eb4d352b6..7cfd0fc3f4f0 100644
> --- a/drivers/mfd/atmel-flexcom.c
> +++ b/drivers/mfd/atmel-flexcom.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/clk.h>
>  #include <dt-bindings/mfd/atmel-flexcom.h>
> +#include <linux/mux/consumer.h>
>  
>  /* I/O register offsets */
>  #define FLEX_MR		0x0	/* Mode Register */
> @@ -28,6 +29,10 @@
>  #define FLEX_MR_OPMODE(opmode)	(((opmode) << FLEX_MR_OPMODE_OFFSET) &	\
>  				 FLEX_MR_OPMODE_MASK)
>  
> +struct atmel_flex_caps {
> +	bool has_flx_mux;
> +};
> +
>  struct atmel_flexcom {
>  	void __iomem *base;
>  	u32 opmode;
> @@ -37,6 +42,7 @@ struct atmel_flexcom {
>  static int atmel_flexcom_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> +	const struct atmel_flex_caps *caps;
>  	struct resource *res;
>  	struct atmel_flexcom *ddata;
>  	int err;
> @@ -76,13 +82,60 @@ static int atmel_flexcom_probe(struct platform_device *pdev)
>  	 */
>  	writel(FLEX_MR_OPMODE(ddata->opmode), ddata->base + FLEX_MR);
>  
> +	caps = of_device_get_match_data(&pdev->dev);
> +	if (!caps) {
> +		dev_err(&pdev->dev, "Could not retrieve flexcom caps\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Flexcom Mux */
> +	if (caps->has_flx_mux && of_property_read_bool(np, "mux-controls")) {
> +		struct mux_control *flx_mux;
> +		struct of_phandle_args args;
> +		int i, count;
> +
> +		flx_mux = devm_mux_control_get(&pdev->dev, NULL);
> +		if (IS_ERR(flx_mux))
> +			return PTR_ERR(flx_mux);
> +
> +		count = of_property_count_strings(np, "mux-control-names");
> +		for (i = 0; i < count; i++) {
> +			err = of_parse_phandle_with_fixed_args(np, "mux-controls", 1, i, &args);
> +			if (err)
> +				break;
> +
> +			err = mux_control_select(flx_mux, args.args[0]);
> +			if (!err) {
> +				mux_control_deselect(flx_mux);
> +			} else {
> +				dev_err(&pdev->dev, "Failed to select FLEXCOM mux\n");
> +				return err;
> +			}
> +		}
> +	}
> +

Can you move this in a separate function and take care of disabling the
clock on error cases?

>  	clk_disable_unprepare(ddata->clk);
>  
>  	return devm_of_platform_populate(&pdev->dev);
>  }
>  
> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
> +
> +static const struct atmel_flex_caps lan966x_flexcom_caps = {
> +	.has_flx_mux = true,
> +};
> +
>  static const struct of_device_id atmel_flexcom_of_match[] = {
> -	{ .compatible = "atmel,sama5d2-flexcom" },
> +	{
> +		.compatible = "atmel,sama5d2-flexcom",
> +		.data = &atmel_flexcom_caps,
> +	},
> +
> +	{
> +		.compatible = "microchip,lan966-flexcom",
> +		.data = &lan966x_flexcom_caps,
> +	},
> +
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index e5c571fd232c..ea09f474bc2f 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -45,6 +45,18 @@ config MUX_GPIO
>  	  To compile the driver as a module, choose M here: the module will
>  	  be called mux-gpio.
>  
> +config MUX_LAN966
> +	tristate "LAN966 Flexcom multiplexer"
> +	depends on OF || COMPILE_TEST
> +	help
> +	Lan966 Flexcom Multiplexer controller.
> +
> +	The driver supports mapping 2 chip-selects of each of the lan966
> +	flexcoms to 21 flexcom shared pins.
> +
> +	To compile the driver as a module, choose M here: the module will
> +	be called mux-lan966.

Ussually the help message is aligned 2 spaces on right after help.

> +
>  config MUX_MMIO
>  	tristate "MMIO/Regmap register bitfield-controlled Multiplexer"
>  	depends on OF || COMPILE_TEST
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index 6e9fa47daf56..53a9840d96fa 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -7,10 +7,12 @@ mux-core-objs			:= core.o
>  mux-adg792a-objs		:= adg792a.o
>  mux-adgs1408-objs		:= adgs1408.o
>  mux-gpio-objs			:= gpio.o
> +mux-lan966-objs			:= lan966-flx.o
>  mux-mmio-objs			:= mmio.o
>  
>  obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
>  obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
>  obj-$(CONFIG_MUX_ADGS1408)	+= mux-adgs1408.o
>  obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
> +obj-$(CONFIG_MUX_LAN966)	+= mux-lan966.o
>  obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
> diff --git a/drivers/mux/lan966-flx.c b/drivers/mux/lan966-flx.c
> new file mode 100644
> index 000000000000..2c7dab616a6a
> --- /dev/null
> +++ b/drivers/mux/lan966-flx.c
> @@ -0,0 +1,121 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LAN966 Flexcom MUX driver
> + *
> + * Copyright (c) 2022 Microchip Inc.
> + *
> + * Author: Kavyasree Kotagiri <kavyasree.kotagiri@...rochip.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/mux/driver.h>
> +#include <linux/io.h>
> +
> +#define FLEX_SHRD_MASK		0x1FFFFF

Or you can use GENMASK()

> +#define LAN966_MAX_CS		21
> +
> +static void __iomem *flx_shared_base;

I agree with Peter's point of view on this.

> +struct mux_lan966x {
> +	u32 offset;
> +	u32 ss_pin;
> +};
> +
> +static int mux_lan966x_set(struct mux_control *mux, int state)
> +{
> +	struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
> +	u32 val;
> +
> +	val = ~(1 << mux_lan966x[state].ss_pin) & FLEX_SHRD_MASK;
> +	writel(val, flx_shared_base + mux_lan966x[state].offset);

writel_relaxed should be enough here.

> +
> +	return 0;
> +}
> +
> +static const struct mux_control_ops mux_lan966x_ops = {
> +	.set = mux_lan966x_set,
> +};
> +
> +static const struct of_device_id mux_lan966x_dt_ids[] = {
> +	{ .compatible = "microchip,lan966-flx-mux", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mux_lan966x_dt_ids);
> +
> +static int mux_lan966x_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct mux_lan966x *mux_lan966x;
> +	struct mux_chip *mux_chip;
> +	int ret, num_fields, i;
> +
> +	ret = of_property_count_u32_elems(np, "mux-offset-pin");
> +	if (ret == 0 || ret % 2)
> +		ret = -EINVAL;
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "mux-offset-pin property missing or invalid");
> +	num_fields = ret / 2;
> +
> +	mux_chip = devm_mux_chip_alloc(dev, num_fields, sizeof(*mux_lan966x));
> +	if (IS_ERR(mux_chip))
> +		return dev_err_probe(dev, PTR_ERR(mux_chip),
> +				     "failed to allocate mux_chips\n");
> +
> +	mux_lan966x = mux_chip_priv(mux_chip);
> +
> +	flx_shared_base = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> +	if (IS_ERR(flx_shared_base))
> +		return dev_err_probe(dev, PTR_ERR(flx_shared_base),
> +				     "failed to get flexcom shared base address\n");
> +
> +	for (i = 0; i < num_fields; i++) {
> +		struct mux_control *mux = &mux_chip->mux[i];
> +		u32 offset, shared_pin;
> +
> +		ret = of_property_read_u32_index(np, "mux-offset-pin",
> +						 2 * i, &offset);
> +		if (ret == 0)
> +			ret = of_property_read_u32_index(np, "mux-offset-pin",
> +							 2 * i + 1,
> +							 &shared_pin);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "failed to read mux-offset-pin property: %d", i);
> +
> +		if (shared_pin >= LAN966_MAX_CS)
> +			return -EINVAL;
> +
> +		mux_lan966x[i].offset = offset;
> +		mux_lan966x[i].ss_pin = shared_pin;
> +
> +		mux->states = LAN966_MAX_CS;
> +	}
> +
> +	mux_chip->ops = &mux_lan966x_ops;
> +
> +	ret = devm_mux_chip_register(dev, mux_chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

you can just return ret; or return devm_mux_chip_register(dev, mux_chip);

> +}
> +
> +static struct platform_driver mux_lan966x_driver = {
> +	.driver = {
> +		.name = "lan966-mux",
> +		.of_match_table	= of_match_ptr(mux_lan966x_dt_ids),
> +	},
> +	.probe = mux_lan966x_probe,
> +};
> +
> +module_platform_driver(mux_lan966x_driver);
> +
> +MODULE_DESCRIPTION("LAN966 Flexcom multiplexer driver");
> +MODULE_AUTHOR("Kavyasree Kotagiri <kavyasree.kotagiri@...rochip.com>");
> +MODULE_LICENSE("GPL v2");

Last time I checked checkpatch.pl complained about using "GPL v2" string
here and suggested to use "GPL".

> +

Powered by blists - more mailing lists