Messages in this thread Patch in this message | | | Subject | Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support | From | Peter Hung <> | Date | Fri, 29 Jan 2016 13:50:00 +0800 |
| |
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
| |