Re: [PATCH 1/3] PNP: Simplify setting of resources

[Witold Szczeponik]

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@xxxxxxxxxx>
>

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@xxxxxxx>
>>
>>
>> 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;
--
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/