lkml.org 
[lkml]   [2010]   [Sep]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [MeeGo-Dev][PATCH] Topcliff: Update PCH_I2C driver to 2.6.35
From
Date
On Fri, 2010-09-03 at 16:15 +0900, Masayuki Ohtak wrote:
[]
> +#define pch_dbg(adap, fmt, arg...) \
> + dev_dbg(adap->pch_adapter.dev.parent, "%s :"fmt, __func__, ##arg)
> +
> +#define pch_err(adap, fmt, arg...) \
> + dev_err(adap->pch_adapter.dev.parent, "%s :"fmt, __func__, ##arg)
> +
> +#define pch_pci_err(pdev, fmt, arg...) \
> + dev_err(&pdev->dev, "%s :"fmt, __func__, ##arg)
> +#define pch_pci_dbg(pdev, fmt, arg...) \
> + dev_dbg(&pdev->dev, "%s :"fmt, __func__, ##arg)

OK, but it seems careless because the two types
are not uniformly indented, there's a blank line
between pch_dbg and pch_err, and the two pch_pci_<level>
defines are in the reverse order without a blank line
between them.

I think it's better to use separate multiple strings
that are concatentated by the preprocessor like:
"%s :" fmt
not
"%s :"fmt

Almost all code in kernel uses "%s: " to format __func__.
Some use "%s(): ". I think "%s :" is unique.

The rest of the logging messages look good.

Some other comments:

> + if ((pch_wait_for_xfer_complete(adap) == 0) &&
> + (pch_getack(adap) == 0)) {

This would look better as:

if ((pch_wait_for_xfer_complete(adap) == 0) &&
(pch_getack(adap) == 0)) {
> + if ((pch_wait_for_xfer_complete(adap) == 0)
> + && (pch_getack(adap) == 0)) {

Here too.

> + for (i = 0; i < PCH_MAX_CHN; i++) {
> + while ((adap_info->pch_data[i].pch_xfer_in_progress)) {
> + /* Wait until all channel transfers are completed */
> + msleep(1);
> + }
> + /* Disable the i2c interrupts */
> + pch_disbl_int(&adap_info->pch_data[i]);
> + }

Would it be better to disable all possible interrupts first
or do you need to disable them in order?

Something like:

bool *disabled = kzalloc(PCH_MAX_CHN * sizeof(bool), GFP_KERNEL);
/*
* or a static with a memset, or check something
* like pch_is_int_enabled(&adap_info->pch_data[i])
* then remove the else because the kzalloc couldn't fail.
*/
if (disabled) {
bool alldone;
do {
alldone = true;
for (i = 0; i < PCH_MAX_CHN; i++) {
if (!adap_info->pch_data[i].pch_xfer_in_progress &&
!disabled[i])) {
pch_disbl_int(&adap_info->pch_data[i]);
disabled[i] = true;
} else
alldone = false;
}
if (!alldone) {
/* Wait until all channel transfers are completed */
msleep(1);
}
} while (!alldone);
kfree(disabled);
/* remove the else if there's a static etc */
} else {
for (i = 0; i < PCH_MAX_CHN; i++) {
while ((adap_info->pch_data[i].pch_xfer_in_progress)) {
/* Wait until all channel transfers are completed */
msleep(1);
}
/* Disable the i2c interrupts */
pch_disbl_int(&adap_info->pch_data[i]);
}
}
cheers, Joe



\
 
 \ /
  Last update: 2010-09-03 10:13    [from the cache]
©2003-2011 Jasper Spaans