Message ID | 1460506523-6249-5-git-send-email-ray.jui@broadcom.com |
---|---|
State | New |
Headers | show |
On Wed, Apr 13, 2016 at 2:15 AM, Ray Jui <ray.jui@broadcom.com> wrote: > In some of the future iProc based SoCs, pinconf is handled by another > block and the iProc GPIO controller is solely used as a GPIO controller. > This patch adds support of a new compatible string "brcm,iproc-gpio-only", > that is introduced to handle this case, where pinconf functions in this > driver are completely disabled > > Signed-off-by: Ray Jui <ray.jui@broadcom.com> > Reviewed-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> > Reviewed-by: Jon Mason <jon.mason@broadcom.com> > Reviewed-by: Scott Branden <scott.branden@broadcom.com> If this was entirely true, then the driver should end up only executing [devm_]gpiochip_add_data() but that does not seem to be the case. You are still registering a pin controller, right? Just disabling some of the pin config options. The pin multiplexing is still there, right? Then it is not "solely a GPIO controller". Not at all. This patch set needs some elaboration I think. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On 4/15/2016 1:24 AM, Linus Walleij wrote: > On Wed, Apr 13, 2016 at 2:15 AM, Ray Jui <ray.jui@broadcom.com> wrote: > >> In some of the future iProc based SoCs, pinconf is handled by another >> block and the iProc GPIO controller is solely used as a GPIO controller. >> This patch adds support of a new compatible string "brcm,iproc-gpio-only", >> that is introduced to handle this case, where pinconf functions in this >> driver are completely disabled >> >> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >> Reviewed-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> >> Reviewed-by: Jon Mason <jon.mason@broadcom.com> >> Reviewed-by: Scott Branden <scott.branden@broadcom.com> > > If this was entirely true, then the driver should end up only executing > [devm_]gpiochip_add_data() but that does not seem to be the case. Yes, in the case of compatible string "brcm,iproc-gpio-only" is detected, the driver only registers 'gpiochip_add_data'. Please check patch 2/4 of this series, which takes care of it. > > You are still registering a pin controller, right? Just disabling some of > the pin config options. The pin multiplexing is still there, right? > Then it is not "solely a GPIO controller". Not at all. This driver does not register itself as a PINCONF driver if "brcm,iproc-gpio-only" compatible string is detected. This is addressed in patch 2/4 of this series. Pin based IOMUX GPIO override is only activated when 'chip->pinmux_is_supported' is true, and it is only true if the optional DT property "gpio-ranges" is defined. > > This patch set needs some elaboration I think. > > Yours, > Linus Walleij > I believe the current issue with this patch series is now only on the naming of the new compatible string "brcm,iproc-gpio-only". Please correct me if I'm wrong. Thanks, Ray -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 18, 2016 at 9:30 PM, Ray Jui <ray.jui@broadcom.com> wrote: > On 4/15/2016 1:24 AM, Linus Walleij wrote: >> >> On Wed, Apr 13, 2016 at 2:15 AM, Ray Jui <ray.jui@broadcom.com> wrote: >> >>> In some of the future iProc based SoCs, pinconf is handled by another >>> block and the iProc GPIO controller is solely used as a GPIO controller. >>> This patch adds support of a new compatible string >>> "brcm,iproc-gpio-only", >>> that is introduced to handle this case, where pinconf functions in this >>> driver are completely disabled >>> >>> Signed-off-by: Ray Jui <ray.jui@broadcom.com> >>> Reviewed-by: Yendapally Reddy Dhananjaya Reddy >>> <yendapally.reddy@broadcom.com> >>> Reviewed-by: Jon Mason <jon.mason@broadcom.com> >>> Reviewed-by: Scott Branden <scott.branden@broadcom.com> >> >> >> If this was entirely true, then the driver should end up only executing >> [devm_]gpiochip_add_data() but that does not seem to be the case. > > Yes, in the case of compatible string "brcm,iproc-gpio-only" is detected, > the driver only registers 'gpiochip_add_data'. Please check patch 2/4 of > this series, which takes care of it. OK. >> You are still registering a pin controller, right? Just disabling some of >> the pin config options. The pin multiplexing is still there, right? >> Then it is not "solely a GPIO controller". Not at all. > > This driver does not register itself as a PINCONF driver if > "brcm,iproc-gpio-only" compatible string is detected. This is addressed in > patch 2/4 of this series. > > Pin based IOMUX GPIO override is only activated when > 'chip->pinmux_is_supported' is true, and it is only true if the optional DT > property "gpio-ranges" is defined. OK. > I believe the current issue with this patch series is now only on the naming > of the new compatible string "brcm,iproc-gpio-only". Please correct me if > I'm wrong. Yeah I think I get it now. The patch set makes sense. Looking forward to the next iteration! Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c index 12a8922..bb5cfd9 100644 --- a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c +++ b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c @@ -716,7 +716,8 @@ static const struct of_device_id iproc_gpio_of_match[] = { { .compatible = "brcm,cygnus-asiu-gpio" }, { .compatible = "brcm,cygnus-crmu-gpio" }, { .compatible = "brcm,iproc-gpio" }, - { } + { .compatible = "brcm,iproc-gpio-only" }, + { /* sentinel */ } }; static int iproc_gpio_probe(struct platform_device *pdev) @@ -781,24 +782,28 @@ static int iproc_gpio_probe(struct platform_device *pdev) return ret; } - ret = iproc_gpio_register_pinconf(chip); - if (ret) { - dev_err(dev, "unable to register pinconf\n"); - goto err_rm_gpiochip; - } - - /* - * Optional DT property to disable unsupported pinconf parameters for - * a particular iProc SoC - */ - ret = of_property_read_u32(dev->of_node, "brcm,pinconf-func-off", - &pinconf_disable_mask); - if (!ret) { - ret = iproc_pinconf_disable_map_create(chip, - pinconf_disable_mask); + if (!of_device_is_compatible(dev->of_node, "brcm,iproc-gpio-only")) { + ret = iproc_gpio_register_pinconf(chip); if (ret) { - dev_err(dev, "unable to create pinconf disable map\n"); - goto err_unregister_pinconf; + dev_err(dev, "unable to register pinconf\n"); + goto err_rm_gpiochip; + } + + /* + * Optional DT property to disable unsupported pinconf + * parameters for a particular iProc SoC + */ + ret = of_property_read_u32(dev->of_node, + "brcm,pinconf-func-off", + &pinconf_disable_mask); + if (!ret) { + ret = iproc_pinconf_disable_map_create(chip, + pinconf_disable_mask); + if (ret) { + dev_err(dev, + "unable to create pinconf disable map\n"); + goto err_unregister_pinconf; + } } }