| Message ID | 1346835718-21325-3-git-send-email-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {
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(-)