| Message ID | 8f5c59f4-1cbf-0198-663d-2410e3826579@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 20, 2017 at 12:57 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > If the device tree for a board did not specify a cec clock, then > adv7511_cec_init would return an error, which would cause adv7511_probe() > to fail and thus there is no HDMI output. > > There is no need to have adv7511_probe() fail if the CEC initialization > fails, so just change adv7511_cec_init() to a void function. In addition, > adv7511_cec_init() should just return silently if the cec clock isn't > found and show a message for any other errors. > > An otherwise correct cleanup patch from Dan Carpenter turned this broken > failure handling into a kernel Oops, so bisection points to commit > 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL") rather > than 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support"). > > Based on earlier patches from Arnd and John. > > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > Cc: Xinliang Liu <xinliang.liu@linaro.org> > Cc: Dan Carpenter <dan.carpenter@oracle.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Archit Taneja <architt@codeaurora.org> > Cc: John Stultz <john.stultz@linaro.org> > Link: https://bugs.linaro.org/show_bug.cgi?id=3345 > Link: https://lkft.validation.linaro.org/scheduler/job/48017#L3551 > Fixes: 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL") > Fixes: 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support") > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > Tested-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > This rework of Arnd and John's patches goes a bit further and makes > cec_init a void function and just silently exits if there is no cec clock > defined in the dts. I'm sure that's the reason why the kirin board failed > on this. BTW: if the kirin board DOES support cec, then it would be nice > if it can be hooked up in the dts! > > Tested with my Dragonboard and Renesas Koelsch board. > > Change since my previous RFC PATCH: > > - added static inline adv7511_cec_init to avoid #ifdef in the probe function > as suggested by John Stultz. Thanks for adding the cleanup! Seems to work well on HiKey! Tested-by: John Stultz <john.stultz@linaro.org> thanks -john
Hi Hans, Thank you for the patch. On Monday, 20 November 2017 22:57:34 EET Hans Verkuil wrote: > If the device tree for a board did not specify a cec clock, then > adv7511_cec_init would return an error, which would cause adv7511_probe() > to fail and thus there is no HDMI output. > > There is no need to have adv7511_probe() fail if the CEC initialization > fails, so just change adv7511_cec_init() to a void function. In addition, > adv7511_cec_init() should just return silently if the cec clock isn't > found and show a message for any other errors. I don't think that's correct. You at least need to defer probing if the clock is specified in DT but has no provider available yet. For other errors I agree that we can still initialize the ADV7511 in a degraded mode without CEC support, although I would prefer to also error out in case of invalid DT to ensure that DT errors are caught as early as possible. > An otherwise correct cleanup patch from Dan Carpenter turned this broken > failure handling into a kernel Oops, so bisection points to commit > 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL") rather > than 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support"). > > Based on earlier patches from Arnd and John. > > Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> > Cc: Xinliang Liu <xinliang.liu@linaro.org> > Cc: Dan Carpenter <dan.carpenter@oracle.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Archit Taneja <architt@codeaurora.org> > Cc: John Stultz <john.stultz@linaro.org> > Link: https://bugs.linaro.org/show_bug.cgi?id=3345 > Link: https://lkft.validation.linaro.org/scheduler/job/48017#L3551 > Fixes: 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL") > Fixes: 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support") > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > Tested-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > This rework of Arnd and John's patches goes a bit further and makes > cec_init a void function and just silently exits if there is no cec clock > defined in the dts. I'm sure that's the reason why the kirin board failed > on this. BTW: if the kirin board DOES support cec, then it would be nice > if it can be hooked up in the dts! > > Tested with my Dragonboard and Renesas Koelsch board. > > Change since my previous RFC PATCH: > > - added static inline adv7511_cec_init to avoid #ifdef in the probe function > as suggested by John Stultz. > > Regards, > > Hans > --- > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h > b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 543a5eb91624..16051bfa5578 > 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > @@ -374,9 +374,17 @@ struct adv7511 { > }; > > #ifdef CONFIG_DRM_I2C_ADV7511_CEC > -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, > - unsigned int offset); > +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, > + unsigned int offset); > void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1); > +#else > +static inline void adv7511_cec_init(struct device *dev, > + struct adv7511 *adv7511, > + unsigned int offset) > +{ > + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, > + ADV7511_CEC_CTRL_POWER_DOWN); > +} > #endif > > #ifdef CONFIG_DRM_I2C_ADV7533 > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c index > b33d730e4d73..c1cd471d31fa 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c > @@ -300,18 +300,20 @@ static int adv7511_cec_parse_dt(struct device *dev, > struct adv7511 *adv7511) return 0; > } > > -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, > - unsigned int offset) > +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, > + unsigned int offset) > { > int ret = adv7511_cec_parse_dt(dev, adv7511); > > if (ret) > - return ret; > + goto disable_cec; > > adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops, > adv7511, dev_name(dev), CEC_CAP_DEFAULTS, ADV7511_MAX_ADDRS); > - if (IS_ERR(adv7511->cec_adap)) > - return PTR_ERR(adv7511->cec_adap); > + if (IS_ERR(adv7511->cec_adap)) { > + ret = PTR_ERR(adv7511->cec_adap); > + goto fail; > + } > > regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, 0); > /* cec soft reset */ > @@ -329,9 +331,15 @@ int adv7511_cec_init(struct device *dev, struct adv7511 > *adv7511, ((adv7511->cec_clk_freq / 750000) - 1) << 2); > > ret = cec_register_adapter(adv7511->cec_adap, dev); > - if (ret) { > - cec_delete_adapter(adv7511->cec_adap); > - adv7511->cec_adap = NULL; > - } > - return ret; > + if (!ret) > + return; This confused me for an instant, I think a goto error_cec_delete would be clearer, but that's up to you (maybe I just wasn't awake enough). > + cec_delete_adapter(adv7511->cec_adap); > + adv7511->cec_adap = NULL; > + > +fail: Nitpicking, I'd name this error to match the labels in the probe function. > + dev_info(dev, "Initializing CEC failed with error %d, disabling CEC\n", > + ret); > +disable_cec: > + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, > + ADV7511_CEC_CTRL_POWER_DOWN); > } > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index > 3a33075dbb22..2eb465827bcd 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -1200,15 +1200,7 @@ static int adv7511_probe(struct i2c_client *i2c, > const struct i2c_device_id *id) adv7511_audio_init(dev, adv7511); > > offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0; > - > -#ifdef CONFIG_DRM_I2C_ADV7511_CEC > - ret = adv7511_cec_init(dev, adv7511, offset); > - if (ret) > - goto err_unregister_cec; > -#else > - regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, > - ADV7511_CEC_CTRL_POWER_DOWN); > -#endif > + adv7511_cec_init(dev, adv7511, offset); Now that the offset is only passed to this function, how about computing it in adv7511_cec_init() to store as much CEC code as possible in drivers/gpu/drm/ bridge/adv7511/adv7511_cec.c ? > > return 0;
On 11/21/2017 07:48 AM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Monday, 20 November 2017 22:57:34 EET Hans Verkuil wrote: >> If the device tree for a board did not specify a cec clock, then >> adv7511_cec_init would return an error, which would cause adv7511_probe() >> to fail and thus there is no HDMI output. >> >> There is no need to have adv7511_probe() fail if the CEC initialization >> fails, so just change adv7511_cec_init() to a void function. In addition, >> adv7511_cec_init() should just return silently if the cec clock isn't >> found and show a message for any other errors. > > I don't think that's correct. You at least need to defer probing if the clock > is specified in DT but has no provider available yet. For other errors I agree > that we can still initialize the ADV7511 in a degraded mode without CEC > support, although I would prefer to also error out in case of invalid DT to > ensure that DT errors are caught as early as possible. Ah yes, probe deferring, I forgot about that. I'll make a v3. The only other possible error is ENOENT for when no CEC clock is defined, which just means it should continue without CEC (i.e. this is not an error). I'll make a v3, also incorporating your other comments below. Regards, Hans > >> An otherwise correct cleanup patch from Dan Carpenter turned this broken >> failure handling into a kernel Oops, so bisection points to commit >> 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL") rather >> than 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support"). >> >> Based on earlier patches from Arnd and John. >> >> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> >> Cc: Xinliang Liu <xinliang.liu@linaro.org> >> Cc: Dan Carpenter <dan.carpenter@oracle.com> >> Cc: Sean Paul <seanpaul@chromium.org> >> Cc: Archit Taneja <architt@codeaurora.org> >> Cc: John Stultz <john.stultz@linaro.org> >> Link: https://bugs.linaro.org/show_bug.cgi?id=3345 >> Link: https://lkft.validation.linaro.org/scheduler/job/48017#L3551 >> Fixes: 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL") >> Fixes: 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support") >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >> Tested-by: Hans Verkuil <hans.verkuil@cisco.com> >> --- >> This rework of Arnd and John's patches goes a bit further and makes >> cec_init a void function and just silently exits if there is no cec clock >> defined in the dts. I'm sure that's the reason why the kirin board failed >> on this. BTW: if the kirin board DOES support cec, then it would be nice >> if it can be hooked up in the dts! >> >> Tested with my Dragonboard and Renesas Koelsch board. >> >> Change since my previous RFC PATCH: >> >> - added static inline adv7511_cec_init to avoid #ifdef in the probe function >> as suggested by John Stultz. >> >> Regards, >> >> Hans >> --- >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h >> b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 543a5eb91624..16051bfa5578 >> 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h >> @@ -374,9 +374,17 @@ struct adv7511 { >> }; >> >> #ifdef CONFIG_DRM_I2C_ADV7511_CEC >> -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, >> - unsigned int offset); >> +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, >> + unsigned int offset); >> void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1); >> +#else >> +static inline void adv7511_cec_init(struct device *dev, >> + struct adv7511 *adv7511, >> + unsigned int offset) >> +{ >> + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, >> + ADV7511_CEC_CTRL_POWER_DOWN); >> +} >> #endif >> >> #ifdef CONFIG_DRM_I2C_ADV7533 >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c >> b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c index >> b33d730e4d73..c1cd471d31fa 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c >> @@ -300,18 +300,20 @@ static int adv7511_cec_parse_dt(struct device *dev, >> struct adv7511 *adv7511) return 0; >> } >> >> -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, >> - unsigned int offset) >> +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, >> + unsigned int offset) >> { >> int ret = adv7511_cec_parse_dt(dev, adv7511); >> >> if (ret) >> - return ret; >> + goto disable_cec; >> >> adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops, >> adv7511, dev_name(dev), CEC_CAP_DEFAULTS, ADV7511_MAX_ADDRS); >> - if (IS_ERR(adv7511->cec_adap)) >> - return PTR_ERR(adv7511->cec_adap); >> + if (IS_ERR(adv7511->cec_adap)) { >> + ret = PTR_ERR(adv7511->cec_adap); >> + goto fail; >> + } >> >> regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, 0); >> /* cec soft reset */ >> @@ -329,9 +331,15 @@ int adv7511_cec_init(struct device *dev, struct adv7511 >> *adv7511, ((adv7511->cec_clk_freq / 750000) - 1) << 2); >> >> ret = cec_register_adapter(adv7511->cec_adap, dev); >> - if (ret) { >> - cec_delete_adapter(adv7511->cec_adap); >> - adv7511->cec_adap = NULL; >> - } >> - return ret; >> + if (!ret) >> + return; > > This confused me for an instant, I think a goto error_cec_delete would be > clearer, but that's up to you (maybe I just wasn't awake enough). > >> + cec_delete_adapter(adv7511->cec_adap); >> + adv7511->cec_adap = NULL; >> + >> +fail: > > Nitpicking, I'd name this error to match the labels in the probe function. > >> + dev_info(dev, "Initializing CEC failed with error %d, disabling CEC\n", >> + ret); >> +disable_cec: >> + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, >> + ADV7511_CEC_CTRL_POWER_DOWN); >> } >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index >> 3a33075dbb22..2eb465827bcd 100644 >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c >> @@ -1200,15 +1200,7 @@ static int adv7511_probe(struct i2c_client *i2c, >> const struct i2c_device_id *id) adv7511_audio_init(dev, adv7511); >> >> offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0; >> - >> -#ifdef CONFIG_DRM_I2C_ADV7511_CEC >> - ret = adv7511_cec_init(dev, adv7511, offset); >> - if (ret) >> - goto err_unregister_cec; >> -#else >> - regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, >> - ADV7511_CEC_CTRL_POWER_DOWN); >> -#endif >> + adv7511_cec_init(dev, adv7511, offset); > > Now that the offset is only passed to this function, how about computing it in > adv7511_cec_init() to store as much CEC code as possible in drivers/gpu/drm/ > bridge/adv7511/adv7511_cec.c ? > >> >> return 0; >
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index 543a5eb91624..16051bfa5578 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -374,9 +374,17 @@ struct adv7511 { }; #ifdef CONFIG_DRM_I2C_ADV7511_CEC -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, - unsigned int offset); +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, + unsigned int offset); void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1); +#else +static inline void adv7511_cec_init(struct device *dev, + struct adv7511 *adv7511, + unsigned int offset) +{ + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, + ADV7511_CEC_CTRL_POWER_DOWN); +} #endif #ifdef CONFIG_DRM_I2C_ADV7533 diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c index b33d730e4d73..c1cd471d31fa 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c @@ -300,18 +300,20 @@ static int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511) return 0; } -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, - unsigned int offset) +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, + unsigned int offset) { int ret = adv7511_cec_parse_dt(dev, adv7511); if (ret) - return ret; + goto disable_cec; adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops, adv7511, dev_name(dev), CEC_CAP_DEFAULTS, ADV7511_MAX_ADDRS); - if (IS_ERR(adv7511->cec_adap)) - return PTR_ERR(adv7511->cec_adap); + if (IS_ERR(adv7511->cec_adap)) { + ret = PTR_ERR(adv7511->cec_adap); + goto fail; + } regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, 0); /* cec soft reset */ @@ -329,9 +331,15 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, ((adv7511->cec_clk_freq / 750000) - 1) << 2); ret = cec_register_adapter(adv7511->cec_adap, dev); - if (ret) { - cec_delete_adapter(adv7511->cec_adap); - adv7511->cec_adap = NULL; - } - return ret; + if (!ret) + return; + cec_delete_adapter(adv7511->cec_adap); + adv7511->cec_adap = NULL; + +fail: + dev_info(dev, "Initializing CEC failed with error %d, disabling CEC\n", + ret); +disable_cec: + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, + ADV7511_CEC_CTRL_POWER_DOWN); } diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 3a33075dbb22..2eb465827bcd 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1200,15 +1200,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) adv7511_audio_init(dev, adv7511); offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0; - -#ifdef CONFIG_DRM_I2C_ADV7511_CEC - ret = adv7511_cec_init(dev, adv7511, offset); - if (ret) - goto err_unregister_cec; -#else - regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, - ADV7511_CEC_CTRL_POWER_DOWN); -#endif + adv7511_cec_init(dev, adv7511, offset); return 0;
If the device tree for a board did not specify a cec clock, then adv7511_cec_init would return an error, which would cause adv7511_probe() to fail and thus there is no HDMI output. There is no need to have adv7511_probe() fail if the CEC initialization fails, so just change adv7511_cec_init() to a void function. In addition, adv7511_cec_init() should just return silently if the cec clock isn't found and show a message for any other errors. An otherwise correct cleanup patch from Dan Carpenter turned this broken failure handling into a kernel Oops, so bisection points to commit 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL") rather than 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support"). Based on earlier patches from Arnd and John. Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org> Cc: Xinliang Liu <xinliang.liu@linaro.org> Cc: Dan Carpenter <dan.carpenter@oracle.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: Archit Taneja <architt@codeaurora.org> Cc: John Stultz <john.stultz@linaro.org> Link: https://bugs.linaro.org/show_bug.cgi?id=3345 Link: https://lkft.validation.linaro.org/scheduler/job/48017#L3551 Fixes: 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL") Fixes: 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support") Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> Tested-by: Hans Verkuil <hans.verkuil@cisco.com> --- This rework of Arnd and John's patches goes a bit further and makes cec_init a void function and just silently exits if there is no cec clock defined in the dts. I'm sure that's the reason why the kirin board failed on this. BTW: if the kirin board DOES support cec, then it would be nice if it can be hooked up in the dts! Tested with my Dragonboard and Renesas Koelsch board. Change since my previous RFC PATCH: - added static inline adv7511_cec_init to avoid #ifdef in the probe function as suggested by John Stultz. Regards, Hans ---