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/29 下午 09:41 寫道:
> On Fri, 2016-01-29 at 13:50 +0800, Peter Hung wrote:
>> Andy Shevchenko 於 2016/1/28 下午 07:55 寫道:
>>> 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.

Sorry for my misunderstanding, not for performance.
The senior just recommend me to replace all size/count variables
to size_t.

https://lkml.org/lkml/2016/1/3/193

size_t in x86 is unsigned int, x86_64 is unsigned long, It maybe
good for prefetch or match register size I guess.

I'll make the size_t of series patches to unsigned int.

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

ok


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

ok


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

ok.


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

ok, I'll indent multi-line statement as your comment and multi-line
function with 2 tabs.


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

I'll change it to pass unsigned int.


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

ok


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

Paul had recommend me to reduce the impact of code change. I'll remove
all F81504/508/512 code in 8250_pci.c until the series patches had
applied.

The device will handle with 8250_pci.c before applide patch no4, and
handle with f81504_code.c when applied patch no4. This maybe reduce the
bisect misleading.

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

I'll still try the series patch based on MFD subsystem.
Thanks for your advices.

-- 
With Best Regards,
Peter Hung
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html