Re: [RCFÂPATCH v2 2/2] pwm: imx: Configure output to GPIO in disabled state

[VokÃÄ Michal]

On 12.10.2018 18:00, Thierry Reding wrote:
> On Wed, Oct 10, 2018 at 09:33:26AM +0000, VokÃÄ Michal wrote:
>> Normally the PWM output is held LOW when PWM is disabled. This can cause
>> problems when inverted PWM signal polarity is needed. With this behavior
>> the connected circuit is fed by 100% duty cycle instead of being shut-off.
>>
>> Allow users to define a "gpio" and a "pwm" pinctrl states. The pwm pinctrl
>> state is then selected when PWM is enabled and the gpio pinctrl state is
>> selected when PWM is disabled. Also add a new pwm-gpios GPIO that is used
>> to drive the output in the gpio state.
>>
>> If all the pinctrl states and the pwm-gpios are not correctly specified
>> in DT the logic will work as before.
>>
>> As an example, with this patch a PWM controlled backlight with inversed
>> signal polarity can be used in full brightness range. Without this patch
>> the backlight can not be turned off as brightness = 0 disables the PWM
>> and that in turn set PWM output LOW, that is full brightness.
>>
>> Output of the PWM with "default" pinctrl and with "pwm"+"gpio" pinctrl:
>>
>> +--------------+------------+---------------+---------------------------+
>> | After reset  | Bootloader | Linux pinctrl | User (sysfs, backlight..) |
>> | 100k pull-up | (not used) |               |   enable    |   disable   |
>> +--------------+------------+---------------+---------------------------+
>>   ___________________________    default        _   _   _
>>                              |_________________| |_| |_| |_|_____________
>>
>>                                 pwm + gpio
>>   ___________________________________________   _   _   _   _____________
>>                                              |_| |_| |_| |_|
>>
>> Signed-off-by: Michal VokÃÄ <michal.vokac@xxxxxxxxx>
>> ---
>> Changes in v2:
>>   - Utilize the "pwm" and "gpio" pinctrl states.
>>   - Use the pwm-gpios signal to drive the output in "gpio" pinctrl state.
>>   - Select the right pinctrl state in probe.
>>
>>   drivers/pwm/pwm-imx.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 86 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>> index 6cd3b72..3502123 100644
>> --- a/drivers/pwm/pwm-imx.c
>> +++ b/drivers/pwm/pwm-imx.c
>> @@ -10,11 +10,13 @@
>>   #include <linux/clk.h>
>>   #include <linux/delay.h>
>>   #include <linux/err.h>
>> +#include <linux/gpio/consumer.h>
>>   #include <linux/io.h>
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>>   #include <linux/of_device.h>
>> +#include <linux/pinctrl/consumer.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/pwm.h>
>>   #include <linux/slab.h>
>> @@ -92,10 +94,45 @@ struct imx_chip {
>>   	void __iomem	*mmio_base;
>>   
>>   	struct pwm_chip	chip;
>> +
>> +	struct pinctrl *pinctrl;
>> +	struct pinctrl_state *pinctrl_pins_gpio;
>> +	struct pinctrl_state *pinctrl_pins_pwm;
>> +	struct gpio_desc *pwm_gpiod;
>>   };
>>   
>> +
>>   #define to_imx_chip(chip)	container_of(chip, struct imx_chip, chip)
>>   
>> +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 (!imx_chip->pinctrl || IS_ERR(imx_chip->pinctrl)) {
> 
> This is not correct. First, I don't think devm_pinctrl_get() will ever
> return NULL, so the !imx_chip->pinctrl is useless. And if it did return
> NULL and imx_chip->pinctrl could therefore be NULL, then...
> 
>> +		dev_info(&pdev->dev, "can not get pinctrl\n");
>> +		return PTR_ERR(imx_chip->pinctrl);
> 
> ... this is rubbish because it returns PTR_ERR(NULL) which is 0 and that
> represents success.

Good catch! Again, this code is actually an exact copy from i2c-imx.c.
I can send a patch to I2C folks with fix quoting your comment if you do
not mind.

> While at it, dev_info() -> dev_err() might be more appropriate here. Or
> if you want to make pinctrl support optional make this dev_dbg() like
> the message further below.

I prefer to make the pinctrl support optional so dev_dbg() seems OK.

>> +	}
>> +
>> +	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");
> 
> Another option would be to make this (and the above) dev_warn() since we
> do want people to update the DTB if at all possible. Without the DTB the
> PWM could be susceptible to the issue that we're trying to fix.

I do not think dev_warn() is appropriate here. IMHO the issue is only
relevant if you want generate inversed PWM signal. Only then you need
to specify all the pinctrl related binding. For normal polarity it is
almost useless I think. It can still be used of course but you would
want to configure the pin with pull-down and as a GPIO_ACTIVE_HIGH in
GPIO state.

And that case would need additional documentation. It seems little bit
redundant to me. I am open to do it that way (use dev_warn() and push
DTS authors to use the additional pinctrl binding in all cases) if you
think it is sane.

Michal