| Message ID | 1394018461-757-1-git-send-email-holler@ahsoftware.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alexander, On 03/05/2014 01:21 PM, Alexander Holler wrote: > The driver missed an of_xlate function to translate gpio numbers > as found in the DT to the correct chip and number. > > While there I've set #gpio_cells to a fixed value of 2. > > I've used gpio-pxa.c as template for those changes and tested my changes > successfully on a da850 board using entries for gpio-leds in a DT. So I didn't > reinvent the wheel but just copied and tested stuff. > > Thanks to Grygorii Strashko for the hint to the existing code in gpio-pxa. > > Signed-off-by: Alexander Holler <holler@ahsoftware.de> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> Looks good to me now. Thanks. > --- > drivers/gpio/gpio-davinci.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > Changes in v2: replaced static variables by indirections. > > diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c > index 7629b4f..92574a0 100644 > --- a/drivers/gpio/gpio-davinci.c > +++ b/drivers/gpio/gpio-davinci.c > @@ -172,6 +172,27 @@ of_err: > return NULL; > } > > +#ifdef CONFIG_OF_GPIO > +static int davinci_gpio_of_xlate(struct gpio_chip *gc, > + const struct of_phandle_args *gpiospec, > + u32 *flags) > +{ > + struct davinci_gpio_controller *chips = dev_get_drvdata(gc->dev); > + struct davinci_gpio_platform_data *pdata = dev_get_platdata(gc->dev); > + > + if (gpiospec->args[0] > pdata->ngpio) > + return -EINVAL; > + > + if (gc != &chips[gpiospec->args[0] / 32].chip) > + return -EINVAL; > + > + if (flags) > + *flags = gpiospec->args[1]; > + > + return gpiospec->args[0] % 32; > +} > +#endif > + > static int davinci_gpio_probe(struct platform_device *pdev) > { > int i, base; > @@ -236,6 +257,9 @@ static int davinci_gpio_probe(struct platform_device *pdev) > chips[i].chip.ngpio = 32; > > #ifdef CONFIG_OF_GPIO > + chips[i].chip.of_gpio_n_cells = 2; > + chips[i].chip.of_xlate = davinci_gpio_of_xlate; > + chips[i].chip.dev = dev; > chips[i].chip.of_node = dev->of_node; > #endif > spin_lock_init(&chips[i].lock); >
On Wed, Mar 5, 2014 at 12:21 PM, Alexander Holler <holler@ahsoftware.de> wrote: > The driver missed an of_xlate function to translate gpio numbers > as found in the DT to the correct chip and number. > > While there I've set #gpio_cells to a fixed value of 2. > > I've used gpio-pxa.c as template for those changes and tested my changes > successfully on a da850 board using entries for gpio-leds in a DT. So I didn't > reinvent the wheel but just copied and tested stuff. > > Thanks to Grygorii Strashko for the hint to the existing code in gpio-pxa. > > Signed-off-by: Alexander Holler <holler@ahsoftware.de> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> This v2 version applied, thanks! Yours, Linus Walleij
Am 11.03.2014 11:15, schrieb Linus Walleij: > On Wed, Mar 5, 2014 at 12:21 PM, Alexander Holler <holler@ahsoftware.de> wrote: > >> The driver missed an of_xlate function to translate gpio numbers >> as found in the DT to the correct chip and number. >> >> While there I've set #gpio_cells to a fixed value of 2. >> >> I've used gpio-pxa.c as template for those changes and tested my changes >> successfully on a da850 board using entries for gpio-leds in a DT. So I didn't >> reinvent the wheel but just copied and tested stuff. >> >> Thanks to Grygorii Strashko for the hint to the existing code in gpio-pxa. >> >> Signed-off-by: Alexander Holler <holler@ahsoftware.de> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > This v2 version applied, thanks! Thanks, but actually that should have been a fix for 3.14 with which the OF functionality for davinci gpio gets introduced. I assum with the patch in for-next, 3.14 will appear with that functionality broken and it will become a candidate for -stable. Regards, Alexander Holler
On Fri, Mar 14, 2014 at 11:08 AM, Alexander Holler <holler@ahsoftware.de> wrote: > Am 11.03.2014 11:15, schrieb Linus Walleij: >> On Wed, Mar 5, 2014 at 12:21 PM, Alexander Holler <holler@ahsoftware.de> wrote: >> >>> The driver missed an of_xlate function to translate gpio numbers >>> as found in the DT to the correct chip and number. >>> >>> While there I've set #gpio_cells to a fixed value of 2. >>> >>> I've used gpio-pxa.c as template for those changes and tested my changes >>> successfully on a da850 board using entries for gpio-leds in a DT. So I didn't >>> reinvent the wheel but just copied and tested stuff. >>> >>> Thanks to Grygorii Strashko for the hint to the existing code in gpio-pxa. >>> >>> Signed-off-by: Alexander Holler <holler@ahsoftware.de> >>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> >> This v2 version applied, thanks! > > Thanks, but actually that should have been a fix for 3.14 with which the > OF functionality for davinci gpio gets introduced. I assum with the > patch in for-next, 3.14 will appear with that functionality broken and > it will become a candidate for -stable. I just get the impression that DT support for DaVinci in v3.14 is so risky and unstable that noone except those implementing it (i.e. you) is really using it, is that correct? In that case it is hardly a fix that we need to rush out to the entire world. But if you have bug reports from DaVinci developers doing advanced DT stuff and scratching their heads, then we can push this to fixes. Yours, Linus Walleij
Am 14.03.2014 12:02, schrieb Linus Walleij: > On Fri, Mar 14, 2014 at 11:08 AM, Alexander Holler <holler@ahsoftware.de> wrote: >> Am 11.03.2014 11:15, schrieb Linus Walleij: >>> On Wed, Mar 5, 2014 at 12:21 PM, Alexander Holler <holler@ahsoftware.de> wrote: >>> >>>> The driver missed an of_xlate function to translate gpio numbers >>>> as found in the DT to the correct chip and number. >>>> >>>> While there I've set #gpio_cells to a fixed value of 2. >>>> >>>> I've used gpio-pxa.c as template for those changes and tested my changes >>>> successfully on a da850 board using entries for gpio-leds in a DT. So I didn't >>>> reinvent the wheel but just copied and tested stuff. >>>> >>>> Thanks to Grygorii Strashko for the hint to the existing code in gpio-pxa. >>>> >>>> Signed-off-by: Alexander Holler <holler@ahsoftware.de> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>> >>> This v2 version applied, thanks! >> >> Thanks, but actually that should have been a fix for 3.14 with which the >> OF functionality for davinci gpio gets introduced. I assum with the >> patch in for-next, 3.14 will appear with that functionality broken and >> it will become a candidate for -stable. > > I just get the impression that DT support for DaVinci in v3.14 is so risky > and unstable that noone except those implementing it (i.e. you) is really > using it, is that correct? Yes. DT support for DaVinci is still incomplete and I don't think there are much people out there which already try to use it. Nevertheless is using DT a much easier way (and more future-proof) than patching board-files, if a board isn't one of those few supported dev-boards and needs different configurations. > In that case it is hardly a fix that we need to rush out to the entire world. Hmm, I think it's better to send something working and tested to world, than something untested and non-working. And especially things like pinctrl and gpio are fundamental to even try to use DT. E.g. in my case I didn't even try to use DT for DaVinci because it doesn't make sense without support for DT in gpio. And now people will get promised but broken support for gpio in 3.14, which might cost a lot of time, if someone tries to use it. And I thought the reason for -rc is actually to fix bugs. But I never understood the magical ways and timings patches make their way into mainline. ;) Regards, Alexander Holler
On Fri, Mar 14, 2014 at 1:38 PM, Alexander Holler <holler@ahsoftware.de> wrote: >> In that case it is hardly a fix that we need to rush out to the entire >> world. > > And I thought the reason for -rc is actually to fix bugs. But I never > understood the magical ways and timings patches make their way into > mainline. ;) OK so it works like this: early in the -rc cycle we fix any bugs, documentation or whatever. At this point it's *regressions* so the fix need to fix something that broke in the merge window (or an earlier merge window). If it is a new feature that never worked in the first place I would not call that a regression. There are no existing users out there that can experience regressions from a previously working system. So this is why I'm a bit conservative. Yours, Linus Walleij
Am 14.03.2014 14:54, schrieb Linus Walleij: > On Fri, Mar 14, 2014 at 1:38 PM, Alexander Holler <holler@ahsoftware.de> wrote: > >>> In that case it is hardly a fix that we need to rush out to the entire >>> world. >> >> And I thought the reason for -rc is actually to fix bugs. But I never >> understood the magical ways and timings patches make their way into >> mainline. ;) > > OK so it works like this: early in the -rc cycle we fix any bugs, documentation > or whatever. At this point it's *regressions* so the fix need to fix something > that broke in the merge window (or an earlier merge window). Sorry, but I don't believe that. It's always time to fix regressions and bugs. > If it is a new feature that never worked in the first place I would > not call that > a regression. There are no existing users out there that can experience > regressions from a previously working system. I believe that no (stable) kernel should introduce new known bugs (if fixes are available) and I wouldn't make a difference if it's a new feature or not. Usually people don't care if something is new or not if it has bugs Anyway, I'm just curious and try to understand, I have a fix. ;) Regards, Alexander Holler
On Fri, Mar 14, 2014 at 7:33 PM, Alexander Holler <holler@ahsoftware.de> wrote: > Am 14.03.2014 14:54, schrieb Linus Walleij: > >> On Fri, Mar 14, 2014 at 1:38 PM, Alexander Holler <holler@ahsoftware.de> >> wrote: >> >>>> In that case it is hardly a fix that we need to rush out to the entire >>>> world. >>> >>> >>> And I thought the reason for -rc is actually to fix bugs. But I never >>> understood the magical ways and timings patches make their way into >>> mainline. ;) >> >> >> OK so it works like this: early in the -rc cycle we fix any bugs, >> documentation >> or whatever. At this point it's *regressions* so the fix need to fix >> something >> that broke in the merge window (or an earlier merge window). > > > Sorry, but I don't believe that. It's always time to fix regressions and > bugs. This thing is not binary. Please consult Documentation/development-process/2.Process In this case we are very late in the release cycle, the patch is not a simple oneliner and I need some more backing proof to know it is a reasonable fix to push at this time and will not cause new problems. So a few Tested-by's from the people using this driver would for example convince me that it is solving a real problem for them and it needs to go into fixes. Yours, Linus Walleij
Am 14.03.2014 20:52, schrieb Linus Walleij: > So a few Tested-by's from the people using this driver would for > example convince me that it is solving a real problem for them > and it needs to go into fixes. 2001: a space odyssey is fast action movie compared with the movie kernel bug fixing. And 2001 is a masterpiece of slowness. Regards, Alexander Holler
On Friday 14 March 2014 04:32 PM, Linus Walleij wrote: > On Fri, Mar 14, 2014 at 11:08 AM, Alexander Holler <holler@ahsoftware.de> wrote: >> Am 11.03.2014 11:15, schrieb Linus Walleij: >>> On Wed, Mar 5, 2014 at 12:21 PM, Alexander Holler <holler@ahsoftware.de> wrote: >>> >>>> The driver missed an of_xlate function to translate gpio numbers >>>> as found in the DT to the correct chip and number. >>>> >>>> While there I've set #gpio_cells to a fixed value of 2. >>>> >>>> I've used gpio-pxa.c as template for those changes and tested my changes >>>> successfully on a da850 board using entries for gpio-leds in a DT. So I didn't >>>> reinvent the wheel but just copied and tested stuff. >>>> >>>> Thanks to Grygorii Strashko for the hint to the existing code in gpio-pxa. >>>> >>>> Signed-off-by: Alexander Holler <holler@ahsoftware.de> >>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>> >>> This v2 version applied, thanks! >> >> Thanks, but actually that should have been a fix for 3.14 with which the >> OF functionality for davinci gpio gets introduced. I assum with the >> patch in for-next, 3.14 will appear with that functionality broken and >> it will become a candidate for -stable. > > I just get the impression that DT support for DaVinci in v3.14 is so risky > and unstable that noone except those implementing it (i.e. you) is really > using it, is that correct? > > In that case it is hardly a fix that we need to rush out to the entire world. One thing to note is that this driver is used by keystone too and all its users are DT-only. Although I do not see any in-kernel DT GPIO users even there. I can confirm the patch does not break my gpiolib based test module (test with and without DT), but then it did not have an issue even before. Thanks, Sekhar
On Mon, Mar 17, 2014 at 2:29 PM, Sekhar Nori <nsekhar@ti.com> wrote: > One thing to note is that this driver is used by keystone too and all > its users are DT-only. Although I do not see any in-kernel DT GPIO users > even there. > > I can confirm the patch does not break my gpiolib based test module > (test with and without DT), but then it did not have an issue even before. Is that a Tested-by tag? :-) If you're also convinced that fix is safe I'll push it as a fix to v3.14-rcN if for nothing else so for getting Mr. Holler to stop poking me in the chest. Yours, Linus Walleij
Am 17.03.2014 14:29, schrieb Sekhar Nori: > One thing to note is that this driver is used by keystone too and all > its users are DT-only. Although I do not see any in-kernel DT GPIO users > even there. Common gpio users are, as always, gpio-keys and gpio-leds. Doesn't have one of the keystone boards at least LEDs? > I can confirm the patch does not break my gpiolib based test module > (test with and without DT), but then it did not have an issue even before. Thanks, maybe someone could add a Cc: <stable@vger.kernel.org> to get the fix back into 3.14 if it has go through -next. Regards, Alexander Holler
On Mon, Mar 17, 2014 at 3:11 PM, Alexander Holler <holler@ahsoftware.de> wrote: > Thanks, maybe someone could add a > > Cc: <stable@vger.kernel.org> OK if it goes in as fix I'll add this. > to get the fix back into 3.14 if it has go through -next. It's already in linux-next. Yours, Linus Walleij
On 03/17/2014 03:29 PM, Sekhar Nori wrote: > On Friday 14 March 2014 04:32 PM, Linus Walleij wrote: >> On Fri, Mar 14, 2014 at 11:08 AM, Alexander Holler <holler@ahsoftware.de> wrote: >>> Am 11.03.2014 11:15, schrieb Linus Walleij: >>>> On Wed, Mar 5, 2014 at 12:21 PM, Alexander Holler <holler@ahsoftware.de> wrote: >>>> >>>>> The driver missed an of_xlate function to translate gpio numbers >>>>> as found in the DT to the correct chip and number. >>>>> >>>>> While there I've set #gpio_cells to a fixed value of 2. >>>>> >>>>> I've used gpio-pxa.c as template for those changes and tested my changes >>>>> successfully on a da850 board using entries for gpio-leds in a DT. So I didn't >>>>> reinvent the wheel but just copied and tested stuff. >>>>> >>>>> Thanks to Grygorii Strashko for the hint to the existing code in gpio-pxa. >>>>> >>>>> Signed-off-by: Alexander Holler <holler@ahsoftware.de> >>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >>>> >>>> This v2 version applied, thanks! >>> >>> Thanks, but actually that should have been a fix for 3.14 with which the >>> OF functionality for davinci gpio gets introduced. I assum with the >>> patch in for-next, 3.14 will appear with that functionality broken and >>> it will become a candidate for -stable. >> >> I just get the impression that DT support for DaVinci in v3.14 is so risky >> and unstable that noone except those implementing it (i.e. you) is really >> using it, is that correct? >> >> In that case it is hardly a fix that we need to rush out to the entire world. > > One thing to note is that this driver is used by keystone too and all > its users are DT-only. Although I do not see any in-kernel DT GPIO users > even there. > > I can confirm the patch does not break my gpiolib based test module > (test with and without DT), but then it did not have an issue even before. The issues isn't observed on Keystone2 as it has <32 gpios as of now, also GPIO support for keystone is going to be added in 3.15 (including DT changes). Regards, -grygorii
On Monday 17 March 2014 07:35 PM, Linus Walleij wrote: > On Mon, Mar 17, 2014 at 2:29 PM, Sekhar Nori <nsekhar@ti.com> wrote: > >> One thing to note is that this driver is used by keystone too and all >> its users are DT-only. Although I do not see any in-kernel DT GPIO users >> even there. >> >> I can confirm the patch does not break my gpiolib based test module >> (test with and without DT), but then it did not have an issue even before. > > Is that a Tested-by tag? :-) Yes. Tested-by: Sekhar Nori <nsekhar@ti.com> > > If you're also convinced that fix is safe I'll push it as a fix to v3.14-rcN > if for nothing else so for getting Mr. Holler to stop poking me in the > chest. It is safe - at the least it does not break anything that is already working. I guess the decision to put it into -rc depends on whether you consider out of tree dtbs to be a valid usecase for the kernel. Thanks, Sekhar
Am 18.03.2014 09:37, schrieb Sekhar Nori: > It is safe - at the least it does not break anything that is already > working. I guess the decision to put it into -rc depends on whether you > consider out of tree dtbs to be a valid usecase for the kernel. That's all DT is about, getting rid of the necessity for in-tree hw-descriptions. ;) But I don't need any rush here, I'm just unable to understand why the -rc phase isn't used for bug fixing as I believe that's what this phase is for. Most people are unable to track the various -next trees, therefor many failures only come up when stuff reaches mainline where it might be tested by some more people. And if all bugs found in the -rc phase are fixed in -next only, the -rc phase would be useless. And it just happened to me too often, that even silly bugfixes (like oneliners) needed between 5 and 9 months to reach a stable kernel and thus users. Regards, Alexander Holler
Hi Alexander, On Tuesday 18 March 2014 03:15 PM, Alexander Holler wrote: > Am 18.03.2014 09:37, schrieb Sekhar Nori: > >> It is safe - at the least it does not break anything that is already >> working. I guess the decision to put it into -rc depends on whether you >> consider out of tree dtbs to be a valid usecase for the kernel. > > That's all DT is about, getting rid of the necessity for in-tree > hw-descriptions. ;) > > But I don't need any rush here, I'm just unable to understand why the > -rc phase isn't used for bug fixing as I believe that's what this phase > is for. The push back you are seeing is because this is pretty late in -rc cycle. If this push back was not there the bug fix cycle would probably never close. In all probability, if this was -rc2 or even -rc3 there would not be so much discussion. Thanks, Sekhar
On Tue, Mar 18, 2014 at 10:45 AM, Alexander Holler <holler@ahsoftware.de> wrote: > But I don't need any rush here, I'm just unable to understand why the -rc > phase isn't used for bug fixing as I believe that's what this phase is for. Right now it is mostly a practical issue to me, as I applied the patch to the devel (for-next) branch, then committed new development on top of it. If I send it for fixes now the same patch will come in two ways as I really do not like to rebase my tree at this point. So I'd prefer to keep this for next and then have it tagged for stable as v3.14 is released, if that is OK? It's as simple as sending a mail to Greg once it's upstream. Yours, Linus Walleij
Am 21.03.2014 09:28, schrieb Linus Walleij: > On Tue, Mar 18, 2014 at 10:45 AM, Alexander Holler <holler@ahsoftware.de> wrote: > >> But I don't need any rush here, I'm just unable to understand why the -rc >> phase isn't used for bug fixing as I believe that's what this phase is for. > > Right now it is mostly a practical issue to me, as I applied the patch to > the devel (for-next) branch, then committed new development on top of > it. > > If I send it for fixes now the same patch will come in two ways as I > really do not like to rebase my tree at this point. > > So I'd prefer to keep this for next and then have it tagged for stable as > v3.14 is released, if that is OK? Sure. > It's as simple as sending a mail to Greg once it's upstream. I already though about how to tag every patch I send as Cc: stable, because almost any fix I send either goes through some -next or even -next-next or already is for some stable kernel. (I usually don't even look at -rc kernels, just for some special things I'm waiting for, like this of-gpio for davinci.) Regards, Alexander Holler
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c index 7629b4f..92574a0 100644 --- a/drivers/gpio/gpio-davinci.c +++ b/drivers/gpio/gpio-davinci.c @@ -172,6 +172,27 @@ of_err: return NULL; } +#ifdef CONFIG_OF_GPIO +static int davinci_gpio_of_xlate(struct gpio_chip *gc, + const struct of_phandle_args *gpiospec, + u32 *flags) +{ + struct davinci_gpio_controller *chips = dev_get_drvdata(gc->dev); + struct davinci_gpio_platform_data *pdata = dev_get_platdata(gc->dev); + + if (gpiospec->args[0] > pdata->ngpio) + return -EINVAL; + + if (gc != &chips[gpiospec->args[0] / 32].chip) + return -EINVAL; + + if (flags) + *flags = gpiospec->args[1]; + + return gpiospec->args[0] % 32; +} +#endif + static int davinci_gpio_probe(struct platform_device *pdev) { int i, base; @@ -236,6 +257,9 @@ static int davinci_gpio_probe(struct platform_device *pdev) chips[i].chip.ngpio = 32; #ifdef CONFIG_OF_GPIO + chips[i].chip.of_gpio_n_cells = 2; + chips[i].chip.of_xlate = davinci_gpio_of_xlate; + chips[i].chip.dev = dev; chips[i].chip.of_node = dev->of_node; #endif spin_lock_init(&chips[i].lock);