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-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
