Re: [PATCH] Input: iqs62x-keys - Suppress duplicated error message in .remove()

[Jeff LaBundy <jeff@labundy.com>]

Hi Uwe,

On Sat, Mar 18, 2023 at 11:51:10PM +0100, Uwe Kleine-König wrote:
> If a platform driver's remove callback returns non-zero the driver core
> emits an error message. In such a case however iqs62x_keys_remove()
> already issued a (better) message. So return zero to suppress the
> generic message.
> 
> This patch has no other side effects as platform_remove() ignores the
> return value of .remove() after the warning.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I was traveling all last week, and therefore unable to voice my opposition
in time. However, I figured I would still provide my feedback in case this
change may be proposed for other cases.

> ---
>  drivers/input/keyboard/iqs62x-keys.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/keyboard/iqs62x-keys.c b/drivers/input/keyboard/iqs62x-keys.c
> index db793a550c25..02ceebad7bda 100644
> --- a/drivers/input/keyboard/iqs62x-keys.c
> +++ b/drivers/input/keyboard/iqs62x-keys.c
> @@ -320,7 +320,7 @@ static int iqs62x_keys_remove(struct platform_device *pdev)
>  	if (ret)
>  		dev_err(&pdev->dev, "Failed to unregister notifier: %d\n", ret);
>  
> -	return ret;
> +	return 0;

In my opinion, we should never silence a function's return value, especially
in service of what is ultimately innocuous and cosmetic in nature. While this
specific example is harmless today, the caller can change and hence should be
the only instance who decides whether the return value is important.

If having both fine and subsequently coarse print statements is unacceptable,
I would have preferred to drop this driver's print statement and continue to
return ret. Or at the very least, include a comment as to why we deliberately
ignore the return value.

However, it's quite common for drivers to print a detailed message from probe
followed by the core printing "failed to probe," so I don't see why the remove
case cannot be the same. At any rate, this is just my $.02.

>  }
>  
>  static struct platform_driver iqs62x_keys_platform_driver = {
> 
> base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
> -- 
> 2.39.2
> 

Kind regards,
Jeff LaBundy