From: "Masayuki Ohtake" <masa-korg@dsn.okisemi.com> To: "Joe Perches" <joe@perches.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, 3 Sep 2010 19:01:52 +0900 [thread overview] Message-ID: <000801cb4b4f$0bc76e70$66f8800a@maildom.okisemi.com> (raw) In-Reply-To: 1283501437.1797.441.camel@Joe-Laptop ----- Original Message ----- 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> Sent: Friday, September 03, 2010 5:10 PM Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_I2C driver to 2.6.35 > 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? * Your proposal If pch_disbl_int is executed firstly, queued data is destroyed. * Current spec If checking status firstly, all data can be sent. Thus, I think current spec is better than yours. > > 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 > I will resubmit modified patch soon. Thanks, Ohtake(OKISemi)
WARNING: multiple messages have this Message-ID (diff)
From: "Masayuki Ohtake" <masa-korg-ECg8zkTtlr0C6LszWs/t0g@public.gmane.org> To: Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@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, 3 Sep 2010 19:01:52 +0900 [thread overview] Message-ID: <000801cb4b4f$0bc76e70$66f8800a@maildom.okisemi.com> (raw) In-Reply-To: 1283501437.1797.441.camel@Joe-Laptop ----- Original Message ----- 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> Sent: Friday, September 03, 2010 5:10 PM Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_I2C driver to 2.6.35 > 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? * Your proposal If pch_disbl_int is executed firstly, queued data is destroyed. * Current spec If checking status firstly, all data can be sent. Thus, I think current spec is better than yours. > > 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 > I will resubmit modified patch soon. Thanks, Ohtake(OKISemi)
next prev parent reply other threads:[~2010-09-03 10:02 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 2010-09-03 8:10 ` Joe Perches 2010-09-03 10:01 ` Masayuki Ohtake [this message] 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='000801cb4b4f$0bc76e70$66f8800a@maildom.okisemi.com' \ --to=masa-korg@dsn.okisemi.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=joe@perches.com \ --cc=khali@linux-fr.org \ --cc=linus.walleij@stericsson.com \ --cc=linux-i2c@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --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.