Messages in this thread | | | Date | Sun, 13 Apr 2014 18:51:22 -0700 | From | Srinivas Pandruvada <> | Subject | Re: [Patch v3 2/6] IIO: core: Introduce read_raw_multi |
| |
On 04/12/2014 09:52 AM, Jonathan Cameron wrote: > On 09/04/14 01:56, Srinivas Pandruvada wrote: >> This callback is introduced to overcome some limitations of existing >> read_raw callback. The functionality of both existing read_raw and >> read_raw_multi is similar, both are used to request values from the >> device. The current read_raw callback allows only two return values. >> The new read_raw_multi allows returning multiple values. Instead of >> passing just address of val and val2, it passes length and pointer >> to values. Depending on the type and length of passed buffer, iio >> client drivers can return multiple values. >> >> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Hi Srinivas. > > This has come together pretty much how I thought it would. Very nice. > Only comment inline is that I'd prefer we took care now with possiblity > of really long sets of values so that we don't get bitten by it sometime > in the future. > I was thinking of using snprintf, but buf had no length passed. If we assume PAGE_SIZE as max length then I can do what you suggested below,
Thanks, Srinivas > If you want to drop the reference to 0 having special meaning in the > comment as well, thats fine by me. > > Jonathan >> --- >> drivers/iio/iio_core.h | 2 +- >> drivers/iio/industrialio-core.c | 65 >> ++++++++++++++++++++++++++-------------- >> drivers/iio/industrialio-event.c | 6 ++-- >> drivers/iio/inkern.c | 16 ++++++++-- >> include/linux/iio/iio.h | 17 +++++++++++ >> include/linux/iio/types.h | 1 + >> 6 files changed, 80 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h >> index f6db6af..30327ad 100644 >> --- a/drivers/iio/iio_core.h >> +++ b/drivers/iio/iio_core.h >> @@ -35,7 +35,7 @@ int __iio_add_chan_devattr(const char *postfix, >> struct list_head *attr_list); >> void iio_free_chan_devattr_list(struct list_head *attr_list); >> >> -ssize_t iio_format_value(char *buf, unsigned int type, int val, int >> val2); >> +ssize_t iio_format_value(char *buf, unsigned int type, int size, int >> *val); >> >> /* Event interface flags */ >> #define IIO_BUSY_BIT_POS 1 >> diff --git a/drivers/iio/industrialio-core.c >> b/drivers/iio/industrialio-core.c >> index ede16aec..3bd565c 100644 >> --- a/drivers/iio/industrialio-core.c >> +++ b/drivers/iio/industrialio-core.c >> @@ -373,41 +373,53 @@ EXPORT_SYMBOL_GPL(iio_enum_write); >> * @buf: The buffer to which the formated value gets written >> * @type: One of the IIO_VAL_... constants. This decides how the >> val and val2 >> * parameters are formatted. >> - * @val: First part of the value, exact meaning depends on the type >> parameter. >> - * @val2: Second part of the value, exact meaning depends on the >> type parameter. >> + * @vals: pointer to the values, exact meaning depends on the type >> parameter. >> */ >> -ssize_t iio_format_value(char *buf, unsigned int type, int val, int >> val2) >> +ssize_t iio_format_value(char *buf, unsigned int type, int size, int >> *vals) >> { >> unsigned long long tmp; >> bool scale_db = false; >> >> switch (type) { >> case IIO_VAL_INT: >> - return sprintf(buf, "%d\n", val); >> + return sprintf(buf, "%d\n", vals[0]); >> case IIO_VAL_INT_PLUS_MICRO_DB: >> scale_db = true; >> case IIO_VAL_INT_PLUS_MICRO: >> - if (val2 < 0) >> - return sprintf(buf, "-%ld.%06u%s\n", abs(val), -val2, >> + if (vals[1] < 0) >> + return sprintf(buf, "-%ld.%06u%s\n", abs(vals[0]), >> + -vals[1], >> scale_db ? " dB" : ""); >> else >> - return sprintf(buf, "%d.%06u%s\n", val, val2, >> + return sprintf(buf, "%d.%06u%s\n", vals[0], vals[1], >> scale_db ? " dB" : ""); >> case IIO_VAL_INT_PLUS_NANO: >> - if (val2 < 0) >> - return sprintf(buf, "-%ld.%09u\n", abs(val), -val2); >> + if (vals[1] < 0) >> + return sprintf(buf, "-%ld.%09u\n", abs(vals[0]), >> + -vals[1]); >> else >> - return sprintf(buf, "%d.%09u\n", val, val2); >> + return sprintf(buf, "%d.%09u\n", vals[0], vals[1]); >> case IIO_VAL_FRACTIONAL: >> - tmp = div_s64((s64)val * 1000000000LL, val2); >> - val2 = do_div(tmp, 1000000000LL); >> - val = tmp; >> - return sprintf(buf, "%d.%09u\n", val, val2); >> + tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]); >> + vals[1] = do_div(tmp, 1000000000LL); >> + vals[0] = tmp; >> + return sprintf(buf, "%d.%09u\n", vals[0], vals[1]); >> case IIO_VAL_FRACTIONAL_LOG2: >> - tmp = (s64)val * 1000000000LL >> val2; >> - val2 = do_div(tmp, 1000000000LL); >> - val = tmp; >> - return sprintf(buf, "%d.%09u\n", val, val2); >> + tmp = (s64)vals[0] * 1000000000LL >> vals[1]; >> + vals[1] = do_div(tmp, 1000000000LL); >> + vals[0] = tmp; >> + return sprintf(buf, "%d.%09u\n", vals[0], vals[1]); >> + case IIO_VAL_INT_MULTIPLE: >> + { >> + int i; >> + int len = 0; >> + >> + for (i = 0; i < size; ++i) >> + len += sprintf(&buf[len], "%d ", vals[i]); >> + buf[len++] = '\n'; >> + buf[len++] = '\0'; > Whilst we know this is of a fixed maxium length, I think we > want to take more care to ensure that we don't overrun the maximum > sysfs length. I'd also prefer specific use of snprintf for the new > line as well to make sure that doesn't cause trouble. i.e. > > for (i = 0; i < size; ++i) > len += snprintf(&buf[len], PAGE_SIZE - len, "%d ", vals[i]); > len += snprintf(&buf[len], PAGE_SIZE - len, "/n"); > > The reasoning being that we could easily mess something up and hit these > limits sometime in the future. Also not using the explicit character > settting, but doing it with snprintf is easier to read (slightly ;) > > >> + return len; >> + } >> default: >> return 0; >> } >> @@ -419,14 +431,23 @@ static ssize_t iio_read_channel_info(struct >> device *dev, >> { >> struct iio_dev *indio_dev = dev_to_iio_dev(dev); >> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); >> - int val, val2; >> - int ret = indio_dev->info->read_raw(indio_dev, this_attr->c, >> - &val, &val2, this_attr->address); >> + int vals[INDIO_MAX_RAW_ELEMENTS]; >> + int ret; >> + int val_len = 2; >> + >> + if (indio_dev->info->read_raw_multi) >> + ret = indio_dev->info->read_raw_multi(indio_dev, this_attr->c, >> + INDIO_MAX_RAW_ELEMENTS, >> + vals, &val_len, >> + this_attr->address); >> + else >> + ret = indio_dev->info->read_raw(indio_dev, this_attr->c, >> + &vals[0], &vals[1], this_attr->address); >> >> if (ret < 0) >> return ret; >> >> - return iio_format_value(buf, ret, val, val2); >> + return iio_format_value(buf, ret, val_len, vals); >> } >> >> /** >> diff --git a/drivers/iio/industrialio-event.c >> b/drivers/iio/industrialio-event.c >> index ea6e06b..1b4f31b 100644 >> --- a/drivers/iio/industrialio-event.c >> +++ b/drivers/iio/industrialio-event.c >> @@ -270,7 +270,7 @@ static ssize_t iio_ev_value_show(struct device *dev, >> { >> struct iio_dev *indio_dev = dev_to_iio_dev(dev); >> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr); >> - int val, val2; >> + int val, val2, val_arr[2]; >> int ret; >> >> ret = indio_dev->info->read_event_value(indio_dev, >> @@ -279,7 +279,9 @@ static ssize_t iio_ev_value_show(struct device *dev, >> &val, &val2); >> if (ret < 0) >> return ret; >> - return iio_format_value(buf, ret, val, val2); >> + val_arr[0] = val; >> + val_arr[1] = val2; >> + return iio_format_value(buf, ret, 2, val_arr); >> } >> >> static ssize_t iio_ev_value_store(struct device *dev, >> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c >> index 0cf5f8e..75e5386 100644 >> --- a/drivers/iio/inkern.c >> +++ b/drivers/iio/inkern.c >> @@ -417,12 +417,24 @@ static int iio_channel_read(struct iio_channel >> *chan, int *val, int *val2, >> enum iio_chan_info_enum info) >> { >> int unused; >> + int vals[INDIO_MAX_RAW_ELEMENTS]; >> + int ret; >> + int val_len = 2; >> >> if (val2 == NULL) >> val2 = &unused; >> >> - return chan->indio_dev->info->read_raw(chan->indio_dev, >> chan->channel, >> - val, val2, info); >> + if (chan->indio_dev->info->read_raw_multi) { >> + ret = chan->indio_dev->info->read_raw_multi(chan->indio_dev, >> + chan->channel, INDIO_MAX_RAW_ELEMENTS, >> + vals, &val_len, info); >> + *val = vals[0]; >> + *val2 = vals[1]; >> + } else >> + ret = chan->indio_dev->info->read_raw(chan->indio_dev, >> + chan->channel, val, val2, info); >> + >> + return ret; >> } >> >> int iio_read_channel_raw(struct iio_channel *chan, int *val) >> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h >> index 5f2d00e..5629c92 100644 >> --- a/include/linux/iio/iio.h >> +++ b/include/linux/iio/iio.h >> @@ -288,6 +288,8 @@ static inline s64 iio_get_time_ns(void) >> #define INDIO_ALL_BUFFER_MODES \ >> (INDIO_BUFFER_TRIGGERED | INDIO_BUFFER_HARDWARE) >> >> +#define INDIO_MAX_RAW_ELEMENTS 4 >> + >> struct iio_trigger; /* forward declaration */ >> struct iio_dev; >> >> @@ -302,6 +304,14 @@ struct iio_dev; >> * the channel in question. Return value will specify the >> * type of value returned by the device. val and val2 will >> * contain the elements making up the returned value. >> + * @read_raw_multi: function to return values from the device. >> + * mask specifies which value. Note 0 means a reading of > This note 0 bit wants to go now it is much more explicit in the way > the code > works, but leave it here for now and we can tidy up both this and the > read_raw > callback at the same time. >> + * the channel in question. Return value will specify the >> + * type of value returned by the device. vals pointer >> + * contain the elements making up the returned value. >> + * max_len specifies maximum number of elements >> + * vals pointer can contain. val_len is used to return >> + * length of valid elements in vals. >> * @write_raw: function to write a value to the device. >> * Parameters are the same as for read_raw. >> * @write_raw_get_fmt: callback function to query the expected >> @@ -328,6 +338,13 @@ struct iio_info { >> int *val2, >> long mask); >> >> + int (*read_raw_multi)(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *chan, >> + int max_len, >> + int *vals, >> + int *val_len, >> + long mask); >> + >> int (*write_raw)(struct iio_dev *indio_dev, >> struct iio_chan_spec const *chan, >> int val, >> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h >> index 084d882..a13c224 100644 >> --- a/include/linux/iio/types.h >> +++ b/include/linux/iio/types.h >> @@ -79,6 +79,7 @@ enum iio_event_direction { >> #define IIO_VAL_INT_PLUS_MICRO 2 >> #define IIO_VAL_INT_PLUS_NANO 3 >> #define IIO_VAL_INT_PLUS_MICRO_DB 4 >> +#define IIO_VAL_INT_MULTIPLE 5 >> #define IIO_VAL_FRACTIONAL 10 >> #define IIO_VAL_FRACTIONAL_LOG2 11 >> >> > > -- > 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 >
| |