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

[Lee Jones <lee.jones@linaro.org>]

On Fri, 01 Feb 2019, Andrew Lunn wrote:

> > > +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?

As I see what you've done.

You have used 8 spaces instead of tabs for the text above.

The help is correct, the bit above it is not.

> > > +/**
> > > + * struct tqmx86_device_data - Internal representation of the PLD device
> > 
> > This is wrong.
> 
> Could you be a bit more specific please. 

tqmx86_device_data != tqmx86_device_ddata

> > > + * @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.

How is that ambiguous?  I guess it would be confusing if you didn't
know the syntax, in which case you should have asked.

s/this/that/ simply means, replace 'this' with 'that'.

> > 
> > > +	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.

What is connected to these IRQs?

> > > +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?

It's a globally available struct.  You can put it into .probe().

> > > +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

That was very forward thinking of me. :)

> 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?

Okay, I just took a peek.  Looks like you misunderstood what I said.

Create a look-up function containing a switch() statement instead.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog