On Mon, 09 May 2022, 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@xxxxxxxxxxxxx>
> ---
> arch/arm/mach-at91/Kconfig | 2 +
> drivers/mfd/atmel-flexcom.c | 55 +++++++++++++++-
Can this be separated into its own patch?
> 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;
Why does this need it's own struct?
> +};
> +
> 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");
dev_err() already prints out the device name, so you can drop the
"flexcom" part. Also, please expand 'caps'. I'm assuming that's
capabilities?
> + return -EINVAL;
> + }
> +
> + /* Flexcom Mux */
> + if (caps->has_flx_mux && of_property_read_bool(np, "mux-controls")) {
> + struct mux_control *flx_mux;
What is 'flx'?
> + 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;
No mux_control_deselect() for previous iterations?
> + err = mux_control_select(flx_mux, args.args[0]);
> + if (!err) {
> + mux_control_deselect(flx_mux);
Not sure I see the point in selecting then deselecting.
Is it just a test?
If so, why don't you wait until you need to select it, then apply
normal error handling protocols there instead?
> + } else {
> + dev_err(&pdev->dev, "Failed to select FLEXCOM mux\n");
> + return err;
> + }
> + }
> + }
> +
> clk_disable_unprepare(ddata->clk);
>
> return devm_of_platform_populate(&pdev->dev);
> }
>
> +static const struct atmel_flex_caps atmel_flexcom_caps = {};
Why not just leave .data as NULL?
> +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,
And this can't be a DT property?
> + },
> +
> + {
> + .compatible = "microchip,lan966-flexcom",
> + .data = &lan966x_flexcom_caps,
> + },
This all seems like a lot of hoop-jumping.
Why not just do:
if (of_device_is_compatible(np, "lan966x_flexcom_caps"))
> +
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
