[PATCH] pinctrl or i2c-s3c2410: fix pinctrl NULL return values in stubs

[PATCH] pinctrl or i2c-s3c2410: fix pinctrl NULL return values in stubs

[Heiko Stübner]

Hi,

while implementing devicetree support for the s3c2416 I noticed a fault
in the i2c-s3c2410 driver. The s3c2416 does not support pinctrl at the
moment (and will probably for a while), so the fallback functions in
pinctrl/consumer.h were used. These functions fail silently and the
relevant pinctrl_get only returns NULL but the i2c driver only checked
for real error-pointers. This resulted in the i2c gpios not getting
configured at all.

There are of course two possible solutions. Check for NULL pinctrl
handles in the driver or return meaningful error codes in the pinctrl
stubs. All other pinctrl drivers also only seem to handle real error
codes and would gladly accept NULL handles, so I'm not sure which is
the correct fix to not break to much existing code.

Therefore I implemented both variants and you get to pick :-) .
This of course means from the following patches only one is necessary.


Heiko

[PATCH] pinctrl: return real error codes when pinctrl is not included

[Heiko Stübner]

Currently the fallback functions when pinctrl is not being built do
return either NULL or 0, either no pinctrl handle or no error,
making them fail silently.

All drivers using pinctrl do only test for error conditions, which
made for example the i2c-s3c2410 driver fail on a devicetree based
machine without pinctrl, as the conditional
	if (IS_ERR(i2c->pctrl) && s3c24xx_i2c_parse_dt_gpio(i2c))
did not reach the second part to initialize the gpios from dt.

Therefore let the fallback pinctrl functions return -ENOTSUPP
or the equivalent ERR_PTR to indicate that pinctrl is not supported.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 include/linux/pinctrl/consumer.h |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 4aad3ce..69d145f 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -44,7 +44,7 @@ extern void devm_pinctrl_put(struct pinctrl *p);
 
 static inline int pinctrl_request_gpio(unsigned gpio)
 {
-	return 0;
+	return -ENOTSUPP;
 }
 
 static inline void pinctrl_free_gpio(unsigned gpio)
@@ -53,17 +53,17 @@ static inline void pinctrl_free_gpio(unsigned gpio)
 
 static inline int pinctrl_gpio_direction_input(unsigned gpio)
 {
-	return 0;
+	return -ENOTSUPP;
 }
 
 static inline int pinctrl_gpio_direction_output(unsigned gpio)
 {
-	return 0;
+	return -ENOTSUPP;
 }
 
 static inline struct pinctrl * __must_check pinctrl_get(struct device *dev)
 {
-	return NULL;
+	return ERR_PTR(-ENOTSUPP);
 }
 
 static inline void pinctrl_put(struct pinctrl *p)
@@ -74,18 +74,18 @@ static inline struct pinctrl_state * __must_check pinctrl_lookup_state(
 							struct pinctrl *p,
 							const char *name)
 {
-	return NULL;
+	return ERR_PTR(-ENOTSUPP);
 }
 
 static inline int pinctrl_select_state(struct pinctrl *p,
 				       struct pinctrl_state *s)
 {
-	return 0;
+	return -ENOTSUPP;
 }
 
 static inline struct pinctrl * __must_check devm_pinctrl_get(struct device *dev)
 {
-	return NULL;
+	return ERR_PTR(-ENOTSUPP);
 }
 
 static inline void devm_pinctrl_put(struct pinctrl *p)
-- 
1.7.2.3

[PATCH] i2c: s3c2410: check for NULL pinctrl handle

[Heiko Stübner]

When pinctrl is not built the fallback functions fail silently
and emit either 0 error codes or NULL pinctrl handles.

Therefore it's needed to also check for this NULL-handle when
falling back to parsing the i2c gpios from devicetree.

Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-s3c2410.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index f6b880b..e58337f 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -1060,7 +1060,8 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 
 	if (i2c->pdata->cfg_gpio) {
 		i2c->pdata->cfg_gpio(to_platform_device(i2c->dev));
-	} else if (IS_ERR(i2c->pctrl) && s3c24xx_i2c_parse_dt_gpio(i2c)) {
+	} else if ((!i2c->pctrl || IS_ERR(i2c->pctrl)) &&
+		   s3c24xx_i2c_parse_dt_gpio(i2c)) {
 		return -EINVAL;
 	}
 
-- 
1.7.2.3

Re: [PATCH] i2c: s3c2410: check for NULL pinctrl handle

[Linus Walleij]

On Sat, Feb 23, 2013 at 6:57 PM, Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:

> When pinctrl is not built the fallback functions fail silently
> and emit either 0 error codes or NULL pinctrl handles.
>
> Therefore it's needed to also check for this NULL-handle when
> falling back to parsing the i2c gpios from devicetree.
>
> Signed-off-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>

NAK.

This is not the right solution for this driver.

It uses pinctrl in a very simplistic way, just grabbing the
default handler.

After commit
ab78029ecc347debbd737f06688d788bd9d60c1d:
"drivers/pinctrl: grab default handles from device core"

The right solution is to simply revert commit
2693ac69880a33d4d9df6f128415b65e745f00ba
"i2c: s3c2410: Add support for pinctrl"

Tomasz are you OK with this, or will you add more
fine-grained pinctrl (like runtime PM etc) to this driver?

Yours,
Linus Walleij

Re: [PATCH] i2c: s3c2410: check for NULL pinctrl handle

[Tomasz Figa]

Hi Linus,

On Sunday 24 of February 2013 01:16:21 Linus Walleij wrote:
> On Sat, Feb 23, 2013 at 6:57 PM, Heiko Stübner <heiko@sntech.de> wrote:
> > When pinctrl is not built the fallback functions fail silently
> > and emit either 0 error codes or NULL pinctrl handles.
> > 
> > Therefore it's needed to also check for this NULL-handle when
> > falling back to parsing the i2c gpios from devicetree.
> > 
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> 
> NAK.
> 
> This is not the right solution for this driver.
> 
> It uses pinctrl in a very simplistic way, just grabbing the
> default handler.
> 
> After commit
> ab78029ecc347debbd737f06688d788bd9d60c1d:
> "drivers/pinctrl: grab default handles from device core"
> 
> The right solution is to simply revert commit
> 2693ac69880a33d4d9df6f128415b65e745f00ba
> "i2c: s3c2410: Add support for pinctrl"
> 
> Tomasz are you OK with this, or will you add more
> fine-grained pinctrl (like runtime PM etc) to this driver?

Yes, I'm fine. However reverting this patch will not solve the problem 
completely.

There are 3 methods of pin configuration that has to be supported by this 
driver (and several other drivers):
 1) cfg_gpio callback passed in platform data,
 2) legacy Samsung GPIO bindings (to be dropped ASAP),
 3) pin control.
Each supported platform will support only one of these methods at the same 
time.

The first one is already handled correctly because it is always used 
wherever it is available. The problem is with the remaining two.

The driver must know whether pin control is available, because it has to 
fall back to legacy GPIO-based pin configuration if it is not. This means 
that we must either check for NULL (which probably is not right, since 
returned handle is considered to be opaque) or pin control core must 
return an error code specific to this situation, e.g. -ENODEV.

Keep in mind that there is no way to check whether method 2) succeeded, 
because all it does is parsing GPIOs from device tree, assuming that the 
custom xlate function of the old Samsung GPIO driver would do all the 
configuration.

I do not see another solution of this problem. Feel free to suggest 
anything better.

Best regards,
Tomasz

Re: [PATCH] pinctrl: return real error codes when pinctrl is not included

[Linus Walleij]

On Sat, Feb 23, 2013 at 6:56 PM, Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:

> Currently the fallback functions when pinctrl is not being built do
> return either NULL or 0, either no pinctrl handle or no error,
> making them fail silently.
>
> All drivers using pinctrl do only test for error conditions, which
> made for example the i2c-s3c2410 driver fail on a devicetree based
> machine without pinctrl, as the conditional
>         if (IS_ERR(i2c->pctrl) && s3c24xx_i2c_parse_dt_gpio(i2c))
> did not reach the second part to initialize the gpios from dt.
>
> Therefore let the fallback pinctrl functions return -ENOTSUPP
> or the equivalent ERR_PTR to indicate that pinctrl is not supported.

NAK.

You are abusing IS_ERR(i2c->pctrl) to check if pinctrl is available
on the system which is *NOT* the intended usecase and that is
why your code fails.

So I discussed the *exact* same thing with Tomasz and Pratyush a
while back in a private thread, which teaches me to probably not
waste time on responding to anything unless it's public.

This make me suspect that you have this ugly patch in some
private repo and I will be seeing it again and again :-(

Here is an excerpt of that conversation:

-------------8<---------------------------8<--------------------------8<----------------------

On Mon, Dec 24, 2012 at 12:26 PM, Prathyush K <prathyush-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:

> Exynos5250 does not yet support pinctrl so CONFIG_PINCTRL is not defined.
>
> The i2c-s3c2410 driver calls 'devm_pinctrl_get_select_default' which returns
> NULL.
> While checking for error, we use IS_ERR and not IS_ERR_OR_NULL inside the
> driver.
> This was causing the i2c hdmi phy to fail.
>
> I checked the other i2c drivers and realized, no-one is actually checking
> for IS_ERR_OR_NULL.
>
> Even ./Documentation/pinctrl.txt says:
>
>         /* Setup */
>         p = devm_pinctrl_get(&device);
>         if (IS_ERR(p))
>
> I think the consumer.h should be modified to return an error and not just
> NULL if CONFIG_PINCTRL is not defined.

No. That is not the idea with compile-out stubs.

The idea is not to check at runtime whether you have this or that
framework enabled, if you need this framework for the driver to work
it should be depends on PINCTRL in Kconfig for the driver so it's non-optional.
This seems to be the case here?

The stubs are for the case where pinctrl is optional for the driver(s) on the
system, for example if one driver is used on many diverse systems,
some which have pinctrl and some which does not.

We cannot have a shared driver, like drivers/tty/serial/amba-pl011.c
fail on RealView just because the U300 need pins when using the
same driver, that would be unmanageable.

Your reasoning above can only work in a world where e.g. all systems
using that driver have pinctrl. And then you can just depend on it in
Kconfig and problem is solved.

On Thu, Dec 27, 2012 at 10:58 AM, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:

> I think that devm_pinctrl_get stub, as well as other pinctrl stubs, should
> not return NULL, but rather a reasonable error code.

No. It is perfectly valid to not implement pinctrl on a system.
And the driver stubs should then work without pins being controlled
(for example as if they were set up at boot time by some bootloader,
or ROM.)

If the driver does not work without pinctrl it should have
depends on PINCTRL in Kconfig.

On Thu, Jan 17, 2013 at 9:50 AM, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:

>> If the driver does not work without pinctrl it should have
>> depends on PINCTRL in Kconfig.
>
> What about drivers that support several pin configuration methods?

The platforms that use some other method shall call
pinctrl_provide_dummies() in their machine set-up code.

Then they will get dummy regulators that appear to work
but does nothing.

Then they shall be converted to use pinctrl proper.

On Thu, Jan 17, 2013 at 3:05 PM, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:

> We have a bunch of drivers that should try pin control and if it is not
> supported on given platform then it should fall back to legacy
> configuration method (like cfg_gpio() callback passed through platform
> data).
>
> From what you are saying, it seems like there is no way to check if pin
> control is supported on platform we are running on.

No we never saw that usecase before :-)

It's very hard to say whether pinctrl is available or not as well,
as drivers can be deferred.

In any case the stubs are not designed to solve this type of problem.

> Of course this can be resolved by trying the legacy method first and try
> pin control only if it is not provided (no platform data or NULL callback
> pointer).

I would just set some bool value in platform_data like .use_legacy_pins
if you need to support this.

-------------8<---------------------------8<--------------------------8<----------------------

All chrystal clear now?

Plus, I think that now that the device core is handling pin control you
should be able to just remove all this default-enabling from your
drivers.

*NO* using of pinctrl to check if "do we need to do it some other
way".

Yours,
Linus Walleij

Re: [PATCH] i2c: s3c2410: check for NULL pinctrl handle

[Linus Walleij]

On Sun, Feb 24, 2013 at 1:38 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:

> The driver must know whether pin control is available, because it has to
> fall back to legacy GPIO-based pin configuration if it is not. This means
> that we must either check for NULL (which probably is not right, since
> returned handle is considered to be opaque) or pin control core must
> return an error code specific to this situation, e.g. -ENODEV.

OK so pass a flag like a bool in your platform data from the
machine like go into <linux/platform_data/i2c-s3c2410.h>
and add:

struct s3c2410_platform_i2c {
        bool  use_that_old_gpio_interface;
        (...)
};

Instead of trying to semi-guess if the pinctrl framework is there?

Surely you know this when setting up the pdata from your machine?

Yours,
Linus Walleij

Re: [PATCH] i2c: s3c2410: check for NULL pinctrl handle

[Tomasz Figa]

On Sunday 24 of February 2013 01:47:49 Linus Walleij wrote:
> On Sun, Feb 24, 2013 at 1:38 AM, Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 
wrote:
> > The driver must know whether pin control is available, because it has
> > to fall back to legacy GPIO-based pin configuration if it is not.
> > This means that we must either check for NULL (which probably is not
> > right, since returned handle is considered to be opaque) or pin
> > control core must return an error code specific to this situation,
> > e.g. -ENODEV.
> OK so pass a flag like a bool in your platform data from the
> machine like go into <linux/platform_data/i2c-s3c2410.h>
> and add:
> 
> struct s3c2410_platform_i2c {
>         bool  use_that_old_gpio_interface;
>         (...)
> };
> 
> Instead of trying to semi-guess if the pinctrl framework is there?
> 
> Surely you know this when setting up the pdata from your machine?

Cases 2) and 3) are both DT-enabled cases, where there is no pdata coming 
from board-specific code.

In case of this particular driver, a check for presence of "gpios" 
property will be enough, because this driver does not use GPIO pins 
normally - they are just used for pin configuration, when legacy method 
has to be used.

However it will not work in case of drivers that also need some GPIO pins 
unrelated to the pins that have to be configured to dedicated functions.

Note that we are talking here about a temporary solution. The legacy DT-
based pin configuration will go away after all the DT-enabled platforms 
using this driver get migrated to pin control and so will the need to 
check if pin control is available.

Best regards,
Tomasz

Re: [PATCH] i2c: s3c2410: check for NULL pinctrl handle

[Linus Walleij]

On Sun, Feb 24, 2013 at 1:58 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:

> [Me]
>> Surely you know this when setting up the pdata from your machine?
>
> Cases 2) and 3) are both DT-enabled cases, where there is no pdata coming
> from board-specific code.
(...)
> Note that we are talking here about a temporary solution. The legacy DT-
> based pin configuration will go away after all the DT-enabled platforms
> using this driver get migrated to pin control and so will the need to
> check if pin control is available.

So use AUXDATA, and you get a pdata for that driver?

Yours,
Linus Walleij

Re: [PATCH] i2c: s3c2410: check for NULL pinctrl handle

[Tomasz Figa]

On Sunday 24 of February 2013 02:01:45 Linus Walleij wrote:
> On Sun, Feb 24, 2013 at 1:58 AM, Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 
wrote:
> > [Me]
> > 
> >> Surely you know this when setting up the pdata from your machine?
> > 
> > Cases 2) and 3) are both DT-enabled cases, where there is no pdata
> > coming from board-specific code.
> 
> (...)
> 
> > Note that we are talking here about a temporary solution. The legacy
> > DT- based pin configuration will go away after all the DT-enabled
> > platforms using this driver get migrated to pin control and so will
> > the need to check if pin control is available.
> 
> So use AUXDATA, and you get a pdata for that driver?

Hmm, and then have some platform data passed statically and some parsed 
from device tree? Not even saying that we are going towards getting rid of 
auxdata, not adding further dependencies for it.

Sorry, but this sounds more broken to me than checking the return value of 
devm_pinctrl_get_select_default for NULL in the driver.

Still, all the platforms relying on the legacy DT GPIO support should have 
been already migrated to pin control, so ideally instead of "fixing" the 
drivers to continue supporting the deprecated method, such platforms 
should be fixed.

Best regards,
Tomasz

Re: [PATCH] pinctrl: return real error codes when pinctrl is not included

[Heiko Stübner]

Am Sonntag, 24. Februar 2013, 01:40:58 schrieb Linus Walleij:
> On Sat, Feb 23, 2013 at 6:56 PM, Heiko Stübner <heiko@sntech.de> wrote:
> > Currently the fallback functions when pinctrl is not being built do
> > return either NULL or 0, either no pinctrl handle or no error,
> > making them fail silently.
> > 
> > All drivers using pinctrl do only test for error conditions, which
> > made for example the i2c-s3c2410 driver fail on a devicetree based
> > machine without pinctrl, as the conditional
> > 
> >         if (IS_ERR(i2c->pctrl) && s3c24xx_i2c_parse_dt_gpio(i2c))
> > 
> > did not reach the second part to initialize the gpios from dt.
> > 
> > Therefore let the fallback pinctrl functions return -ENOTSUPP
> > or the equivalent ERR_PTR to indicate that pinctrl is not supported.
> 
> NAK.
> 
> You are abusing IS_ERR(i2c->pctrl) to check if pinctrl is available
> on the system which is *NOT* the intended usecase and that is
> why your code fails.
> 
> So I discussed the *exact* same thing with Tomasz and Pratyush a
> while back in a private thread, which teaches me to probably not
> waste time on responding to anything unless it's public.
> 
> This make me suspect that you have this ugly patch in some
> private repo and I will be seeing it again and again :-(

All my s3c24xx work is done is my spare time, so I have to confess I came up 
with this "ugly patch" all by myself when working on dt support for my machine 
and stumbling upon the described problem with the pin configuration.

So, as it is obviously wrong, I also won't hold onto it.

In any case, thanks for the thorough explanation, which I probably won't 
forget anytime soon.


Heiko


> Here is an excerpt of that conversation:
> 
> -------------8<---------------------------8<--------------------------8<---
> -------------------
> 
> On Mon, Dec 24, 2012 at 12:26 PM, Prathyush K <prathyush@chromium.org> 
wrote:
> > Exynos5250 does not yet support pinctrl so CONFIG_PINCTRL is not defined.
> > 
> > The i2c-s3c2410 driver calls 'devm_pinctrl_get_select_default' which
> > returns NULL.
> > While checking for error, we use IS_ERR and not IS_ERR_OR_NULL inside the
> > driver.
> > This was causing the i2c hdmi phy to fail.
> > 
> > I checked the other i2c drivers and realized, no-one is actually checking
> > for IS_ERR_OR_NULL.
> > 
> > Even ./Documentation/pinctrl.txt says:
> >         /* Setup */
> >         p = devm_pinctrl_get(&device);
> >         if (IS_ERR(p))
> > 
> > I think the consumer.h should be modified to return an error and not just
> > NULL if CONFIG_PINCTRL is not defined.
> 
> No. That is not the idea with compile-out stubs.
> 
> The idea is not to check at runtime whether you have this or that
> framework enabled, if you need this framework for the driver to work
> it should be depends on PINCTRL in Kconfig for the driver so it's
> non-optional. This seems to be the case here?
> 
> The stubs are for the case where pinctrl is optional for the driver(s) on
> the system, for example if one driver is used on many diverse systems,
> some which have pinctrl and some which does not.
> 
> We cannot have a shared driver, like drivers/tty/serial/amba-pl011.c
> fail on RealView just because the U300 need pins when using the
> same driver, that would be unmanageable.
> 
> Your reasoning above can only work in a world where e.g. all systems
> using that driver have pinctrl. And then you can just depend on it in
> Kconfig and problem is solved.
> 
> On Thu, Dec 27, 2012 at 10:58 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> > I think that devm_pinctrl_get stub, as well as other pinctrl stubs,
> > should not return NULL, but rather a reasonable error code.
> 
> No. It is perfectly valid to not implement pinctrl on a system.
> And the driver stubs should then work without pins being controlled
> (for example as if they were set up at boot time by some bootloader,
> or ROM.)
> 
> If the driver does not work without pinctrl it should have
> depends on PINCTRL in Kconfig.
> 
> On Thu, Jan 17, 2013 at 9:50 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> >> If the driver does not work without pinctrl it should have
> >> depends on PINCTRL in Kconfig.
> > 
> > What about drivers that support several pin configuration methods?
> 
> The platforms that use some other method shall call
> pinctrl_provide_dummies() in their machine set-up code.
> 
> Then they will get dummy regulators that appear to work
> but does nothing.
> 
> Then they shall be converted to use pinctrl proper.
> 
> On Thu, Jan 17, 2013 at 3:05 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> > We have a bunch of drivers that should try pin control and if it is not
> > supported on given platform then it should fall back to legacy
> > configuration method (like cfg_gpio() callback passed through platform
> > data).
> > 
> > From what you are saying, it seems like there is no way to check if pin
> > control is supported on platform we are running on.
> 
> No we never saw that usecase before :-)
> 
> It's very hard to say whether pinctrl is available or not as well,
> as drivers can be deferred.
> 
> In any case the stubs are not designed to solve this type of problem.
> 
> > Of course this can be resolved by trying the legacy method first and try
> > pin control only if it is not provided (no platform data or NULL callback
> > pointer).
> 
> I would just set some bool value in platform_data like .use_legacy_pins
> if you need to support this.
> 
> -------------8<---------------------------8<--------------------------8<---
> -------------------
> 
> All chrystal clear now?
> 
> Plus, I think that now that the device core is handling pin control you
> should be able to just remove all this default-enabling from your
> drivers.
> 
> *NO* using of pinctrl to check if "do we need to do it some other
> way".
> 
> Yours,
> Linus Walleij

Re: [PATCH] i2c: s3c2410: check for NULL pinctrl handle

[Linus Walleij]

On Sun, Feb 24, 2013 at 6:00 PM, Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

>> > Note that we are talking here about a temporary solution. The legacy
>> > DT- based pin configuration will go away after all the DT-enabled
>> > platforms using this driver get migrated to pin control and so will
>> > the need to check if pin control is available.
>>
>> So use AUXDATA, and you get a pdata for that driver?
>
> Hmm, and then have some platform data passed statically and some parsed
> from device tree?

This is done by several in-tree drivers today. It is even necessary for things
like machine-specific callbacks.

> Not even saying that we are going towards getting rid of
> auxdata, not adding further dependencies for it.

The other option is to do the non-temporary solution you are
referring to below...

> Sorry, but this sounds more broken to me than checking the return value of
> devm_pinctrl_get_select_default for NULL in the driver.

Both are bad solution, auxdata is less bad than trying to check
struct pinctrl * handles for non-NULL, which has *never* been a
good thing to do and should never have been merged in the first
place.

(Maybe I ACKed that, then I was doing something stupid.)

> Still, all the platforms relying on the legacy DT GPIO support should have
> been already migrated to pin control, so ideally instead of "fixing" the
> drivers to continue supporting the deprecated method, such platforms
> should be fixed.

I agree.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: return real error codes when pinctrl is not included

[Linus Walleij]

On Sun, Feb 24, 2013 at 11:34 PM, Heiko Stübner <heiko@sntech.de> wrote:

> [Me]
>> This make me suspect that you have this ugly patch in some
>> private repo and I will be seeing it again and again :-(
>
> All my s3c24xx work is done is my spare time, so I have to confess I came up
> with this "ugly patch" all by myself when working on dt support for my machine
> and stumbling upon the described problem with the pin configuration.
>
> So, as it is obviously wrong, I also won't hold onto it.
>
> In any case, thanks for the thorough explanation, which I probably won't
> forget anytime soon.

Hm, maybe I have come across as too harsh and then I feel bad about
that :-(

I really want spare-time contributors, they are often more valueable
in addition to the corporate ones since they provide an outside view
of things.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: return real error codes when pinctrl is not included

[Heiko Stübner]

Am Sonntag, 24. Februar 2013, 23:42:32 schrieb Linus Walleij:
> On Sun, Feb 24, 2013 at 11:34 PM, Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
> > [Me]
> > 
> >> This make me suspect that you have this ugly patch in some
> >> private repo and I will be seeing it again and again :-(
> > 
> > All my s3c24xx work is done is my spare time, so I have to confess I came
> > up with this "ugly patch" all by myself when working on dt support for
> > my machine and stumbling upon the described problem with the pin
> > configuration.
> > 
> > So, as it is obviously wrong, I also won't hold onto it.
> > 
> > In any case, thanks for the thorough explanation, which I probably won't
> > forget anytime soon.
> 
> Hm, maybe I have come across as too harsh and then I feel bad about
> that :-(
>
> I really want spare-time contributors, they are often more valueable
> in addition to the corporate ones since they provide an outside view
> of things.

Just to ease your mind, it didn't sound to harsh and my mail wasn't meant to 
criticize (hopefully it didn't sound that way) - I am very much grateful for 
the explanation.

Writing a pinctrl driver for the s3c24xx arches is still on my list, so my 
understanding of pinctrl (above knowing of its existence) is not very broad 
yet. With your explanation, in retrospect I can understand the ugliness of the 
patch now :-) as it goes against a core principle.


Heiko

Re: [PATCH] i2c: s3c2410: check for NULL pinctrl handle

[Heiko Stübner]

Am Sonntag, 24. Februar 2013, 23:39:44 schrieb Linus Walleij:
> On Sun, Feb 24, 2013 at 6:00 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> >> > Note that we are talking here about a temporary solution. The legacy
> >> > DT- based pin configuration will go away after all the DT-enabled
> >> > platforms using this driver get migrated to pin control and so will
> >> > the need to check if pin control is available.
> >> 
> >> So use AUXDATA, and you get a pdata for that driver?
> > 
> > Hmm, and then have some platform data passed statically and some parsed
> > from device tree?
> 
> This is done by several in-tree drivers today. It is even necessary for
> things like machine-specific callbacks.
> 
> > Not even saying that we are going towards getting rid of
> > auxdata, not adding further dependencies for it.
> 
> The other option is to do the non-temporary solution you are
> referring to below...
> 
> > Sorry, but this sounds more broken to me than checking the return value
> > of devm_pinctrl_get_select_default for NULL in the driver.
> 
> Both are bad solution, auxdata is less bad than trying to check
> struct pinctrl * handles for non-NULL, which has *never* been a
> good thing to do and should never have been merged in the first
> place.
> 
> (Maybe I ACKed that, then I was doing something stupid.)
> 
> > Still, all the platforms relying on the legacy DT GPIO support should
> > have been already migrated to pin control, so ideally instead of
> > "fixing" the drivers to continue supporting the deprecated method, such
> > platforms should be fixed.
> 
> I agree.

Fine by me ... I'll work on a pinctrl driver for s3c24xx then :-)


Heiko

Re: [PATCH] i2c: s3c2410: check for NULL pinctrl handle

[Tomasz Figa]

Hi,

On Monday 25 of February 2013 00:16:49 Heiko Stübner wrote:
> Am Sonntag, 24. Februar 2013, 23:39:44 schrieb Linus Walleij:
> > On Sun, Feb 24, 2013 at 6:00 PM, Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 
wrote:
> > >> > Note that we are talking here about a temporary solution. The
> > >> > legacy
> > >> > DT- based pin configuration will go away after all the DT-enabled
> > >> > platforms using this driver get migrated to pin control and so
> > >> > will
> > >> > the need to check if pin control is available.
> > >> 
> > >> So use AUXDATA, and you get a pdata for that driver?
> > > 
> > > Hmm, and then have some platform data passed statically and some
> > > parsed
> > > from device tree?
> > 
> > This is done by several in-tree drivers today. It is even necessary
> > for
> > things like machine-specific callbacks.
> > 
> > > Not even saying that we are going towards getting rid of
> > > auxdata, not adding further dependencies for it.
> > 
> > The other option is to do the non-temporary solution you are
> > referring to below...
> > 
> > > Sorry, but this sounds more broken to me than checking the return
> > > value
> > > of devm_pinctrl_get_select_default for NULL in the driver.
> > 
> > Both are bad solution, auxdata is less bad than trying to check
> > struct pinctrl * handles for non-NULL, which has *never* been a
> > good thing to do and should never have been merged in the first
> > place.
> > 
> > (Maybe I ACKed that, then I was doing something stupid.)
> > 
> > > Still, all the platforms relying on the legacy DT GPIO support
> > > should
> > > have been already migrated to pin control, so ideally instead of
> > > "fixing" the drivers to continue supporting the deprecated method,
> > > such
> > > platforms should be fixed.
> > 
> > I agree.
> 
> Fine by me ... I'll work on a pinctrl driver for s3c24xx then :-)

Good.

I belive that pinctrl-samsung driver can be easily used for s3c24xx, with 
minor modifications to remove some assumptions that are true for all the 
newer Samsung SoCs.

I'm currently working (in my free time) on pinctrl driver for s3c64xx (as 
a part of my DT-enablement work), so possibly some of the code I will 
create may be useful for s3c24xx as well.

Best regards,
Tomasz