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:peda@xxxxxxxxxx] > Sent: Wednesday, May 11, 2022 9:14 PM > To: Kavyasree Kotagiri - I30978 <Kavyasree.Kotagiri@xxxxxxxxxxxxx> > Cc: devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; lee.jones@xxxxxxxxxx; linux@xxxxxxxxxxxxxxx; > Manohar Puri - I30488 <Manohar.Puri@xxxxxxxxxxxxx>; UNGLinuxDriver > <UNGLinuxDriver@xxxxxxxxxxxxx>; krzysztof.kozlowski+dt@xxxxxxxxxx; > Nicolas Ferre - M43238 <Nicolas.Ferre@xxxxxxxxxxxxx>; > alexandre.belloni@xxxxxxxxxxx; Claudiu Beznea - M18063 > <Claudiu.Beznea@xxxxxxxxxxxxx> > 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, Kavyasree.Kotagiri@xxxxxxxxxxxxx wrote: > >> 2022-05-10 at 16:59, Kavyasree.Kotagiri@xxxxxxxxxxxxx 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 +++++++++++++++- > >>>>> 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 <kavyasree.kotagiri@xxxxxxxxxxxxx> > >>>> > >>>> 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 > >>>> <kavyasree.kotagiri@xxxxxxxxxxxxx>"); > >>>>> +MODULE_LICENSE("GPL v2"); > >>>>> +
