Hi Andy,

Andy Shevchenko 於 2016/1/29 下午 09:41 寫道:

On Fri, 2016-01-29 at 13:50 +0800, Peter Hung wrote:

quoted

Andy Shevchenko 於 2016/1/28 下午 07:55 寫道:

quoted

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.

quoted

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.

quoted

  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.

quoted

quoted

quoted

+	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

quoted

quoted

quoted

+	if (priv) {
+		/* Reinit from resume(), read the previous value
from priv */

quoted

+		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

quoted

quoted

quoted

+
+		pci_write_config_byte(dev, config_base + 0x06,
dev-

quoted

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.

quoted

+		if (status) {

quoted

quoted

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

quoted

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.

quoted

+		f81504_gpio_cell.pdata_size = sizeof(i);

quoted

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.

quoted

+	pci_read_config_byte(dev, F81504_GPIO_IO_MSB_REG, &tmp);

quoted

quoted

+	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

quoted

quoted

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.

quoted

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html