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

[VokÃÄ Michal]

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