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

[Jonathan Cameron <jic23@kernel.org>]

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.

> 
> 
>> 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
>