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.
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.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
