[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190201150549.GA4973@dell>
Date: Fri, 1 Feb 2019 15:05:49 +0000
From: Lee Jones <lee.jones@...aro.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: linux-kernel@...r.kernel.org,
Emeric Dupont <emeric.dupont@....aero>
Subject: Re: [PATCH v4] mfd: tqmx86: IO controller with i2c, wachdog and gpio
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
Powered by blists - more mailing lists