From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko =?iso-8859-1?q?St=FCbner?= Subject: Re: [PATCH] pinctrl: return real error codes when pinctrl is not included Date: Sun, 24 Feb 2013 23:34:42 +0100 Message-ID: <201302242334.42408.heiko@sntech.de> References: <201302231855.47560.heiko@sntech.de> <201302231856.35083.heiko@sntech.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Linus Walleij Cc: Kukjin Kim , Wolfram Sang , Tomasz Figa , linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-i2c@vger.kernel.org, Prathyush K , linux-arm-kernel@lists.infradead.org List-Id: linux-i2c@vger.kernel.org Am Sonntag, 24. Februar 2013, 01:40:58 schrieb Linus Walleij: > On Sat, Feb 23, 2013 at 6:56 PM, Heiko St=FCbner 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 u= p = with this "ugly patch" all by myself when working on dt support for my mach= ine = 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 = wrote: > > Exynos5250 does not yet support pinctrl so CONFIG_PINCTRL is not define= d. > > = > > 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 t= he > > driver. > > This was causing the i2c hdmi phy to fail. > > = > > I checked the other i2c drivers and realized, no-one is actually checki= ng > > for IS_ERR_OR_NULL. > > = > > Even ./Documentation/pinctrl.txt says: > > /* Setup */ > > p =3D devm_pinctrl_get(&device); > > if (IS_ERR(p)) > > = > > I think the consumer.h should be modified to return an error and not ju= st > > 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 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 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 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 callba= ck > > 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