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

[Andy Shevchenko]

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

> Andy Shevchenko æ 2016/1/28 äå 07:55 åé:
> > > +obj-$(CONFIG_MFD_FINTEK_F81504_CORE)	+= f81504-core.o

> +static int f81504_port_init(struct pci_dev *dev)
> > > +{
> > > +	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.


> > > +
> > > +	/* Init GPIO IO Address */
> > > +	pci_read_config_dword(dev, 0x18, &gpio_addr);
> > > +	gpio_addr &= 0xffffffe0;
> > 
> > 
> > > +	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.

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

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


> > > +	for (i = 0; i < max_port; ++i) {
> > > +		/* Check UART is enabled */
> > > 
> > 
> > > +		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?
> > 
> > > +
> > > +		/* 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) {
> > > +			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);
> > 
> > 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);
> > > +	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.

>Â
> > > +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 <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy