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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome