[PATCH] [RFT] drm: adv7511/33: fix adv7511_cec_init() failure handling

[PATCH] [RFT] drm: adv7511/33: fix adv7511_cec_init() failure handling

[Arnd Bergmann]

An otherwise correct cleanup patch from Dan Carpenter turned a broken
failure handling from a feature patch by Hans Verkuil 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").

I've managed to piece together several partial problems, though
I'm still struggling with the bigger picture:

adv7511_probe() registers a drm_bridge structure that was allocated
with devm_kzalloc(). It calls adv7511_cec_init(), which fails for an
unknown reason, which in turn triggers the registered structure to be
removed.

Elsewhere, kirin_drm_platform_probe() gets called, which calls
of_graph_get_remote_node(), and that returns NULL. Before Dan's
patch we would go on with a NULL pointer here and register that,
now kirin_drm_platform_probe() fails with -ENODEV.

In a third driver, dsi_parse_dt() calls drm_of_find_panel_or_bridge(),
which after not finding a panel goes on to call of_drm_find_bridge(),
and that crashes due to the earlier list corruption.

This addresses the first issue by making sure that adv7511_probe()
does not leave behind any corrupted list entries. This should
get the system back to boot but needs testing. From my understanding,
there is at least one more bug that needs to be resolved to actually
get everything to work again.

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: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Archit Taneja <architt@codeaurora.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: Arnd Bergmann <arnd@arndb.de>
---
Untested so far, this is what I came up with after reading the
WARN_ON log from a modified kernel.

Naresh, can you give this one a go?

Hans and others, can you review in the meantime?

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 0e14f1572d05..93d1ecafe8fa 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1204,7 +1204,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 #ifdef CONFIG_DRM_I2C_ADV7511_CEC
 	ret = adv7511_cec_init(dev, adv7511, offset);
 	if (ret)
-		goto err_unregister_cec;
+		goto err_unregister_bridge;
 #else
 	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
 		     ADV7511_CEC_CTRL_POWER_DOWN);
@@ -1212,6 +1212,11 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 
 	return 0;
 
+#ifdef CONFIG_DRM_I2C_ADV7511_CEC
+err_unregister_bridge:
+	adv7511_audio_exit(adv7511);
+	drm_bridge_remove(&adv7511->bridge);
+#endif
 err_unregister_cec:
 	i2c_unregister_device(adv7511->i2c_cec);
 	if (adv7511->cec_clk)
-- 
2.9.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] [RFT] drm: adv7511/33: fix adv7511_cec_init() failure handling

[Hans Verkuil]

On 15/11/17 13:37, Arnd Bergmann wrote:
> An otherwise correct cleanup patch from Dan Carpenter turned a broken
> failure handling from a feature patch by Hans Verkuil 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").
> 
> I've managed to piece together several partial problems, though
> I'm still struggling with the bigger picture:
> 
> adv7511_probe() registers a drm_bridge structure that was allocated
> with devm_kzalloc(). It calls adv7511_cec_init(), which fails for an
> unknown reason, which in turn triggers the registered structure to be
> removed.
> 
> Elsewhere, kirin_drm_platform_probe() gets called, which calls
> of_graph_get_remote_node(), and that returns NULL. Before Dan's
> patch we would go on with a NULL pointer here and register that,
> now kirin_drm_platform_probe() fails with -ENODEV.
> 
> In a third driver, dsi_parse_dt() calls drm_of_find_panel_or_bridge(),
> which after not finding a panel goes on to call of_drm_find_bridge(),
> and that crashes due to the earlier list corruption.
> 
> This addresses the first issue by making sure that adv7511_probe()
> does not leave behind any corrupted list entries. This should
> get the system back to boot but needs testing. From my understanding,
> there is at least one more bug that needs to be resolved to actually
> get everything to work again.
> 
> 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: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Archit Taneja <architt@codeaurora.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: Arnd Bergmann <arnd@arndb.de>
> ---
> Untested so far, this is what I came up with after reading the
> WARN_ON log from a modified kernel.
> 
> Naresh, can you give this one a go?
> 
> Hans and others, can you review in the meantime?
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 0e14f1572d05..93d1ecafe8fa 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1204,7 +1204,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>  #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>  	ret = adv7511_cec_init(dev, adv7511, offset);
>  	if (ret)
> -		goto err_unregister_cec;
> +		goto err_unregister_bridge;

Rather than adding the err_unregister_bridge label, I think it is better to move
this code up to just before the call to drm_bridge_add().

I think I just didn't realize that doing it after would require additional cleanup.
But it should be perfectly fine to move it up so we can avoid doing that.

I can't test it until Monday as I don't have access to the hardware at the moment.

Regards,

	Hans

>  #else
>  	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>  		     ADV7511_CEC_CTRL_POWER_DOWN);
> @@ -1212,6 +1212,11 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>  
>  	return 0;
>  
> +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
> +err_unregister_bridge:
> +	adv7511_audio_exit(adv7511);
> +	drm_bridge_remove(&adv7511->bridge);
> +#endif
>  err_unregister_cec:
>  	i2c_unregister_device(adv7511->i2c_cec);
>  	if (adv7511->cec_clk)
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] [RFT] drm: adv7511/33: fix adv7511_cec_init() failure handling

[Naresh Kamboju]

On 15 November 2017 at 18:28, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 15/11/17 13:37, Arnd Bergmann wrote:
>> An otherwise correct cleanup patch from Dan Carpenter turned a broken
>> failure handling from a feature patch by Hans Verkuil 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").
>>
>> I've managed to piece together several partial problems, though
>> I'm still struggling with the bigger picture:
>>
>> adv7511_probe() registers a drm_bridge structure that was allocated
>> with devm_kzalloc(). It calls adv7511_cec_init(), which fails for an
>> unknown reason, which in turn triggers the registered structure to be
>> removed.
>>
>> Elsewhere, kirin_drm_platform_probe() gets called, which calls
>> of_graph_get_remote_node(), and that returns NULL. Before Dan's
>> patch we would go on with a NULL pointer here and register that,
>> now kirin_drm_platform_probe() fails with -ENODEV.
>>
>> In a third driver, dsi_parse_dt() calls drm_of_find_panel_or_bridge(),
>> which after not finding a panel goes on to call of_drm_find_bridge(),
>> and that crashes due to the earlier list corruption.
>>
>> This addresses the first issue by making sure that adv7511_probe()
>> does not leave behind any corrupted list entries. This should
>> get the system back to boot but needs testing. From my understanding,
>> there is at least one more bug that needs to be resolved to actually
>> get everything to work again.
>>
>> 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: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Archit Taneja <architt@codeaurora.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: Arnd Bergmann <arnd@arndb.de>
>> ---
>> Untested so far, this is what I came up with after reading the
>> WARN_ON log from a modified kernel.
>>
>> Naresh, can you give this one a go?

Tested with this patch.
I did not notice kernel crash/WARNING in dmesg log on HiKey (arm64) board.

Ref test log:
Link: https://pastebin.com/t8iLEFwF

Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>

>>
>> Hans and others, can you review in the meantime?
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index 0e14f1572d05..93d1ecafe8fa 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -1204,7 +1204,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>  #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>>       ret = adv7511_cec_init(dev, adv7511, offset);
>>       if (ret)
>> -             goto err_unregister_cec;
>> +             goto err_unregister_bridge;
>
> Rather than adding the err_unregister_bridge label, I think it is better to move
> this code up to just before the call to drm_bridge_add().
>
> I think I just didn't realize that doing it after would require additional cleanup.
> But it should be perfectly fine to move it up so we can avoid doing that.
>
> I can't test it until Monday as I don't have access to the hardware at the moment.
>
> Regards,
>
>         Hans
>
>>  #else
>>       regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>>                    ADV7511_CEC_CTRL_POWER_DOWN);
>> @@ -1212,6 +1212,11 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>>
>>       return 0;
>>
>> +#ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> +err_unregister_bridge:
>> +     adv7511_audio_exit(adv7511);
>> +     drm_bridge_remove(&adv7511->bridge);
>> +#endif
>>  err_unregister_cec:
>>       i2c_unregister_device(adv7511->i2c_cec);
>>       if (adv7511->cec_clk)
>>
>

Re: [PATCH] [RFT] drm: adv7511/33: fix adv7511_cec_init() failure handling

[John Stultz]

On Wed, Nov 15, 2017 at 4:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> An otherwise correct cleanup patch from Dan Carpenter turned a broken
> failure handling from a feature patch by Hans Verkuil 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").
>
> I've managed to piece together several partial problems, though
> I'm still struggling with the bigger picture:
>
> adv7511_probe() registers a drm_bridge structure that was allocated
> with devm_kzalloc(). It calls adv7511_cec_init(), which fails for an
> unknown reason, which in turn triggers the registered structure to be
> removed.
>
> Elsewhere, kirin_drm_platform_probe() gets called, which calls
> of_graph_get_remote_node(), and that returns NULL. Before Dan's
> patch we would go on with a NULL pointer here and register that,
> now kirin_drm_platform_probe() fails with -ENODEV.
>
> In a third driver, dsi_parse_dt() calls drm_of_find_panel_or_bridge(),
> which after not finding a panel goes on to call of_drm_find_bridge(),
> and that crashes due to the earlier list corruption.
>
> This addresses the first issue by making sure that adv7511_probe()
> does not leave behind any corrupted list entries. This should
> get the system back to boot but needs testing. From my understanding,
> there is at least one more bug that needs to be resolved to actually
> get everything to work again.

So I've started hitting the issue this patch tries to address (now
that the related code landed in Linus' tree). The only issue is that
with this fix, I don't see graphics initializing properly, so I
suspect something is still wrong with the error handling (though what
exactly I'm not sure).

If you have a new version of the patch with Hans' feedback in it, I'd
be happy to re-test.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] [RFT] drm: adv7511/33: fix adv7511_cec_init() failure handling

[John Stultz]

On Thu, Nov 16, 2017 at 1:50 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Nov 15, 2017 at 4:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> An otherwise correct cleanup patch from Dan Carpenter turned a broken
>> failure handling from a feature patch by Hans Verkuil 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").
>>
>> I've managed to piece together several partial problems, though
>> I'm still struggling with the bigger picture:
>>
>> adv7511_probe() registers a drm_bridge structure that was allocated
>> with devm_kzalloc(). It calls adv7511_cec_init(), which fails for an
>> unknown reason, which in turn triggers the registered structure to be
>> removed.
>>
>> Elsewhere, kirin_drm_platform_probe() gets called, which calls
>> of_graph_get_remote_node(), and that returns NULL. Before Dan's
>> patch we would go on with a NULL pointer here and register that,
>> now kirin_drm_platform_probe() fails with -ENODEV.
>>
>> In a third driver, dsi_parse_dt() calls drm_of_find_panel_or_bridge(),
>> which after not finding a panel goes on to call of_drm_find_bridge(),
>> and that crashes due to the earlier list corruption.
>>
>> This addresses the first issue by making sure that adv7511_probe()
>> does not leave behind any corrupted list entries. This should
>> get the system back to boot but needs testing. From my understanding,
>> there is at least one more bug that needs to be resolved to actually
>> get everything to work again.
>
> So I've started hitting the issue this patch tries to address (now
> that the related code landed in Linus' tree). The only issue is that
> with this fix, I don't see graphics initializing properly, so I
> suspect something is still wrong with the error handling (though what
> exactly I'm not sure).

So this seems to only happen when CONFIG_DRM_I2C_ADV7511_CEC is
enabled. If it is on, I don't get any graphics, but if its disabled
graphics works.

Tying this with Arnd's patch, I'm guessing adv7511_cec_init() is
failing, but it seems like instead of just disabling the CEC feature,
we're failing to load the driver entirely.

Maybe should the logic be something like:
#ifdef CONFIG_DRM_I2C_ADV7511_CEC
        ret = adv7511_cec_init(dev, adv7511, offset);
        if (ret)
#endif
            regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
                        ADV7511_CEC_CTRL_POWER_DOWN);

?
thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] [RFT] drm: adv7511/33: fix adv7511_cec_init() failure handling

[John Stultz]

On Thu, Nov 16, 2017 at 2:20 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Nov 16, 2017 at 1:50 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On Wed, Nov 15, 2017 at 4:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> An otherwise correct cleanup patch from Dan Carpenter turned a broken
>>> failure handling from a feature patch by Hans Verkuil 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").
>>>
>>> I've managed to piece together several partial problems, though
>>> I'm still struggling with the bigger picture:
>>>
>>> adv7511_probe() registers a drm_bridge structure that was allocated
>>> with devm_kzalloc(). It calls adv7511_cec_init(), which fails for an
>>> unknown reason, which in turn triggers the registered structure to be
>>> removed.
>>>
>>> Elsewhere, kirin_drm_platform_probe() gets called, which calls
>>> of_graph_get_remote_node(), and that returns NULL. Before Dan's
>>> patch we would go on with a NULL pointer here and register that,
>>> now kirin_drm_platform_probe() fails with -ENODEV.
>>>
>>> In a third driver, dsi_parse_dt() calls drm_of_find_panel_or_bridge(),
>>> which after not finding a panel goes on to call of_drm_find_bridge(),
>>> and that crashes due to the earlier list corruption.
>>>
>>> This addresses the first issue by making sure that adv7511_probe()
>>> does not leave behind any corrupted list entries. This should
>>> get the system back to boot but needs testing. From my understanding,
>>> there is at least one more bug that needs to be resolved to actually
>>> get everything to work again.
>>
>> So I've started hitting the issue this patch tries to address (now
>> that the related code landed in Linus' tree). The only issue is that
>> with this fix, I don't see graphics initializing properly, so I
>> suspect something is still wrong with the error handling (though what
>> exactly I'm not sure).
>
> So this seems to only happen when CONFIG_DRM_I2C_ADV7511_CEC is
> enabled. If it is on, I don't get any graphics, but if its disabled
> graphics works.
>
> Tying this with Arnd's patch, I'm guessing adv7511_cec_init() is
> failing, but it seems like instead of just disabling the CEC feature,
> we're failing to load the driver entirely.
>
> Maybe should the logic be something like:
> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>         ret = adv7511_cec_init(dev, adv7511, offset);
>         if (ret)
> #endif
>             regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>                         ADV7511_CEC_CTRL_POWER_DOWN);
>
> ?

I just tested with this, and this approach seems to work for me.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC][PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[John Stultz]

From: Arnd Bergmann <arnd@arndb.de>

An otherwise correct cleanup patch from Dan Carpenter turned a broken
failure handling from a feature patch by Hans Verkuil 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").

I've managed to piece together several partial problems, though
I'm still struggling with the bigger picture:

adv7511_probe() registers a drm_bridge structure that was allocated
with devm_kzalloc(). It calls adv7511_cec_init(), which fails for an
unknown reason, which in turn triggers the registered structure to be
removed.

Elsewhere, kirin_drm_platform_probe() gets called, which calls
of_graph_get_remote_node(), and that returns NULL. Before Dan's
patch we would go on with a NULL pointer here and register that,
now kirin_drm_platform_probe() fails with -ENODEV.

In a third driver, dsi_parse_dt() calls drm_of_find_panel_or_bridge(),
which after not finding a panel goes on to call of_drm_find_bridge(),
and that crashes due to the earlier list corruption.

This addresses the first issue by making sure that adv7511_probe()
does not completely fail when the adv7511_cec_init() function fails,
and instead we just disable the CEC feature. This avoids having the
driver entirely fail to load if just the CEC initialization fails.

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: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Archit Taneja <architt@codeaurora.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: Arnd Bergmann <arnd@arndb.de>
[jstultz: Reworked so when adv7511_cec_init() fails, we disable the feature instead
of disabling the entire driver, which causes graphics to not funciton]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
Just wanted to send out my rework of Arnd's patch here.
Feedback would be welcome.

thanks
-john

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 0e14f15..939c3b9 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1203,12 +1203,12 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 
 #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);
+	ret = 1;
 #endif
+	if (ret)
+		regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
+			     ADV7511_CEC_CTRL_POWER_DOWN);
 
 	return 0;
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC] [PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Hans Verkuil]

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: Hans Verkuil <hans.verkuil@cisco.com>
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>
---
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!

I'll test this with my two adv7511/33 boards on Monday.

Regards,

	Hans
---
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
index 543a5eb91624..bc17aa965e58 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
@@ -374,8 +374,8 @@ 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);
 #endif

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..56eeeea6a1fa 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1202,9 +1202,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	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;
+	adv7511_cec_init(dev, adv7511, offset);
 #else
 	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
 		     ADV7511_CEC_CTRL_POWER_DOWN);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] [PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Hans Verkuil]

On 11/17/2017 09:43 AM, 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: Hans Verkuil <hans.verkuil@cisco.com>
> 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>

I tested this patch on a Dragonboard and a Renesas Koelsch board. I also forced
errors in parsing the dts or registering the CEC adapter to test those failure
paths on both boards.

So:

Tested-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
> 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!
> 
> I'll test this with my two adv7511/33 boards on Monday.
> 
> Regards,
> 
> 	Hans
> ---
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index 543a5eb91624..bc17aa965e58 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -374,8 +374,8 @@ 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);
>  #endif
> 
> 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..56eeeea6a1fa 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1202,9 +1202,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>  	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;
> +	adv7511_cec_init(dev, adv7511, offset);
>  #else
>  	regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>  		     ADV7511_CEC_CTRL_POWER_DOWN);
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] [PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Hans Verkuil]

On 11/20/2017 04:05 PM, Hans Verkuil wrote:
> On 11/17/2017 09:43 AM, 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: Hans Verkuil <hans.verkuil@cisco.com>
>> 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>
> 
> I tested this patch on a Dragonboard and a Renesas Koelsch board. I also forced
> errors in parsing the dts or registering the CEC adapter to test those failure
> paths on both boards.
> 
> So:
> 
> Tested-by: Hans Verkuil <hans.verkuil@cisco.com>

As far as I am concerned the [RFC] part can be dropped from this patch.

Regards,

	Hans

Re: [RFC] [PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[John Stultz]

On Fri, Nov 17, 2017 at 12:43 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 3a33075dbb22..56eeeea6a1fa 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1202,9 +1202,7 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>         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;
> +       adv7511_cec_init(dev, adv7511, offset);
>  #else
>         regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>                      ADV7511_CEC_CTRL_POWER_DOWN);


One tiny nit-pick I realized I should have made in my patch...

In the !CONFIG_DRM_I2C_ADV7511_CEC, can you just define adv7511_cec_init() as
{
    regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
ADV7511_CEC_CTRL_POWER_DOWN);
}

Then we can call it either way, and don't need to have the ufly
#ifdefs in the adv7511_probe function?

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCHv2] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Hans Verkuil]

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
---
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;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[John Stultz]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

[PATCHv3] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Hans Verkuil]

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
---
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:
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv2] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Hans Verkuil]

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;
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCHv3] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Laurent Pinchart]

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:

-- 
Regards,

Laurent Pinchart

Re: [PATCHv3] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[John Stultz]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Archit Taneja]

Hi,

On 11/17/2017 04:29 AM, John Stultz wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> An otherwise correct cleanup patch from Dan Carpenter turned a broken
> failure handling from a feature patch by Hans Verkuil 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").
> 
> I've managed to piece together several partial problems, though
> I'm still struggling with the bigger picture:
> 
> adv7511_probe() registers a drm_bridge structure that was allocated
> with devm_kzalloc(). It calls adv7511_cec_init(), which fails for an
> unknown reason, which in turn triggers the registered structure to be
> removed.
> 
> Elsewhere, kirin_drm_platform_probe() gets called, which calls
> of_graph_get_remote_node(), and that returns NULL. Before Dan's
> patch we would go on with a NULL pointer here and register that,
> now kirin_drm_platform_probe() fails with -ENODEV.
> 
> In a third driver, dsi_parse_dt() calls drm_of_find_panel_or_bridge(),
> which after not finding a panel goes on to call of_drm_find_bridge(),
> and that crashes due to the earlier list corruption.
> 
> This addresses the first issue by making sure that adv7511_probe()
> does not completely fail when the adv7511_cec_init() function fails,
> and instead we just disable the CEC feature. This avoids having the
> driver entirely fail to load if just the CEC initialization fails.
> 
> 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: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Archit Taneja <architt@codeaurora.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: Arnd Bergmann <arnd@arndb.de>
> [jstultz: Reworked so when adv7511_cec_init() fails, we disable the feature instead
> of disabling the entire driver, which causes graphics to not funciton]
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> Just wanted to send out my rework of Arnd's patch here.
> Feedback would be welcome.
> 
> thanks
> -john
> 
>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 0e14f15..939c3b9 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1203,12 +1203,12 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>   
>   #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);
> +	ret = 1;
>   #endif
> +	if (ret)
> +		regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
> +			     ADV7511_CEC_CTRL_POWER_DOWN);

This would force CEC to be powered off even if adv7511_cec_init() returned 0, right?
We wouldn't want that if we want to use CEC on a platform that supports it.

Do we know why the call to adv7511_cec_init() is failing on the Hikey board? If it's
because there isn't a "cec" clock specified in DT, it's not really a fatal error, it
just means that the platform hasn't been set up to support CEC. In that case, we
should just power down the CEC block. So, if adv7511_cec_init() would return a
-ENOENT, which we could use as a hint to power down CEC. So, maybe something like this?:

#ifdef CONFIG_DRM_I2C_ADV7511_CEC
         ret = adv7511_cec_init(dev, adv7511, offset);
         if (ret && ret != -ENOENT)	
		goto err_unregister_cec;
#endif
	if (ret)
             regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
                         ADV7511_CEC_CTRL_POWER_DOWN);

Apart from this, we should also move adv7511_cec_init() up in the probe so that
it's called before the drm_bridge is registered.

Thanks,
Archit

>   
>   	return 0;
>   
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC][PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[John Stultz]

On Sun, Nov 26, 2017 at 4:56 AM, Archit Taneja <architt@codeaurora.org> wrote:
>
> On 11/17/2017 04:29 AM, John Stultz wrote:
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index 0e14f15..939c3b9 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -1203,12 +1203,12 @@ static int adv7511_probe(struct i2c_client *i2c,
>> const struct i2c_device_id *id)
>>     #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);
>> +       ret = 1;
>>   #endif
>> +       if (ret)
>> +               regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL +
>> offset,
>> +                            ADV7511_CEC_CTRL_POWER_DOWN);
>
>
> This would force CEC to be powered off even if adv7511_cec_init() returned
> 0, right?

I don't think so. The initent was its only powered off if
adv7511_cec_init returns an error or if  CONFIG_DRM_I2C_ADV7511_CEC is
not set.

> Do we know why the call to adv7511_cec_init() is failing on the Hikey board?
> If it's
> because there isn't a "cec" clock specified in DT, it's not really a fatal
> error, it
> just means that the platform hasn't been set up to support CEC. In that

Yea. I believe this is the case w/ HiKey.  I don't have deeply
detailed docs on the board so I'm not sure if we will be able to
enable that or not (Xinliang/Guodong: Do you know if its possible?).
In the meantime though, we shouldn't regress.

> case, we
> should just power down the CEC block. So, if adv7511_cec_init() would return
> a
> -ENOENT, which we could use as a hint to power down CEC. So, maybe something
> like this?:
>
> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>         ret = adv7511_cec_init(dev, adv7511, offset);
>         if (ret && ret != -ENOENT)
>                 goto err_unregister_cec;
> #endif
>         if (ret)
>             regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>                         ADV7511_CEC_CTRL_POWER_DOWN);
>
> Apart from this, we should also move adv7511_cec_init() up in the probe so
> that
> it's called before the drm_bridge is registered.

Hans has since reworked the patch w/ a new version. You might want to
check his patches and see if they fit what your imagining.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Archit Taneja]

On 11/29/2017 03:02 AM, John Stultz wrote:
> On Sun, Nov 26, 2017 at 4:56 AM, Archit Taneja <architt@codeaurora.org> wrote:
>>
>> On 11/17/2017 04:29 AM, John Stultz wrote:
>>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>> index 0e14f15..939c3b9 100644
>>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>> @@ -1203,12 +1203,12 @@ static int adv7511_probe(struct i2c_client *i2c,
>>> const struct i2c_device_id *id)
>>>      #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);
>>> +       ret = 1;
>>>    #endif
>>> +       if (ret)
>>> +               regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL +
>>> offset,
>>> +                            ADV7511_CEC_CTRL_POWER_DOWN);
>>
>>
>> This would force CEC to be powered off even if adv7511_cec_init() returned
>> 0, right?
> 
> I don't think so. The initent was its only powered off if
> adv7511_cec_init returns an error or if  CONFIG_DRM_I2C_ADV7511_CEC is
> not set.
> 
>> Do we know why the call to adv7511_cec_init() is failing on the Hikey board?
>> If it's
>> because there isn't a "cec" clock specified in DT, it's not really a fatal
>> error, it
>> just means that the platform hasn't been set up to support CEC. In that
> 
> Yea. I believe this is the case w/ HiKey.  I don't have deeply
> detailed docs on the board so I'm not sure if we will be able to
> enable that or not (Xinliang/Guodong: Do you know if its possible?).
> In the meantime though, we shouldn't regress.
> 
>> case, we
>> should just power down the CEC block. So, if adv7511_cec_init() would return
>> a
>> -ENOENT, which we could use as a hint to power down CEC. So, maybe something
>> like this?:
>>
>> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>>          ret = adv7511_cec_init(dev, adv7511, offset);
>>          if (ret && ret != -ENOENT)
>>                  goto err_unregister_cec;
>> #endif
>>          if (ret)
>>              regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>>                          ADV7511_CEC_CTRL_POWER_DOWN);
>>
>> Apart from this, we should also move adv7511_cec_init() up in the probe so
>> that
>> it's called before the drm_bridge is registered.
> 
> Hans has since reworked the patch w/ a new version. You might want to
> check his patches and see if they fit what your imagining.

Yes, I saw Hans's patch after I wrote to you. That patch looks perfect. I'll
queue that to the drm-misc repo once it's rebased on 4.15-rc1.

Thanks,
Archit

> 
> thanks
> -john
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Arnd Bergmann]

On Wed, Nov 29, 2017 at 6:05 AM, Archit Taneja <architt@codeaurora.org> wrote:
> On 11/29/2017 03:02 AM, John Stultz wrote:
>> On Sun, Nov 26, 2017 at 4:56 AM, Archit Taneja <architt@codeaurora.org> wrote:
>>> Apart from this, we should also move adv7511_cec_init() up in the probe
>>> so that it's called before the drm_bridge is registered.
>>
>>
>> Hans has since reworked the patch w/ a new version. You might want to
>> check his patches and see if they fit what your imagining.
>
>
> Yes, I saw Hans's patch after I wrote to you. That patch looks perfect. I'll
> queue that to the drm-misc repo once it's rebased on 4.15-rc1.

It would be nice to get it merged into -rc2 if possible, as this currently
blocks all of the kselftest regression tracking that we're doing on Hikey,
since the arm64 defconfig fails to boot in both mainline and linux-next,
see https://bugs.linaro.org/show_bug.cgi?id=3345

     Arnd

Re: [PATCHv3] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Archit Taneja]

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

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel