lkml.org 
[lkml]   [2019]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2019-02-01 15:45    [W:0.171 / U:0.384 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site