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.
> +/* ---------------------------------------------------------------------------------------------- */
> +resource_size_t SP_IOP_RESERVE_BASE;
> +resource_size_t SP_IOP_RESERVE_SIZE;
> +/* ---------------------------------------------------------------------------------------------- */
Again, static.
And why the odd comment lines?
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? :)
> +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.
> +
> +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.
thanks,
greg k-h
