From: Witold Szczeponik <Witold.Szczeponik@gmx.net>
To: Bjorn Helgaas <bhelgaas@google.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/3] PNP: Simplify setting of resources
Date: Sun, 01 Apr 2012 18:05:52 +0200 [thread overview]
Message-ID: <4F787CE0.2050406@gmx.net> (raw)
In-Reply-To: <CAErSpo6VxdvpmSA_RbPLcqduwqXbJg_VCqSdW7_+_UGLeWWT+w@mail.gmail.com>
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;
next prev parent reply other threads:[~2012-04-01 16:06 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-03-20 18:49 [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface) Witold Szczeponik
2012-03-20 19:57 ` [PATCH 1/3] PNP: Simplify setting of resources Witold Szczeponik
2012-03-27 20:32 ` Bjorn Helgaas
2012-04-01 16:05 ` Witold Szczeponik [this message]
2012-03-20 20:00 ` [PATCH 2/3] PNP: Allow resources to be set as disabled Witold Szczeponik
2012-03-27 20:38 ` Bjorn Helgaas
2012-04-01 16:06 ` Witold Szczeponik
2012-03-20 20:05 ` [PATCH 3/3] PNP: Allow resource flags to be set explicitly Witold Szczeponik
2012-03-27 20:52 ` Bjorn Helgaas
2012-04-01 16:07 ` Witold Szczeponik
2012-03-27 20:57 ` [PATCH 0/3] PNP: Allow PNP resources to be disabled (interface) Bjorn Helgaas
2012-04-01 16:04 ` Witold Szczeponik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F787CE0.2050406@gmx.net \
--to=witold.szczeponik@gmx.net \
--cc=bhelgaas@google.com \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.