| Message ID | 20170110191908.GV2630@atomide.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
On Tue, Jan 10, 2017 at 8:19 PM, Tony Lindgren <tony@atomide.com> wrote: > Below is an experimental fix to intorduce pinctrl_start() that I've > tested with pinctrl-single. Then we should probably make all pin controller > drivers call pinctrl_start() to properly fix the issue of struct pinctrl_dev > handle not being initialized before driver functions are called. Hm I guess that could work, but can we keep pinctrl_register() with the old semantics and add a separate pinctrl_register_and_defer() for those who just wanna start it later by a separate call? Then we don't need any special flags. > Or do you guys have any better ideas? Not really. So you mean revert the previous patch and apply something like this instead? Yours, Linus Walleij
* Linus Walleij <linus.walleij@linaro.org> [170111 07:34]: > On Tue, Jan 10, 2017 at 8:19 PM, Tony Lindgren <tony@atomide.com> wrote: > > > Below is an experimental fix to intorduce pinctrl_start() that I've > > tested with pinctrl-single. Then we should probably make all pin controller > > drivers call pinctrl_start() to properly fix the issue of struct pinctrl_dev > > handle not being initialized before driver functions are called. > > Hm I guess that could work, but can we keep pinctrl_register() with the old > semantics and add a separate pinctrl_register_and_defer() > for those who just wanna start it later by a separate call? > > Then we don't need any special flags. OK I'll take a look. > > Or do you guys have any better ideas? > > Not really. So you mean revert the previous patch and apply something > like this instead? Let me first take a look to see if we can fix it by making drivers using GENERIC_PINCTRL_GROUPS or GENERIC_PINMUX_FUNCTIONS register with pinctrl_register_and_defer(). I'll post a patch for that today. Then maybe for v4.12 we can attempt to move all pin controller drivers to using it so we can fix the problem for good. Regards, Tony
* Tony Lindgren <tony@atomide.com> [170111 08:29]: > * Linus Walleij <linus.walleij@linaro.org> [170111 07:34]: > > On Tue, Jan 10, 2017 at 8:19 PM, Tony Lindgren <tony@atomide.com> wrote: > > > > > Below is an experimental fix to intorduce pinctrl_start() that I've > > > tested with pinctrl-single. Then we should probably make all pin controller > > > drivers call pinctrl_start() to properly fix the issue of struct pinctrl_dev > > > handle not being initialized before driver functions are called. > > > > Hm I guess that could work, but can we keep pinctrl_register() with the old > > semantics and add a separate pinctrl_register_and_defer() > > for those who just wanna start it later by a separate call? > > > > Then we don't need any special flags. > > OK I'll take a look. > > > > Or do you guys have any better ideas? > > > > Not really. So you mean revert the previous patch and apply something > > like this instead? > > Let me first take a look to see if we can fix it by making drivers using > GENERIC_PINCTRL_GROUPS or GENERIC_PINMUX_FUNCTIONS register with > pinctrl_register_and_defer(). I'll post a patch for that today. Yeah we can fix this by reverting the late_init parts of the earlier attempt and introducing a new pinctrl_register_and_init() for controllers to use: extern int pinctrl_register_and_init(struct pinctrl_desc *pctldesc, struct device *dev, void *driver_data, struct pinctrl_dev **pctldev); > Then maybe for v4.12 we can attempt to move all pin controller drivers > to using it so we can fix the problem for good. And that will also make converting existing drivers to use it later on trivial. Will post a patch shortly after some more testing. Regards, Tony
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -1962,6 +1962,17 @@ static void pinctrl_late_init(struct work_struct *work) pinctrl_init_device_debugfs(pctldev); } +int pinctrl_start(struct pinctrl_dev *pctldev) +{ + if (!IS_ERR(pctldev->p)) + return -EEXIST; + + pinctrl_late_init(&pctldev->late_init.work); + + return 0; +} +EXPORT_SYMBOL_GPL(pinctrl_start); + /** * pinctrl_register() - register a pin controller device * @pctldesc: descriptor for this pin controller @@ -2035,9 +2046,14 @@ struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc, /* * If the device has hogs we want the probe() function of the driver * to complete before we go in and hog them and add the pin controller - * to the list of controllers. If it has no hogs, we can just complete - * the registration immediately. + * to the list of controllers. If the pin controller driver initializes + * hogs, or the pin controller instance has no hogs, we can just + * complete the registration immediately. */ + + if (pctldesc->flags & PINCTRL_DRIVER_START) + return pctldev; + if (pinctrl_dt_has_hogs(pctldev)) schedule_delayed_work(&pctldev->late_init, 0); else diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -1741,6 +1741,7 @@ static int pcs_probe(struct platform_device *pdev) pcs->desc.pmxops = &pcs_pinmux_ops; if (PCS_HAS_PINCONF) pcs->desc.confops = &pcs_pinconf_ops; + pcs->desc.flags = PINCTRL_DRIVER_START; pcs->desc.owner = THIS_MODULE; ret = pcs_allocate_pin_table(pcs); @@ -1754,6 +1755,10 @@ static int pcs_probe(struct platform_device *pdev) goto free; } + ret = pinctrl_start(pcs->pctl); + if (ret) + goto free; + ret = pcs_add_gpio_func(np, pcs); if (ret < 0) goto free; diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c --- a/drivers/pinctrl/sh-pfc/pinctrl.c +++ b/drivers/pinctrl/sh-pfc/pinctrl.c @@ -815,7 +815,15 @@ int sh_pfc_register_pinctrl(struct sh_pfc *pfc) pmx->pctl_desc.confops = &sh_pfc_pinconf_ops; pmx->pctl_desc.pins = pmx->pins; pmx->pctl_desc.npins = pfc->info->nr_pins; + pmx->pctl_desc.flags = PINCTRL_DRIVER_START; pmx->pctl = devm_pinctrl_register(pfc->dev, &pmx->pctl_desc, pmx); - return PTR_ERR_OR_ZERO(pmx->pctl); + if (IS_ERR(pmx->pctl)) + return PTR_ERR(pmx->pctl); + + ret = pinctrl_start(pmx->pctl); + if (ret) + return ret; + + return 0; } diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h --- a/include/linux/pinctrl/pinctrl.h +++ b/include/linux/pinctrl/pinctrl.h @@ -104,6 +104,8 @@ struct pinctrl_ops { struct pinctrl_map *map, unsigned num_maps); }; +#define PINCTRL_DRIVER_START BIT(0) + /** * struct pinctrl_desc - pin controller descriptor, register this to pin * control subsystem @@ -112,6 +114,8 @@ struct pinctrl_ops { * this pin controller * @npins: number of descriptors in the array, usually just ARRAY_SIZE() * of the pins field above + * @flags: Optional pin controller feature flags + * handling is needed in the pin controller driver. * @pctlops: pin control operation vtable, to support global concepts like * grouping of pins, this is optional. * @pmxops: pinmux operations vtable, if you support pinmuxing in your driver @@ -129,6 +133,7 @@ struct pinctrl_desc { const char *name; const struct pinctrl_pin_desc *pins; unsigned int npins; + unsigned int flags; const struct pinctrl_ops *pctlops; const struct pinmux_ops *pmxops; const struct pinconf_ops *confops; @@ -149,6 +154,7 @@ extern struct pinctrl_dev *devm_pinctrl_register(struct device *dev, void *driver_data); extern void devm_pinctrl_unregister(struct device *dev, struct pinctrl_dev *pctldev); +extern int pinctrl_start(struct pinctrl_dev *pctldev); extern bool pin_is_valid(struct pinctrl_dev *pctldev, int pin); extern void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,