| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 29 Jan 2016 13:50:00 +0800
From: Peter Hung <hpeter@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
linus.walleij@...aro.org, gnurou@...il.com,
gregkh@...uxfoundation.org, paul.gortmaker@...driver.com,
lee.jones@...aro.org, jslaby@...e.com, peter_hong@...tek.com.tw
Cc: heikki.krogerus@...ux.intel.com, peter@...leysoftware.com,
soeren.grunewald@...y.de, udknight@...il.com,
adam.lee@...onical.com, arnd@...db.de, manabian@...il.com,
scottwood@...escale.com, yamada.masahiro@...ionext.com,
paul.burton@...tec.com, mans@...sr.com, matthias.bgg@...il.com,
ralf@...ux-mips.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, linux-serial@...r.kernel.org,
tom_tsai@...tek.com.tw, Peter Hung <hpeter+linux_kernel@...il.com>
Subject: Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512
PCIE-to-UART/GPIO core support
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
Powered by blists - more mailing lists