by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] mux: lan966: Add support for flexcom mux controller

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 <[email protected]>
> ---
> 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

2022-05-27 12:02:11

by Kavyasree Kotagiri

[permalink] [raw]

Subject: RE: [PATCH v2 4/4] mux: lan966: Add support for flexcom mux controller

Hi Peter,

Thanks for your comments.
> If you are content with just programming a fixed set of values to
> a couple of registers depending on how the board is wired, some
> extra DT property on some node related to the flexcom seems like
> a better fit than a mux driver.
Based on your inputs, I planned to send a new patch with new DT properties
introduced in atmel-flexcom.c driver rather than mux driver.

Thanks,
Kavya

> -----Original Message-----
> From: Peter Rosin [mailto:[email protected]]
> Sent: Wednesday, May 11, 2022 9:14 PM
> To: Kavyasree Kotagiri - I30978 <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> Manohar Puri - I30488 <[email protected]>; UNGLinuxDriver
> <[email protected]>; [email protected];
> Nicolas Ferre - M43238 <[email protected]>;
> [email protected]; Claudiu Beznea - M18063
> <[email protected]>
> Subject: Re: [PATCH v2 4/4] mux: lan966: Add support for flexcom mux
> controller
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hi!
>
> 2022-05-11 at 16:28, [email protected] wrote:
> >> 2022-05-10 at 16:59, [email protected] 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
> <[email protected]>
> >>>>> ---
> >>>>> 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);
> >>>>
> >>>> This is suspect. When you deselect the mux you rely on the mux to be
> >>>> configured with "as-is" as the idle state. But you do not document that.
> >>>> This is also fragile in that you silently rely on noone else selecting
> >>>> the mux to some unwanted state behind your back (mux controls are
> not
> >>>> exclusive the way e.g. gpio pins or pwms are). The protocol is that
> >>>> others may get a ref to the mux control and select it as long as noone
> >>>> else has selected it.
> >>>>
> >>>> The only sane thing to do is to keep the mux selected for the whole
> >>>> duration when you rely on it to be in your desired state.
> >>>>
> >>>
> >>> Yes, mux is kept selected until configuring register is done. Please see
> >> below log where
> >>> I added debug prints just for understanding:
> >>> # dmesg | grep KK
> >>> [ 0.779827] KK: Func: atmel_flexcom_probe ***** [START flx muxing]
> >> ********
> >>> [ 0.779875] KK: Func: atmel_flexcom_probe i = 0 args[0] = 0
> >>> [ 0.779890] KK: Func: mux_control_select [Entered]
> >>> [ 0.779902] KK: Func: mux_lan966x_set [Entered] state = 0
> >>> [ 0.779977] KK: Func: mux_lan966x_set [Read] = 0x1fffef <<<-----
> setting
> >> mux_lan966x[state].ss_pin "4" which is passed from dts
> >>> [ 0.779992] KK: Func: mux_lan966x_set [Exit]
> >>> [ 0.780002] KK: Func: mux_control_select [Exit]
> >>> [ 0.780011] KK: Func: mux_control_deselect [Entered]
> >>> [ 0.780021] KK: Func: mux_control_deselect [Exit]
> >>
> >> You misunderstand. The mux control is only "selected" between the call
> >> to mux_control_select() and the following call to
> >> mux_control_deselect().
> >>
> >> After that, the mux control is "idle". However, in your case the
> >> "idle-state" is configured to "as-is" (which is the default), so the
> >> mux control (naturally) remains in the previously selected state while
> >> idle. But you are not documenting that limitation, and with this
> >> implementation you *must* have a mux control that uses "as-is" as its
> >> idle state. But that is an unexpected and broken limitation, and a
> >> much better solution is to simply keep the mux control "selected" for
> >> the complete duration when you rely on it to be in whatever state you
> >> need it to be in.
> >>
> > I am new to mux drivers.
> > Let me try to explain why I have mux_control_deselect if there is no err
> from mux_control_select()
> > For example,
> > 1. When I have one only chip-select CS0 for flexcom3 to be mapped to
> flexcom shared pin 9:
> > As per previously shared log, FLEXCOM_SHARED[3]:SS_MASK[0] is being
> configured to expected value
> > even before mux_control_deseletc().
> > 2. When I have to map two chip-selects of flx3 - CS0 to flexcom shared 9
> > CS1 to flexcom shared pin 7
> > FLEXCOM_SHARED[3]:SS_MASK[0] is set to expected value 0x1fffef
> > I see console hangs at mux_control_select() if I don’t call
> mux_control_deselect
> > while mapping second chip-select FLEXCOM_SHARED[3]:SS_MASK[1].
> > After reading below description from mux_control_select() :
> > " * On successfully selecting the mux-control state, it will be locked until
> > * there is a call to mux_control_deselect()."
> > Following this help text, I called mux_control_deselect() if there is no error
> from mux_control_select()
> > and then it worked. FLEXCOM_SHARED[3]:SS_MASK[1] is now set to
> expected value 0x1fffbf.
> >
> > Please explain me if I am missing something.
>
> You should wait with the deselect until you need to change the state
> of the mux. I.e., on init/probe you set some variable to -1, like so:
>
> priv->mux_error = -1;
>
> and when you later need to set/change the state of the mux, you do:
>
> if (!priv->mux_error)
> mux_control_deselect(...);
> priv->mux_error = mux_control_select(...);
>
> and then you cleanup at the end of life:
>
> if (!priv->mux_error)
> mux_control_deselect(...);
>
> Or something like that.
>
> The mux api is obviously not a good fit for the use case of long
> lived selects like you appear to have here. The api was designed
> for short lived selects, along the lines of:
>
> int
> so_something(...)
> {
> int err;
>
> err = mux_control_select(...);
> if (err)
> return err;
>
> do_it(...);
>
> mux_control_deselect(...);
> }
>
>
> >
> >>>>> + } 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 = {};
> >>>>> +
> >>>>> +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.
> >>>>> +
> >>>>> 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 <[email protected]>
> >>>>
> >>>> Looks like it is based on mmio.c?
> >>>>
> >>> Yes, I took mmio.c driver as reference.
> >>>
> >>
> >> So, then the above copyright and authorship info is not complete.
> >>
> >>>>> + */
> >>>>> +
> >>>>> +#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
> >>>>> +#define LAN966_MAX_CS 21
> >>>>> +
> >>>>> +static void __iomem *flx_shared_base;
> >>>>
> >>>> I would much prefer to store the combined address (base+offset)
> >>>> in the mux private data instead of only storing the offset and
> >>>> then unnecessarily rely on some piece of global state (that
> >>>> will be clobbered by other instances).
> >>>>
> >>> Ok. I will try to see if this is relevant and change accordingly.
> >>>
> >>>>> +struct mux_lan966x {
> >>>>
> >>>> Why is the file named lan966, but then everything inside 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);
> >>>>
> >>>> This reads memory you have not allocated, if you select a state
> >>>> other than zero.
> >>>>
> >>> Ok. I will return error condition in case of trying to access none existing
> >> entry.
> >>>>> +
> >>>>> + 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));
> >>>>
> >>>> I might be thoroughly mistaken and confused by the code, but it seems
> >>>> very strange that a subsequenct select is not undoing what a previous
> >>>> select once did. Each state seems to write to its own register offset,
> >>>> and there is nothing that restores the first register offset with you
> >>>> switch states.
> >>>>
> >>>> Care to explain how muxing works and what you are expected to do to
> >>>> manipulate the mux? Is there some link to public documentation? I did
> >>>> a quick search for lan966 but came up with nothing relevant.
> >>>>
> >>> LAN966 has 5 flexcoms(which can be used as USART/SPI/I2C interface).
> >>> FLEXCOM has two chip-select I/O lines namely CS0 and CS1
> >>> in SPI mode, CTS and RTS in USART mode. These FLEXCOM pins are
> >> optional.
> >>> These chip-selects can be mapped to flexcom shared pin [0-21] which
> can
> >> be
> >>> done by configuring flexcom multiplexer register(FLEXCOM_SHARED[0-
> >> 4]:SS_MASK[0-1])
> >>> Driver explanation:
> >>> "flx_shared_base" is used to get base address of Flexcom shared
> >> multiplexer
> >>> "mux-offset-pin" property is used to get cs0/cs1 offset and flexcom
> shared
> >> pin[0-21] of a flexcom.
> >>> state value passed from atmel-flexcom is used to configure respective
> >>> FLEXCOM_SHARED[0-4]:SS_MASK[0-1] register with offset and flexcom
> >> shared pin.
> >>
> >> Ok, let me try to interpret that. You wish enable a "fan out" of these
> >> two chip-selects for any of the 5 flexcoms (in SPI mode?) such that
> >> these 10 internal chip-selects can be connected to any of 21 external
> >> pins?
> >>
> >> If that's correct and if you wish to interface with e.g. 20 chips,
> >> then it would be possible to have 20 states for one mux control and
> >> then reach the 20 chips using only CS0 from FLEXCOM0, or it would be
> >> possible to have 2 states for 10 mux controls, one each for CS0/CS1 of
> >> all five flexcoms and also reach 20 chips. Or any wild combo you
> >> imagine is useful.
> >>
> >> Is that correctly understood?
> >>
> >> Assuming so, then you can have a maximum of 10 mux controls, and for
> >> each mux control you need a variable number of legitimate states. Each
> >> mux control would also need to know at what address/offset its SS_MASK
> >> register is at and what pins/states are valid.
> >>
> >> But your code does not implemnent the above. You allocate num_fields
> >> mux controls, which is what confuses the hell out of me. num_fields is
> >> the number of states, not the number of mux controls! And you also
> >> need to specify an individual offset for each state, and that makes no
> >> sense at all, at least not to me.
> >>
> >> So, instead, I think you want to have:
> >>
> >> struct mux_lan966x {
> >> u32 ss_mask;
> >> int num_pins;
> >> u32 ss_pin[];
> >> };
> >>
> >> And then do:
> >>
> >> mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_lan966x) +
> >> num_pins * sizeof(u32));
> >>
> >> (or however that size is best spelled, I haven't kept up)
> >>
> >> The set operation would be along the lines of:
> >>
> >> static int mux_lan966x_set(struct mux_control *mux, int state)
> >> {
> >> struct mux_lan966x *mux_lan966x = mux_chip_priv(mux->chip);
> >> u32 val;
> >>
> >> if (state < 0 || state >= mux_lan966x->num_pins)
> >> return -1;
> >>
> >> val = ~(1 << mux_lan966x->ss_pin[state]) & FLEX_SHRD_MASK;
> >> writel(val, flx_shared_base + mux_lan966x->ss_mask);
> >>
> >> return 0;
> >> }
> >>
> >> Because, one mux control should only ever need to know about one
> offset,
> >> as it should only ever write to its own SS_MASK register. And you will
> >> need some private space such that you can keep track of which states
> >> are legit.
> >>
> >> I would also separate out the SS_MASK offset from the mux-offset-pin
> >> property and have one property for that value and then a straight
> >> array for the valid pin numbers in another property (no longer named
> >> mux-offset-pin of course).
> >>
> >> Or something like that and assuming I understand how the FLEXCOMs
> work
> >> and what you want to do etc.
> >>
> >
> > Thank you for your comments
> > I agree, I will change number of mux controls to 1 in
> devm_mux_chip_alloc()
> > I would like to use mux-offset-pin property because
> > each chip-select must be mapped to a unique flexcom shared pin.
> > For example,
> > mux-offset-pin = <0x18 9>, /* 0: flexcom3 CS0 mapped to pin-9 */
> > <0x1c 7>, /* 1: flexcom3 CS1 mapped to pin-7 */
> > <0x20 4>; /* 2: flexcom4 CS0 mapped to pin-4 */};
> > I want to use mux-offset-pin property to be clear about which offset is
> mapped to which
> > flexcom shared pin. Here, 0, 1, 2.. entries represents state passed from
> mux_control_select(mux, state).
> >
> > Please provide your comments.
>
> It seems you want to just use the static info from the devicetree
> to program your registers once and for all? That's not a problem
> that a mux driver is aming at solving. My thought was that if you
> have one SPI device with chip select on pin-7, another with chip-
> select on pin-9 and a third on pin-4, then you /could/ use them all
> from e.g. flexcom3/CS0 by reprogramming that SS_MASK register
> at the time you want to access one of the three chips. You could
> then of course not access the three chips in parallel, but the
> muxing of the accesses to three chips like that is the problem
> that the mux subsystem helps with.
>
> I.e. pretty much exactly as is described in
>
> Documentation/devicetree/bindings/spi/spi-mux.yaml
>
> but with a mux node bringing your new driver into the picture
> instead of the "gpio-mux" in that example (and the "CS-X" signal
> and the "Mux" block in that picture would be internal to the SoC).
>
> If you are content with just programming a fixed set of values to
> a couple of registers depending on how the board is wired, some
> extra DT property on some node related to the flexcom seems like
> a better fit than a mux driver.
>
> Having said that, the mux solution above would solve this static
> case too. You would just need to configure a mux with one state.
> It would be a couple of extra nodes in the device tree, but it
> would certainly work (and also cover the more complex case when
> someone pops up and needs that).
>
> Cheers,
> Peter
>
> >> Cheers,
> >> Peter
> >>
> >>
> >>>>> + 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;
> >>>>
> >>>> This clobbers memory you have not allocated, if num_fields >= 1.
> >>>>
> >>>> Cheers,
> >>>> Peter
> >>>>
> >>>>> +
> >>>>> + 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;
> >>>>> +}
> >>>>> +
> >>>>> +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
> >>>> <[email protected]>");
> >>>>> +MODULE_LICENSE("GPL v2");
> >>>>> +