| Message ID | 20210324152058.69022-3-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] pwm: Drop unused error path from pwmchip_remove() | expand |
On Wed, Mar 24, 2021 at 04:20:58PM +0100, Uwe Kleine-König wrote: > pwmchip_remove() always returns 0. Don't use the value to make it > possible to eventually change the function to return void. This is a > good thing as pwmchip_remove() is usually called from a remove function > (mostly for platform devices) and their return value is ignored by the > device core anyhow. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pwm/pwm-imx27.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) It's true that there's not much we can do upon failure, so failing doesn't make much sense and therefore returning an error doesn't make sense. So how about we WARN on the -EBUSY case instead of returning an error? That should have an even higher impact than an error that's being silently ignored. Thierry
Hello Thierry, On Fri, Apr 09, 2021 at 02:00:20PM +0200, Thierry Reding wrote: > On Wed, Mar 24, 2021 at 04:20:58PM +0100, Uwe Kleine-König wrote: > > pwmchip_remove() always returns 0. Don't use the value to make it > > possible to eventually change the function to return void. This is a > > good thing as pwmchip_remove() is usually called from a remove function > > (mostly for platform devices) and their return value is ignored by the > > device core anyhow. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/pwm/pwm-imx27.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > It's true that there's not much we can do upon failure, so failing > doesn't make much sense and therefore returning an error doesn't make > sense. So how about we WARN on the -EBUSY case instead of returning an > error? That should have an even higher impact than an error that's being > silently ignored. After patch 1 in this series (or the reworked version from https://lore.kernel.org/r/20210423165902.2439564-1-u.kleine-koenig@pengutronix.de) pwmchip_remove() really always returns 0, so adding a WARN in the imx27 (or any other) driver doesn't make much sense. It might make sense to add such a WARN in pwmchip_remove(), is it that what you intended to suggest? Best regards Uwe
Hello Thierry, On Tue, May 25, 2021 at 09:59:05PM +0200, Uwe Kleine-König wrote: > On Fri, Apr 09, 2021 at 02:00:20PM +0200, Thierry Reding wrote: > > On Wed, Mar 24, 2021 at 04:20:58PM +0100, Uwe Kleine-König wrote: > > > pwmchip_remove() always returns 0. Don't use the value to make it > > > possible to eventually change the function to return void. This is a > > > good thing as pwmchip_remove() is usually called from a remove function > > > (mostly for platform devices) and their return value is ignored by the > > > device core anyhow. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > drivers/pwm/pwm-imx27.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > It's true that there's not much we can do upon failure, so failing > > doesn't make much sense and therefore returning an error doesn't make > > sense. So how about we WARN on the -EBUSY case instead of returning an > > error? That should have an even higher impact than an error that's being > > silently ignored. > > After patch 1 in this series (or the reworked version from > https://lore.kernel.org/r/20210423165902.2439564-1-u.kleine-koenig@pengutronix.de) > pwmchip_remove() really always returns 0, so adding a WARN in the imx27 > (or any other) driver doesn't make much sense. > > It might make sense to add such a WARN in pwmchip_remove(), is it that > what you intended to suggest? You never replied to this question. Given that today pwmchip_remove() indeed always returns 0, your concern here should not stop application of the change to the pwm-imx27 driver. Best regards Uwe
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index ba695115c160..9c5b336b00bc 100644 --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -354,7 +354,9 @@ static int pwm_imx27_remove(struct platform_device *pdev) imx = platform_get_drvdata(pdev); - return pwmchip_remove(&imx->chip); + pwmchip_remove(&imx->chip); + + return 0; } static struct platform_driver imx_pwm_driver = {
pwmchip_remove() always returns 0. Don't use the value to make it possible to eventually change the function to return void. This is a good thing as pwmchip_remove() is usually called from a remove function (mostly for platform devices) and their return value is ignored by the device core anyhow. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/pwm-imx27.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)