On Wednesday, January 02, 2013 6:27 AM, Ian Abbott wrote:
> On 2012-12-19 22:39, H Hartley Sweeten wrote:
>> Consolidate the local variables 'read_subdev' and 'write_subdev' into a
>> single local variable 's'.
>>
>> Use comedi_dev_from_minor() to simplify the return -ENODEV tests.
>>
>> Use a goto in the !dev->attached test so that the mutex_unlock() call
>> is in a common place.
>>
>> Cleanup the formating of the || in the read and write subdevice poll
>> code.
>>
>> Signed-off-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
>> Cc: Ian Abbott <abbotti@xxxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> ---
>> drivers/staging/comedi/comedi_fops.c | 49 +++++++++++++++---------------------
>> 1 file changed, 20 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c
>> index f67b12a..093f403 100644
>> --- a/drivers/staging/comedi/comedi_fops.c
>> +++ b/drivers/staging/comedi/comedi_fops.c
>> @@ -1823,49 +1823,40 @@ static unsigned int comedi_poll(struct file *file, poll_table *wait)
>> {
>> unsigned int mask = 0;
>> const unsigned minor = iminor(file->f_dentry->d_inode);
>> - struct comedi_subdevice *read_subdev;
>> - struct comedi_subdevice *write_subdev;
>> struct comedi_file_info *info = comedi_file_info_from_minor(minor);
>> - struct comedi_device *dev;
>> + struct comedi_device *dev = comedi_dev_from_minor(minor);
>
> Isn't the comedi_dev_from_minor call a bit wasteful here if we have
> already called comedi_file_info_from_minor? How about a
> comedi_dev_from_file_info(info) function that can deal with info being
> NULL, e.g.:
>
> static inline struct comedi_device *
> comedi_dev_from_file_info(struct comedi_file_info *info)
> {
> return info ? info->device : NULL;
> }
>
> Then you could use:
>
> struct comedi_file_info *info = comedi_file_info_from_minor(minor);
> struct comedi_device *dev = comedi_dev_from_file_info(info);
>
> This also applies to patches 12, 13 and 25 in this series.
Yes, the dev parameter could be determined directly from the info parameter.
For consistency in the code I choose to use the comedi_dev_from_minor()
helper to get it instead of introducing a new helper to get the dev parameter
from the info parameter.
The comedi_mmap(), comedi_poll(), comedi_write() and comedi_read()
functions are the only ones that use both the info and dev. The other users
of the comedi_dev_from_minor() helper only use the dev parameter.
I'm not sure if it's worth creating a new helper to get the dev from the info.
If you feel it's worth it, the helper should not be exported since the info
structure is now private to comedi_fops.c.
Assuming Greg takes this patch series, a follow on patch can introduce a
comedi_dev_from_file_info() helper. It should be a static function in this
file to keep the comedi_file_info from being exposed globally.
Regards and happy new year!
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
