From: Joe Perches <joe@perches.com> To: Masayuki Ohtak <masa-korg@dsn.okisemi.com> Cc: "Jean Delvare (PC drivers, core)" <khali@linux-fr.org>, "Ben Dooks (embedded platforms)" <ben-linux@fluff.org>, Crane Cai <crane.cai@amd.com>, Samuel Ortiz <sameo@linux.intel.com>, Linus Walleij <linus.walleij@stericsson.com>, Ralf Baechle <ralf@linux-mips.org>, srinidhi kasagar <srinidhi.kasagar@stericsson.com>, linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, yong.y.wang@intel.com, qi.wang@intel.com, andrew.chih.howe.khor@intel.com, arjan@linux.intel.com, Tomoya MORINAGA <morinaga526@dsn.okisemi.com>, Arnd Bergmann <arnd@arndb.de> Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_I2C driver to 2.6.35 Date: Fri, 03 Sep 2010 01:10:37 -0700 [thread overview] Message-ID: <1283501437.1797.441.camel@Joe-Laptop> (raw) In-Reply-To: <4C80A089.508@dsn.okisemi.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> To: Masayuki Ohtak <masa-korg-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org> Cc: "Jean Delvare (PC drivers, core)" <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>, "Ben Dooks (embedded platforms)" <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>, Crane Cai <crane.cai-5C7GfCeVMHo@public.gmane.org>, Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>, Linus Walleij <linus.walleij-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>, Ralf Baechle <ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org>, srinidhi kasagar <srinidhi.kasagar-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>, linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, yong.y.wang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, qi.wang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, andrew.chih.howe.khor-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, arjan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org, Tomoya MORINAGA <morinaga526-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org>, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_I2C driver to 2.6.35 Date: Fri, 03 Sep 2010 01:10:37 -0700 [thread overview] Message-ID: <1283501437.1797.441.camel@Joe-Laptop> (raw) In-Reply-To: <4C80A089.508-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org> 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
next prev parent reply other threads:[~2010-09-03 8:10 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-09-03 7:15 [MeeGo-Dev][PATCH] Topcliff: Update PCH_I2C driver to 2.6.35 Masayuki Ohtak 2010-09-03 7:15 ` Masayuki Ohtak 2010-09-03 8:10 ` Joe Perches [this message] 2010-09-03 8:10 ` Joe Perches 2010-09-03 10:01 ` Masayuki Ohtake 2010-09-03 10:01 ` Masayuki Ohtake -- strict thread matches above, loose matches on Subject: below -- 2010-09-06 1:31 Masayuki Ohtak 2010-09-06 1:31 ` Masayuki Ohtak 2010-09-06 1:10 Masayuki Ohtak 2010-09-06 1:10 ` Masayuki Ohtak 2010-09-03 10:19 Masayuki Ohtak 2010-09-03 10:19 ` Masayuki Ohtak 2010-09-03 12:36 ` Alan Cox 2010-09-03 12:36 ` Alan Cox 2010-09-03 12:26 ` Wolfram Sang 2010-09-03 12:26 ` Wolfram Sang 2010-09-06 0:44 ` Masayuki Ohtake 2010-09-06 0:44 ` Masayuki Ohtake 2010-09-06 0:43 ` Masayuki Ohtake 2010-09-06 0:43 ` Masayuki Ohtake 2010-09-07 23:55 ` Ben Dooks 2010-09-07 23:55 ` Ben Dooks 2010-09-08 12:06 ` Masayuki Ohtake 2010-09-08 12:06 ` Masayuki Ohtake 2010-09-03 3:19 Masayuki Ohtak 2010-09-03 3:19 ` Masayuki Ohtak 2010-09-03 4:38 ` Joe Perches 2010-09-03 4:38 ` Joe Perches 2010-09-03 8:39 ` Linus WALLEIJ [not found] ` <4C80692E.80004-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org> 2010-09-03 8:39 ` Linus WALLEIJ 2010-09-01 2:26 Masayuki Ohtak 2010-09-01 2:26 Masayuki Ohtak 2010-09-01 2:43 ` Joe Perches 2010-09-01 2:43 ` Joe Perches 2010-09-01 4:16 ` Masayuki Ohtake 2010-09-01 4:16 ` Masayuki Ohtake 2010-09-01 5:08 ` Masayuki Ohtake 2010-09-01 5:08 ` Masayuki Ohtake 2010-09-01 5:24 ` Joe Perches 2010-09-01 5:44 ` Masayuki Ohtake 2010-09-01 5:44 ` Masayuki Ohtake 2010-09-01 19:44 ` Linus Walleij 2010-09-01 19:44 ` Linus Walleij 2010-09-03 3:12 ` Masayuki Ohtake 2010-09-03 3:12 ` Masayuki Ohtake [not found] <4C5B9094.5090205@dsn.okisemi.com> [not found] ` <4C5B9229.1040403@linux.intel.com> 2010-09-01 1:50 ` Masayuki Ohtake [not found] ` <20100806142418.GA4921@suse.de> 2010-09-01 1:57 ` Masayuki Ohtake
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1283501437.1797.441.camel@Joe-Laptop \ --to=joe@perches.com \ --cc=andrew.chih.howe.khor@intel.com \ --cc=arjan@linux.intel.com \ --cc=arnd@arndb.de \ --cc=ben-linux@fluff.org \ --cc=crane.cai@amd.com \ --cc=khali@linux-fr.org \ --cc=linus.walleij@stericsson.com \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=masa-korg@dsn.okisemi.com \ --cc=morinaga526@dsn.okisemi.com \ --cc=qi.wang@intel.com \ --cc=ralf@linux-mips.org \ --cc=sameo@linux.intel.com \ --cc=srinidhi.kasagar@stericsson.com \ --cc=yong.y.wang@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.