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 :)
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
