lkml.org 
[lkml]   [2018]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [RFC PATCH v3 2/2] pwm: imx: Configure output to GPIO in disabled state
    Date
    On 10.12.2018 12:17, Uwe Kleine-König wrote:
    > On Mon, Dec 10, 2018 at 11:15:05AM +0000, Vokáč Michal wrote:
    >> 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;
    >> +}
    >
    > This looks right.

    Wow, you're quick! OK, thanks.

    >>>>> 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?
    >
    > But then at least the pwm would run until the first consumer
    > reconfigures it.

    That is right. I think it is actually possible (and maybe good idea?)
    to drop the probe part from this pinctrl/GPIO series and put it into
    the .get_state series if the .get_state series would land in later.
    What do you think?

    Michal
    \
     
     \ /
      Last update: 2018-12-10 12:39    [W:2.989 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site