From: Lee Jones <lee.jones@linaro.org> To: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com> Cc: krzysztof.kozlowski+dt@linaro.org, nicolas.ferre@microchip.com, alexandre.belloni@bootlin.com, claudiu.beznea@microchip.com, peda@axentia.se, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux@armlinux.org.uk, Manohar.Puri@microchip.com, UNGLinuxDriver@microchip.com Subject: Re: [PATCH v2 4/4] mux: lan966: Add support for flexcom mux controller Date: Mon, 23 May 2022 15:52:22 +0100 [thread overview] Message-ID: <YoufpkeLTgchjESL@google.com> (raw) In-Reply-To: <20220509084920.14529-5-kavyasree.kotagiri@microchip.com> 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@microchip.com> > --- > 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
WARNING: multiple messages have this Message-ID (diff)
From: Lee Jones <lee.jones@linaro.org> To: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com> Cc: devicetree@vger.kernel.org, alexandre.belloni@bootlin.com, linux-kernel@vger.kernel.org, claudiu.beznea@microchip.com, krzysztof.kozlowski+dt@linaro.org, Manohar.Puri@microchip.com, linux@armlinux.org.uk, UNGLinuxDriver@microchip.com, peda@axentia.se, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 4/4] mux: lan966: Add support for flexcom mux controller Date: Mon, 23 May 2022 15:52:22 +0100 [thread overview] Message-ID: <YoufpkeLTgchjESL@google.com> (raw) In-Reply-To: <20220509084920.14529-5-kavyasree.kotagiri@microchip.com> 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@microchip.com> > --- > 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-05-23 14:52 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-09 8:49 [PATCH v2 0/4] Add support for lan966 flexcom multiplexer Kavyasree Kotagiri 2022-05-09 8:49 ` Kavyasree Kotagiri 2022-05-09 8:49 ` [PATCH v2 1/4] dt-bindings: mfd: atmel, flexcom: Convert to json-schema Kavyasree Kotagiri 2022-05-09 8:49 ` [PATCH v2 1/4] dt-bindings: mfd: atmel,flexcom: " Kavyasree Kotagiri 2022-05-10 10:32 ` Krzysztof Kozlowski 2022-05-10 10:32 ` Krzysztof Kozlowski 2022-05-09 8:49 ` [PATCH v2 2/4] dt-bindings: mfd: atmel, flexcom: Add lan966 compatible string and mux properties Kavyasree Kotagiri 2022-05-09 8:49 ` [PATCH v2 2/4] dt-bindings: mfd: atmel,flexcom: " Kavyasree Kotagiri 2022-05-10 10:33 ` Krzysztof Kozlowski 2022-05-10 10:33 ` Krzysztof Kozlowski 2022-05-09 8:49 ` [PATCH v2 3/4] dt-bindings: mux: Add lan966 flexcom mux controller Kavyasree Kotagiri 2022-05-09 8:49 ` Kavyasree Kotagiri 2022-05-10 10:37 ` Krzysztof Kozlowski 2022-05-10 10:37 ` Krzysztof Kozlowski 2022-05-09 8:49 ` [PATCH v2 4/4] mux: lan966: Add support for " Kavyasree Kotagiri 2022-05-09 8:49 ` Kavyasree Kotagiri 2022-05-09 11:37 ` Peter Rosin 2022-05-09 11:37 ` Peter Rosin 2022-05-09 11:41 ` Peter Rosin 2022-05-09 11:41 ` Peter Rosin 2022-05-10 14:59 ` Kavyasree.Kotagiri 2022-05-10 14:59 ` Kavyasree.Kotagiri 2022-05-10 19:38 ` Peter Rosin 2022-05-10 19:38 ` Peter Rosin 2022-05-11 14:28 ` Kavyasree.Kotagiri 2022-05-11 14:28 ` Kavyasree.Kotagiri 2022-05-11 15:43 ` Peter Rosin 2022-05-11 15:43 ` Peter Rosin 2022-05-27 6:00 ` Kavyasree.Kotagiri 2022-05-27 6:00 ` Kavyasree.Kotagiri 2022-05-17 11:53 ` Michael Walle 2022-05-17 11:53 ` Michael Walle 2022-05-17 12:00 ` Peter Rosin 2022-05-17 12:00 ` Peter Rosin 2022-05-10 1:46 ` kernel test robot 2022-05-10 1:46 ` kernel test robot 2022-05-10 3:28 ` kernel test robot 2022-05-10 3:28 ` kernel test robot 2022-05-11 9:06 ` Claudiu.Beznea 2022-05-11 9:06 ` Claudiu.Beznea 2022-05-23 14:52 ` Lee Jones [this message] 2022-05-23 14:52 ` Lee Jones
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YoufpkeLTgchjESL@google.com \ --to=lee.jones@linaro.org \ --cc=Manohar.Puri@microchip.com \ --cc=UNGLinuxDriver@microchip.com \ --cc=alexandre.belloni@bootlin.com \ --cc=claudiu.beznea@microchip.com \ --cc=devicetree@vger.kernel.org \ --cc=kavyasree.kotagiri@microchip.com \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=nicolas.ferre@microchip.com \ --cc=peda@axentia.se \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.