Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support
From: Peter Hung
Date: Fri Jan 29 2016 - 00:50:11 EST
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