| Message ID | 20210429080639.6379-1-arnaud.pouliquen@foss.st.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rpmsg: char: Remove useless includes | expand |
On Thu, Apr 29, 2021 at 10:06:39AM +0200, Arnaud Pouliquen wrote: > Remove includes that are not requested to build the module. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > --- > applied without issue on Bjorn next branch (dc0e14fa833b) > --- > drivers/rpmsg/rpmsg_char.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c > index 2bebc9b2d163..e4e54f515af6 100644 > --- a/drivers/rpmsg/rpmsg_char.c > +++ b/drivers/rpmsg/rpmsg_char.c > @@ -10,19 +10,10 @@ > * was based on TI & Google OMX rpmsg driver. > */ > #include <linux/cdev.h> > -#include <linux/device.h> This is where the declaration for struct device is along with other goodies like get/put_device(). > -#include <linux/fs.h> That is where struct file is declared. > -#include <linux/idr.h> This is where you get ida_simple_get() and ida_simple_remove() from. > #include <linux/kernel.h> > #include <linux/module.h> > -#include <linux/poll.h> This is where struct poll_table and poll_wait() comes from. > #include <linux/rpmsg.h> > #include <linux/skbuff.h> > -#include <linux/slab.h> This gives you kzalloc() and kfree(). > -#include <linux/uaccess.h> This gives you copy_from_user(). > -#include <uapi/linux/rpmsg.h> This gives you RPMSG_CREATE_EPT_IOCTL and RPMSG_DESTROY_EPT_IOCTL. > - > -#include "rpmsg_internal.h" That one I agree with. Thanks, Mathieu > > #define RPMSG_DEV_MAX (MINORMASK + 1) > > -- > 2.17.1 >
Hello Mathieu, On 5/3/21 7:42 PM, Mathieu Poirier wrote: > On Thu, Apr 29, 2021 at 10:06:39AM +0200, Arnaud Pouliquen wrote: >> Remove includes that are not requested to build the module. >> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> >> --- >> applied without issue on Bjorn next branch (dc0e14fa833b) >> --- >> drivers/rpmsg/rpmsg_char.c | 9 --------- >> 1 file changed, 9 deletions(-) >> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c >> index 2bebc9b2d163..e4e54f515af6 100644 >> --- a/drivers/rpmsg/rpmsg_char.c >> +++ b/drivers/rpmsg/rpmsg_char.c >> @@ -10,19 +10,10 @@ >> * was based on TI & Google OMX rpmsg driver. >> */ >> #include <linux/cdev.h> >> -#include <linux/device.h> > > This is where the declaration for struct device is along with other goodies like > get/put_device(). > >> -#include <linux/fs.h> > > That is where struct file is declared. > >> -#include <linux/idr.h> > > This is where you get ida_simple_get() and ida_simple_remove() from. > >> #include <linux/kernel.h> >> #include <linux/module.h> >> -#include <linux/poll.h> > > This is where struct poll_table and poll_wait() comes from. > >> #include <linux/rpmsg.h> >> #include <linux/skbuff.h> >> -#include <linux/slab.h> > > This gives you kzalloc() and kfree(). > >> -#include <linux/uaccess.h> > > This gives you copy_from_user(). > >> -#include <uapi/linux/rpmsg.h> > > This gives you RPMSG_CREATE_EPT_IOCTL and RPMSG_DESTROY_EPT_IOCTL. > >> - >> -#include "rpmsg_internal.h" > > That one I agree with. I started by this one and then I got carried away tested the other include... You are right, I just don't follow her the first rule of the "submit checklist" "If you use a facility then #include the file that defines/declares that facility. Don’t depend on other header files pulling in ones that you use." That said I just have a doubt for uapi/linux/rpmsg.h that will be include by rpmsg.h[2], as these includes are part of the rpmsg framework API, should we keep both, considering the rule as strict? [1] https://www.kernel.org/doc/html/latest/process/submit-checklist.html [2] https://patchwork.kernel.org/project/linux-remoteproc/patch/20210311140413.31725-3-arnaud.pouliquen@foss.st.com/ Thanks, Arnaud > > Thanks, > Mathieu > >> >> #define RPMSG_DEV_MAX (MINORMASK + 1) >> >> -- >> 2.17.1 >>
Hi Arnaud, [...] > > I started by this one and then I got carried away tested the other include... > You are right, I just don't follow her the first rule of the "submit checklist" > > "If you use a facility then #include the file that defines/declares that > facility. Don’t depend on other header files pulling in ones that you use." > > That said I just have a doubt for uapi/linux/rpmsg.h that will be include > by rpmsg.h[2], as these includes are part of the rpmsg framework API, should we > keep both, considering the rule as strict? I red the last paragraph several times I can't understand what you are trying to convey. Please rephrase, provide more context or detail exactly where you think we have a problem. Thanks, Mathieu > > [1] https://www.kernel.org/doc/html/latest/process/submit-checklist.html > [2] > https://patchwork.kernel.org/project/linux-remoteproc/patch/20210311140413.31725-3-arnaud.pouliquen@foss.st.com/ > > Thanks, > Arnaud > > > > > Thanks, > > Mathieu > > > >> > >> #define RPMSG_DEV_MAX (MINORMASK + 1) > >> > >> -- > >> 2.17.1 > >>
On 5/4/21 7:05 PM, Mathieu Poirier wrote: > Hi Arnaud, > > [...] > >> >> I started by this one and then I got carried away tested the other include... >> You are right, I just don't follow her the first rule of the "submit checklist" >> >> "If you use a facility then #include the file that defines/declares that >> facility. Don’t depend on other header files pulling in ones that you use." >> >> That said I just have a doubt for uapi/linux/rpmsg.h that will be include >> by rpmsg.h[2], as these includes are part of the rpmsg framework API, should we >> keep both, considering the rule as strict? > > I red the last paragraph several times I can't understand what you are > trying to convey. Please rephrase, provide more context or detail exactly where > you think we have a problem. There is no problem, just a question before sending an update. As you mention the #include "rpmsg_internal.h" line can be removed, I plan to send a patch V2 for this. That's said before sending a new version I would like to propose to also remove the #include <uapi/linux/rpmsg.h> line. The rational to remove it is that include/rpmsg.h would already include <uapi/linux/rpmsg.h> in 5.13 [2]. And looking at some frameworks (e.g I2C, TTY) the drivers seem to include only the include/xxx.h and not the uapi/linux/xxx.h in such case. So my question is should I remove #include <uapi/linux/rpmsg.h> line? Or do you prefer that i keep it? Hope it is more clear... else please just forget my proposal, I wouldn't want you to waste too much time for a point of detail. Thanks, Arnaud > > Thanks, > Mathieu > > >> >> [1] https://www.kernel.org/doc/html/latest/process/submit-checklist.html >> [2] >> https://patchwork.kernel.org/project/linux-remoteproc/patch/20210311140413.31725-3-arnaud.pouliquen@foss.st.com/ >> >> Thanks, >> Arnaud >> >>> >>> Thanks, >>> Mathieu >>> >>>> >>>> #define RPMSG_DEV_MAX (MINORMASK + 1) >>>> >>>> -- >>>> 2.17.1 >>>>
On Tue, May 04, 2021 at 08:20:25PM +0200, Arnaud POULIQUEN wrote: > > > On 5/4/21 7:05 PM, Mathieu Poirier wrote: > > Hi Arnaud, > > > > [...] > > > >> > >> I started by this one and then I got carried away tested the other include... > >> You are right, I just don't follow her the first rule of the "submit checklist" > >> > >> "If you use a facility then #include the file that defines/declares that > >> facility. Don’t depend on other header files pulling in ones that you use." > >> > >> That said I just have a doubt for uapi/linux/rpmsg.h that will be include > >> by rpmsg.h[2], as these includes are part of the rpmsg framework API, should we > >> keep both, considering the rule as strict? > > > > I red the last paragraph several times I can't understand what you are > > trying to convey. Please rephrase, provide more context or detail exactly where > > you think we have a problem. > > There is no problem, just a question before sending an update. > > As you mention the #include "rpmsg_internal.h" line can be removed, I plan to > send a patch V2 for this. > > That's said before sending a new version I would like to propose to also remove > the #include <uapi/linux/rpmsg.h> line. > > The rational to remove it is that include/rpmsg.h would already include > <uapi/linux/rpmsg.h> in 5.13 [2]. And looking at some frameworks (e.g I2C, TTY) > the drivers seem to include only the include/xxx.h and not the uapi/linux/xxx.h > in such case. > > So my question is should I remove #include <uapi/linux/rpmsg.h> line? Or do > you prefer that i keep it? Thanks for the clarifications, this is much much better. Less changes is always preferred, so unless there is a clear guideline or a good reason to make a change I would prefer to keep things the way they are. > > Hope it is more clear... else please just forget my proposal, I wouldn't want > you to waste too much time for a point of detail. > > Thanks, > Arnaud > > > > > Thanks, > > Mathieu > > > > > >> > >> [1] https://www.kernel.org/doc/html/latest/process/submit-checklist.html > >> [2] > >> https://patchwork.kernel.org/project/linux-remoteproc/patch/20210311140413.31725-3-arnaud.pouliquen@foss.st.com/ > >> > >> Thanks, > >> Arnaud > >> > >>> > >>> Thanks, > >>> Mathieu > >>> > >>>> > >>>> #define RPMSG_DEV_MAX (MINORMASK + 1) > >>>> > >>>> -- > >>>> 2.17.1 > >>>>
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c index 2bebc9b2d163..e4e54f515af6 100644 --- a/drivers/rpmsg/rpmsg_char.c +++ b/drivers/rpmsg/rpmsg_char.c @@ -10,19 +10,10 @@ * was based on TI & Google OMX rpmsg driver. */ #include <linux/cdev.h> -#include <linux/device.h> -#include <linux/fs.h> -#include <linux/idr.h> #include <linux/kernel.h> #include <linux/module.h> -#include <linux/poll.h> #include <linux/rpmsg.h> #include <linux/skbuff.h> -#include <linux/slab.h> -#include <linux/uaccess.h> -#include <uapi/linux/rpmsg.h> - -#include "rpmsg_internal.h" #define RPMSG_DEV_MAX (MINORMASK + 1)
Remove includes that are not requested to build the module. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> --- applied without issue on Bjorn next branch (dc0e14fa833b) --- drivers/rpmsg/rpmsg_char.c | 9 --------- 1 file changed, 9 deletions(-)