On 12.10.2015 17:06, Boris Brezillon wrote: > On Mon, 12 Oct 2015 16:54:39 +0300 > Vladimir Zapolskiy <[email protected]> wrote: > >> Hi Boris, >> >> On 12.10.2015 16:30, Boris Brezillon wrote: >>> Hi Vladimir, >>> >>> On Mon, 12 Oct 2015 15:16:44 +0200 >>> Nicolas Ferre <[email protected]> wrote: >>> >>>> Le 12/10/2015 14:29, Vladimir Zapolskiy a écrit : >>>>> Platform PWM backlight data provided by board's device tree should be >>>>> complete enough to successfully request a pwm device using pwm_get() >>>>> API. This change fixes a bug, when an arbitrary (first found) PWM is >>>>> connected to a "pwm-backlight" compatible device, when explicit PWM >>>>> device reference is not given. >>>>> >>>>> Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt >>>>> already describes "pwms" as a required property, instead of blind >>>>> selection of a potentially wrong PWM reject legacy PWM device >>>>> registration request, leave legacy API only for non-dt cases. >>>>> >>>>> Based on initial implementation done by Dmitry Eremin-Solenikov. >>>>> >>>>> Reported-by: Dmitry Eremin-Solenikov <[email protected]> >>>>> Signed-off-by: Vladimir Zapolskiy <[email protected]> >>>>> Acked-by: Thierry Reding <[email protected]> >>>>> Acked-by: Lee Jones <[email protected]> >>>> >>>> It seems good to me: >>>> Acked-by: Nicolas Ferre <[email protected]> >>>> >>>> (Adding some people to the Cc: list). >>>> >>>> >>>>> --- >>>>> The change is based on lee-backlight/for-backlight-next >>>>> >>>>> Changes from v1 to v2: >>>>> * rebased on top of Nicolas' commit >>>>> 68feaca0b13 ("backlight: pwm: Handle EPROBE_DEFER while requesting >>>>> the PWM") >>>>> >>>>> Links to previous discussions of the change: >>>>> * https://patchwork.ozlabs.org/patch/483993/ >>>>> * https://patchwork.ozlabs.org/patch/398849/ >>>>> >>>>> drivers/video/backlight/pwm_bl.c | 19 +++++++++---------- >>>>> 1 file changed, 9 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/drivers/video/backlight/pwm_bl.c >>>>> b/drivers/video/backlight/pwm_bl.c >>>>> index eff379b..ae3c6b6 100644 >>>>> --- a/drivers/video/backlight/pwm_bl.c >>>>> +++ b/drivers/video/backlight/pwm_bl.c >>>>> @@ -271,19 +271,18 @@ static int pwm_backlight_probe(struct >>>>> platform_device *pdev) >>>>> } >>>>> >>>>> pb->pwm = devm_pwm_get(&pdev->dev, NULL); >>>>> - if (IS_ERR(pb->pwm)) { >>>>> - ret = PTR_ERR(pb->pwm); >>>>> - if (ret == -EPROBE_DEFER) >>>>> - goto err_alloc; >>>>> - >>>>> + if (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER >>>>> + && !pdev->dev.of_node) { >>>>> dev_err(&pdev->dev, "unable to request PWM, trying legacy >>>>> API\n"); >>>>> pb->legacy = true; >>>>> pb->pwm = pwm_request(data->pwm_id, "pwm-backlight"); >>>>> - if (IS_ERR(pb->pwm)) { >>>>> - dev_err(&pdev->dev, "unable to request legacy PWM\n"); >>>>> - ret = PTR_ERR(pb->pwm); >>>>> - goto err_alloc; >>>>> - } >>>>> + } >>>>> + >>>>> + if (IS_ERR(pb->pwm)) { >>>>> + ret = PTR_ERR(pb->pwm); >>>>> + if (ret != -EPROBE_DEFER) >>>>> + dev_err(&pdev->dev, "unable to request PWM\n"); >>>>> + goto err_alloc; >>>>> } >>>>> >>>>> dev_dbg(&pdev->dev, "got pwm for backlight\n"); >>>>> >>>> >>>> >>> >>> I still think it would be cleaner to do what Thierry proposed here [1]. >>> IMO, embedding the complexity of different error cases depending on the >>> way PWM devices were defined (OF, pdata, ...) is rather risky and >>> make the code even more complicated. >> >> please correct me if I'm wrong, I suppose Thierry's change fixes >> Nicolas' commit 68feaca0b13 only, and the intention of my change is to >> fix an absolutely unrelated problem, see the commit message. >> >> So, since still there is a remained chance of getting -EPROBE_DEFER from >> pwm_get(), e.g. from of_pwm_get() or failed pwmchip_find_by_name() or >> pwm->chip->ops->request() I don't see how Thierry's change alone may >> help me to overcome the problem I'm trying to solve here. > > The only valid case where EPROBE_DEFER should be returned is when we > have a device that is not ready to be used yet (but we're sure that we > have this device declared, using either the PWM lookup table or the DT > definition in the PWM subsystem case).
That's fine, and it is reflected in my change. > Thierry's patch makes sure that EPROBE_DEFER is not returned when the > PWM device definition is not found using in the PWM lookup tables or > the DT definition, This is okay, but I'm interested in proper handling of cases other than EPROBE_DEFER. EPROBE_DEFER and the related issues are on your balance and I'm attempting to avoid interfering with it here :) > and in this case the pwm_bl code will fallback to > the legacy PWM API, which AFAICT is what you're trying to solve. Fallback must happen exclusively under (IS_ERR(pb->pwm) && PTR_ERR(pb->pwm) != -EPROBE_DEFER && !pdev->dev.of_node) condition IMHO. Before EPROBE_DEFER appeared on the scene the condition was (IS_ERR(pb->pwm) && !pdev->dev.of_node). So, the question is if my change requires any updates or not from your point of view. -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
