Re: [PATCH 1/3] hpet: convert to PNP driver

[Bjorn Helgaas]

On Wednesday 17 March 2010 04:18:27 pm Venkatesh Pallipadi wrote:
> On Wed, Mar 17, 2010 at 10:35 AM, Bjorn Helgaas <bjorn.helgaas@xxxxxx> wrote:
> >
> > PNPACPI already parses _CRS, and does a more complete job than we did here,
> > so let's just take advantage of that.
> 
> Patch looks good.
> One comment is that we now have a dependency on CONFIG_PNP. That
> probably needs to be added in drivers/char/Kconfig. But, that also
> means we may fail for users who do not have PNP in their config today.
> May be just select PNP when HPET is selected?

Good point; I missed that.  I could just add #ifdef CONFIG_PNP around
the relevant code in hpet.c.  That's the typical way we handle this.

drivers/char/hpet.c already depends on ACPI for another reason
(acpi_irq_model), and we do select PNP whenever CONFIG_ACPI=y, so
I don't think the CONFIG_PNP=n case will arise in practice.  But
it'd probably be better to have the #ifdef wrapper than to have the
implicit dependency on the "select PNP" in drivers/acpi/Kconfig.

> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@xxxxxx>
> > ---
> >
> >  drivers/char/hpet.c |  103 +++++++++++++--------------------------------------
> >  1 files changed, 27 insertions(+), 76 deletions(-)
> >
> >
> >
> > -       if (!data.hd_address || !data.hd_nirqs) {
> > -               printk("%s: no address or irqs in _CRS\n", __func__);
> > -               return -ENODEV;
> > -       }
> > +       data.hd_address = ioremap(mem->start, resource_size(mem));
> >
> >        return hpet_alloc(&data);
> >  }
> 
> Not a problem with this patch. But, don't we have a problem of rogue
> ioremap when hpet_alloc() fails here?

You're right.  I'll add a patch to fix that while I'm at it.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/