| Message ID | 1353503469-18634-6-git-send-email-avinashphilip@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 21, 2012 at 06:41:02PM +0530, Philip, Avinash wrote: [...] > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > + if (IS_ERR(pinctrl)) > + dev_warn(&pdev->dev, "failed to configure pins from driver\n"); I think we already discussed this, but shouldn't this be a fatal error? Thierry
On Wed, Nov 21, 2012 at 06:41:02PM +0530, Philip, Avinash wrote:
> Enable pinctrl for pwm-tiecap
This could probably be more verbose. Maybe explain some more what
exactly this means.
Thierry
On Fri, Nov 23, 2012 at 02:29:44, Thierry Reding wrote: > On Wed, Nov 21, 2012 at 06:41:02PM +0530, Philip, Avinash wrote: > [...] > > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > > + if (IS_ERR(pinctrl)) > > + dev_warn(&pdev->dev, "failed to configure pins from driver\n"); > > I think we already discussed this, but shouldn't this be a fatal error? I had checked relevant discussion and found this can be warning message. This is because most boards don't have pinctrl implemented at this point, or may never have. https://lkml.org/lkml/2012/9/11/369 Any way I will change to dev_warn(&pdev->dev, "unable to select pin group\n"); as in other drivers. Thanks Avinash > > Thierry >
On Fri, Nov 23, 2012 at 10:34:02AM +0000, Philip, Avinash wrote: > On Fri, Nov 23, 2012 at 02:29:44, Thierry Reding wrote: > > On Wed, Nov 21, 2012 at 06:41:02PM +0530, Philip, Avinash wrote: > > [...] > > > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > > > + if (IS_ERR(pinctrl)) > > > + dev_warn(&pdev->dev, "failed to configure pins from driver\n"); > > > > I think we already discussed this, but shouldn't this be a fatal error? > > I had checked relevant discussion and found this can be warning message. > This is because most boards don't have pinctrl implemented at this point, > or may never have. > > https://lkml.org/lkml/2012/9/11/369 > > Any way I will change to > dev_warn(&pdev->dev, "unable to select pin group\n"); > as in other drivers. But in case pinctrl isn't implemented, shouldn't the implementation of devm_pinctrl_get_select_default() just be a no-op? Thierry
On Fri, Nov 23, 2012 at 16:21:10, Thierry Reding wrote: > On Fri, Nov 23, 2012 at 10:34:02AM +0000, Philip, Avinash wrote: > > On Fri, Nov 23, 2012 at 02:29:44, Thierry Reding wrote: > > > On Wed, Nov 21, 2012 at 06:41:02PM +0530, Philip, Avinash wrote: > > > [...] > > > > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > > > > + if (IS_ERR(pinctrl)) > > > > + dev_warn(&pdev->dev, "failed to configure pins from driver\n"); > > > > > > I think we already discussed this, but shouldn't this be a fatal error? > > > > I had checked relevant discussion and found this can be warning message. > > This is because most boards don't have pinctrl implemented at this point, > > or may never have. > > > > https://lkml.org/lkml/2012/9/11/369 > > > > Any way I will change to > > dev_warn(&pdev->dev, "unable to select pin group\n"); > > as in other drivers. > > But in case pinctrl isn't implemented, shouldn't the implementation of > devm_pinctrl_get_select_default() just be a no-op? So driver can still exist but pin mux functionality missing. Then warning message will give hints. I am also ok with error message & failure of driver with EPROBE_DEFER, if you want? Thanks Avinash > > Thierry >
On Fri, Nov 23, 2012 at 11:12:15AM +0000, Philip, Avinash wrote: > On Fri, Nov 23, 2012 at 16:21:10, Thierry Reding wrote: > > On Fri, Nov 23, 2012 at 10:34:02AM +0000, Philip, Avinash wrote: > > > On Fri, Nov 23, 2012 at 02:29:44, Thierry Reding wrote: > > > > On Wed, Nov 21, 2012 at 06:41:02PM +0530, Philip, Avinash wrote: > > > > [...] > > > > > + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); > > > > > + if (IS_ERR(pinctrl)) > > > > > + dev_warn(&pdev->dev, "failed to configure pins from driver\n"); > > > > > > > > I think we already discussed this, but shouldn't this be a fatal error? > > > > > > I had checked relevant discussion and found this can be warning message. > > > This is because most boards don't have pinctrl implemented at this point, > > > or may never have. > > > > > > https://lkml.org/lkml/2012/9/11/369 > > > > > > Any way I will change to > > > dev_warn(&pdev->dev, "unable to select pin group\n"); > > > as in other drivers. > > > > But in case pinctrl isn't implemented, shouldn't the implementation of > > devm_pinctrl_get_select_default() just be a no-op? > > So driver can still exist but pin mux functionality missing. Then warning > message will give hints. > I am also ok with error message & failure of driver with EPROBE_DEFER, > if you want? Everybody seems to be doing it with a warning, so I guess that's fine for now. I just find it strange that if you request the default pin group to be selected when in fact the hardware doesn't support pinctrl at all you shouldn't be getting an error either. I'm adding LinusW on Cc, perhaps he can shed some light on it. Thierry
>>>>> "Thierry" == Thierry Reding <thierry.reding@avionic-design.de> writes:
Hi,
Thierry> Everybody seems to be doing it with a warning, so I guess
Thierry> that's fine for now. I just find it strange that if you
Thierry> request the default pin group to be selected when in fact the
Thierry> hardware doesn't support pinctrl at all you shouldn't be
Thierry> getting an error either.
There's several different situations:
- Platform without pinctrl support
- Platform with pinctrl support but no pinmux specified in dt for device
(E.G. pinmux setup in bootloader)
- Pinmux specified in dt
- Some kind of misconfiguration in dt
You could argue that devm_pinctrl_get_select_default() shouldn't return
an error for the first situation, but how should it be able to know the
difference between 2 and 4?
On Fri, Nov 23, 2012 at 01:48:51PM +0100, Peter Korsgaard wrote: > >>>>> "Thierry" == Thierry Reding <thierry.reding@avionic-design.de> writes: > > Hi, > > Thierry> Everybody seems to be doing it with a warning, so I guess > Thierry> that's fine for now. I just find it strange that if you > Thierry> request the default pin group to be selected when in fact the > Thierry> hardware doesn't support pinctrl at all you shouldn't be > Thierry> getting an error either. > > There's several different situations: > > - Platform without pinctrl support > - Platform with pinctrl support but no pinmux specified in dt for device > (E.G. pinmux setup in bootloader) > - Pinmux specified in dt > - Some kind of misconfiguration in dt > > You could argue that devm_pinctrl_get_select_default() shouldn't return > an error for the first situation, but how should it be able to know the > difference between 2 and 4? In case where the platform supports pinctrl but no pinmux is specified for the device it should just assume that no pinmuxing is needed. That sounds like the most logical behaviour to me. In those cases pinctrl could just assume that the default has already been selected and not return an error. But you can't reasonably expect to cope with misconfigured DT content. Heck, there's no way for you to even know if it is misconfigured. That said, I'm not sure how much of an issue this really is. Pinmuxing is only used for functions local to a given chip, right? So if an SoC supports pinctrl and a given peripheral needs pinmuxing then we can reasonably assume that your second case can't happen, can't we? Thierry
>>>>> "Thierry" == Thierry Reding <thierry.reding@avionic-design.de> writes: Hi, >> There's several different situations: >> >> - Platform without pinctrl support >> - Platform with pinctrl support but no pinmux specified in dt for device >> (E.G. pinmux setup in bootloader) >> - Pinmux specified in dt >> - Some kind of misconfiguration in dt >> >> You could argue that devm_pinctrl_get_select_default() shouldn't return >> an error for the first situation, but how should it be able to know the >> difference between 2 and 4? Thierry> In case where the platform supports pinctrl but no pinmux is specified Thierry> for the device it should just assume that no pinmuxing is needed. That Thierry> sounds like the most logical behaviour to me. In those cases pinctrl Thierry> could just assume that the default has already been selected and not Thierry> return an error. Agreed. Thierry> But you can't reasonably expect to cope with misconfigured DT Thierry> content. Heck, there's no way for you to even know if it is Thierry> misconfigured. With 'misconfigured DT' I just mean any kind of real error. Thierry> That said, I'm not sure how much of an issue this really Thierry> is. Pinmuxing is only used for functions local to a given Thierry> chip, right? So if an SoC supports pinctrl and a given Thierry> peripheral needs pinmuxing then we can reasonably assume that Thierry> your second case can't happen, can't we? Well, peripherals only need pinmuxing if the bootloader didn't already set it up in advance. We could naturally enforce people to add "redundant" pinmux info to their .dts files, but I'm not sure I like that idea.
diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c index e0bcc85..646f8b4 100644 --- a/drivers/pwm/pwm-tiecap.c +++ b/drivers/pwm/pwm-tiecap.c @@ -26,6 +26,7 @@ #include <linux/pm_runtime.h> #include <linux/pwm.h> #include <linux/of_device.h> +#include <linux/pinctrl/consumer.h> #include "pwm-tipwmss.h" @@ -202,6 +203,11 @@ static int __devinit ecap_pwm_probe(struct platform_device *pdev) struct clk *clk; struct ecap_pwm_chip *pc; u16 status; + struct pinctrl *pinctrl; + + pinctrl = devm_pinctrl_get_select_default(&pdev->dev); + if (IS_ERR(pinctrl)) + dev_warn(&pdev->dev, "failed to configure pins from driver\n"); pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); if (!pc) {
Enable pinctrl for pwm-tiecap Signed-off-by: Philip, Avinash <avinashphilip@ti.com> --- :100644 100644 e0bcc85... 646f8b4... M drivers/pwm/pwm-tiecap.c drivers/pwm/pwm-tiecap.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)