Re: [PATCH 01/18] iio: Extend the event config interface

[Lars-Peter Clausen <lars@metafoo.de>]

On 09/29/2013 10:17 PM, Jonathan Cameron wrote:
> On 09/29/13 19:56, Lars-Peter Clausen wrote:
>> On 09/29/2013 08:44 PM, Jonathan Cameron wrote:
>>> On 09/26/13 13:58, Lars-Peter Clausen wrote:
>>>> The event configuration interface of the IIO framework has not been getting the
>>>> same attention as other parts. As a result it has not seen the same improvements
>>>> as e.g. the channel interface has seen with the introduction of the channel spec
>>>> struct. Currently all the event config callbacks take a u64 (the so called event
>>>> code) to pass all the different information about for which event the callback
>>>> is invoked. The callback function then has to extract the information it is
>>>> interested in using some macros with rather long names. Most information encoded
>>>> in the event code comes straight from the iio_chan_spec struct the event was
>>>> registered for. Since we always have a handle to the channel spec when we call
>>>> the event callbacks the first step is to add the channel spec as a parameter to
>>>> the event callbacks. The two remaining things encoded in the event code are the
>>>> type and direction of the event. Instead of passing them in one parameter, add
>>>> one parameter for each of them and remove the eventcode from the event
>>>> callbacks. The patch also adds a new iio_event_info parameter to the
>>>> {read,write}_event_value callbacks. This makes it possible, similar to the
>>>> iio_chan_info_enum for channels, to specify additional properties other than
>>>> just the value for an event. Furthermore the new interface will allow to
>>>> register shared events. This is e.g. useful if a device allows configuring a
>>>> threshold event, but the threshold setting is the same for all channels.
>>>>
>>>> To implement this the patch adds a new iio_event_spec struct which is similar to
>>>> the iio_chan_spec struct. It as two field to specify the type and the direction
>>>> of the event. Furthermore it has a mask field for each one of the different
>>>> iio_shared_by types. These mask fields holds which kind of attributes should be
>>>> registered for the event. Creation of the attributes follows the same rules as
>>>> the for the channel attributes. E.g. for the separate_mask there will be a
>>>> attribute for each channel with this event, for the shared_by_type there will
>>>> only be one attribute per channel type. The iio_chan_spec struct gets two new
>>>> fields, 'event_spec' and 'num_event_specs', which is used to specify which the
>>>> events for this channel. These two fields are going to replace the channel's
>>>> event_mask field.
>>>>
>>>> For now both the old and the new event config interface coexist, but over the
>>>> few patches all drivers will be converted from the old to the new interface.
>>>> Once that is done all code for supporting the old interface will be removed.
>>>>
>>>
>>> A nice bit of work.
>>>
>>> Only one thing really came to mind. Is it worth us adding another field to the
>>> struct iio_device_attribute to avoid the dance to cram the event index and the
>>> offset into the one value?  If we want to avoid adding memory usage, small though
>>> it would be, perhaps drop an appropriate union in there to make this code easier
>>> to follow?  Obviously the attribute creation functions might get fiddlier though.
>>>
>>> Although it would cause a fair bit of churn to change it, the name of 'address'
>>> in there is rather misleading now as it very rarely actually contains an address!
>>>
>>
>> I think a transparent union should work nicely and doesn't cause any code
>> churn. Maybe even with a direct pointer to the event_spec.
>>
>> Something like:
>>
>> struct iio_dev_attr {
>> 	...
>> 	union {
>> 		u64 address;
>> 		struct {
>> 			struct iio_event_spec *spec;
>> 			enum iio_event_info info;
>> 		} event;
>> 	};
>> 	...
>> };
> Looks good to me.
> 

Ok, reset of the series looks good as well?

>>
>>
>>> One trivial naming question otherwise.
>>>
>>> ...
>>>> -static int iio_device_add_event_sysfs(struct iio_dev *indio_dev,
>>>> +static int iio_device_add_event(struct iio_dev *indio_dev,
>>>> +	const struct iio_chan_spec *chan, unsigned int spec_offset,
>>> spec_offset vs spec_index?
>>> Unless some fairly nefarous games are played in a driver, this value
>>> will always be an index into an array.  Offset made me wonder, wrt to
>>> which base position...
>>
>> spec_index is fine as well.
>>
>>>> +	enum iio_event_type type, enum iio_event_direction dir,
>>>> +	enum iio_shared_by shared_by, const unsigned long *mask)
>>>> +{
>>>> +	ssize_t (*show)(struct device *, struct device_attribute *, char *);
>>> ...
>>>
>>
>> Thanks for the review,
>> - Lars
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>