Greg KH <greg@xxxxxxxxx> wrote:
>On Tue, Dec 13, 2011 at 10:01:19AM +0100, Lars-Peter Clausen wrote:
>> On 12/13/2011 01:45 AM, Greg KH wrote:
>> > On Mon, Dec 12, 2011 at 11:08:46AM +0100, Lars-Peter Clausen wrote:
>> >> Add some convenience wrapper functions around the buffer access
>operations. This
>> >> makes the resulting code both a bit easier to read and to write.
>> >
>> > Yeah, but why are you abstracting this away?
>> >
>>
>> Because it's nicer to read and to write :) This is a purely cosmetic
>patch
>> which is supposed to ease to code flow a bit.
>>
>> But it also hides the actual implementation from the user, which
>makes it
>> easier to change the implementation at a later point without having
>to patch
>> each user.
>>
>> And of course it brings consistency to the users of these functions
>in regard
>> to whether a callback is checked, because it is optional, or not,
>because it is
>> mandatory.
>
>Ok, but you aren't consistent in your error codes or checking it seems.
>
>> >> +static inline int buffer_store_to(struct iio_buffer *buffer, u8
>*data,
>> >> + s64 timestamp)
>> >> +{
>> >> + return buffer->access->store_to(buffer, data, timestamp);
>> >
>> > WHy didn't you check this one here?
>>
>> Because the callback is not really optional.
>
>And these are all documented, right?
>
>> >> +static inline int buffer_mark_param_change(struct iio_buffer
>*buffer)
>> >> +{
>> >> + if (buffer->access->mark_param_change)
>> >> + return buffer->access->mark_param_change(buffer);
>> >> +
>> >> + return 0;
>> >
>> > Why 0? Not an error?
>>
>> Why an error, not 0?
>>
>> If the buffer doesn't implement a mark_param_change callback it is
>probably not
>> interested in being notified about changes. So not implementing the
>function is
>> not an error to the caller.
>
>Ok, documenting this would be nice...
>
>> >> +static inline int buffer_get_length(struct iio_buffer *buffer)
>> >> +{
>> >> + if (buffer->access->get_length)
>> >> + return buffer->access->get_length(buffer);
>> >> +
>> >> + return -ENOSYS;
>> >
>> > Here you return an error, but why ENOSYS?
>> >
>> > Consistancy is key, and you don't have it here at all. Or if you
>do, I
>> > sure don't understand it...
>>
>> Well, different types of functions require different semantics. While
>the
>> previous ones did either return 0 in case of success or a error value
>in case
>> of an error, buffer_get_length returns an integer value where 0 is a
>valid
>> value. Since we can't make any meaningful assumptions about the
>buffer size if
>> the callback is not implemented we return an error value. Why ENOSYS?
>Because
>> it is the code for 'function not implemented' and is used throughout
>the kernel
>> in similar situations.
>
>Is the caller always supposed to check this? If so, please mark the
>function as such so the compiler will complain if it isn't.
>
>> >> --- a/drivers/staging/iio/industrialio-buffer.c
>> >> +++ b/drivers/staging/iio/industrialio-buffer.c
>> >> @@ -43,9 +43,9 @@ ssize_t iio_buffer_read_first_n_outer(struct
>file *filp, char __user *buf,
>> >> struct iio_dev *indio_dev = filp->private_data;
>> >> struct iio_buffer *rb = indio_dev->buffer;
>> >>
>> >> - if (!rb || !rb->access->read_first_n)
>> >> + if (!rb)
>> >> return -EINVAL;
>> >> - return rb->access->read_first_n(rb, n, buf);
>> >> + return buffer_read_first_n(rb, n, buf);
>> >
>> > Oops, you just crashed if there wasn't a read_first_n() function
>here.
>>
>> I suppose it's pretty save to assume that if we have a buffer
>implementation
>> where you can't read any samples from it is broken anyway.
>
>I would think so, but the original code didn't think so :)
This isn't actually true as the data may leave iio. Still, if the driver doesn't know that something buggy is going on! The callback buffer in my rfc of a few weeks ago has no read back abilities.
>
>greg k-h
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@xxxxxxxxxxxxxxx
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android phone
with K-9 Mail. Please excuse my brevity.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
