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
