Message-Id: <201302242334.42408.heiko@sntech.de>
Date:	Sun, 24 Feb 2013 23:34:42 +0100
From:	Heiko Stübner <heiko@...ech.de>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Tomasz Figa <t.figa@...sung.com>,
	Prathyush K <prathyush@...omium.org>,
	Wolfram Sang <wsa@...-dreams.de>, linux-kernel@...r.kernel.org,
	Kukjin Kim <kgene.kim@...sung.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-samsung-soc@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH] pinctrl: return real error codes when pinctrl is not included

Am Sonntag, 24. Februar 2013, 01:40:58 schrieb Linus Walleij:
> On Sat, Feb 23, 2013 at 6:56 PM, Heiko Stübner <heiko@...ech.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@...omium.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@...sung.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@...sung.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@...sung.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists