Dear Bjorn Helgaas,
On Fri, 5 Jul 2013 15:51:10 -0600, Bjorn Helgaas wrote:
> > ---
> > drivers/pci/msi.c | 22 ++++++++++++++++++++++
> > drivers/pci/probe.c | 1 +
> > include/linux/msi.h | 11 +++++++++++
> > include/linux/pci.h | 1 +
> > 4 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 289fbfd..62eb3d5 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -32,15 +32,37 @@ static int pci_msi_enable = 1;
> >
> > int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> > {
> > + struct msi_chip *chip = dev->bus->msi;
> > +
> > + if (chip && chip->setup_irq) {
> > + int err;
> > +
> > + err = chip->setup_irq(chip, dev, desc);
> > + if (err < 0)
> > + return err;
> > +
> > + irq_set_chip_data(desc->irq, chip);
> > + return err;
> > + }
> > +
> > return -EINVAL;
>
> It's sub-optimal to indent the whole body of a function like this. I
> think this is a bit more readable:
>
> if (!chip || !chip->setup_irq)
> return -EINVAL
>
> err = chip->setup_irq(...);
> ...
> return err;
Right.
> The return value of ->setup_irq() (and hence of arch_setup_msi_irq())
> is a bit unclear. Apparently it can return negative values (errors)
> or positive values (not sure what they mean), or zero (again, not
> sure). A comment would clear this up.
Ok, I'll have to look into this. Maybe Thierry Redding can comment on
this.
> It might even be worth introducing a no-op chip with pointers to no-op
> functions so we don't have to do these checks ("if (chip &&
> chip->xxx)" everywhere. I'm not sure if there's a Linux consensus on
> that -- certainly there are many examples of code that *does* make
> these checks everywhere -- so I'll ack it either way.
Ok, I'll see if it makes the overall thing cleaner.
> > int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> > {
> > + struct msi_chip *chip = dev->bus->msi;
> > +
> > + if (chip && chip->check_device)
> > + return chip->check_device(chip, dev, nvec, type);
> > +
>
> These functions are poorly named. They give no clue what
> "check_device" means. Are we checking that it exists, that it
> supports some property, that it's enabled, ... ?
Maybe Thierry Redding can comment on this one?
Thanks,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
