From: Prashant Malani <pmalani@chromium.org> To: "Mani, Rajmohan" <rajmohan.mani@intel.com> Cc: "Darren Hart" <dvhart@infradead.org>, "Andy Shevchenko" <andy@infradead.org>, "Mika Westerberg" <mika.westerberg@linux.intel.com>, "Dmitry Torokhov" <dmitry.torokhov@gmail.com>, "Lee Jones" <lee.jones@linaro.org>, "Ayman Bagabas" <ayman.bagabas@gmail.com>, "Masahiro Yamada" <masahiroy@kernel.org>, "Joseph, Jithu" <jithu.joseph@intel.com>, "Blaž Hrastnik" <blaz@mxxn.io>, "Srinivas Pandruvada" <srinivas.pandruvada@linux.intel.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "platform-driver-x86@vger.kernel.org" <platform-driver-x86@vger.kernel.org>, "Heikki Krogerus" <heikki.krogerus@linux.intel.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>, "bleung@chromium.org" <bleung@chromium.org> Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver Date: Mon, 24 Aug 2020 15:47:26 -0700 [thread overview] Message-ID: <20200824224726.GA48297@google.com> (raw) In-Reply-To: <DM6PR11MB3963121C820F570F845BBAEEF6560@DM6PR11MB3963.namprd11.prod.outlook.com> Hi Rajmohan, On Mon, Aug 24, 2020 at 10:19:27PM +0000, Mani, Rajmohan wrote: > Hi Prashant, > > Thanks for the quick review. > > > > + > > > + if (!iom || !iom->dev || !iom->regbar) > > > > Do we need to check for !iom->dev and !iom->regbar? > > It's a good practice to have sanity checks on pointer members dereferenced. > > So I can lose the check on iom->dev, but prefer to keep the check on regbar. > Let me know if you feel strongly about losing the check for regbar as well. Sounds good. > > > Is there a valid situation > > where iom != NULL but iom->dev and/or iom->regbar == NULL? > > Sounds like it shouldn't, but I may be missing something. > > > > I think I am being conservative here. > > > > + return -ENODEV; > > > + > > > + if (!status || (port > IOM_MAX_PORTS - 1)) > > > > I think parentheses around "port > IOM_MAX_PORT - 1" aren't required. > > Ack > > > > + return -EINVAL; > > > + > > > + reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN * > > port; > > > + > > > + *status = ioread32(reg); > > > > Perhaps just inline reg within the parentheses? > > Kept this way to increase readability. Let me know if you feel strongly towards > inline reg. I'd rather this be inlined, you save a couple lines from the variable declaration, and IMO we're not gaining much in terms of readability by declaring this separately. > > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(intel_iom_port_status); > > > + > > > +static int intel_iom_probe(struct platform_device *pdev) { > > > + void __iomem *addr; > > > + > > > + /* only one IOM device is supported */ > > > > Minor nit: s/only/Only > > And then I may need to end the comment with a period. > Let me know if you feel strongly. Yes, let's capitalize and add the period. Let's try to use the right punctuation where possible. Best regards, -Prashant
WARNING: multiple messages have this Message-ID (diff)
From: Prashant Malani <pmalani-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> To: "Mani, Rajmohan" <rajmohan.mani-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> Cc: "Darren Hart" <dvhart-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>, "Andy Shevchenko" <andy-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>, "Mika Westerberg" <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>, "Dmitry Torokhov" <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, "Lee Jones" <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>, "Ayman Bagabas" <ayman.bagabas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, "Masahiro Yamada" <masahiroy-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, "Joseph, Jithu" <jithu.joseph-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, "Blaž Hrastnik" <blaz-piQ53kQ5nys@public.gmane.org>, "Srinivas Pandruvada" <srinivas.pandruvada-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "platform-driver-x86-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <platform-driver-x86-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "Heikki Krogerus" <heikki.krogerus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>, "Greg Kroah-Hartman" <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>, "linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" <linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, "bleung-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org" <bleung@chromium> Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver Date: Mon, 24 Aug 2020 15:47:26 -0700 [thread overview] Message-ID: <20200824224726.GA48297@google.com> (raw) In-Reply-To: <DM6PR11MB3963121C820F570F845BBAEEF6560-10zMTfzfexfnaCbnFtwEg5PPoyLQLiKMvxpqHgZTriW3zl9H0oFU5g@public.gmane.org> Hi Rajmohan, On Mon, Aug 24, 2020 at 10:19:27PM +0000, Mani, Rajmohan wrote: > Hi Prashant, > > Thanks for the quick review. > > > > + > > > + if (!iom || !iom->dev || !iom->regbar) > > > > Do we need to check for !iom->dev and !iom->regbar? > > It's a good practice to have sanity checks on pointer members dereferenced. > > So I can lose the check on iom->dev, but prefer to keep the check on regbar. > Let me know if you feel strongly about losing the check for regbar as well. Sounds good. > > > Is there a valid situation > > where iom != NULL but iom->dev and/or iom->regbar == NULL? > > Sounds like it shouldn't, but I may be missing something. > > > > I think I am being conservative here. > > > > + return -ENODEV; > > > + > > > + if (!status || (port > IOM_MAX_PORTS - 1)) > > > > I think parentheses around "port > IOM_MAX_PORT - 1" aren't required. > > Ack > > > > + return -EINVAL; > > > + > > > + reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN * > > port; > > > + > > > + *status = ioread32(reg); > > > > Perhaps just inline reg within the parentheses? > > Kept this way to increase readability. Let me know if you feel strongly towards > inline reg. I'd rather this be inlined, you save a couple lines from the variable declaration, and IMO we're not gaining much in terms of readability by declaring this separately. > > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(intel_iom_port_status); > > > + > > > +static int intel_iom_probe(struct platform_device *pdev) { > > > + void __iomem *addr; > > > + > > > + /* only one IOM device is supported */ > > > > Minor nit: s/only/Only > > And then I may need to end the comment with a period. > Let me know if you feel strongly. Yes, let's capitalize and add the period. Let's try to use the right punctuation where possible. Best regards, -Prashant
next prev parent reply other threads:[~2020-08-24 22:47 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-08-22 4:05 [PATCH v2 0/3] Add Intel Input Output Manager driver Rajmohan Mani 2020-08-22 4:05 ` [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver Rajmohan Mani 2020-08-22 9:56 ` Prashant Malani 2020-08-22 9:56 ` Prashant Malani 2020-08-24 22:19 ` Mani, Rajmohan 2020-08-24 22:19 ` Mani, Rajmohan 2020-08-24 22:47 ` Prashant Malani [this message] 2020-08-24 22:47 ` Prashant Malani 2020-08-25 1:03 ` Mani, Rajmohan 2020-08-25 1:03 ` Mani, Rajmohan 2020-08-28 7:43 ` Greg Kroah-Hartman 2020-08-28 9:08 ` Heikki Krogerus 2020-08-28 10:03 ` Greg Kroah-Hartman 2020-08-28 11:55 ` Heikki Krogerus 2020-08-28 11:55 ` Heikki Krogerus 2020-08-28 15:21 ` Mani, Rajmohan 2020-08-28 15:21 ` Mani, Rajmohan 2020-08-28 15:20 ` Mani, Rajmohan 2020-08-28 15:20 ` Mani, Rajmohan 2020-08-29 6:27 ` Greg Kroah-Hartman 2020-08-29 6:27 ` Greg Kroah-Hartman 2020-08-31 21:40 ` Mani, Rajmohan 2020-08-31 21:40 ` Mani, Rajmohan 2020-08-22 4:05 ` [PATCH v2 2/3] usb: typec: intel_pmc_mux: Check the port status before connect Rajmohan Mani 2020-08-22 4:05 ` Rajmohan Mani 2020-08-22 4:05 ` [PATCH v2 3/3] usb: typec: intel_pmc_mux: Support for device role (UFP) Rajmohan Mani 2020-08-22 4:05 ` Rajmohan Mani
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=20200824224726.GA48297@google.com \ --to=pmalani@chromium.org \ --cc=andy@infradead.org \ --cc=ayman.bagabas@gmail.com \ --cc=blaz@mxxn.io \ --cc=bleung@chromium.org \ --cc=dmitry.torokhov@gmail.com \ --cc=dvhart@infradead.org \ --cc=gregkh@linuxfoundation.org \ --cc=heikki.krogerus@linux.intel.com \ --cc=jithu.joseph@intel.com \ --cc=lee.jones@linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=masahiroy@kernel.org \ --cc=mika.westerberg@linux.intel.com \ --cc=platform-driver-x86@vger.kernel.org \ --cc=rajmohan.mani@intel.com \ --cc=srinivas.pandruvada@linux.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.