| Message ID | 1358889025-8530-3-git-send-email-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 22, 2013 at 10:10 PM, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Now that pca953x driver can handle GPIO expanders with more than 32 > bits this patch adds the support for the pca9505 which cam with 40 > GPIOs. > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> Patch applied, thanks. But guys, this driver contains some horrific stuff, look at this: static int pxa_gpio_nums(void) { int count = 0; #ifdef CONFIG_ARCH_PXA if (cpu_is_pxa25x()) { #ifdef CONFIG_CPU_PXA26x count = 89; gpio_type = PXA26X_GPIO; #elif defined(CONFIG_PXA25x) count = 84; gpio_type = PXA26X_GPIO; #endif /* CONFIG_CPU_PXA26x */ } else if (cpu_is_pxa27x()) { count = 120; gpio_type = PXA27X_GPIO; } else if (cpu_is_pxa93x()) { count = 191; gpio_type = PXA93X_GPIO; } else if (cpu_is_pxa3xx()) { count = 127; gpio_type = PXA3XX_GPIO; } #endif /* CONFIG_ARCH_PXA */ #ifdef CONFIG_ARCH_MMP if (cpu_is_pxa168() || cpu_is_pxa910()) { count = 127; gpio_type = MMP_GPIO; } else if (cpu_is_mmp2()) { count = 191; gpio_type = MMP_GPIO; } #endif /* CONFIG_ARCH_MMP */ return count; } This is totally killing all attempts at a single kernel for multiple machines in this family. The above should not be #ifdef's but instead use either platform data or the compatible property to figure out which one to use. It's fine to introduce new compatible= strings or device names to distinguish between these. As an example, in pinctrl-nomadik.c we have this: static const struct of_device_id nmk_pinctrl_match[] = { { .compatible = "stericsson,nmk_pinctrl", .data = (void *)PINCTRL_NMK_DB8500, }, {}, }; static const struct platform_device_id nmk_pinctrl_id[] = { { "pinctrl-stn8815", PINCTRL_NMK_STN8815 }, { "pinctrl-db8500", PINCTRL_NMK_DB8500 }, { "pinctrl-db8540", PINCTRL_NMK_DB8540 }, { } }; static struct platform_driver nmk_pinctrl_driver = { .driver = { .owner = THIS_MODULE, .name = "pinctrl-nomadik", .of_match_table = nmk_pinctrl_match, }, .probe = nmk_pinctrl_probe, .id_table = nmk_pinctrl_id, }; The first match is for DT boot the latter for using the platform device name directly. Then in the probefunction we do: static int nmk_pinctrl_probe(struct platform_device *pdev) { const struct platform_device_id *platid = platform_get_device_id(pdev); (...) if (platid) version = platid->driver_data; else if (np) { const struct of_device_id *match; match = of_match_device(nmk_pinctrl_match, &pdev->dev); if (!match) return -ENODEV; version = (unsigned int) match->data; } (...) if (version == PINCTRL_NMK_STN8815) nmk_pinctrl_stn8815_init(&npct->soc); if (version == PINCTRL_NMK_DB8500) nmk_pinctrl_db8500_init(&npct->soc); if (version == PINCTRL_NMK_DB8540) nmk_pinctrl_db8540_init(&npct->soc); } Surely a scheme like this must be possible to use to distinguish between the different versions at runtime rather than using these #ifdefs? Yours, Linus Walleij
On 01/25/2013 09:16 AM, Linus Walleij wrote: > On Tue, Jan 22, 2013 at 10:10 PM, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: > >> Now that pca953x driver can handle GPIO expanders with more than 32 >> bits this patch adds the support for the pca9505 which cam with 40 >> GPIOs. >> >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > > Patch applied, thanks. > > But guys, this driver contains some horrific stuff, look at this: > > static int pxa_gpio_nums(void) > { > int count = 0; > > #ifdef CONFIG_ARCH_PXA > if (cpu_is_pxa25x()) { > #ifdef CONFIG_CPU_PXA26x > count = 89; > gpio_type = PXA26X_GPIO; > #elif defined(CONFIG_PXA25x) > count = 84; > gpio_type = PXA26X_GPIO; > #endif /* CONFIG_CPU_PXA26x */ > } else if (cpu_is_pxa27x()) { > count = 120; > gpio_type = PXA27X_GPIO; > } else if (cpu_is_pxa93x()) { > count = 191; > gpio_type = PXA93X_GPIO; > } else if (cpu_is_pxa3xx()) { > count = 127; > gpio_type = PXA3XX_GPIO; > } > #endif /* CONFIG_ARCH_PXA */ > > #ifdef CONFIG_ARCH_MMP > if (cpu_is_pxa168() || cpu_is_pxa910()) { > count = 127; > gpio_type = MMP_GPIO; > } else if (cpu_is_mmp2()) { > count = 191; > gpio_type = MMP_GPIO; > } > #endif /* CONFIG_ARCH_MMP */ > return count; > } > > This is totally killing all attempts at a single kernel for multiple > machines in this family. The above should not be #ifdef's but instead > use either platform data or the compatible property to figure out which > one to use. > > It's fine to introduce new compatible= strings or device names to > distinguish between these. > > As an example, in pinctrl-nomadik.c we have this: > > static const struct of_device_id nmk_pinctrl_match[] = { > { > .compatible = "stericsson,nmk_pinctrl", > .data = (void *)PINCTRL_NMK_DB8500, > }, > {}, > }; > > static const struct platform_device_id nmk_pinctrl_id[] = { > { "pinctrl-stn8815", PINCTRL_NMK_STN8815 }, > { "pinctrl-db8500", PINCTRL_NMK_DB8500 }, > { "pinctrl-db8540", PINCTRL_NMK_DB8540 }, > { } > }; > > static struct platform_driver nmk_pinctrl_driver = { > .driver = { > .owner = THIS_MODULE, > .name = "pinctrl-nomadik", > .of_match_table = nmk_pinctrl_match, > }, > .probe = nmk_pinctrl_probe, > .id_table = nmk_pinctrl_id, > }; > > > The first match is for DT boot the latter for using the platform > device name directly. > > Then in the probefunction we do: > > static int nmk_pinctrl_probe(struct platform_device *pdev) > { > const struct platform_device_id *platid = platform_get_device_id(pdev); > (...) > if (platid) > version = platid->driver_data; > else if (np) { > const struct of_device_id *match; > > match = of_match_device(nmk_pinctrl_match, &pdev->dev); > if (!match) > return -ENODEV; > version = (unsigned int) match->data; > } > (...) > if (version == PINCTRL_NMK_STN8815) > nmk_pinctrl_stn8815_init(&npct->soc); > if (version == PINCTRL_NMK_DB8500) > nmk_pinctrl_db8500_init(&npct->soc); > if (version == PINCTRL_NMK_DB8540) > nmk_pinctrl_db8540_init(&npct->soc); > } > > Surely a scheme like this must be possible to use to distinguish between > the different versions at runtime rather than using these #ifdefs? > Well, at the beginning I thought adding support for pca9505 was just a matter of a couple of lines to add. Then I realized that I need to handle the 40 bits case, and I ended up refactoring all access to the registers. So now I am on it, it seems I am volunteer to continue to improve this driver. However I won't be able to test it, the only PXA based platform I have is a Zaurus SL-C3100 which embeds a PXA270 if I remember well, but I doubt it come with gpio expander on i2c. Gregory
On Fri, Jan 25, 2013 at 9:36 AM, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Well, at the beginning I thought adding support for pca9505 was just a matter > of a couple of lines to add. Then I realized that I need to handle the 40 bits > case, and I ended up refactoring all access to the registers. So now I am on it, > it seems I am volunteer to continue to improve this driver. I like the sound of this ;-) To get you started I just sent out two other patches you can consider as RFC, they're regrettably not even compile-tested. I mainly wanted to indicate what needs to be done so we can throw them away, just wanted to give a hint. > However I won't be able to test it, the only PXA based platform I have is a > Zaurus SL-C3100 which embeds a PXA270 if I remember well, but I doubt it come > with gpio expander on i2c. Well I guess if there is nobody testing it, then nobody cares. The world must be full of people with PXA platforms doing nothing but regression testing... Actually just days ago I asked Haoijan to help me testing a set of patches for the PXA SPI controller, and he kindly helped out, so there are some people booting these platforms, sometimes :-) Yours, Linus Walleij
On 01/25/2013 09:51 AM, Linus Walleij wrote: > On Fri, Jan 25, 2013 at 9:36 AM, Gregory CLEMENT > <gregory.clement@free-electrons.com> wrote: > >> Well, at the beginning I thought adding support for pca9505 was just a matter >> of a couple of lines to add. Then I realized that I need to handle the 40 bits >> case, and I ended up refactoring all access to the registers. So now I am on it, >> it seems I am volunteer to continue to improve this driver. > > I like the sound of this ;-) I was about to fix the issues you have pointed but I didn't find anything like #ifdef CONFIG_ARCH_PXA if (cpu_is_pxa25x()) { #ifdef CONFIG_CPU_PXA26x count = 89; gpio_type = PXA26X_GPIO; #elif defined(CONFIG_PXA25x) in the pca953x driver! I think you messed up with another patch set! I saw that Haojian Zhuang have sent a patch set for gpio-pxa and among this set the patch "[PATCH 06/10] gpio: pxa: define nr gpios in platform data" seemed to exactly what you've expected. > > To get you started I just sent out two other patches you can consider > as RFC, they're regrettably not even compile-tested. I mainly wanted > to indicate what needs to be done so we can throw them away, just > wanted to give a hint. > >> However I won't be able to test it, the only PXA based platform I have is a >> Zaurus SL-C3100 which embeds a PXA270 if I remember well, but I doubt it come >> with gpio expander on i2c. > > Well I guess if there is nobody testing it, then nobody cares. > The world must be full of people with PXA platforms doing nothing > but regression testing... > > Actually just days ago I asked Haoijan to help me testing a set of > patches for the PXA SPI controller, and he kindly helped out, so there > are some people booting these platforms, sometimes :-) > > Yours, > Linus Walleij >
On 27 January 2013 06:02, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > On 01/25/2013 09:51 AM, Linus Walleij wrote: >> On Fri, Jan 25, 2013 at 9:36 AM, Gregory CLEMENT >> <gregory.clement@free-electrons.com> wrote: >> >>> Well, at the beginning I thought adding support for pca9505 was just a matter >>> of a couple of lines to add. Then I realized that I need to handle the 40 bits >>> case, and I ended up refactoring all access to the registers. So now I am on it, >>> it seems I am volunteer to continue to improve this driver. >> >> I like the sound of this ;-) > > I was about to fix the issues you have pointed but I didn't find anything like > > #ifdef CONFIG_ARCH_PXA > if (cpu_is_pxa25x()) { > #ifdef CONFIG_CPU_PXA26x > count = 89; > gpio_type = PXA26X_GPIO; > #elif defined(CONFIG_PXA25x) > > > in the pca953x driver! I think you messed up with another patch set! > > I saw that Haojian Zhuang have sent a patch set for gpio-pxa and > among this set the patch "[PATCH 06/10] gpio: pxa: define nr gpios > in platform data" seemed to exactly what you've expected. > PCA953X is a GPIO expander that is relied on I2C bus. It's a device in the cirucit, not in the PXA chips. So there's no cpu related code in this driver. Gregory's concern is that he found that this device is used on pxa27x platform, and he don't have the hardware to test. I also don't have pxa27x platform. I think that he can ping the volunteers in the mailing list. Regards Haojian
On Sat, Jan 26, 2013 at 11:02 PM, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > I was about to fix the issues you have pointed but I didn't find anything like > > #ifdef CONFIG_ARCH_PXA > if (cpu_is_pxa25x()) { > #ifdef CONFIG_CPU_PXA26x > count = 89; > gpio_type = PXA26X_GPIO; > #elif defined(CONFIG_PXA25x) > > in the pca953x driver! I think you messed up with another patch set! Yes I did. Forget about this, thanks for the other nice patches! Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index b35ba06..3a68aed 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -46,6 +46,7 @@ #define PCA957X_TYPE 0x2000 static const struct i2c_device_id pca953x_id[] = { + { "pca9505", 40 | PCA953X_TYPE | PCA_INT, }, { "pca9534", 8 | PCA953X_TYPE | PCA_INT, }, { "pca9535", 16 | PCA953X_TYPE | PCA_INT, }, { "pca9536", 4 | PCA953X_TYPE, }, @@ -835,6 +836,7 @@ static int pca953x_remove(struct i2c_client *client) } static const struct of_device_id pca953x_dt_ids[] = { + { .compatible = "nxp,pca9505", }, { .compatible = "nxp,pca9534", }, { .compatible = "nxp,pca9535", }, { .compatible = "nxp,pca9536", },
Now that pca953x driver can handle GPIO expanders with more than 32 bits this patch adds the support for the pca9505 which cam with 40 GPIOs. Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> --- drivers/gpio/gpio-pca953x.c | 2 ++ 1 file changed, 2 insertions(+)