Re: [RFC PATCH v3 2/2] pwm: imx: Configure output to GPIO in disabled state

[VokÃÄ Michal]

On 6.12.2018 17:16, Uwe Kleine-KÃnig wrote:
> On Thu, Dec 06, 2018 at 03:37:55PM +0000, VokÃÄ Michal wrote:
>> On 6.12.2018 14:59, Uwe Kleine-KÃnig wrote:
>>> On Thu, Dec 06, 2018 at 01:41:31PM +0000, VokÃÄ Michal wrote:
>>>
>>> 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?
> 
> You don't, I just wondered if this could happen and the function should
> return -EPROBE_DEFER in this case, too.

OK.

>>> 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.
> 
> Yes, you put the pinctrl, but not the GPIO. I.e. you're not undoing
> devm_gpiod_get_optional(). Maybe only do this if the pinctrl stuff
> succeeded to not touch the GPIO if it won't be used?

OK, I agree it seems better to get the pinctrl first and if it succeeds
only then try to get the GPIO. In that case I need to use the non-optional
variant of devm_gpio_get(). Note that then I do not really need to put the
GPIO in the error path as it means I did not get it.
The code would look like:

+static int imx_pwm_init_pinctrl_info(struct imx_chip *imx_chip,
+				      struct platform_device *pdev)
+{
+	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");
+
+	if (IS_ERR(imx_chip->pinctrl_pins_pwm) ||
+	    IS_ERR(imx_chip->pinctrl_pins_gpio)) {
+		dev_dbg(&pdev->dev, "pinctrl information incomplete\n");
+		goto out;
+	}
+
+	imx_chip->pwm_gpiod = devm_gpiod_get(&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)) {
+		dev_dbg(&pdev->dev, "GPIO information incomplete\n");
+		goto out;
+	}
+
+	return 0;
+
+out:
+	devm_pinctrl_put(imx_chip->pinctrl);
+	imx_chip->pinctrl = NULL;
+
+	return 0;
+}

>>> 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.
> 
> Without .get_state it won't be possible to smoothly take over a running
> PWM.

But that is exactly how the current pwm-imx code works, right?

> It doesn't hurt if the PWM isn't running though. Still I'd like to
> see the .get_state patch to go in first to not get this (admittedly
> small) regression.

I would not mind that. The problem is that the .get_state patch have
not received any comments yet. I planned to resend it once this series
is in. If we choose to fine-tune the .get_state patch first and then
put this series on top of that I am afraid we will miss another few
releases.

>>> 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.
> 
> OK. If you agree with my split and as you have to rework your patch
> anyhow: Would you mind to rebase on top of my patch series? (Unless
> Thierry disagrees with my patches, but unfortunately he didn't comment
> yet.)

No problem for me to rebase on top of your series. But it really
depends on Thierry. And it generates the same problem as I mentioned
above. Unlike your split and my .get_state series the pinctrl/GPIO
series has already been thoroughly discussed and seems to be quite
close to be accepted. Maybe it is not a good idea to postpone that
with the current pace of patches merged per release into PWM subsystem.

The other thing is we mostly discussed the concept so far. So now
as it looks like this pinctrl/GPIO series may be eventually merged
a review on the dt-binding part is also needed. Hopefully it gets
Rob's attention once submitted as non-RFC.

Best regards,
Michal