Messages in this thread | | | From | Tony Huang 黃懷厚 <> | Subject | RE: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021 | Date | Mon, 6 Dec 2021 03:50:03 +0000 |
| |
Dear Greg KH:
> -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Friday, December 3, 2021 6:39 PM > To: Tony Huang <tonyhuang.sunplus@gmail.com> > Cc: derek.kiernan@xilinx.com; dragan.cvetic@xilinx.com; arnd@arndb.de; > linux-kernel@vger.kernel.org; robh+dt@kernel.org; devicetree@vger.kernel.org; > Wells Lu 呂芳騰 <wells.lu@sunplus.com>; Tony Huang 黃懷厚 > <tony.huang@sunplus.com> > Subject: Re: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021 > > On Fri, Dec 03, 2021 at 11:48:45AM +0800, Tony Huang wrote: > > +#define NORMAL_CODE_MAX_SIZE 0X1000 > > +#define STANDBY_CODE_MAX_SIZE 0x4000 > > +unsigned char iop_normal_code[NORMAL_CODE_MAX_SIZE]; > > +unsigned char iop_standby_code[STANDBY_CODE_MAX_SIZE]; > > Please make your local variables static so that yhou do not polute the kernel's > global symbol table. >
I will modify it.
> > +/* > > +--------------------------------------------------------------------- > > +------------------------- */ resource_size_t SP_IOP_RESERVE_BASE; > > +resource_size_t SP_IOP_RESERVE_SIZE; > > +/* > > +--------------------------------------------------------------------- > > +------------------------- */ > > Again, static. > > And why the odd comment lines? >
I will remove it.
> And those are not good variable names. > > > +struct sp_iop { > > + struct miscdevice dev; // iop device > > + struct mutex write_lock; > > + void __iomem *iop_regs; > > + void __iomem *pmc_regs; > > + void __iomem *moon0_regs; > > + int irq; > > +}; > > > +/************************************************************** > *** > > + * G L O B A L D A T A > > + > **************************************************************** > **/ > > Global where? What about the ones above? :) >
I will remove it.
> > +static struct sp_iop *iop; > > Why do you think you only have one device in the system? Please do not use > a single variable like this. It is easy to make your driver handle an unlimited > number of devices just as easy as to handle 1 device. > Please do that instead and hang your device-specific data off of the correct > data structures that the driver core gives you. >
I will modify it.
> > + > > +void iop_normal_mode(void) > > Did you run sparse on this code? Please do so. > > Also, no need for a .h file for a driver that only has one .c file. >
I need to keep sunplus_iop.h. Other files will use sp_iop_platform_driver_poweroff(void) in poweroff flow.
> thanks, > > greg k-h
| |