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

[Peter Hung <hpeter@gmail.com>]

Hi Andy,

Andy Shevchenko 於 2016/1/28 下午 07:55 寫道:
>> +        default y
>
> I'm not sure we have to have this always y. Perhaps
> default SERIAL_8250_PCI

Your comment is right, this device major function is serial port.
GPIO maybe not enabled by H/W manufacturer.

I'll set it default with SERIAL_8250_PCI.

>> +obj-$(CONFIG_MFD_FINTEK_F81504_CORE)	+= f81504-core.o
>
> I think '_' is better than '-'. What I saw and usually do is '_' for
> regular source modules and '-' for the resulting objects when they have
> more than one file.

I used f81504_core.c originally, but I found most of files are named
xxx-ooo.c when I try to modify makefile. Should I change it to
f81504_core.c ??


>> +#define F81504_DEV_DESC		"Fintek F81504/508/512 PCIE-
>> to-UART core"
>
> Do you need this definition? Is it used more than once?

ok, I'll direct use the string without define.


>> +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. In this case, should I use u8 to replace
i & j ? It should be less than 12 & 6.

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

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

The f81504_port_init() will be called probe()/resume().

We'll initialize gpio_en = (f0h | ~f3h) and save it in private data
when probe(), reload gpio_en from private data when resume().

The F81504/508/512 can be used EEPROM to override F0h/F3h on power
on. Some motherboard designer will put the IC on board and want to cost
down EEPROM. They will setting F0/F3h with ASL code, but without EEPROM
, the IC back from resume() will get F0/F3h with 0x00, so we should
save gpio_en when probe(), and restore it when resume().

>> +
>> +		/* re-save GPIO IO addr for called by resume() */
>
> re-save -> Re-save
> addr -> address

ok

>> +		priv->gpio_ioaddr = gpio_addr;
>> +	} else {
>> +		/* Driver first init */
>> +		pci_read_config_byte(dev, F81504_GPIO_ENABLE_REG,
>> &f0h_data);
>> +		pci_read_config_byte(dev, F81504_GPIO_MODE_REG,
>> &f3h_data);
>> +
>> +		/* find the max set of GPIOs */
>
> Use one pattern for all comments, like "Capital letter first, and full
> words avoiding abbreviations".

ok

>> +	case FINTEK_F81504: /* 4 ports */
>> +		/* F81504 max 2 sets of GPIO, others are max 6
>> sets*/
>
> Space before * is needed.

ok

>> +	for (i = 0; i < max_port; ++i) {
>
> i++, since it's more usual pattern.

ok

>> +		/* find every port to check is multi-function port?
>> */
>> +		for (j = 0; j < ARRAY_SIZE(fintek_gpio_mapping);
>> ++j) {
>> +			if (fintek_gpio_mapping[j] != i || !(gpio_en
>> & BIT(j)))
>> +				continue;
>> +
>> +			/*
>> +			 * This port is multi-function and enabled
>> as gpio
>> +			 * mode. So we'll not configure it as serial
>> port.
>> +			 */
>> +			is_gpio = true;
>> +			break;
>> +		}
>
> Looks like a separate function.
>
> func()
> {
>   for () {
>     if ()
>      return X;
>   }
>   return Y;
> }

ok.


>> +		/* Select 128-byte FIFO and 8x FIFO threshold */
>> +		pci_write_config_byte(dev, config_base + 0x01,
>> 0x33);
>> +
>> +		/* LSB UART */
>> +		pci_write_config_byte(dev, config_base + 0x04,
>> +				(u8)(iobase & 0xff));
>
> Redundant explicit casting.

ok

>> +
>> +		/* MSB UART */
>> +		pci_write_config_byte(dev, config_base + 0x05,
>> +				(u8)((iobase & 0xff00) >> 8));
>
> Ditto.
>
> And similar question, can it be written as a word?

Here can be written with a word, I'll rewrite it.

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

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


>> +		f81504_serial_cell.pdata_size = sizeof(i);
>
> This is static, can be part of definition.

ok

>> +		f81504_serial_cell.platform_data = &i;
>> +
>> +		status = mfd_add_devices(&dev->dev,
>> PLATFORM_DEVID_AUTO,
>> +					&f81504_serial_cell, 1,
>> NULL, dev->irq,
>> +					NULL);
>> +		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.


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


>> +static int f81504_probe(struct pci_dev *dev, const struct
>> pci_device_id
>> +			*dev_id)
>
> Usually drivers are using pdev, id pair in parameters. Shorter; known
> pattern.

ok.

>> +{
>> +	int status;
>> +	u8 tmp;
>> +	struct f81504_pci_private *priv;
>> +
>> +	status = pci_enable_device(dev);
>
> Please, pcim_, see comment below.
>> +	if (status)
>> +		return status;
>> +
>> +	/* Init PCI Configuration Space */
>> +	status = f81504_port_init(dev);
>> +	if (status)
>
> Device left enabled if not managed.
>> +		return status;
>> +
>> +	priv = devm_kzalloc(&dev->dev, sizeof(struct
>> f81504_pci_private),
>> +				GFP_KERNEL);
>> +	if (!priv)
>
> Ditto.

ok, I'll rewrite it with managed type.

>> +		return -ENOMEM;
>> +
>> +	/* Save the GPIO_ENABLE_REG after f81504_port_init() for
>> future use */
>> +	pci_read_config_byte(dev, F81504_GPIO_ENABLE_REG, &priv-
>>> gpio_en);
>> +
>> +	/* Save GPIO IO Addr to private data */
>>
>
>> +	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.


>> +	mfd_remove_devices(&dev->dev);
>
> mfd_add_devices takes care.
>
>> +	pci_disable_device(dev);
>
> pcim_ takes case.

ok

>> +	return status;
>> +}
>> +
>> +static void f81504_remove(struct pci_dev *dev)
>> +{
>
>> +	mfd_remove_devices(&dev->dev);
>> +	pci_disable_device(dev);
>
> Ditto.

ok


>> +static int f81504_resume(struct device *dev)
>> +{
>> +	int status;
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	status = pci_enable_device(pdev);
>> +	if (status)
>> +		return status;
>
> It's done by PCI core I believe.

ok

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

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?

>> +static struct pci_driver f81504_driver = {
>> +	.name = F81504_DRIVER_NAME,
>> +	.probe = f81504_probe,
>> +	.remove = f81504_remove,
>> +	.driver		= {
>> +		.pm	= &f81504_pm_ops,
>
>
>> +		.owner	= THIS_MODULE,
>
> kbuild  already complained about.

ok

>> diff --git a/include/linux/mfd/f81504.h b/include/linux/mfd/f81504.h
>> new file mode 100644
>> index 0000000..13bd0ae
>> --- /dev/null
>> +++ b/include/linux/mfd/f81504.h
>> @@ -0,0 +1,52 @@
>> +#ifndef __F81504_H__
>
> __MFD_F…

ok

Thanks for your advices
-- 
With Best Regards,
Peter Hung