Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support

[Peter Hung]

Hi Andy,

Andy Shevchenko æ 2016/1/29 äå 09:41 åé:
> On Fri, 2016-01-29 at 13:50 +0800, Peter Hung wrote:
> > Andy Shevchenko æ 2016/1/28 äå 07:55 åé:
> > > I would suggest to rearrange definition block here (and in the rest
> > > of
> > > the functions in entire series) to somehow follow the following
> > > pattern
> > >
> > > 1) assignments go first, especially container_of() based ones;
> > > 2) group variables by meaning and put longer lines upper;
> > > 3) return value variable, if exists, goes last.
> > >
> > > Besides that choose types carefully. If there is known limit for
> > > counters, no need to define wider type than needed. Use proper
> > > types
> > > for corresponding values.
> >
> > I'll try to rearrange the definition blocks.
>
>
> > For the counter issue, some senior recommend me to change count from
> > int
> > to size_t for performance.
>
> Wow, how come? On which arch?
>
> Also mark this as (1) to refer below.
>
> >   In this case, should I use u8 to replace
> > i & j ? It should be less than 12 & 6.
>
> At least you tell compiler that it may use any suitable type. In any
> case the last decision is by compiler if you don't do any tricks.
>
> So, I suggest to use non-fixed width type like (unsigned) int and leave
> everything else on compiler.

Sorry for my misunderstanding, not for performance.
The senior just recommend me to replace all size/count variables
to size_t.

https://lkml.org/lkml/2016/1/3/193

size_t in x86 is unsigned int, x86_64 is unsigned long, It maybe
good for prefetch or match register size I guess.

I'll make the size_t of series patches to unsigned int.

> > > > +	pci_write_config_byte(dev, F81504_GPIO_IO_LSB_REG,
> > > > gpio_addr
> > > > & 0xff);
> > > > +	pci_write_config_byte(dev, F81504_GPIO_IO_MSB_REG,
> > > > (gpio_addr >> 8) &
> > > > +			0xff);
> > >
> > > Could it be written at once as a word?
> >
> > I tested with writing a word to "F81504_GPIO_IO_LSB_REG", but
> > it'll failed, so I'll remain it.
>
> Please, add a comment line above.

ok


> > > > +	if (priv) {
> > > > +		/* Reinit from resume(), read the previous value
> > > > from priv */
> > >
> > > > +		gpio_en = priv->gpio_en;
> > >
> > > I would suggest to move this line down to follow same pattern in
> > > else
> > > branch.
>
> I'm talking here to make simple rearrangement like
>
> if () {
>   â
>   gpio_en = â
> } else {
> â
>   gpio_en = â
> }
>
> Thus the gpio_en assignment goes last in both branches.

ok


> > > > +
> > > > +		pci_write_config_byte(dev, config_base + 0x06,
> > > > dev-
> > > > > irq);
> > > > +
> > > > +		/*
> > > > +		 * Force init to RS232 / Share Mode, recovery
> > > > previous mode
> > > > +		 * will done in F81504 8250 platform driver
> > > > resume()
> > >
> > > Period at the end of sentences in multi-line comments.
> >
> > Sorry, what this mean for ?
> > I cant use multi-line comments in the end ??
>
> Sentences are started with capital letters and end with period '.'
> character, like this one.

ok.


> > +		if (status) {
> > > > +			dev_warn(&dev->dev, "%s: add device
> > > > failed:
> > > > %d\n",
> > > > +					__func__, status);
> > >
> > > Indent _ under &.
> >
> > Some senior recommend me to do at least 2-tabs to do indent when
> > code cross multi-line. So I'll use to 2-tabs from "dev" to do indent.
>
> > How should I do with indent ?? It seems no consensus, but Lindent
> > will do indent like your comments.
>
> I would suggest to use what is used in most recent drivers and modules.
> I don't remember that 2 tabs fact is somehow reflected in CodingStyle.
>
> Maybe your seniour was talkin about multi-line function definition?

ok, I'll indent multi-line statement as your comment and multi-line
function with 2 tabs.


> > +		f81504_gpio_cell.pdata_size = sizeof(i);
> > > Static. The problem here is badly chosen type of i. Like I
> > > mentioned at
> > > the top of reviewâ
> >
> > I'll try to rewrite it with pass a structure. It seems more make
> > sense.
>
> That's correct on one side, on the other I'm not sure you got my
> comment. size_t is arch-dependent type, sizeof() is not the same
> everywhere.

I'll change it to pass unsigned int.


> > +	pci_read_config_byte(dev, F81504_GPIO_IO_MSB_REG, &tmp);
> > > > +	priv->gpio_ioaddr = tmp << 8;
> > > > +	pci_read_config_byte(dev, F81504_GPIO_IO_LSB_REG, &tmp);
> > > > +	priv->gpio_ioaddr |= tmp;
> > >
> > > One read as word?
> >
> > It can't read as word. The "F81504_GPIO_IO_LSB_REG" is 0xf1, It
> > seems can't be read word/dword from a odd address.
> >
> > I'll remain it.
>
> Put comment about that.

ok


> > > So, if you have default y for this and 8250_pci, which one will be
> > > loaded?
> >
> > I had tested on x86, It'll handle by 8250_pci.c when this &
> > 8250_pci.c
> > all built-in mode.
>
> Yeah. here is the problem. When you introduce that you have to be sure
> that no-one will try to build kernel with both included right now.

Paul had recommend me to reduce the impact of code change. I'll remove
all F81504/508/512 code in 8250_pci.c until the series patches had
applied.

The device will handle with 8250_pci.c before applide patch no4, and
handle with f81504_code.c when applied patch no4. This maybe reduce the
bisect misleading.

> > BTW, due to the series patches across 3 subsystems MFD/GPIO/8250.
> > I make the series patches under MFD subsystem, but the buildbot
> > report git repository conflict with GPIO & 8250 (patch 2 & 3).
> >
> > Should I split the series patches into 3 independent patch and
> > based on their maintainer git repository?
>
> It should go via one subsystem, otherwise user may end up with non-
> working interim kernel version, i.e. when bisecting.
>
> In case if it based on some stuff which is not in upstream yet, you
> have to describe this carefully to maintainers that they may create
> immutable branches and cross-apply the stuff. But I suppose it's not
> your case and your series is quite straightforward.

I'll still try the series patch based on MFD subsystem.
Thanks for your advices.

--
With Best Regards,
Peter Hung