Re: [PATCH 8/8] DSPBRIDGE: Use _IOxx macro to define ioctls

[Nishanth Menon <nm@ti.com>]

Ramirez Luna, Omar had written, on 01/08/2010 11:11 AM, the following:
>> From: Menon, Nishanth
>>
>> Ramirez Luna, Omar had written, on 01/07/2010 07:01 PM, the following:
>>> - Use standard convention to define ioctls.
>>> - Removed runtime check for ioctl matching table number.
>>> - Added __deprectaed marker to functions that are not used anymore.
>>>
>>> Currently 'DB' is used as identifier for dspbridge.
>>              ^^
>> include/asm-generic/ioctl.h
>> #define _IOC(dir,type,nr,size) \
>>         (((dir)  << _IOC_DIRSHIFT) | \
>>          ((type) << _IOC_TYPESHIFT) | \
>>          ((nr)   << _IOC_NRSHIFT) | \
>>          ((size) << _IOC_SIZESHIFT))
>>
>> define _IOWR(type,nr,size)
>> _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
>> should'nt type be a single char?
> 
> Actually its meant to be 0xDB... I'll change this
ok..

> 
>>> Added TODOs for removing the function table and, deprecated
>>> and not implemented ioctls, this can be done when all the ioctls
>>> are accessed through a switch instead of a pointer to function.
>>>
>>> *** NOTE: An update in api ioctl definitions is required. ***
>> Overall strategy:
>> as an example:
>> MGR_ENUMNODE_INFO was offset 0,
>> so, WCD_cmdTable had entry to point it to this as a map to
>> MGRWRAP_EnumNode_Info
>> now,
>> WCD_cmdTable[0] = MGRWRAP_EnumNode_Info which is referenced off cmd
>> parameter in (*WCD_cmdTable[cmd].fxn) (args, pr_ctxt);
>>
>> IMHO, _IOWR -> to mark a ioctl, good idea. indexing off
>> WCD_cmdTable[cmd] to grab the caller - not exactly my fav idea. esp if
>> you need to introduce newer ioctls in the middle.
>>
>> Now that you are breaking IOCTL number compatibility altogether,
>>
>> a) why cant' we split the list into multiple ones? +
>> #define MGR_BASE 'M'
>> #define MGR_ENUMNODE_INFO      _IOWR(MGR_BASE, 0, unsigned long)
> 
> Documentation/ioctl/ioctl-number.txt, didn't have time to check if outdated
yep
> 
> 'M'     all     linux/soundcard.h
> 
>> ..
>> ..
>>
>> #define PROC_BASE 'P'
>> #define PROC_ATTACH            _IOWR(PROC_BASE, 0, unsigned long)
> 
> 'P'     all     linux/soundcard.h
> 
>> ..
>> ..
>>
>> #define NODE_BASE 'N'
>> #define NODE_ALLOCATE          _IOWR(NODE_BASE, 0, unsigned long)
> 
> 'N'     00-1F   drivers/usb/scanner.h
EB onwards to DD seems to be free.
alright.. Please try and use consequtive ones from 0xD0,D1,D2,D3 etc.. 
choose something if you dont like conflicts ;). I'd think that these are 
ioctls for /dev/DspBridge anyways.. shrug.. you could even add a patch 
in the series to create Documentation/arm/OMAP/DspBridge and Document 
info if you like :).

> 
>> and so on..
>> b) then, you can populate different arrays for each type with the handlers.
>> based on
>> switch(_IOC_TYPE())
>>       case NODE_BASE: cmd_array= node_base_cmd_array; break;
>> ..
>>
>> this will allow you to add to any of these without having to be worried
>> about causing backward incompatibility.
>>
>> I guess given the number of handler's we gotta live with an array
>> anyways.. but we can at least reduce it's impact.. just my 2cents..
> 
> Given the current shape of the driver, I don't know why we would want to add more ioctls, 
 >others than to simplify some of its operations (like combining reserve 
and map)...
 >and if so, then it wouldn't have a name like PROC_RESERVE_N_MAP nor 
fit in between.
that is the point -we are trying to improve it. We did not know few 
years back that you would be sending this patch right? flexibility is 
easy thing to have if you do what I mention here, adding/deprecating an 
IOCTL without impact to userspace is important.

[...]
-- 
Regards,
Nishanth Menon