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

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

quoted

quoted

+obj-$(CONFIG_MFD_FINTEK_F81504_CORE)	+= f81504-core.o

+static int f81504_port_init(struct pci_dev *dev)

quoted

quoted

+{
+	size_t i, j;
+	u32 max_port, iobase, gpio_addr;
+	u32 bar_data[3];
+	u16 tmp;
+	u8 config_base, gpio_en, f0h_data, f3h_data;
+	bool is_gpio;
+	struct f81504_pci_private *priv = pci_get_drvdata(dev);

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.

quoted

quoted

+
+	/* Init GPIO IO Address */
+	pci_read_config_dword(dev, 0x18, &gpio_addr);
+	gpio_addr &= 0xffffffe0;

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.

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.

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.

quoted

quoted

+	for (i = 0; i < max_port; ++i) {
+		/* Check UART is enabled */

quoted

+		pci_read_config_byte(dev, F81504_UART_START_ADDR
+ i
*
+				F81504_UART_OFFSET, &tmp);
+		if (!tmp)
+			continue;

So, this is the same as below, right?

quoted

+
+		/* Get UART IO Address */
+		pci_read_config_word(dev, F81504_UART_START_ADDR
+ i
*
+				F81504_UART_OFFSET + 4,
&iobase);

…why not to check here?

It's a bit difference, this section will check if UART enabled (tmp).
It'll generate platform device and get IO address when tmp != 0. If
tmp = 0, skip this idx, dont need to get current IO address and try
with next.

Yeah, I missed that the offset is different.

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

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?

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

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

quoted

quoted

+static const struct pci_device_id f81504_dev_table[] = {
+	/* Fintek PCI serial cards */
+	{PCI_DEVICE(FINTEK_VID, FINTEK_F81504), .driver_data =
4},
+	{PCI_DEVICE(FINTEK_VID, FINTEK_F81508), .driver_data =
8},
+	{PCI_DEVICE(FINTEK_VID, FINTEK_F81512), .driver_data =
12},
+	{}

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.

To avoid potential problem you may split the driver to main part and
Kconfig enabling part. Lee, what is your advice here?

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.


-- 
Andy Shevchenko [off-list ref]
Intel Finland Oy