Re: [PATCH v3] drm/bridge: adv7511: Fix cec clock EPROBE_DEFER handling

[Vincent Whitchurch <vincent.whitchurch@axis.com>]

On Thu, Jul 02, 2020 at 04:22:34PM +0200, Andrzej Hajda wrote:
> On 14.05.2020 13:30, Vincent Whitchurch wrote:
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> > index a20a45c0b353..ee0ed4fb67c1 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> > @@ -286,28 +286,17 @@ static const struct cec_adap_ops adv7511_cec_adap_ops = {
> >   	.adap_transmit = adv7511_cec_adap_transmit,
> >   };
> >   
> > -static int adv7511_cec_parse_dt(struct device *dev, struct adv7511 *adv7511)
> > -{
> > -	adv7511->cec_clk = devm_clk_get(dev, "cec");
> > -	if (IS_ERR(adv7511->cec_clk)) {
> > -		int ret = PTR_ERR(adv7511->cec_clk);
> > -
> > -		adv7511->cec_clk = NULL;
> > -		return ret;
> > -	}
> > -	clk_prepare_enable(adv7511->cec_clk);
> > -	adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk);
> > -	return 0;
> > -}
> > -
> > -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
> > +void 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);
> > +	int ret;
> >   
> > -	if (ret)
> > -		goto err_cec_parse_dt;
> > +	if (!adv7511->cec_clk)
> > +		goto err_cec_no_clock;
> > +
> > +	clk_prepare_enable(adv7511->cec_clk);
> > +	adv7511->cec_clk_freq = clk_get_rate(adv7511->cec_clk);
> >   
> >   	adv7511->cec_adap = cec_allocate_adapter(&adv7511_cec_adap_ops,
> >   		adv7511, dev_name(dev), CEC_CAP_DEFAULTS, ADV7511_MAX_ADDRS);
> > @@ -334,7 +323,7 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
> >   	ret = cec_register_adapter(adv7511->cec_adap, dev);
> >   	if (ret)
> >   		goto err_cec_register;
> > -	return 0;
> > +	return;
> >   
> >   err_cec_register:
> >   	cec_delete_adapter(adv7511->cec_adap);
> > @@ -342,8 +331,11 @@ int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511)
> >   err_cec_alloc:
> >   	dev_info(dev, "Initializing CEC failed with error %d, disabling CEC\n",
> >   		 ret);
> > -err_cec_parse_dt:
> > +	clk_disable_unprepare(adv7511->cec_clk);
> > +	devm_clk_put(dev, adv7511->cec_clk);
> > +	/* Ensure that adv7511_remove() doesn't attempt to disable it again. */
> > +	adv7511->cec_clk = NULL;
> 
> Are you sure these three lines above are necessary? devm mechanism 
> should free the clock and with failed probe remove should not be called.

This driver ignores all failures in CEC registration/initialisation and
does not fail the probe for them.  I find that odd, but it seems to be
done like this on purpose (see commit 1b6fba458c0a2e8513 "drm/bridge:
adv7511/33: Fix adv7511_cec_init() failure handling") and my patch
preserves that behaviour.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel