On 6.12.2018 14:59, Uwe Kleine-König wrote:
> On Thu, Dec 06, 2018 at 01:41:31PM +0000, Vokáč Michal wrote:
>>
>> +static int imx_pwm_init_pinctrl_info(struct imx_chip *imx_chip,
>> + struct platform_device *pdev)
>
> Please indent the follow up line to the opening parenthesis.
Meh, I overlooked that one. I will fix it.
>> +{
>> + imx_chip->pinctrl = devm_pinctrl_get(&pdev->dev);
>> + if (IS_ERR(imx_chip->pinctrl)) {
>> + dev_dbg(&pdev->dev, "can not get pinctrl\n");
>> + return PTR_ERR(imx_chip->pinctrl);
>> + }
>> +
>> + imx_chip->pinctrl_pins_pwm = pinctrl_lookup_state(imx_chip->pinctrl,
>> + "pwm");
>> + imx_chip->pinctrl_pins_gpio = pinctrl_lookup_state(imx_chip->pinctrl,
>> + "gpio");
>> + imx_chip->pwm_gpiod = devm_gpiod_get_optional(&pdev->dev, "pwm",
>> + GPIOD_IN);
>> +
>> + if (PTR_ERR(imx_chip->pwm_gpiod) == -EPROBE_DEFER) {
>> + return -EPROBE_DEFER;
>> + } else if (IS_ERR(imx_chip->pwm_gpiod) ||
>> + IS_ERR(imx_chip->pinctrl_pins_pwm) ||
>> + IS_ERR(imx_chip->pinctrl_pins_gpio)) {
>> + dev_dbg(&pdev->dev, "PWM pinctrl information incomplete\n");
>> + devm_pinctrl_put(imx_chip->pinctrl);
>> + imx_chip->pinctrl = NULL;
>
> Can it happen, that pinctrl_pins_pwm is PTR_ERR(-EPROBE_DEFER)?
No. The pinctrl_lookup_state either returns pointer to the pinctrl state
or ERR_PTR(-ENODEV). But I do not explicitly test the pinctrl_pins_pwm
for PTR_ERR(-EPROBE_DEFER), or do I?
> Maybe you only want to ignore PTR_ERR(-ENODEV) and for example propagate
> -EIO? I think you want to put the gpio if the failure is because there
> is a pinctrl related error.
I think that is what I am doing. In case the GPIO is not ready the probe
is deferred. In case of any other error with the GPIO or pinctrl failure
I put the pinctrl. Or maybe I do not really understand what you mean.
>> + }
>> +
>> + return 0;
>> +}
>> +
[...]
>> @@ -303,6 +362,24 @@ static int imx_pwm_probe(struct platform_device *pdev)
>> if (ret < 0)
>> return ret;
>>
>> + if (imx->pinctrl) {
>> + /*
>> + * Update cstate after pwmchip_add() call as the core might
>> + * call the get_state() function to read the PWM registers
>> + * to get the actual HW state.
>> + */
>> + pwm_get_state(imx->chip.pwms, &cstate);
>> + if (cstate.enabled) {
>> + dev_dbg(&pdev->dev,
>> + "PWM entered probe in enabled state\n");
>> + pinctrl_select_state(imx->pinctrl,
>> + imx->pinctrl_pins_pwm);
>> + } else {
>> + pinctrl_select_state(imx->pinctrl,
>> + imx->pinctrl_pins_gpio);
>> + }
>> + }
>> +
>
> ISTR that there was a patch that implements get_state for imx. Is there
> a dependency on that one? Otherwise the state returned by
> pwm_get_state() might not be what is actually configured.
No, it should be independent. One can go without the other. I tested all
three combinations (mainline with .get_state, mainline with this series,
mainline with .get_state AND this series) and got the expected results.
Without the .get_state() patch the core always returns the default which
is disabled state so the gpio pinctrl state is selected in probe.
> Do you know if this is required for the old i.MX pwm, e.g. on i.MX21?
> I ask because of https://patchwork.ozlabs.org/patch/1000071/
Yep, I am aware of that patch. IMHO this is not needed for the v1 on
older i.MX SoCs but I do not have a hands-on experience with those.
Thank you for the review Uwe!
Michal
