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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
