lkml.org 
[lkml]   [2021]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v2 2/2] misc: Add iop driver for Sunplus SP7021
Date
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
\
 
 \ /
  Last update: 2021-12-06 04:50    [W:0.202 / U:1.864 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site