Dear Greg KH:
> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Friday, December 3, 2021 6:39 PM
> To: Tony Huang <tonyhuang.sunplus@xxxxxxxxx>
> Cc: derek.kiernan@xxxxxxxxxx; dragan.cvetic@xxxxxxxxxx; arnd@xxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> Wells Lu 呂芳騰 <wells.lu@xxxxxxxxxxx>; Tony Huang 黃懷厚
> <tony.huang@xxxxxxxxxxx>
> 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
