On Wednesday, September 10, 2014 3:56 AM, Ian Abbott wrote:
> On 2014-09-10 00:15, H Hartley Sweeten wrote:
>> The validation of the cmd->stop_arg when the cmd->stop_src == TRIG_EXT
>> is a bit over thought. The comments state that the stop_arg is validated
>> to force an external trigger of 0 and allow the CR_EDGE flag, which is
>> ignored. In reality the stop_arg is not even used by the driver when
>> the stop_src is TRIG_EXT.
>>
>> Simplify the validation so that the stop_arg must simply be '0' when
>> the stop_src is TRIG_EXT.
>>
>> Signed-off-by: H Hartley Sweeten <hswee...@visionengravers.com>
>> Cc: Ian Abbott <abbo...@mev.co.uk>
>> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
>
> I'd say that CR_EDGE is a perfectly legal flag to use here (as the
> external trigger *is* edge-triggered), even if the driver allows the
> user not to set it. If the user code went to the trouble of setting the
> flag only for the driver to clear it and flag it as an errors, its like
> the driver saying "Sorry, I don't support edge-triggered triggers here."
Fair enough. I asked Greg to drop this patch (along with patch 8).
I still feel the TRIG_EXT arg validation is a bit messy. How about
adding a new helper to comedi_fc? Something like:
/**
* cfc_check_ext_trigger_arg() - trivially validate a TRIG_EXT argument
* @arg: pointer to the trigger arg to validate
* @min_chan: the minimum channel to use as the external trigger
* @max_chan: the maximum channel to use as the external trigger
* @allowed_flags: mask of allowed flags
* @reg_flags: mask of required flags
*/
static int cfc_check_ext_trigger_arg(unsigned int *arg,
unsigned int min_chan,
unsigned int max_chan,
unsigned int allowed_flags,
unsigned int req_flags)
{
unsigned int chan = CR_CHAN(arg);
unsigned int flags = arg & CR_FLAGS_MASK;
int ret = 0;
/* validate that the channel is in range */
if (chan < min_chan || chan > max_chan) {
/*preserve flags and default to the minimum channel */
*arg &= CR_FLAGS_MASK;
*arg |= CR_PACK(min_chan, 0, 0);
ret |= -EINVAL;
}
/* validate that only the allowed flags are set */
if (flags & ~allowed_flags) {
/* preserve channel and default to the required flags */
*arg &= ~ CR_FLAGS_MASK;
*arg |= req_flags;
ret |= -EINVAL;
}
/* validate that the required flags are set */
if ((flags & req_flags) == 0) {
*arg |= req_flags;
ret |= -EINVAL;
}
return ret;
}
The (*do_cmdtest) functions then just do something like:
/* trigger is channel 0, CR_EDGE is allowed but ignored */
err |= cfc_check_ext_trigger_arg(&cmd->stop_arg, 0, 0,
CR_EDGE, 0);
Regards,
Hartley
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
