* [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits @ 2012-09-05 9:01 Peter Ujfalusi 2012-09-05 9:01 ` [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask Peter Ujfalusi ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Peter Ujfalusi @ 2012-09-05 9:01 UTC (permalink / raw) To: Linus Walleij, Tony Lindgren; +Cc: linux-kernel, linux-omap Hello, When configuring pinmux with pinctrl-single there could be a case when one register is used to configure mux for more than one pin. In this case the use of pinctrl-single,pins is a bit problematic since we can only update the whole register (restricted by the mask). In such a situations the pinctrl-single,bits could provide a safe way to handle the mux. pinctrl-single,bits takes three parameters: <reg offset, value, sub-mask> The sub mask is used to mask part of the register to make sure we do not change bits outside of the scope of this pin. The first patch in this series is to fix the previous pinctrl-since,pins implementation because it was not using the mask on the value which could result changed bits outside of the mask. Regards, Peter --- Peter Ujfalusi (2): pinctrl: pinctrl-single: Make sure we do not change bits outside of mask pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux .../devicetree/bindings/pinctrl/pinctrl-single.txt | 9 +++++ drivers/pinctrl/pinctrl-single.c | 42 ++++++++++++++++------ 2 files changed, 41 insertions(+), 10 deletions(-) -- 1.7.12 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask 2012-09-05 9:01 [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits Peter Ujfalusi @ 2012-09-05 9:01 ` Peter Ujfalusi 2012-09-06 18:59 ` Tony Lindgren 2012-09-10 7:09 ` Linus Walleij 2012-09-05 9:01 ` [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux Peter Ujfalusi 2012-09-05 12:10 ` [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits Linus Walleij 2 siblings, 2 replies; 16+ messages in thread From: Peter Ujfalusi @ 2012-09-05 9:01 UTC (permalink / raw) To: Linus Walleij, Tony Lindgren; +Cc: linux-kernel, linux-omap Use the pcs->fmask to make sure that the value is not changing (setting) bits in areas where it should not. To avoid situations like this: pmx_dummy: pinmux@4a100040 { compatible = "pinctrl-single"; reg = <0x4a100040 0x0196>; #address-cells = <1>; #size-cells = <0>; pinctrl-single,register-width = <16>; pinctrl-single,function-mask = <0x00ff>; }; &pmx_dummy { pinctrl-names = "default"; pinctrl-0 = <&board_pins>; board_pins: pinmux_board_pins { pinctrl-single,pins = < 0x6c 0xf0f 0x6e 0x10f 0x70 0x23f 0x72 0xa5f >; }; }; Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- drivers/pinctrl/pinctrl-single.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 76a4260..3508631 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -337,7 +337,7 @@ static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector, vals = &func->vals[i]; val = pcs->read(vals->reg); val &= ~pcs->fmask; - val |= vals->val; + val |= (vals->val & pcs->fmask); pcs->write(val, vals->reg); } -- 1.7.12 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask 2012-09-05 9:01 ` [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask Peter Ujfalusi @ 2012-09-06 18:59 ` Tony Lindgren 2012-09-07 21:13 ` Linus Walleij 2012-09-10 7:09 ` Linus Walleij 1 sibling, 1 reply; 16+ messages in thread From: Tony Lindgren @ 2012-09-06 18:59 UTC (permalink / raw) To: Peter Ujfalusi; +Cc: Linus Walleij, linux-kernel, linux-omap * Peter Ujfalusi <peter.ujfalusi@ti.com> [120905 02:02]: > Use the pcs->fmask to make sure that the value is not changing (setting) > bits in areas where it should not. > To avoid situations like this: > > pmx_dummy: pinmux@4a100040 { > compatible = "pinctrl-single"; > reg = <0x4a100040 0x0196>; > #address-cells = <1>; > #size-cells = <0>; > pinctrl-single,register-width = <16>; > pinctrl-single,function-mask = <0x00ff>; > }; > > &pmx_dummy { > pinctrl-names = "default"; > pinctrl-0 = <&board_pins>; > > board_pins: pinmux_board_pins { > pinctrl-single,pins = < > 0x6c 0xf0f > 0x6e 0x10f > 0x70 0x23f > 0x72 0xa5f > >; > }; > }; > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> Thanks this is a valid fix: Acked-by: Tony Lindgren <tony@atomide.com> > --- > drivers/pinctrl/pinctrl-single.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > index 76a4260..3508631 100644 > --- a/drivers/pinctrl/pinctrl-single.c > +++ b/drivers/pinctrl/pinctrl-single.c > @@ -337,7 +337,7 @@ static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector, > vals = &func->vals[i]; > val = pcs->read(vals->reg); > val &= ~pcs->fmask; > - val |= vals->val; > + val |= (vals->val & pcs->fmask); > pcs->write(val, vals->reg); > } > > -- > 1.7.12 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask 2012-09-06 18:59 ` Tony Lindgren @ 2012-09-07 21:13 ` Linus Walleij 2012-09-07 21:39 ` Tony Lindgren 0 siblings, 1 reply; 16+ messages in thread From: Linus Walleij @ 2012-09-07 21:13 UTC (permalink / raw) To: Tony Lindgren; +Cc: Peter Ujfalusi, linux-kernel, linux-omap On Thu, Sep 6, 2012 at 8:59 PM, Tony Lindgren <tony@atomide.com> wrote: > * Peter Ujfalusi <peter.ujfalusi@ti.com> [120905 02:02]: >> Use the pcs->fmask to make sure that the value is not changing (setting) >> bits in areas where it should not. >> To avoid situations like this: >> >> pmx_dummy: pinmux@4a100040 { >> compatible = "pinctrl-single"; >> reg = <0x4a100040 0x0196>; >> #address-cells = <1>; >> #size-cells = <0>; >> pinctrl-single,register-width = <16>; >> pinctrl-single,function-mask = <0x00ff>; >> }; >> >> &pmx_dummy { >> pinctrl-names = "default"; >> pinctrl-0 = <&board_pins>; >> >> board_pins: pinmux_board_pins { >> pinctrl-single,pins = < >> 0x6c 0xf0f >> 0x6e 0x10f >> 0x70 0x23f >> 0x72 0xa5f >> >; >> }; >> }; >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > > Thanks this is a valid fix: > > Acked-by: Tony Lindgren <tony@atomide.com> Since nothing in v3.6 is using pinctrl-simple yet, it's not a regression right? So can you just group this with the other pinctrl things you are harvesting in the OMAP tree? I was going to push it for the v3.7 cycle otherwise. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask 2012-09-07 21:13 ` Linus Walleij @ 2012-09-07 21:39 ` Tony Lindgren 0 siblings, 0 replies; 16+ messages in thread From: Tony Lindgren @ 2012-09-07 21:39 UTC (permalink / raw) To: Linus Walleij; +Cc: Peter Ujfalusi, linux-kernel, linux-omap * Linus Walleij <linus.walleij@linaro.org> [120907 14:13]: > On Thu, Sep 6, 2012 at 8:59 PM, Tony Lindgren <tony@atomide.com> wrote: > > * Peter Ujfalusi <peter.ujfalusi@ti.com> [120905 02:02]: > >> Use the pcs->fmask to make sure that the value is not changing (setting) > >> bits in areas where it should not. > >> To avoid situations like this: > >> > >> pmx_dummy: pinmux@4a100040 { > >> compatible = "pinctrl-single"; > >> reg = <0x4a100040 0x0196>; > >> #address-cells = <1>; > >> #size-cells = <0>; > >> pinctrl-single,register-width = <16>; > >> pinctrl-single,function-mask = <0x00ff>; > >> }; > >> > >> &pmx_dummy { > >> pinctrl-names = "default"; > >> pinctrl-0 = <&board_pins>; > >> > >> board_pins: pinmux_board_pins { > >> pinctrl-single,pins = < > >> 0x6c 0xf0f > >> 0x6e 0x10f > >> 0x70 0x23f > >> 0x72 0xa5f > >> >; > >> }; > >> }; > >> > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > > > > Thanks this is a valid fix: > > > > Acked-by: Tony Lindgren <tony@atomide.com> > > Since nothing in v3.6 is using pinctrl-simple yet, it's not a regression > right? Right. > So can you just group this with the other pinctrl things you are > harvesting in the OMAP tree? I was going to push it for > the v3.7 cycle otherwise. You can take this for v3.7, the changes I have are just adding .dts entries to use the driver. The driver related changes are being merged by the related driver lists. Regards, Tony ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask 2012-09-05 9:01 ` [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask Peter Ujfalusi 2012-09-06 18:59 ` Tony Lindgren @ 2012-09-10 7:09 ` Linus Walleij 1 sibling, 0 replies; 16+ messages in thread From: Linus Walleij @ 2012-09-10 7:09 UTC (permalink / raw) To: Peter Ujfalusi; +Cc: Tony Lindgren, linux-kernel, linux-omap On Wed, Sep 5, 2012 at 11:01 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> Applied with Tony's ACK. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux 2012-09-05 9:01 [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits Peter Ujfalusi 2012-09-05 9:01 ` [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask Peter Ujfalusi @ 2012-09-05 9:01 ` Peter Ujfalusi 2012-09-06 19:10 ` Tony Lindgren 2012-09-10 7:10 ` Linus Walleij 2012-09-05 12:10 ` [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits Linus Walleij 2 siblings, 2 replies; 16+ messages in thread From: Peter Ujfalusi @ 2012-09-05 9:01 UTC (permalink / raw) To: Linus Walleij, Tony Lindgren; +Cc: linux-kernel, linux-omap With pinctrl-single,bits it is possible to update just part of the register within the pinctrl-single,function-mask area. This is useful when one register configures mmore than one pin's mux. pinctrl-single,bits takes three parameters: <reg offset, value, sub-mask> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- .../devicetree/bindings/pinctrl/pinctrl-single.txt | 9 +++++ drivers/pinctrl/pinctrl-single.c | 42 ++++++++++++++++------ 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt index 5187f0d..287801d 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt @@ -31,6 +31,15 @@ device pinctrl register, and 0x118 contains the desired value of the pinctrl register. See the device example and static board pins example below for more information. +In case when one register changes more than one pin's mux the +pinctrl-single,bits can be used which takes three parameters: + + pinctrl-single,bits = <0xdc 0x18, 0xff>; + +Where 0xdc is the offset from the pinctrl register base address for the +device pinctrl register, 0x18 is the desired value, and 0xff is the sub mask to +be used when applying this change to the register. + Example: /* SoC common file */ diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 3508631..aec338e 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -26,7 +26,8 @@ #include "core.h" #define DRIVER_NAME "pinctrl-single" -#define PCS_MUX_NAME "pinctrl-single,pins" +#define PCS_MUX_PINS_NAME "pinctrl-single,pins" +#define PCS_MUX_BITS_NAME "pinctrl-single,bits" #define PCS_REG_NAME_LEN ((sizeof(unsigned long) * 2) + 1) #define PCS_OFF_DISABLED ~0U @@ -54,6 +55,7 @@ struct pcs_pingroup { struct pcs_func_vals { void __iomem *reg; unsigned val; + unsigned mask; }; /** @@ -332,12 +334,17 @@ static int pcs_enable(struct pinctrl_dev *pctldev, unsigned fselector, for (i = 0; i < func->nvals; i++) { struct pcs_func_vals *vals; - unsigned val; + unsigned val, mask; vals = &func->vals[i]; val = pcs->read(vals->reg); - val &= ~pcs->fmask; - val |= (vals->val & pcs->fmask); + if (!vals->mask) + mask = pcs->fmask; + else + mask = pcs->fmask & vals->mask; + + val &= ~mask; + val |= (vals->val & mask); pcs->write(val, vals->reg); } @@ -657,18 +664,29 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, { struct pcs_func_vals *vals; const __be32 *mux; - int size, rows, *pins, index = 0, found = 0, res = -ENOMEM; + int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM; struct pcs_function *function; - mux = of_get_property(np, PCS_MUX_NAME, &size); - if ((!mux) || (size < sizeof(*mux) * 2)) { - dev_err(pcs->dev, "bad data for mux %s\n", - np->name); + mux = of_get_property(np, PCS_MUX_PINS_NAME, &size); + if (mux) { + params = 2; + } else { + mux = of_get_property(np, PCS_MUX_BITS_NAME, &size); + if (!mux) { + dev_err(pcs->dev, "no valid property for %s\n", + np->name); + return -EINVAL; + } + params = 3; + } + + if (size < (sizeof(*mux) * params)) { + dev_err(pcs->dev, "bad data for %s\n", np->name); return -EINVAL; } size /= sizeof(*mux); /* Number of elements in array */ - rows = size / 2; /* Each row is a key value pair */ + rows = size / params; /* Each row is a key value pair */ vals = devm_kzalloc(pcs->dev, sizeof(*vals) * rows, GFP_KERNEL); if (!vals) @@ -686,6 +704,10 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, val = be32_to_cpup(mux + index++); vals[found].reg = pcs->base + offset; vals[found].val = val; + if (params == 3) { + val = be32_to_cpup(mux + index++); + vals[found].mask = val; + } pin = pcs_get_pin_by_offset(pcs, offset); if (pin < 0) { -- 1.7.12 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux 2012-09-05 9:01 ` [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux Peter Ujfalusi @ 2012-09-06 19:10 ` Tony Lindgren 2012-09-07 15:13 ` Peter Ujfalusi 2012-09-10 7:10 ` Linus Walleij 1 sibling, 1 reply; 16+ messages in thread From: Tony Lindgren @ 2012-09-06 19:10 UTC (permalink / raw) To: Peter Ujfalusi; +Cc: Linus Walleij, linux-kernel, linux-omap Hi Peter, * Peter Ujfalusi <peter.ujfalusi@ti.com> [120905 02:02]: > With pinctrl-single,bits it is possible to update just part of the register > within the pinctrl-single,function-mask area. > This is useful when one register configures mmore than one pin's mux. You have a typo here: ^^^^^ > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt > @@ -31,6 +31,15 @@ device pinctrl register, and 0x118 contains the desired value of the > pinctrl register. See the device example and static board pins example > below for more information. > > +In case when one register changes more than one pin's mux the > +pinctrl-single,bits can be used which takes three parameters: > + > + pinctrl-single,bits = <0xdc 0x18, 0xff>; > + > +Where 0xdc is the offset from the pinctrl register base address for the > +device pinctrl register, 0x18 is the desired value, and 0xff is the sub mask to > +be used when applying this change to the register. > + Is it now safe to assume that we always have width of three if pinctrl-single,bits is specified? The reason I'm asking is.. > @@ -657,18 +664,29 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, > { > struct pcs_func_vals *vals; > const __be32 *mux; > - int size, rows, *pins, index = 0, found = 0, res = -ENOMEM; > + int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM; > struct pcs_function *function; > > - mux = of_get_property(np, PCS_MUX_NAME, &size); > - if ((!mux) || (size < sizeof(*mux) * 2)) { > - dev_err(pcs->dev, "bad data for mux %s\n", > - np->name); > + mux = of_get_property(np, PCS_MUX_PINS_NAME, &size); > + if (mux) { > + params = 2; > + } else { > + mux = of_get_property(np, PCS_MUX_BITS_NAME, &size); > + if (!mux) { > + dev_err(pcs->dev, "no valid property for %s\n", > + np->name); > + return -EINVAL; > + } > + params = 3; > + } ..because here we could assume the default value for params is 2 if pinctrl-single,pins is specified, and otherwise params is 3 if pinctrl-single,bits is specified for the controller. That would avoid querying a potentially non-exiting property for each entry. > @@ -686,6 +704,10 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, > val = be32_to_cpup(mux + index++); > vals[found].reg = pcs->base + offset; > vals[found].val = val; > + if (params == 3) { > + val = be32_to_cpup(mux + index++); > + vals[found].mask = val; > + } > > pin = pcs_get_pin_by_offset(pcs, offset); > if (pin < 0) { Here params too would be then set during probe already. Other than that, seems to still work for me for my test cases. Regards, Tony ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux 2012-09-06 19:10 ` Tony Lindgren @ 2012-09-07 15:13 ` Peter Ujfalusi 2012-09-07 16:55 ` Tony Lindgren 0 siblings, 1 reply; 16+ messages in thread From: Peter Ujfalusi @ 2012-09-07 15:13 UTC (permalink / raw) To: Tony Lindgren; +Cc: Linus Walleij, linux-kernel, linux-omap Hi Tony, On 09/06/2012 10:10 PM, Tony Lindgren wrote: > Hi Peter, > > * Peter Ujfalusi <peter.ujfalusi@ti.com> [120905 02:02]: >> With pinctrl-single,bits it is possible to update just part of the register >> within the pinctrl-single,function-mask area. >> This is useful when one register configures mmore than one pin's mux. > > You have a typo here: ^^^^^ Oh, I'll fix this up. >> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt >> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt >> @@ -31,6 +31,15 @@ device pinctrl register, and 0x118 contains the desired value of the >> pinctrl register. See the device example and static board pins example >> below for more information. >> >> +In case when one register changes more than one pin's mux the >> +pinctrl-single,bits can be used which takes three parameters: >> + >> + pinctrl-single,bits = <0xdc 0x18, 0xff>; >> + >> +Where 0xdc is the offset from the pinctrl register base address for the >> +device pinctrl register, 0x18 is the desired value, and 0xff is the sub mask to >> +be used when applying this change to the register. >> + > > Is it now safe to assume that we always have width of three if > pinctrl-single,bits is specified? The reason I'm asking is.. > >> @@ -657,18 +664,29 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, >> { >> struct pcs_func_vals *vals; >> const __be32 *mux; >> - int size, rows, *pins, index = 0, found = 0, res = -ENOMEM; >> + int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM; >> struct pcs_function *function; >> >> - mux = of_get_property(np, PCS_MUX_NAME, &size); >> - if ((!mux) || (size < sizeof(*mux) * 2)) { >> - dev_err(pcs->dev, "bad data for mux %s\n", >> - np->name); >> + mux = of_get_property(np, PCS_MUX_PINS_NAME, &size); >> + if (mux) { >> + params = 2; >> + } else { >> + mux = of_get_property(np, PCS_MUX_BITS_NAME, &size); >> + if (!mux) { >> + dev_err(pcs->dev, "no valid property for %s\n", >> + np->name); >> + return -EINVAL; >> + } >> + params = 3; >> + } > > ..because here we could assume the default value for params is 2 > if pinctrl-single,pins is specified, and otherwise params is 3 > if pinctrl-single,bits is specified for the controller. That would > avoid querying a potentially non-exiting property for each entry. > >> @@ -686,6 +704,10 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, >> val = be32_to_cpup(mux + index++); >> vals[found].reg = pcs->base + offset; >> vals[found].val = val; >> + if (params == 3) { >> + val = be32_to_cpup(mux + index++); >> + vals[found].mask = val; >> + } >> >> pin = pcs_get_pin_by_offset(pcs, offset); >> if (pin < 0) { > > Here params too would be then set during probe already. I'm afraid you lost me here... We only know if the user specified the mux configuration with pinctrl-single,pins or pinctrl-single,bits in this function. One thing I could do to make the code a bit better to look at is: int params = 2; mux = of_get_property(np, PCS_MUX_PINS_NAME, &size); if (!mux) { mux = of_get_property(np, PCS_MUX_BITS_NAME, &size); if (!mux) { dev_err(pcs->dev, "no valid property for %s\n", np->name); return -EINVAL; } params = 3; } This might make the code a bit more compact but at the same time one might need to spend few more seconds to understand it. Regards, Péter ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux 2012-09-07 15:13 ` Peter Ujfalusi @ 2012-09-07 16:55 ` Tony Lindgren 2012-09-10 11:55 ` Peter Ujfalusi 0 siblings, 1 reply; 16+ messages in thread From: Tony Lindgren @ 2012-09-07 16:55 UTC (permalink / raw) To: Peter Ujfalusi; +Cc: Linus Walleij, linux-kernel, linux-omap * Peter Ujfalusi <peter.ujfalusi@ti.com> [120907 08:13]: > On 09/06/2012 10:10 PM, Tony Lindgren wrote: > > > > Is it now safe to assume that we always have width of three if > > pinctrl-single,bits is specified? The reason I'm asking is.. > > > >> @@ -657,18 +664,29 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, > >> { > >> struct pcs_func_vals *vals; > >> const __be32 *mux; > >> - int size, rows, *pins, index = 0, found = 0, res = -ENOMEM; > >> + int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM; > >> struct pcs_function *function; > >> > >> - mux = of_get_property(np, PCS_MUX_NAME, &size); > >> - if ((!mux) || (size < sizeof(*mux) * 2)) { > >> - dev_err(pcs->dev, "bad data for mux %s\n", > >> - np->name); > >> + mux = of_get_property(np, PCS_MUX_PINS_NAME, &size); > >> + if (mux) { > >> + params = 2; > >> + } else { > >> + mux = of_get_property(np, PCS_MUX_BITS_NAME, &size); > >> + if (!mux) { > >> + dev_err(pcs->dev, "no valid property for %s\n", > >> + np->name); > >> + return -EINVAL; > >> + } > >> + params = 3; > >> + } > > > > ..because here we could assume the default value for params is 2 > > if pinctrl-single,pins is specified, and otherwise params is 3 > > if pinctrl-single,bits is specified for the controller. That would > > avoid querying a potentially non-exiting property for each entry. > > > >> @@ -686,6 +704,10 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, > >> val = be32_to_cpup(mux + index++); > >> vals[found].reg = pcs->base + offset; > >> vals[found].val = val; > >> + if (params == 3) { > >> + val = be32_to_cpup(mux + index++); > >> + vals[found].mask = val; > >> + } > >> > >> pin = pcs_get_pin_by_offset(pcs, offset); > >> if (pin < 0) { > > > > Here params too would be then set during probe already. > > I'm afraid you lost me here... > We only know if the user specified the mux configuration with > pinctrl-single,pins or pinctrl-single,bits in this function. Sorry right, yeah we don't know that at probe time currently. I'd like to have something that specifies the controller type so we don't need to mix two types of controllers and test for non-existing properties when parsing the pins. How about we require something specified for the pinctrl driver in the "one-bit-per-mux" case like pinctrl-single,bit-per-mux? And then in the pinctrl-single probe we set params = 3 if pinctrl-single,bit-per-mux is specified. And if no pinctrl-single,bit-per-mux is specified then set params = 2. That way pcs_parse_one_pinctrl_entry() can just test for params. Sorry I don't have a better name in mind than bit-per-mux.. > One thing I could do to make the code a bit better to look at is: > int params = 2; > > mux = of_get_property(np, PCS_MUX_PINS_NAME, &size); > if (!mux) { > mux = of_get_property(np, PCS_MUX_BITS_NAME, &size); > if (!mux) { > dev_err(pcs->dev, "no valid property for %s\n", > np->name); > return -EINVAL; > } > params = 3; > } > > This might make the code a bit more compact but at the same time one might > need to spend few more seconds to understand it. Yes well there's no need to do of_get_property second guessing if we already know params from the pinctrl-single.c probe time. I think we're safe to assume that we don't need to mix parsing two different types of configuration for the same controller as they can always be set up as separate controllers. Regards, Tony ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux 2012-09-07 16:55 ` Tony Lindgren @ 2012-09-10 11:55 ` Peter Ujfalusi 2012-09-10 17:10 ` Tony Lindgren 0 siblings, 1 reply; 16+ messages in thread From: Peter Ujfalusi @ 2012-09-10 11:55 UTC (permalink / raw) To: Tony Lindgren; +Cc: Linus Walleij, linux-kernel, linux-omap On 09/07/2012 07:55 PM, Tony Lindgren wrote: > * Peter Ujfalusi <peter.ujfalusi@ti.com> [120907 08:13]: >> On 09/06/2012 10:10 PM, Tony Lindgren wrote: >>> >>> Is it now safe to assume that we always have width of three if >>> pinctrl-single,bits is specified? The reason I'm asking is.. >>> >>>> @@ -657,18 +664,29 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, >>>> { >>>> struct pcs_func_vals *vals; >>>> const __be32 *mux; >>>> - int size, rows, *pins, index = 0, found = 0, res = -ENOMEM; >>>> + int size, params, rows, *pins, index = 0, found = 0, res = -ENOMEM; >>>> struct pcs_function *function; >>>> >>>> - mux = of_get_property(np, PCS_MUX_NAME, &size); >>>> - if ((!mux) || (size < sizeof(*mux) * 2)) { >>>> - dev_err(pcs->dev, "bad data for mux %s\n", >>>> - np->name); >>>> + mux = of_get_property(np, PCS_MUX_PINS_NAME, &size); >>>> + if (mux) { >>>> + params = 2; >>>> + } else { >>>> + mux = of_get_property(np, PCS_MUX_BITS_NAME, &size); >>>> + if (!mux) { >>>> + dev_err(pcs->dev, "no valid property for %s\n", >>>> + np->name); >>>> + return -EINVAL; >>>> + } >>>> + params = 3; >>>> + } >>> >>> ..because here we could assume the default value for params is 2 >>> if pinctrl-single,pins is specified, and otherwise params is 3 >>> if pinctrl-single,bits is specified for the controller. That would >>> avoid querying a potentially non-exiting property for each entry. >>> >>>> @@ -686,6 +704,10 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, >>>> val = be32_to_cpup(mux + index++); >>>> vals[found].reg = pcs->base + offset; >>>> vals[found].val = val; >>>> + if (params == 3) { >>>> + val = be32_to_cpup(mux + index++); >>>> + vals[found].mask = val; >>>> + } >>>> >>>> pin = pcs_get_pin_by_offset(pcs, offset); >>>> if (pin < 0) { >>> >>> Here params too would be then set during probe already. >> >> I'm afraid you lost me here... >> We only know if the user specified the mux configuration with >> pinctrl-single,pins or pinctrl-single,bits in this function. > > Sorry right, yeah we don't know that at probe time currently. > > I'd like to have something that specifies the controller type so > we don't need to mix two types of controllers and test for > non-existing properties when parsing the pins. > > How about we require something specified for the pinctrl driver > in the "one-bit-per-mux" case like pinctrl-single,bit-per-mux? > > And then in the pinctrl-single probe we set params = 3 if > pinctrl-single,bit-per-mux is specified. And if no > pinctrl-single,bit-per-mux is specified then set params = 2. > > That way pcs_parse_one_pinctrl_entry() can just test for params. > > Sorry I don't have a better name in mind than bit-per-mux.. I'm not sure if this would make things more prone to error or make the DTS or the code more clean in any ways. Both pinctrl-single,pins and pinctrl-single,bits works on top of the same pinctrl-single area. In most cases we are going to use pinctrl-single,pins for the mux configuration and only in few places we need to fall back to pinctrl-single,bits. With this patch we will have maximum of two query to find out which type is used, while if we add the 'bit-per-mux' property we will always have need to have two of_get_property() call to figure out what is used. IMHO in this way we could also have copy-paste errors coming at us when adding the mux configurations for the pins and at the end we also need to do same safety/sanity checks if the user really provided the correct type with correlation to the 'bit-per-mux'. This would just complicate the code. The existence of pinctrl-single,pins or pinctrl-single,bits property alone gives enough information for the number of parameters. > >> One thing I could do to make the code a bit better to look at is: >> int params = 2; >> >> mux = of_get_property(np, PCS_MUX_PINS_NAME, &size); >> if (!mux) { >> mux = of_get_property(np, PCS_MUX_BITS_NAME, &size); >> if (!mux) { >> dev_err(pcs->dev, "no valid property for %s\n", >> np->name); >> return -EINVAL; >> } >> params = 3; >> } >> >> This might make the code a bit more compact but at the same time one might >> need to spend few more seconds to understand it. > > Yes well there's no need to do of_get_property second guessing > if we already know params from the pinctrl-single.c probe time. > > I think we're safe to assume that we don't need to mix parsing > two different types of configuration for the same controller > as they can always be set up as separate controllers. > > Regards, > > Tony > -- Péter ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux 2012-09-10 11:55 ` Peter Ujfalusi @ 2012-09-10 17:10 ` Tony Lindgren 0 siblings, 0 replies; 16+ messages in thread From: Tony Lindgren @ 2012-09-10 17:10 UTC (permalink / raw) To: Peter Ujfalusi; +Cc: Linus Walleij, linux-kernel, linux-omap * Peter Ujfalusi <peter.ujfalusi@ti.com> [120910 04:55]: > On 09/07/2012 07:55 PM, Tony Lindgren wrote: > > > > I'd like to have something that specifies the controller type so > > we don't need to mix two types of controllers and test for > > non-existing properties when parsing the pins. > > > > How about we require something specified for the pinctrl driver > > in the "one-bit-per-mux" case like pinctrl-single,bit-per-mux? > > > > And then in the pinctrl-single probe we set params = 3 if > > pinctrl-single,bit-per-mux is specified. And if no > > pinctrl-single,bit-per-mux is specified then set params = 2. > > > > That way pcs_parse_one_pinctrl_entry() can just test for params. > > > > Sorry I don't have a better name in mind than bit-per-mux.. > > I'm not sure if this would make things more prone to error or make the DTS or > the code more clean in any ways. > > Both pinctrl-single,pins and pinctrl-single,bits works on top of the same > pinctrl-single area. > In most cases we are going to use pinctrl-single,pins for the mux > configuration and only in few places we need to fall back to pinctrl-single,bits. What about hardware that has tons of one-bit-per-mux type of registers? Then we end up trying to find the non-existing property potentially hundreds of times as the pinctrl-single,pins is never specified. > With this patch we will have maximum of two query to find out which type is > used, while if we add the 'bit-per-mux' property we will always have need to > have two of_get_property() call to figure out what is used. > IMHO in this way we could also have copy-paste errors coming at us when adding > the mux configurations for the pins and at the end we also need to do same > safety/sanity checks if the user really provided the correct type with > correlation to the 'bit-per-mux'. Well I think we should specify the binding for the type of the controller, not mix different types of bindings for the same controller. So we should probably have something just like address-cells, gpio-cells and interrupt-cells, although in this case it would be specific to pinctrl-single only and not be called cells. > This would just complicate the code. > The existence of pinctrl-single,pins or pinctrl-single,bits property alone > gives enough information for the number of parameters. Yes but we don't want to allow mixing different kind of registers within the same controller, they should be specified as separate controllers. Regards, Tony ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux 2012-09-05 9:01 ` [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux Peter Ujfalusi 2012-09-06 19:10 ` Tony Lindgren @ 2012-09-10 7:10 ` Linus Walleij 2012-09-10 18:49 ` Tony Lindgren 1 sibling, 1 reply; 16+ messages in thread From: Linus Walleij @ 2012-09-10 7:10 UTC (permalink / raw) To: Peter Ujfalusi; +Cc: Tony Lindgren, linux-kernel, linux-omap On Wed, Sep 5, 2012 at 11:01 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > With pinctrl-single,bits it is possible to update just part of the register > within the pinctrl-single,function-mask area. > This is useful when one register configures mmore than one pin's mux. > > pinctrl-single,bits takes three parameters: > <reg offset, value, sub-mask> > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> Do we have a conclusion on this patch 2/2? I'm holding it back until Tony explicitly ACKs it for now. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux 2012-09-10 7:10 ` Linus Walleij @ 2012-09-10 18:49 ` Tony Lindgren 0 siblings, 0 replies; 16+ messages in thread From: Tony Lindgren @ 2012-09-10 18:49 UTC (permalink / raw) To: Linus Walleij; +Cc: Peter Ujfalusi, linux-kernel, linux-omap * Linus Walleij <linus.walleij@linaro.org> [120910 00:11]: > On Wed, Sep 5, 2012 at 11:01 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > > > With pinctrl-single,bits it is possible to update just part of the register > > within the pinctrl-single,function-mask area. > > This is useful when one register configures mmore than one pin's mux. > > > > pinctrl-single,bits takes three parameters: > > <reg offset, value, sub-mask> > > > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > > Do we have a conclusion on this patch 2/2? > > I'm holding it back until Tony explicitly ACKs it for now. I don't like the idea of mixing different types of controllers here, so let's wait a little bit on 2/2. Tony ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits 2012-09-05 9:01 [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits Peter Ujfalusi 2012-09-05 9:01 ` [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask Peter Ujfalusi 2012-09-05 9:01 ` [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux Peter Ujfalusi @ 2012-09-05 12:10 ` Linus Walleij 2012-09-05 18:20 ` Tony Lindgren 2 siblings, 1 reply; 16+ messages in thread From: Linus Walleij @ 2012-09-05 12:10 UTC (permalink / raw) To: Peter Ujfalusi; +Cc: Tony Lindgren, linux-kernel, linux-omap On Wed, Sep 5, 2012 at 11:01 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > When configuring pinmux with pinctrl-single there could be a case when one > register is used to configure mux for more than one pin. > In this case the use of pinctrl-single,pins is a bit problematic since we can > only update the whole register (restricted by the mask). > In such a situations the pinctrl-single,bits could provide a safe way to handle > the mux. > > pinctrl-single,bits takes three parameters: <reg offset, value, sub-mask> > The sub mask is used to mask part of the register to make sure we do not change > bits outside of the scope of this pin. > > The first patch in this series is to fix the previous pinctrl-since,pins > implementation because it was not using the mask on the value which could result > changed bits outside of the mask. This looks sane to me, but I'd like Tony to ACK before I apply it. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits 2012-09-05 12:10 ` [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits Linus Walleij @ 2012-09-05 18:20 ` Tony Lindgren 0 siblings, 0 replies; 16+ messages in thread From: Tony Lindgren @ 2012-09-05 18:20 UTC (permalink / raw) To: Linus Walleij; +Cc: Peter Ujfalusi, linux-kernel, linux-omap * Linus Walleij <linus.walleij@linaro.org> [120905 05:11]: > On Wed, Sep 5, 2012 at 11:01 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote: > > > When configuring pinmux with pinctrl-single there could be a case when one > > register is used to configure mux for more than one pin. > > In this case the use of pinctrl-single,pins is a bit problematic since we can > > only update the whole register (restricted by the mask). > > In such a situations the pinctrl-single,bits could provide a safe way to handle > > the mux. > > > > pinctrl-single,bits takes three parameters: <reg offset, value, sub-mask> > > The sub mask is used to mask part of the register to make sure we do not change > > bits outside of the scope of this pin. > > > > The first patch in this series is to fix the previous pinctrl-since,pins > > implementation because it was not using the mask on the value which could result > > changed bits outside of the mask. > > This looks sane to me, but I'd like Tony to ACK before I apply it. Cool, this should allow handling cases where some pinctrl devices can have extra aux registers for signal strength etc for some of the pins. Will take a look. Tony ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-09-10 18:50 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-09-05 9:01 [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits Peter Ujfalusi 2012-09-05 9:01 ` [PATCH 1/2] pinctrl: pinctrl-single: Make sure we do not change bits outside of mask Peter Ujfalusi 2012-09-06 18:59 ` Tony Lindgren 2012-09-07 21:13 ` Linus Walleij 2012-09-07 21:39 ` Tony Lindgren 2012-09-10 7:09 ` Linus Walleij 2012-09-05 9:01 ` [PATCH 2/2] pinctrl: pinctrl-single: Add pinctrl-single,bits type of mux Peter Ujfalusi 2012-09-06 19:10 ` Tony Lindgren 2012-09-07 15:13 ` Peter Ujfalusi 2012-09-07 16:55 ` Tony Lindgren 2012-09-10 11:55 ` Peter Ujfalusi 2012-09-10 17:10 ` Tony Lindgren 2012-09-10 7:10 ` Linus Walleij 2012-09-10 18:49 ` Tony Lindgren 2012-09-05 12:10 ` [PATCH 0/2] pinctrl: pinctrl-single: new type: pinctrl-single,bits Linus Walleij 2012-09-05 18:20 ` Tony Lindgren
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).