linux-kernel - Re: [PATCH v4] mfd: tqmx86: IO controller with i2c, wachdog and gpio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190201144410.GG30908@lunn.ch>
Date:   Fri, 1 Feb 2019 15:44:10 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     linux-kernel@...r.kernel.org,
        Emeric Dupont <emeric.dupont@....aero>
Subject: Re: [PATCH v4] mfd: tqmx86: IO controller with i2c, wachdog and gpio

> > +config MFD_TQMX86
> > +       tristate "TQ-Systems IO controller TQMX86"
> > +       select MFD_CORE
> > +       help
> > +	  Say yes here to enable support for various functions of the
> > +	  TQ-Systems IO controller and watchdog device, found on their
> > +	  ComExpress CPU modules.

Hi Lee
 
> The help should be indented by two spaces.

It is. If you look carefully, there is "<TAB>  ".  Maybe what you
actually want is the <TAB> replaced by spaces?

> > +/**
> > + * struct tqmx86_device_data - Internal representation of the PLD device
> 
> This is wrong.

Could you be a bit more specific please. 

> > + * @io_base:		Pointer to the IO memory
> > + * @pld_clock_rate:	PLD clock frequency
> > + * @dev:		Pointer to kernel device structure
> 
> > + * @i2c_type:		Hard of soft I2C hardware macro
> > + */
> > +struct tqmx86_device_ddata {
> 
> > +	void __iomem	*io_base;
> > +	u32		pld_clock_rate;
> > +	u32		i2c_type;
> > +};
> > +
> > +/**
> > + * struct tqmx86_platform_data - PLD hardware configuration structure
> > + * @ioresource:		IO addresses of the PLD
> > + */
> > +struct tqmx86_platform_ddata {
> 
> ddata (device data) and pdata (platform data) are different.
> 
> For platform data, it should be "*_platform_data" or "*_pdata".

It would of been useful if you had said this in the first review,
rather than s/data/ddata/, which is rather ambiguous.

> 
> > +	struct resource	*ioresource;
> > +};
> > +
> > +static uint gpio_irq;
> > +module_param(gpio_irq, uint, 0);
> > +MODULE_PARM_DESC(gpio_irq, "GPIO IRQ number (7, 9, 12)");
> 
> What is the purpose of providing the IRQ number via a module param?
> 
> These seems like a very bad idea.

I explained this in my reply to your review comments for version 2.

> Can this driver be built-in?

Yes it can. But you can pass module parameters on the command line, so
that is not an issue. This is something i actually use.

> 
> > +static const u8 i2c_irq_ctl[] = {
> > +	[7] = 1,
> > +	[9] = 2,
> > +	[12] = 3
> > +};
> 
> Rather than wasting memory, you could do:
> 
> static const u8 i2c_irq_ctl[] = { 7, 9, 12 };
> 
> You'll then have to loop over it once to get the index.
> 
> It also does not need to be global.

It is not global. It has the static keyword. Or are you meaning
something else?

> > +static const struct tq_board_info {
> > +	u8 board_id;
> > +	char *name;
> > +	u32 pld_clock_rate;
> > +} tq_board_info[] = {
> > +	{ 0, "", 0 },
> > +	{ 1, "TQMxE38M", 33000 },
> > +	{ 2, "TQMx50UC", 24000 },
> > +	{ 3, "TQMxE38C", 33000 },
> > +	{ 4, "TQMx60EB", 24000 },
> > +	{ 5, "TQMxE39M", 25000 },
> > +	{ 6, "TQMxE39C", 25000 },
> > +	{ 7, "TQMxE39x", 25000 },
> > +	{ 8, "TQMx70EB", 24000 },
> > +	{ 9, "TQMx80UC", 24000 },
> > +	{10, "TQMx90UC", 24000 }
> > +};

> There is not much point having a numbered struct attribute which
> directly matches the index.  See below for a better use of this -
> saves some CPU cycles too.

You comment for v2 was, what happens if the next board_id is 0xFC.  So
i changed the code to allow for this. Are you now saying you have
changed your mind, 0xFC cannot be the next board ID, there is no need
to have the numbers?

   Andrew