On Thursday, April 18, 2013 10:26 AM, Ian Abbott wrote:
> On 2013/04/18 04:35 PM, H Hartley Sweeten wrote:
>> On Thursday, April 18, 2013 3:37 AM, Ian Abbott wrote:
>>> On 2013-04-17 19:16, H Hartley Sweeten wrote:
>>>> This function is intended to be used by the comedi legacy (ISA) drivers
>>>> either directly as the (*detach) function or as a helper in the drivers
>>>> private (*detach) function.
>>>>
>>>> Modify the __comedi_request_region() helper so that it stores the first
>>>> struct resource returned by request_region() in the comedi_device.
>>>>
>>>> The comedi_legacy_detach() function will then automatically release the
>>>> region during the detach of the driver.
>>>>
>>>> Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>>>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>>>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>
>> <snip>
>>
>>> An alternative implementation would be to leave
>>> __comedi_request_region() alone, but change comedi_request_region() to
>>> record the length in a new member dev->iolen at the same time that it
>>> records the start address in dev->iobase. Then your
>>> comedi_legacy_detach() could do something like:
>>>
>>> if (dev->iolen) {
>>> release_region(dev->iobase, dev->iolen);
>>> dev->iolen = 0;
>>> }
>>>
>>> I think it seems slightly simpler and shouldn't affect your other
>>> patches. Any thoughts?
>>
>> I feel its cleaner to save the actual resource that was allocated by
>> request_resource(), it's less error prone.
>
> Apart from it describing the resource (whose details are already known),
> the actual struct resource isn't all that useful.
>
>> But, I could change __comedi_request_region() so it returns the
>> allocated resource instead of an errno. Then comedi_request_resource()
>> can save it in the comedi_device. Any drivers that request additional
>> resources could so the same and use the saved resource when detaching.
>> Also, comedi_request_resource() could (should?) sanity check the
>> saved resource to make sure a driver does not call it multiple times.
>
> That would mean changing all the callers of __comedi_request_region() again.
Yah. But there are only 6 drivers that use it.
I did a quick look and 5 of them can be changed easily. They are the das16,
Das16m1, das1800, pcl816 and unioxx5 drivers. The 8255 driver is the only
other caller of __comedi_request_region(). That one is a bit ugly. I think
it's better leaving it the way it is now.
> Your current method requires drivers requesting multiple regions to call
> comedi_request_region() and __comedi_request_region() in a particular
> order (if you don't want drivers to have to call release_region() for
> dev->iobase by themselves, and remember which region was going to be
> released automatically by comedi_legacy_detach()), whereas my suggestion
> would allow drivers (regardless of the order they call
> comedi_request_region() and __comedi_request_region()) to only have to
> call release_region() for regions they requested with
> __comedi_request_region(), and leave the releasing of the main
> dev->iobase region up to comedi_legacy_detach().
>
> In other words, with my suggestion, a driver using
> comedi_legacy_detach() would always pair __comedi_request_region() with
> release_region(), and always pair comedi_request_region() with
> comedi_legacy_detach() for the dev->iobase region, regardless of the
> order in which it requests the regions.
Fair enough.
I'll update the patch and hopefully get the series reposted today.
Regards,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
