| Message ID | 201302231857.46445.heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? Yours, Linus Walleij
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
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
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@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? 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
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
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@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? 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
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. Yours, Linus Walleij
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
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@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 :-) 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
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; }
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> --- drivers/i2c/busses/i2c-s3c2410.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)