| Message ID | 9097b2a4-b6b9-5fca-e039-0a17694b1143@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hans, Thank you for the patch. On Tuesday, 21 November 2017 10:17:43 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. > > 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> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > This rework of Arnd and John's patches goes a bit further 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. Also tested what > happens when probing is deferred due to missing cec clock. > > John, can you test this again? > > Changes since v2: > - reverted adv7511_cec_init back to an int function for EPROBE_DEFER > - incorporated Laurent's comments > - moved the adv7511_cec_init call up in the probe function to prevent > having to remove the bridge in case of an error. > > 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 b4efcbabf7f7..d034b2cb5eee > 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h > @@ -372,9 +372,18 @@ struct adv7511 { > }; > > #ifdef CONFIG_DRM_I2C_ADV7511_CEC > -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, > - unsigned int offset); > +int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511); > void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1); > +#else > +static inline int adv7511_cec_init(struct device *dev, struct adv7511 > *adv7511) +{ > + unsigned int offset = adv7511->type == ADV7533 ? > + ADV7533_REG_CEC_OFFSET : 0; > + > + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, > + ADV7511_CEC_CTRL_POWER_DOWN); > + return 0; > +} > #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..a20a45c0b353 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c > @@ -300,18 +300,21 @@ 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) > +int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) > { > + unsigned int offset = adv7511->type == ADV7533 ? > + ADV7533_REG_CEC_OFFSET : 0; > int ret = adv7511_cec_parse_dt(dev, adv7511); > > if (ret) > - return ret; > + goto err_cec_parse_dt; > > 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 err_cec_alloc; > + } > > regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, 0); > /* cec soft reset */ > @@ -329,9 +332,18 @@ 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) > + goto err_cec_register; > + return 0; > + > +err_cec_register: > + cec_delete_adapter(adv7511->cec_adap); > + adv7511->cec_adap = NULL; > +err_cec_alloc: > + dev_info(dev, "Initializing CEC failed with error %d, disabling CEC\n", > + ret); > +err_cec_parse_dt: > + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, > + ADV7511_CEC_CTRL_POWER_DOWN); > + return ret == -EPROBE_DEFER ? ret : 0; > } > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index > 0e14f1572d05..efa29db5fc2b 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -1084,7 +1084,6 @@ static int adv7511_probe(struct i2c_client *i2c, const > struct i2c_device_id *id) struct device *dev = &i2c->dev; > unsigned int main_i2c_addr = i2c->addr << 1; > unsigned int edid_i2c_addr = main_i2c_addr + 4; > - unsigned int offset; > unsigned int val; > int ret; > > @@ -1192,24 +1191,16 @@ static int adv7511_probe(struct i2c_client *i2c, > const struct i2c_device_id *id) if (adv7511->type == ADV7511) > adv7511_set_link_config(adv7511, &link_config); > > + ret = adv7511_cec_init(dev, adv7511); > + if (ret) > + goto err_unregister_cec; > + > adv7511->bridge.funcs = &adv7511_bridge_funcs; > adv7511->bridge.of_node = dev->of_node; > > drm_bridge_add(&adv7511->bridge); > > 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 > - > return 0; > > err_unregister_cec:
On Tue, Nov 21, 2017 at 12:17 AM, 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 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. Also tested what > happens when probing is deferred due to missing cec clock. > > John, can you test this again? Sorry I didn't get back to you yesterday on this! Seems to be working ok for me! Tested-by: John Stultz <john.stultz@linaro.org> thanks! -john
On 11/23/2017 05:52 AM, John Stultz wrote: > On Tue, Nov 21, 2017 at 12:17 AM, 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 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. Also tested what >> happens when probing is deferred due to missing cec clock. >> >> John, can you test this again? > > Sorry I didn't get back to you yesterday on this! > > Seems to be working ok for me! > > Tested-by: John Stultz <john.stultz@linaro.org> Queued to drm-misc-fixes. Thanks for fixing this. Archit
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h index b4efcbabf7f7..d034b2cb5eee 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h @@ -372,9 +372,18 @@ struct adv7511 { }; #ifdef CONFIG_DRM_I2C_ADV7511_CEC -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511, - unsigned int offset); +int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511); void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1); +#else +static inline int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) +{ + unsigned int offset = adv7511->type == ADV7533 ? + ADV7533_REG_CEC_OFFSET : 0; + + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, + ADV7511_CEC_CTRL_POWER_DOWN); + return 0; +} #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..a20a45c0b353 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c @@ -300,18 +300,21 @@ 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) +int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511) { + unsigned int offset = adv7511->type == ADV7533 ? + ADV7533_REG_CEC_OFFSET : 0; int ret = adv7511_cec_parse_dt(dev, adv7511); if (ret) - return ret; + goto err_cec_parse_dt; 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 err_cec_alloc; + } regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, 0); /* cec soft reset */ @@ -329,9 +332,18 @@ 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) + goto err_cec_register; + return 0; + +err_cec_register: + cec_delete_adapter(adv7511->cec_adap); + adv7511->cec_adap = NULL; +err_cec_alloc: + dev_info(dev, "Initializing CEC failed with error %d, disabling CEC\n", + ret); +err_cec_parse_dt: + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, + ADV7511_CEC_CTRL_POWER_DOWN); + return ret == -EPROBE_DEFER ? ret : 0; } diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 0e14f1572d05..efa29db5fc2b 100644 --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c @@ -1084,7 +1084,6 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) struct device *dev = &i2c->dev; unsigned int main_i2c_addr = i2c->addr << 1; unsigned int edid_i2c_addr = main_i2c_addr + 4; - unsigned int offset; unsigned int val; int ret; @@ -1192,24 +1191,16 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id) if (adv7511->type == ADV7511) adv7511_set_link_config(adv7511, &link_config); + ret = adv7511_cec_init(dev, adv7511); + if (ret) + goto err_unregister_cec; + adv7511->bridge.funcs = &adv7511_bridge_funcs; adv7511->bridge.of_node = dev->of_node; drm_bridge_add(&adv7511->bridge); 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 - return 0; err_unregister_cec:
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 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. Also tested what happens when probing is deferred due to missing cec clock. John, can you test this again? Changes since v2: - reverted adv7511_cec_init back to an int function for EPROBE_DEFER - incorporated Laurent's comments - moved the adv7511_cec_init call up in the probe function to prevent having to remove the bridge in case of an error. 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 ---