Messages in this thread | | | Date | Sun, 01 Apr 2012 18:05:52 +0200 | From | Witold Szczeponik <> | Subject | Re: [PATCH 1/3] PNP: Simplify setting of resources |
| |
On 27/03/12 22:32, Bjorn Helgaas wrote:
[...]
> > A few minor comments below, but I think this looks good. In the past, > I've sent PNP patches through linux-acpi and Len's ACPI tree. > > Reviewed-by: Bjorn Helgaas<bhelgaas@google.com> >
I will send an updated version of the patch series also to Len and linux-acpi. Thanks for pointing this out to me!
>> Signed-off-by: Witold Szczeponik<Witold.Szczeponik@gmx.net> >> >> >> Index: linux/drivers/pnp/interface.c >> =================================================================== >> --- linux.orig/drivers/pnp/interface.c >> +++ linux/drivers/pnp/interface.c >> @@ -298,6 +298,39 @@ static ssize_t pnp_show_current_resource >> return ret; >> } >> >> +static char *pnp_get_resource_value(char *buf, >> + unsigned long type, >> + resource_size_t *start, >> + resource_size_t *end, >> + unsigned long *flags) >> +{ >> + if (start) >> + *start = 0; >> + if (end) >> + *end = 0; >> + if (flags) >> + *flags = 0; >> + >> + /* TBD: allow for disabled resources */ >> + >> + buf = skip_spaces(buf); >> + if (start) { >> + *start = simple_strtoull(buf,&buf, 0); >> + if (end) { >> + buf = skip_spaces(buf); >> + if (*buf == '-') { >> + buf = skip_spaces(buf + 1); >> + *end = simple_strtoull(buf,&buf, 0); >> + } else >> + *end = *start; >> + } >> + } >> + >> + /* TBD: allow for additional flags, e.g., IORESOURCE_WINDOW */ >> + >> + return buf; >> +} >> + >> static ssize_t pnp_set_current_resources(struct device *dmdev, >> struct device_attribute *attr, >> const char *ubuf, size_t count) >> @@ -305,7 +338,6 @@ static ssize_t pnp_set_current_resources >> struct pnp_dev *dev = to_pnp_dev(dmdev); >> char *buf = (void *)ubuf; >> int retval = 0; >> - resource_size_t start, end; >> >> if (dev->status& PNP_ATTACHED) { >> retval = -EBUSY; >> @@ -349,6 +381,10 @@ static ssize_t pnp_set_current_resources >> goto done; >> } >> if (!strnicmp(buf, "set", 3)) { >> + resource_size_t start; >> + resource_size_t end; >> + unsigned long flags; >> + >> if (dev->active) >> goto done; >> buf += 3; >> @@ -357,42 +393,42 @@ static ssize_t pnp_set_current_resources >> while (1) { >> buf = skip_spaces(buf); >> if (!strnicmp(buf, "io", 2)) { >> - buf = skip_spaces(buf + 2); >> - start = simple_strtoul(buf,&buf, 0); >> - buf = skip_spaces(buf); >> - if (*buf == '-') { >> - buf = skip_spaces(buf + 1); >> - end = simple_strtoul(buf,&buf, 0); >> - } else >> - end = start; >> - pnp_add_io_resource(dev, start, end, 0); >> - continue; >> - } >> - if (!strnicmp(buf, "mem", 3)) { >> - buf = skip_spaces(buf + 3); >> - start = simple_strtoul(buf,&buf, 0); >> - buf = skip_spaces(buf); >> - if (*buf == '-') { >> - buf = skip_spaces(buf + 1); >> - end = simple_strtoul(buf,&buf, 0); >> - } else >> - end = start; >> - pnp_add_mem_resource(dev, start, end, 0); >> - continue; >> - } >> - if (!strnicmp(buf, "irq", 3)) { >> - buf = skip_spaces(buf + 3); >> - start = simple_strtoul(buf,&buf, 0); >> - pnp_add_irq_resource(dev, start, 0); >> - continue; >> - } >> - if (!strnicmp(buf, "dma", 3)) { >> - buf = skip_spaces(buf + 3); >> - start = simple_strtoul(buf,&buf, 0); >> - pnp_add_dma_resource(dev, start, 0); >> - continue; >> - } >> - break; >> + buf = pnp_get_resource_value(buf + 2, >> + IORESOURCE_IO, >> +&start, >> +&end, >> +&flags); > > Looks like at least "&end" will fit on the same line as "&start".
Indeed, they do: will change this.
> >> + pnp_add_io_resource(dev, start, end, flags); >> + } else if (!strnicmp(buf, "mem", 3)) { >> + buf = pnp_get_resource_value(buf + 3, >> + IORESOURCE_MEM, >> +&start, >> +&end, >> +&flags); >> + pnp_add_mem_resource(dev, start, end, flags); >> + } else if (!strnicmp(buf, "irq", 3)) { >> + buf = pnp_get_resource_value(buf + 3, >> + IORESOURCE_IRQ, >> +&start, >> + NULL, >> +&flags); >> + pnp_add_irq_resource(dev, start, flags); >> + } else if (!strnicmp(buf, "dma", 3)) { >> + buf = pnp_get_resource_value(buf + 3, >> + IORESOURCE_DMA, >> +&start, >> + NULL, >> +&flags); >> + pnp_add_dma_resource(dev, start, flags); >> + } else if (!strnicmp(buf, "bus", 3)) { >> + buf = pnp_get_resource_value(buf + 3, >> + IORESOURCE_BUS, >> +&start, >> +&end, >> + NULL); >> + pnp_add_bus_resource(dev, start, end); > > It makes sense to allow a user to set bus number resources just like > any other resources, but I don't think the rest of PNP supports this > yet. I think only PNPACPI supports bus number resources at all, so > when you "activate" the device after setting its resources, we'll call > pnpacpi_set_resources() and pnpacpi_encode_resources(), but > pnpacpi_encode_resources() doesn't know how to encode the address16 > descriptor that will probably be used for bus numbers. > > I guess that's all right, though. We already have the same situation > for all the address space descriptors when used for IO or MEM. > >> + } else >> + break; >> } >> mutex_unlock(&pnp_res_mutex); >> goto done;
| |