| Message ID | CA+V-a8vwdCkyMqiynj5BodXbjwJ=e4qNb-29FhLweqbjc7DBQA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/10/2013 02:12 AM, Prabhakar Lad wrote: ... > Following is the proposed fix/hack let me know if its OK. > > Regards, > --Prabhakar > > ->>>>>>>>>>>>>>>>>>> > > diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h > index ee72f1f..78fb42d 100644 > --- a/drivers/pinctrl/core.h > +++ b/drivers/pinctrl/core.h > @@ -46,6 +46,7 @@ struct pinctrl_dev { > struct pinctrl *p; > struct pinctrl_state *hog_default; > struct pinctrl_state *hog_sleep; > + bool bits_per_mux; This clearly isn't correct; any change to solve this problem should only touch the internals of the pinctrl-single driver, not the pinctrl core.
* Stephen Warren <swarren@wwwdotorg.org> [130410 10:37]: > On 04/10/2013 02:12 AM, Prabhakar Lad wrote: > ... > > Following is the proposed fix/hack let me know if its OK. > > > > Regards, > > --Prabhakar > > > > ->>>>>>>>>>>>>>>>>>> > > > > diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h > > index ee72f1f..78fb42d 100644 > > --- a/drivers/pinctrl/core.h > > +++ b/drivers/pinctrl/core.h > > @@ -46,6 +46,7 @@ struct pinctrl_dev { > > struct pinctrl *p; > > struct pinctrl_state *hog_default; > > struct pinctrl_state *hog_sleep; > > + bool bits_per_mux; > > This clearly isn't correct; any change to solve this problem should only > touch the internals of the pinctrl-single driver, not the pinctrl core. Yeah how about just change the pintctrl-single,bits register naming to be register + bit? Something like 0xdeadbeef.0 and 0xdeadbeef.1 and so on. Regards, Tony
Hi Tony, On Thu, Apr 11, 2013 at 2:04 AM, Tony Lindgren <tony@atomide.com> wrote: > * Stephen Warren <swarren@wwwdotorg.org> [130410 10:37]: >> On 04/10/2013 02:12 AM, Prabhakar Lad wrote: >> ... >> > Following is the proposed fix/hack let me know if its OK. >> > >> > Regards, >> > --Prabhakar >> > >> > ->>>>>>>>>>>>>>>>>>> >> > >> > diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h >> > index ee72f1f..78fb42d 100644 >> > --- a/drivers/pinctrl/core.h >> > +++ b/drivers/pinctrl/core.h >> > @@ -46,6 +46,7 @@ struct pinctrl_dev { >> > struct pinctrl *p; >> > struct pinctrl_state *hog_default; >> > struct pinctrl_state *hog_sleep; >> > + bool bits_per_mux; >> >> This clearly isn't correct; any change to solve this problem should only >> touch the internals of the pinctrl-single driver, not the pinctrl core. > > Yeah how about just change the pintctrl-single,bits register > naming to be register + bit? Something like 0xdeadbeef.0 and > 0xdeadbeef.1 and so on. > How and where the changing the register naming would help ? Regards, --Prabhakar > Regards, > > Tony
Hi Tony, On Mon, Apr 15, 2013 at 10:39 AM, Prabhakar Lad <prabhakar.csengg@gmail.com> wrote: > Hi Tony, > > On Thu, Apr 11, 2013 at 2:04 AM, Tony Lindgren <tony@atomide.com> wrote: >> * Stephen Warren <swarren@wwwdotorg.org> [130410 10:37]: >>> On 04/10/2013 02:12 AM, Prabhakar Lad wrote: >>> ... >>> > Following is the proposed fix/hack let me know if its OK. >>> > >>> > Regards, >>> > --Prabhakar >>> > >>> > ->>>>>>>>>>>>>>>>>>> >>> > >>> > diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h >>> > index ee72f1f..78fb42d 100644 >>> > --- a/drivers/pinctrl/core.h >>> > +++ b/drivers/pinctrl/core.h >>> > @@ -46,6 +46,7 @@ struct pinctrl_dev { >>> > struct pinctrl *p; >>> > struct pinctrl_state *hog_default; >>> > struct pinctrl_state *hog_sleep; >>> > + bool bits_per_mux; >>> >>> This clearly isn't correct; any change to solve this problem should only >>> touch the internals of the pinctrl-single driver, not the pinctrl core. >> >> Yeah how about just change the pintctrl-single,bits register >> naming to be register + bit? Something like 0xdeadbeef.0 and >> 0xdeadbeef.1 and so on. >> > How and where the changing the register naming would help ? > No matter we change the register name, it would fail in pin_request() function, see the following code snippet static int pin_request(struct pinctrl_dev *pctldev, int pin, const char *owner, struct pinctrl_gpio_range *gpio_range) { struct pin_desc *desc; const struct pinmux_ops *ops = pctldev->desc->pmxops; int status = -EINVAL; desc = pin_desc_get(pctldev, pin); if (desc == NULL) { dev_err(pctldev->dev, "pin %d is not registered so it cannot be requested\n", pin); goto out; } dev_dbg(pctldev->dev, "request pin %d (%s) for %s\n", pin, desc->name, owner); if (gpio_range) { /* There's no need to support multiple GPIO requests */ if (desc->gpio_owner) { dev_err(pctldev->dev, "pin %s already requested by %s; cannot claim for %s\n", desc->name, desc->gpio_owner, owner); goto out; } desc->gpio_owner = owner; } else { if (desc->mux_usecount && strcmp(desc->mux_owner, owner)) { dev_err(pctldev->dev, "pin %s already requested by %s; cannot claim for %s\n", desc->name, desc->mux_owner, owner); goto out; } desc->mux_usecount++; if (desc->mux_usecount > 1) return 0; desc->mux_owner = owner; } ........... } Since desc->mux_owner and owner would obviously differ. Regards, --Prabhakar > Regards, > --Prabhakar > >> Regards, >> >> Tony
On 04/10/2013 10:34 PM, Tony Lindgren wrote: > Yeah how about just change the pintctrl-single,bits register > naming to be register + bit? Something like 0xdeadbeef.0 and > 0xdeadbeef.1 and so on. Something like this might work I think. It is going to be a bit tricky IMHO since we might need span out new 'register' every time a device requests for a new pinctrl-single,bits for already used register in the pinctrl-single,bit-per-mux area. In this way we still can make sure that certain bit are only used by a single driver.
* Peter Ujfalusi <peter.ujfalusi@ti.com> [130415 01:30]: > On 04/10/2013 10:34 PM, Tony Lindgren wrote: > > Yeah how about just change the pintctrl-single,bits register > > naming to be register + bit? Something like 0xdeadbeef.0 and > > 0xdeadbeef.1 and so on. > > Something like this might work I think. It is going to be a bit tricky IMHO > since we might need span out new 'register' every time a device requests for a > new pinctrl-single,bits for already used register in the > pinctrl-single,bit-per-mux area. In this way we still can make sure that > certain bit are only used by a single driver. OK. If it's one bit per mux type register we should be able to create the right amount of entries based on the submask in pinctrs-single,bit? Regards, Tony
On 04/16/2013 11:32 PM, Tony Lindgren wrote: > * Peter Ujfalusi <peter.ujfalusi@ti.com> [130415 01:30]: >> On 04/10/2013 10:34 PM, Tony Lindgren wrote: >>> Yeah how about just change the pintctrl-single,bits register >>> naming to be register + bit? Something like 0xdeadbeef.0 and >>> 0xdeadbeef.1 and so on. >> >> Something like this might work I think. It is going to be a bit tricky IMHO >> since we might need span out new 'register' every time a device requests for a >> new pinctrl-single,bits for already used register in the >> pinctrl-single,bit-per-mux area. In this way we still can make sure that >> certain bit are only used by a single driver. > > OK. If it's one bit per mux type register we should be able to create > the right amount of entries based on the submask in pinctrs-single,bit? Right now it seams to be true that we have one bit per mux (in DEVCONF0 on OMAP3 for example). So that would work fine, but There could be different registers around with more than one bit per mux. Another way to deal with this is to: in case of pinctrl-single,bit-per-mux we assume one bit per mux and create entries based on the pinctrl-single,function-mask's bits. In case we have more than one bit for the mux in the register we could have optional property stating the number of different muxes handled by the register. One bit per mux type: control_devconf0: pinmux@48002274 { compatible = "pinctrl-single"; reg = <0x48002274 4>; /* Single register */ #address-cells = <1>; #size-cells = <0>; pinctrl-single,bit-per-mux; pinctrl-single,register-width = <32>; pinctrl-single,function-mask = <0x5F>; }; Results six entries. control_devconf0: pinmux@48002274 { compatible = "pinctrl-single"; reg = <0x48002274 4>; /* Single register */ #address-cells = <1>; #size-cells = <0>; pinctrl-single,bit-per-mux; pinctrl-single,functions-in-register = <3>; pinctrl-single,register-width = <32>; pinctrl-single,function-mask = <0x5F>; }; Will results three entries. In both cases we still need to test overlaps in the handled bits.
* Peter Ujfalusi <peter.ujfalusi@ti.com> [130423 00:47]: > On 04/16/2013 11:32 PM, Tony Lindgren wrote: > > * Peter Ujfalusi <peter.ujfalusi@ti.com> [130415 01:30]: > >> On 04/10/2013 10:34 PM, Tony Lindgren wrote: > >>> Yeah how about just change the pintctrl-single,bits register > >>> naming to be register + bit? Something like 0xdeadbeef.0 and > >>> 0xdeadbeef.1 and so on. > >> > >> Something like this might work I think. It is going to be a bit tricky IMHO > >> since we might need span out new 'register' every time a device requests for a > >> new pinctrl-single,bits for already used register in the > >> pinctrl-single,bit-per-mux area. In this way we still can make sure that > >> certain bit are only used by a single driver. > > > > OK. If it's one bit per mux type register we should be able to create > > the right amount of entries based on the submask in pinctrs-single,bit? > > Right now it seams to be true that we have one bit per mux (in DEVCONF0 on > OMAP3 for example). So that would work fine, but There could be different > registers around with more than one bit per mux. Yes you are right, we should cover that case too. But maybe we can wait until we have such an example :) > Another way to deal with this is to: > in case of pinctrl-single,bit-per-mux we assume one bit per mux and create > entries based on the pinctrl-single,function-mask's bits. > > In case we have more than one bit for the mux in the register we could have > optional property stating the number of different muxes handled by the register. Yes that's doable. > One bit per mux type: > > control_devconf0: pinmux@48002274 { > compatible = "pinctrl-single"; > reg = <0x48002274 4>; /* Single register */ > #address-cells = <1>; > #size-cells = <0>; > pinctrl-single,bit-per-mux; > pinctrl-single,register-width = <32>; > pinctrl-single,function-mask = <0x5F>; > }; > > Results six entries. Yup, I think this is the way to go for now, see below.. > control_devconf0: pinmux@48002274 { > compatible = "pinctrl-single"; > reg = <0x48002274 4>; /* Single register */ > #address-cells = <1>; > #size-cells = <0>; > pinctrl-single,bit-per-mux; > pinctrl-single,functions-in-register = <3>; > pinctrl-single,register-width = <32>; > pinctrl-single,function-mask = <0x5F>; > }; > > Will results three entries. ..but let's not add this. The reason why I'd like to postpone adding pinctrl-single,functions-in-register is because we may be able to do it automatically. If it was just the naming is the issue, we could use the submask for the naming the bits with something like 0x0x48002274-3-1 and 0x0x48002274-1-1. But I think currently the pinctrl framework expects a static pin table, so we can't create these entries dynamically when the consumer driver starts using the bits. So the right fix there would be to improve the pinctrl framework to allow adding pins dynamically, see some of the REVISIT comments in pinctrl-single.c. > In both cases we still need to test overlaps in the handled bits. Yes that's missing too. So how about we do the following for now: 1. Fix the pinctrl-single,bits naming to use 0x0x48002274-1-1 to represent the bit range 2. Automatically create entries for each bit and don't yet support bit ranges except in the naming for future use 3. Add something to check the overlaps in the submasks 4. Assume we have one-bit-per-mux until we have a real example, then improve the pinctrl framework to allow adding pins dynamically as the consumer driver probes. Or if there are other issues, add the pinctrl-single,functions-in-register property at that point. Regards, Tony
Hi, On Tue, Apr 23, 2013 at 23:47:54, Tony Lindgren wrote: > * Peter Ujfalusi <peter.ujfalusi@ti.com> [130423 00:47]: > > On 04/16/2013 11:32 PM, Tony Lindgren wrote: > > > * Peter Ujfalusi <peter.ujfalusi@ti.com> [130415 01:30]: > > >> On 04/10/2013 10:34 PM, Tony Lindgren wrote: > > >>> Yeah how about just change the pintctrl-single,bits register > > >>> naming to be register + bit? Something like 0xdeadbeef.0 and > > >>> 0xdeadbeef.1 and so on. > > >> > > >> Something like this might work I think. It is going to be a bit tricky IMHO > > >> since we might need span out new 'register' every time a device requests for a > > >> new pinctrl-single,bits for already used register in the > > >> pinctrl-single,bit-per-mux area. In this way we still can make sure that > > >> certain bit are only used by a single driver. > > > > > > OK. If it's one bit per mux type register we should be able to create > > > the right amount of entries based on the submask in pinctrs-single,bit? > > > > Right now it seams to be true that we have one bit per mux (in DEVCONF0 on > > OMAP3 for example). So that would work fine, but There could be different > > registers around with more than one bit per mux. > > Yes you are right, we should cover that case too. But maybe we > can wait until we have such an example :) > > > Another way to deal with this is to: > > in case of pinctrl-single,bit-per-mux we assume one bit per mux and create > > entries based on the pinctrl-single,function-mask's bits. > > > > In case we have more than one bit for the mux in the register we could have > > optional property stating the number of different muxes handled by the register. > > Yes that's doable. > > > One bit per mux type: > > > > control_devconf0: pinmux@48002274 { > > compatible = "pinctrl-single"; > > reg = <0x48002274 4>; /* Single register */ > > #address-cells = <1>; > > #size-cells = <0>; > > pinctrl-single,bit-per-mux; > > pinctrl-single,register-width = <32>; > > pinctrl-single,function-mask = <0x5F>; > > }; > > > > Results six entries. > > Yup, I think this is the way to go for now, see below.. > > > control_devconf0: pinmux@48002274 { > > compatible = "pinctrl-single"; > > reg = <0x48002274 4>; /* Single register */ > > #address-cells = <1>; > > #size-cells = <0>; > > pinctrl-single,bit-per-mux; > > pinctrl-single,functions-in-register = <3>; > > pinctrl-single,register-width = <32>; > > pinctrl-single,function-mask = <0x5F>; > > }; > > > > Will results three entries. > > ..but let's not add this. The reason why I'd like to postpone adding > pinctrl-single,functions-in-register is because we may be able to > do it automatically. > I have posted patch to take care of this: http://davinci-linux-open-source.1494791.n2.nabble.com/PATCH-0-3-pinctrl-pinctrl-single-Add-full-fledge-support-to-configure-multiple-pins-of-different-mods-tt7583066.html Please review this solution. Thanks, Prakash
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h index ee72f1f..78fb42d 100644 --- a/drivers/pinctrl/core.h +++ b/drivers/pinctrl/core.h @@ -46,6 +46,7 @@ struct pinctrl_dev { struct pinctrl *p; struct pinctrl_state *hog_default; struct pinctrl_state *hog_sleep; + bool bits_per_mux; #ifdef CONFIG_DEBUG_FS struct dentry *device_root; #endif diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 5c32e88..d2a91ec 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -974,6 +974,7 @@ static int pcs_probe(struct platform_device *pdev) ret = -EINVAL; goto free; } + pcs->pctl->bits_per_mux = pcs->bits_per_mux; dev_info(pcs->dev, "%i pins at pa %p size %u\n", pcs->desc.npins, pcs->base, pcs->size); diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c index 1a00658..4989f01 100644 --- a/drivers/pinctrl/pinmux.c +++ b/drivers/pinctrl/pinmux.c @@ -110,7 +110,7 @@ static int pin_request(struct pinctrl_dev *pctldev, desc->gpio_owner = owner; } else { - if (desc->mux_usecount && strcmp(desc->mux_owner, owner)) { + if (!pctldev->bits_per_mux && desc->mux_usecount && strcmp(desc->mux_owner, owner)) { dev_err(pctldev->dev, "pin %s already requested by %s;