Messages in this thread | | | Date | Fri, 5 Apr 2024 10:46:58 +0300 | Subject | Re: [PATCH v2 5/6] staging: pi433: Remove duplicated code using the "goto" error recovery scheme. | From | Shahar Avidar <> |
| |
On 02/04/2024 19:46, Dan Carpenter wrote: > On Tue, Apr 02, 2024 at 02:14:13PM +0300, Shahar Avidar wrote: >> pi433_init had "unregister_chrdev" called twice. >> Remove it using goto statements. >> >> Signed-off-by: Shahar Avidar <ikobh7@gmail.com> > > Your commit message needs to mention all the other changes you made, > especially the bug fix. I feel like you're doing too many changes > really. Just use the gotos to clean up but don't re-order the calls. Thanks again for your inputs. I've reverted some of the changes & split the 5th patch.
> > You need to add a Fixes tag. > I've added the "Fixes" tag, so as the "Reported-by" tag, naming you.
>> --- >> v2->v1: >> Followed by Dan Carpenter's <dan.carpenter@linaro.org> comments: >> - Remove empty "fail" goto tag. >> - Reorder pi433 init & exit calls so they have reverse order. > > Keep the init calls the same. Re-order the exit calls. It's a bit of > a gray area if you can re-order the exit calls in this same patch. Do > it in a separate patch just to be safe. > >> - Add "unreg_spi_drv" goto tag. >> - Check "debugfs_create_dir" return value. > > No. This is wrong. Debugfs function are not supposed to be checked. > https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked > Great blog post again. I'm learning.
>> - Update "if" statments for consistency. > > I like this cleanup, but it's not worth the effort. Just leave it > as-is unless you really really need to change it. Do it in a separate > patch. Reverted.
> > What qualifies as One Thing Per Patch is a bit of gray area. Greg tends > to want things split up into tiny patches. You have to know your > maintainer. > > regards, > dan carpenter > -- Regards,
Shahar
| |