[PATCH] staging:iio: Add wrapper functions around buffer access ops

[PATCH] staging:iio: Add wrapper functions around buffer access ops

[Lars-Peter Clausen]

Add some convenience wrapper functions around the buffer access operations. This
makes the resulting code both a bit easier to read and to write.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/staging/iio/buffer.h              |   68 +++++++++++++++++++++++++++++
 drivers/staging/iio/industrialio-buffer.c |   63 +++++++++++---------------
 2 files changed, 95 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/iio/buffer.h b/drivers/staging/iio/buffer.h
index 44593b2..46e0867 100644
--- a/drivers/staging/iio/buffer.h
+++ b/drivers/staging/iio/buffer.h
@@ -194,6 +194,74 @@ ssize_t iio_buffer_show_enable(struct device *dev,
 
 int iio_sw_buffer_preenable(struct iio_dev *indio_dev);
 
+static inline void buffer_mark_in_use(struct iio_buffer *buffer)
+{
+	if (buffer->access->mark_in_use)
+		buffer->access->mark_in_use(buffer);
+}
+
+static inline void buffer_unmark_in_use(struct iio_buffer *buffer)
+{
+	if (buffer->access->unmark_in_use)
+		buffer->access->unmark_in_use(buffer);
+}
+
+static inline int buffer_store_to(struct iio_buffer *buffer, u8 *data,
+	s64 timestamp)
+{
+	return buffer->access->store_to(buffer, data, timestamp);
+}
+
+static inline int buffer_read_first_n(struct iio_buffer *buffer, size_t n,
+	char __user *buf)
+{
+	return buffer->access->read_first_n(buffer, n, buf);
+}
+
+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;
+}
+
+static inline int buffer_request_update(struct iio_buffer *buffer)
+{
+	if (buffer->access->request_update)
+		return buffer->access->request_update(buffer);
+
+	return 0;
+}
+
+static inline int buffer_get_bytes_per_datum(struct iio_buffer *buffer)
+{
+	return buffer->access->get_bytes_per_datum(buffer);
+}
+
+static inline int buffer_set_bytes_per_datum(struct iio_buffer *buffer,
+	size_t bpd)
+{
+	return buffer->access->set_bytes_per_datum(buffer, bpd);
+}
+
+static inline int buffer_get_length(struct iio_buffer *buffer)
+{
+	if (buffer->access->get_length)
+		return buffer->access->get_length(buffer);
+
+	return -ENOSYS;
+}
+
+static inline int buffer_set_length(struct iio_buffer *buffer,
+	int length)
+{
+	if (buffer->access->set_length)
+		return buffer->access->set_length(buffer, length);
+
+	return -ENOSYS;
+}
+
 #else /* CONFIG_IIO_BUFFER */
 
 static inline int iio_buffer_register(struct iio_dev *indio_dev,
diff --git a/drivers/staging/iio/industrialio-buffer.c b/drivers/staging/iio/industrialio-buffer.c
index a03a574..8472570 100644
--- 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);
 }
 
 /**
@@ -69,8 +69,7 @@ int iio_chrdev_buffer_open(struct iio_dev *indio_dev)
 	struct iio_buffer *rb = indio_dev->buffer;
 	if (!rb)
 		return 0;
-	if (rb->access->mark_in_use)
-		rb->access->mark_in_use(rb);
+	buffer_mark_in_use(rb);
 	return 0;
 }
 
@@ -81,8 +80,7 @@ void iio_chrdev_buffer_release(struct iio_dev *indio_dev)
 	if (!rb)
 		return;
 	clear_bit(IIO_BUSY_BIT_POS, &rb->flags);
-	if (rb->access->unmark_in_use)
-		rb->access->unmark_in_use(rb);
+	buffer_unmark_in_use(rb);
 }
 
 void iio_buffer_init(struct iio_buffer *buffer)
@@ -369,12 +367,13 @@ ssize_t iio_buffer_read_length(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct iio_buffer *buffer = indio_dev->buffer;
+	int len;
 
-	if (buffer->access->get_length)
-		return sprintf(buf, "%d\n",
-			       buffer->access->get_length(buffer));
+	len = buffer_get_length(buffer);
+	if (len < 0)
+		return 0;
 
-	return 0;
+	return sprintf(buf, "%d\n", len);
 }
 EXPORT_SYMBOL(iio_buffer_read_length);
 
@@ -392,15 +391,13 @@ ssize_t iio_buffer_write_length(struct device *dev,
 	if (ret)
 		return ret;
 
-	if (buffer->access->get_length)
-		if (val == buffer->access->get_length(buffer))
-			return len;
+	ret = buffer_get_length(buffer);
+	if (ret >= 0 && ret == val)
+		return len;
 
-	if (buffer->access->set_length) {
-		buffer->access->set_length(buffer, val);
-		if (buffer->access->mark_param_change)
-			buffer->access->mark_param_change(buffer);
-	}
+	ret = buffer_set_length(buffer, val);
+	if (ret)
+		buffer_mark_param_change(buffer);
 
 	return len;
 }
@@ -435,25 +432,21 @@ ssize_t iio_buffer_store_enable(struct device *dev,
 				goto error_ret;
 			}
 		}
-		if (buffer->access->request_update) {
-			ret = buffer->access->request_update(buffer);
-			if (ret) {
-				printk(KERN_INFO
-				       "Buffer not started:"
-				       "buffer parameter update failed\n");
-				goto error_ret;
-			}
+		ret = buffer_request_update(buffer);
+		if (ret) {
+			printk(KERN_INFO
+				   "Buffer not started:"
+				   "buffer parameter update failed\n");
+			goto error_ret;
 		}
-		if (buffer->access->mark_in_use)
-			buffer->access->mark_in_use(buffer);
+		buffer_mark_in_use(buffer);
 		/* Definitely possible for devices to support both of these.*/
 		if (indio_dev->modes & INDIO_BUFFER_TRIGGERED) {
 			if (!indio_dev->trig) {
 				printk(KERN_INFO
 				       "Buffer not started: no trigger\n");
 				ret = -EINVAL;
-				if (buffer->access->unmark_in_use)
-					buffer->access->unmark_in_use(buffer);
+				buffer_unmark_in_use(buffer);
 				goto error_ret;
 			}
 			indio_dev->currentmode = INDIO_BUFFER_TRIGGERED;
@@ -470,8 +463,7 @@ ssize_t iio_buffer_store_enable(struct device *dev,
 				printk(KERN_INFO
 				       "Buffer not started:"
 				       "postenable failed\n");
-				if (buffer->access->unmark_in_use)
-					buffer->access->unmark_in_use(buffer);
+				buffer_unmark_in_use(buffer);
 				indio_dev->currentmode = previous_mode;
 				if (indio_dev->setup_ops->postdisable)
 					indio_dev->setup_ops->
@@ -485,8 +477,7 @@ ssize_t iio_buffer_store_enable(struct device *dev,
 			if (ret)
 				goto error_ret;
 		}
-		if (buffer->access->unmark_in_use)
-			buffer->access->unmark_in_use(buffer);
+		buffer_unmark_in_use(buffer);
 		indio_dev->currentmode = INDIO_DIRECT_MODE;
 		if (indio_dev->setup_ops->postdisable) {
 			ret = indio_dev->setup_ops->postdisable(indio_dev);
@@ -552,7 +543,7 @@ int iio_sw_buffer_preenable(struct iio_dev *indio_dev)
 		bytes = ALIGN(bytes, length);
 		bytes += length;
 	}
-	buffer->access->set_bytes_per_datum(buffer, bytes);
+	buffer_set_bytes_per_datum(buffer, bytes);
 
 	/* What scan mask do we actually have ?*/
 	if (indio_dev->available_scan_masks)
@@ -662,7 +653,7 @@ int iio_push_to_buffer(struct iio_buffer *buffer, unsigned char *data,
 {
 	unsigned char *dataout = iio_demux(buffer, data);
 
-	return buffer->access->store_to(buffer, dataout, timestamp);
+	return buffer_store_to(buffer, dataout, timestamp);
 }
 EXPORT_SYMBOL_GPL(iio_push_to_buffer);
 
-- 
1.7.7.3

Re: [PATCH] staging:iio: Add wrapper functions around buffer access ops

[Greg KH]

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?


> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/staging/iio/buffer.h              |   68 +++++++++++++++++++++++++++++
>  drivers/staging/iio/industrialio-buffer.c |   63 +++++++++++---------------
>  2 files changed, 95 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/staging/iio/buffer.h b/drivers/staging/iio/buffer.h
> index 44593b2..46e0867 100644
> --- a/drivers/staging/iio/buffer.h
> +++ b/drivers/staging/iio/buffer.h
> @@ -194,6 +194,74 @@ ssize_t iio_buffer_show_enable(struct device *dev,
>  
>  int iio_sw_buffer_preenable(struct iio_dev *indio_dev);
>  
> +static inline void buffer_mark_in_use(struct iio_buffer *buffer)
> +{
> +	if (buffer->access->mark_in_use)

Why would this check ever fail?

> +		buffer->access->mark_in_use(buffer);
> +}
> +
> +static inline void buffer_unmark_in_use(struct iio_buffer *buffer)
> +{
> +	if (buffer->access->unmark_in_use)

Same for this one?

> +		buffer->access->unmark_in_use(buffer);
> +}
> +
> +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?

> +}
> +
> +static inline int buffer_read_first_n(struct iio_buffer *buffer, size_t n,
> +	char __user *buf)
> +{
> +	return buffer->access->read_first_n(buffer, n, buf);
> +}
> +
> +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?

> +}
> +
> +static inline int buffer_request_update(struct iio_buffer *buffer)
> +{
> +	if (buffer->access->request_update)
> +		return buffer->access->request_update(buffer);
> +
> +	return 0;
> +}
> +
> +static inline int buffer_get_bytes_per_datum(struct iio_buffer *buffer)
> +{
> +	return buffer->access->get_bytes_per_datum(buffer);
> +}
> +
> +static inline int buffer_set_bytes_per_datum(struct iio_buffer *buffer,
> +	size_t bpd)
> +{
> +	return buffer->access->set_bytes_per_datum(buffer, bpd);
> +}
> +
> +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...

Are you trying to keep people from touching the access field of the
buffer directly?  If so, that's great, but you don't prevent that here.

Perhaps you need to reduce the levels of indirection and work on making
an easier buffer object to work with?  If you have to have these types
of "helper" functions, just to keep the levels of pointers you have to
type, perhaps that's not really a good data structure in the first place
to be using?



> +}
> +
> +static inline int buffer_set_length(struct iio_buffer *buffer,
> +	int length)
> +{
> +	if (buffer->access->set_length)
> +		return buffer->access->set_length(buffer, length);
> +
> +	return -ENOSYS;
> +}
> +
>  #else /* CONFIG_IIO_BUFFER */
>  
>  static inline int iio_buffer_register(struct iio_dev *indio_dev,
> diff --git a/drivers/staging/iio/industrialio-buffer.c b/drivers/staging/iio/industrialio-buffer.c
> index a03a574..8472570 100644
> --- 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.

See consistancy just tripped you up :)

Sorry, I don't want to take this patch as-is.

greg k-h

Re: [PATCH] staging:iio: Add wrapper functions around buffer access ops

[Lars-Peter Clausen]

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.

>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> Acked-by: Jonathan Cameron <jic23@kernel.org>
>> ---
>>  drivers/staging/iio/buffer.h              |   68 +++++++++++++++++++++++++++++
>>  drivers/staging/iio/industrialio-buffer.c |   63 +++++++++++---------------
>>  2 files changed, 95 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/staging/iio/buffer.h b/drivers/staging/iio/buffer.h
>> index 44593b2..46e0867 100644
>> --- a/drivers/staging/iio/buffer.h
>> +++ b/drivers/staging/iio/buffer.h
>> @@ -194,6 +194,74 @@ ssize_t iio_buffer_show_enable(struct device *dev,
>>  
>>  int iio_sw_buffer_preenable(struct iio_dev *indio_dev);
>>  
>> +static inline void buffer_mark_in_use(struct iio_buffer *buffer)
>> +{
>> +	if (buffer->access->mark_in_use)
> 
> Why would this check ever fail?

Because the callback is optional.

> 
>> +		buffer->access->mark_in_use(buffer);
>> +}
>> +
>> +static inline void buffer_unmark_in_use(struct iio_buffer *buffer)
>> +{
>> +	if (buffer->access->unmark_in_use)
> 
> Same for this one?
> 
>> +		buffer->access->unmark_in_use(buffer);
>> +}
>> +
>> +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.

> 
>> +}
>> +
>> +static inline int buffer_read_first_n(struct iio_buffer *buffer, size_t n,
>> +	char __user *buf)
>> +{
>> +	return buffer->access->read_first_n(buffer, n, buf);
>> +}
>> +
>> +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.

> 
>> +}
>> +
>> +static inline int buffer_request_update(struct iio_buffer *buffer)
>> +{
>> +	if (buffer->access->request_update)
>> +		return buffer->access->request_update(buffer);
>> +
>> +	return 0;
>> +}
>> +
>> +static inline int buffer_get_bytes_per_datum(struct iio_buffer *buffer)
>> +{
>> +	return buffer->access->get_bytes_per_datum(buffer);
>> +}
>> +
>> +static inline int buffer_set_bytes_per_datum(struct iio_buffer *buffer,
>> +	size_t bpd)
>> +{
>> +	return buffer->access->set_bytes_per_datum(buffer, bpd);
>> +}
>> +
>> +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.

> 
> Are you trying to keep people from touching the access field of the
> buffer directly?  If so, that's great, but you don't prevent that here.

As said before the main purpose of this patch is cosmetics.

> 
> Perhaps you need to reduce the levels of indirection and work on making
> an easier buffer object to work with?

But, if we were ever to changes the levels of indirection it will be a lot
easier with this patch, since you only have to change the one implementation of
iio_buffer_whatever() instead of fixup up all the users.


> If you have to have these types
> of "helper" functions, just to keep the levels of pointers you have to
> type, perhaps that's not really a good data structure in the first place
> to be using?
> 

This is just a normal virtual function table. You have multiple types of
buffers and each type can have multiple instances. It's common practice to
provide such helper or wrapper functions for virtual function tables.

> 
> 
>> +}
>> +
>> +static inline int buffer_set_length(struct iio_buffer *buffer,
>> +	int length)
>> +{
>> +	if (buffer->access->set_length)
>> +		return buffer->access->set_length(buffer, length);
>> +
>> +	return -ENOSYS;
>> +}
>> +
>>  #else /* CONFIG_IIO_BUFFER */
>>  
>>  static inline int iio_buffer_register(struct iio_dev *indio_dev,
>> diff --git a/drivers/staging/iio/industrialio-buffer.c b/drivers/staging/iio/industrialio-buffer.c
>> index a03a574..8472570 100644
>> --- 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.

> 
> See consistancy just tripped you up :)
> 
> Sorry, I don't want to take this patch as-is.
> 

Re: [PATCH] staging:iio: Add wrapper functions around buffer access ops

[Greg KH]

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

Re: [PATCH] staging:iio: Add wrapper functions around buffer access ops

[Jonathan Cameron]

Greg KH <greg@kroah.com> wrote:

>On Tue, Dec 13, 2011 at 10:01:19AM +010=
0, 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 writ=
e.
>> > 
>> > Yeah, but why are you abstracting this away?
>> > 
>> 
>> Bec=
ause 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 t=
he 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 n=
ot,
>because it is
>> mandatory.
>
>Ok, but you aren't consistent in your e=
rror 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);
>> > 
>> > WH=
y didn't you check this one here?
>> 
>> Because the callback is not really=
 optional.
>
>And these are all documented, right?
>
>> >> +static inline i=
nt buffer_mark_param_change(struct iio_buffer
>*buffer)
>> >> +{
>> >> +	if=
 (buffer->access->mark_param_change)
>> >> +		return buffer->access->mark_p=
aram_change(buffer);
>> >> +
>> >> +	return 0;
>> > 
>> > Why 0?  Not an er=
ror?
>> 
>> Why an error, not 0?
>> 
>> If the buffer doesn't implement a m=
ark_param_change callback it is
>probably not
>> interested in being notifi=
ed about changes. So not implementing the
>function is
>> not an error to t=
he caller.
>
>Ok, documenting this would be nice...
>
>> >> +static inline =
int buffer_get_length(struct iio_buffer *buffer)
>> >> +{
>> >> +	if (buffe=
r->access->get_length)
>> >> +		return buffer->access->get_length(buffer);
=
>> >> +
>> >> +	return -ENOSYS;
>> > 
>> > Here you return an error, but wh=
y 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
>> va=
lue. 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 thro=
ughout
>the kernel
>> in similar situations.
>
>Is the caller always suppos=
ed 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 =3D filp->private_data;
>> >=
>  	struct iio_buffer *rb =3D 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 re=
ad_first_n() function
>here.
>> 
>> I suppose it's pretty save to assume th=
at 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 bu=
ffer 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@vger.kernel.org
>More majordomo info a=
t  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android pho=
ne 
with K-9 Mail. Please excuse my brevity.

Re: [PATCH] staging:iio: Add wrapper functions around buffer access ops

[Lars-Peter Clausen]

On 12/14/2011 12:59 AM, Greg KH wrote:
> 
>>>> +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.

Marking the function as __must_check doesn't make much sense here. Since it
will either return an error or the buffer length. So you'll always use the
returned result one way or the other.

Re: [PATCH] staging:iio: Add wrapper functions around buffer access ops

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]

On Wed, Dec 14, 2011 at 11:15:49AM +0100, Lars-Peter Clausen wrote:
> Marking the function as __must_check doesn't make much sense here. Since it
> will either return an error or the buffer length. So you'll always use the
> returned result one way or the other.

Isn't that the point of a __must_check?

This reminds me of when we added __must_check to ERR_PTR() and
PTR_ERR().  We found places that didn't check.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] staging:iio: Add wrapper functions around buffer access ops

[Lars-Peter Clausen]

On 12/14/2011 03:31 PM, Dan Carpenter wrote:
> On Wed, Dec 14, 2011 at 11:15:49AM +0100, Lars-Peter Clausen wrote:
>> Marking the function as __must_check doesn't make much sense here. Since it
>> will either return an error or the buffer length. So you'll always use the
>> returned result one way or the other.
> 
> Isn't that the point of a __must_check?

My understanding is that you should use __must_check if it is potentially
dangerous to ignore the return value. Which is not the case here, if you
don't look at the return value it's kind of pointless to call the function
in the first, but it is not dangerous.

- Lars

Re: [PATCH] staging:iio: Add wrapper functions around buffer access ops

[Greg KH]

On Wed, Dec 14, 2011 at 11:15:49AM +0100, Lars-Peter Clausen wrote:
> On 12/14/2011 12:59 AM, Greg KH wrote:
> > 
> >>>> +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.
> 
> Marking the function as __must_check doesn't make much sense here. Since it
> will either return an error or the buffer length. So you'll always use the
> returned result one way or the other.

That's exactly the point, you must use it, so mark it as such.

greg k-h

Re: [PATCH] staging:iio: Add wrapper functions around buffer access ops

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

On Wed, Dec 14, 2011 at 04:05:12PM +0100, Lars-Peter Clausen wrote:
> On 12/14/2011 03:31 PM, Dan Carpenter wrote:
> > On Wed, Dec 14, 2011 at 11:15:49AM +0100, Lars-Peter Clausen wrote:
> >> Marking the function as __must_check doesn't make much sense here. Since it
> >> will either return an error or the buffer length. So you'll always use the
> >> returned result one way or the other.
> > 
> > Isn't that the point of a __must_check?
> 
> My understanding is that you should use __must_check if it is potentially
> dangerous to ignore the return value. Which is not the case here, if you
> don't look at the return value it's kind of pointless to call the function
> in the first, but it is not dangerous.
> 

I only responded to the previous email because you described exactly
the situation that __must_check is designed for, as a reason to not
use it.  It struck me as humourous.

ERR_PTR() is likewise not dangerous.  It's just a cast, but it
doesn't make sense to not check it, so that's why it has a
__must_check tag.  If a function is part of the infrastructure and
gets called a lot then a __must_check is appropriate.

regards,
dan carpenter

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] staging:iio: Add wrapper functions around buffer access ops

[Lars-Peter Clausen]

On 12/14/2011 04:49 PM, Greg KH wrote:
> On Wed, Dec 14, 2011 at 11:15:49AM +0100, Lars-Peter Clausen wrote:
>> On 12/14/2011 12:59 AM, Greg KH wrote:
>>>
>>>>>> +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.
>>
>> Marking the function as __must_check doesn't make much sense here. Since it
>> will either return an error or the buffer length. So you'll always use the
>> returned result one way or the other.
> 
> That's exactly the point, you must use it, so mark it as such.
> 
So by that logic all functions without side effects should be marked as
__must_check?

- Lars