RE: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021

["Tony Huang 黃懷厚" <tony.huang@sunplus.com>]

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