Re: [PATCH] clockevents: Introduce mode specific callbacks

[Viresh Kumar]

On 10 February 2015 at 22:15, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> On Wed, Feb 04, 2015 at 01:06:23PM +0530, Viresh Kumar wrote:
>> +     /*
>> +      * Mode transition callback(s): Only one of the two groups should be
>> +      * defined:
>> +      * - set_mode(), only for modes <= CLOCK_EVT_MODE_RESUME.
>> +      * - set_mode_{shutdown|periodic|oneshot|resume}().
>> +      */
>>       void                    (*set_mode)(enum clock_event_mode mode,
>>                                           struct clock_event_device *);
>> +     int                     (*set_mode_periodic)(struct clock_event_device *);
>> +     int                     (*set_mode_oneshot)(struct clock_event_device *);
>> +     int                     (*set_mode_shutdown)(struct clock_event_device *);
>> +     int                     (*set_mode_resume)(struct clock_event_device *);
>
>
>
>
>> +/* Sanity check of mode transition callbacks */
>> +static int clockevents_sanity_check(struct clock_event_device *dev)
>> +{
>> +     /* Legacy set_mode() callback */
>> +     if (dev->set_mode)
>> +             return 0;
>> +
>> +     if (dev->features & CLOCK_EVT_FEAT_DUMMY)
>> +             return 0;
>> +
>> +     /* New mode-specific callbacks */
>> +     if (!dev->set_mode_shutdown)
>> +             return -EINVAL;
>> +
>> +     if ((dev->features & CLOCK_EVT_FEAT_PERIODIC) &&
>> +         !dev->set_mode_periodic)
>> +             return -EINVAL;
>> +
>> +     if ((dev->features & CLOCK_EVT_FEAT_ONESHOT) &&
>> +         !dev->set_mode_oneshot)
>> +             return -EINVAL;
>> +
>> +     return 0;
>> +}
>
> It appears to me you've not actually checked that condition outlined
> above, a driver could set both the legacy and the new callbacks.

Exactly for this reason I mentioned this in the logs:

>> If the legacy ->set_mode() callback is provided, all mode specific
>> callbacks would be ignored.

So, either we can mention that in the code as well OR add code to
check and WARN about that. Will do whatever looks better to you
guys.

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/