* Tony Lindgren <tony@xxxxxxxxxxx> [170110 07:32]:
> * Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> [170110 06:09]:
> > Hi Tony,
> >
> > On Tue, Dec 27, 2016 at 6:19 PM, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> > > Having the pin control framework call pin controller functions
> > > before it's probe has finished is not nice as the pin controller
> > > device driver does not yet have struct pinctrl_dev handle.
> > >
> > > Let's fix this issue by adding deferred work for late init. This is
> > > needed to be able to add pinctrl generic helper functions that expect
> > > to know struct pinctrl_dev handle. Note that we now need to call
> > > create_pinctrl() directly as we don't want to add the pin controller
> > > to the list of controllers until the hogs are claimed. We also need
> > > to pass the pinctrl_dev to the device tree parser functions as they
> > > otherwise won't find the right controller at this point.
> > >
> > > Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> >
> > I believe this patch causes a regression on r8a7740/armadillo, where the
> > pin controller is also a GPIO controller, and lcd0 needs a hog
> > (cfr. arch/arm/boot/dts/r8a7740-armadillo800eva.dts):
> >
> > -GPIO line 176 (lcd0) hogged as output/high
> > -sh-pfc e6050000.pfc: r8a7740_pfc handling gpio 0 -> 211
> > +gpiochip_add_data: GPIOs 0..211 (r8a7740_pfc) failed to register
> > +sh-pfc e6050000.pfc: failed to init GPIO chip, ignoring...
> > sh-pfc e6050000.pfc: r8a7740_pfc support registered
> >
> > Hence all drivers using GPIOs fail to initialize because their GPIOs never
> > become available.
> >
> > Adding debug prints to the failure paths shows that the call to
> > of_pinctrl_get() in of_gpiochip_add_pin_range() fails with -EPROBE_DEFER.
> > Adding a debug print to the top of gpiochip_add_data() makes the problem go
> > away, presumably because it introduces a delay that allows the delayed work
> > to kick in...
>
> OK. What if we added also an optional pinctrl function that the pin
> controller driver could call to initialize hogs? Then the pin controller
> driver could call it during or after probe as needed. That is after
> there's a valid struct pinctrl_dev handle.
...
> We could also pass some flag if should always call pinctrl_late_init()
> directly. But that does not remove the problem of struct pinctrl_dev handle
> being uninitialized when the pin controller driver functionas are called.
Looks like we need both a flag and a way for the pin controller driver
to start things.
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.
Or do you guys have any better ideas?
Regards,
Tony
8< --------------------------------
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,
--
2.11.0
