Jonathan Cameron wrote on 2010-10-07: > On 10/07/10 13:58, michael.hennerich@xxxxxxxxxx wrote: >> From: Michael Hennerich <michael.hennerich@xxxxxxxxxx> >> >> drop in_precision in favor of new in_type add sign and storagebits >> to struct ad799x_chip_info properly mask the results based on >> ad799x_chip_info:bits > This also fixes the bug from the scan elements move (as a side effect). > Normally I'd say that needed flagging up but seeing how short a time > the driver has been in place, the chances of it having bitten anyone > are pretty low! Still if you re roll the patch it is probably worth > adding a mention. > > Couple of minor queries below. >> >> Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx> >> --- >> drivers/staging/iio/adc/ad799x.h | 4 ++- >> drivers/staging/iio/adc/ad799x_core.c | 42 >> +++++++++++++++++++++++--------- drivers/staging/iio/adc/ad799x_ring.c >> | 2 +- 3 files changed, 34 insertions(+), 14 deletions(-) >> diff --git a/drivers/staging/iio/adc/ad799x.h >> b/drivers/staging/iio/adc/ad799x.h >> index 2325929..f023212 100644 >> --- a/drivers/staging/iio/adc/ad799x.h >> +++ b/drivers/staging/iio/adc/ad799x.h >> @@ -97,7 +97,9 @@ struct ad799x_state; struct ad799x_chip_info { >> u8 num_inputs; >> u8 bits; >> - u16 int_vref_mv; >> + u8 storagebits; >> + char sign; >> + unsigned int int_vref_mv; > Why the type change? If it is needed, please submit as as separate > patch as it definitely isn't conceptually linked to the rest we have > here. Not actually needed - I revert. >> bool monitor_mode; >> u16 default_config; >> struct attribute_group *dev_attrs; >> diff --git a/drivers/staging/iio/adc/ad799x_core.c >> b/drivers/staging/iio/adc/ad799x_core.c >> index 2589856..a804515 100644 >> --- a/drivers/staging/iio/adc/ad799x_core.c >> +++ b/drivers/staging/iio/adc/ad799x_core.c >> @@ -123,17 +123,18 @@ static AD799X_SCAN_EL(5); static >> AD799X_SCAN_EL(6); static AD799X_SCAN_EL(7); >> >> -static ssize_t ad799x_show_precision(struct device *dev, >> +static ssize_t ad799x_show_type(struct device *dev, >> struct device_attribute *attr, >> char *buf) >> { >> - struct iio_dev *dev_info = dev_get_drvdata(dev); >> - struct ad799x_state *st = iio_dev_get_devdata(dev_info); >> - return sprintf(buf, "%d\n", st->chip_info->bits); >> -} >> + struct iio_ring_buffer *ring = dev_get_drvdata(dev); >> + struct iio_dev *indio_dev = ring->indio_dev; >> + struct ad799x_state *st = indio_dev->dev_data; >> >> -static IIO_DEVICE_ATTR(in_precision, S_IRUGO, ad799x_show_precision, >> - NULL, 0); + return sprintf(buf, "%c%d/%d\n", >> st->chip_info->sign, + st->chip_info->bits, >> st->chip_info->storagebits); } static +IIO_DEVICE_ATTR(in_type, >> S_IRUGO, ad799x_show_type, NULL, 0); >> >> static int ad7991_5_9_set_scan_mode(struct ad799x_state *st, > unsigned >> mask) { @@ -185,6 +186,7 @@ static ssize_t >> ad799x_read_single_channel(struct device *dev, >> data = ret = ad799x_single_channel_from_ring(st, mask); >> if (ret < 0) >> goto error_ret; >> + > Why the new line? Got there by accident >> ret = 0; >> } else { >> switch (st->id) { >> @@ -211,11 +213,11 @@ static ssize_t > ad799x_read_single_channel(struct device *dev, >> if (ret < 0) >> goto error_ret; >> - data = rxbuf[0] & 0xFFF; >> + data = rxbuf[0]; >> } >> >> /* Pretty print the result */ >> - len = sprintf(buf, "%u\n", data); >> + len = sprintf(buf, "%u\n", data & ((1 << (st->chip_info->bits)) - >> +1)); >> >> error_ret: >> mutex_unlock(&dev_info->mlock); >> @@ -473,7 +475,7 @@ static struct attribute > *ad7991_5_9_3_4_scan_el_attrs[] = { >> &iio_const_attr_in2_index.dev_attr.attr, >> &iio_scan_el_in3.dev_attr.attr, >> &iio_const_attr_in3_index.dev_attr.attr, >> - &iio_dev_attr_in_precision.dev_attr.attr, >> + &iio_dev_attr_in_type.dev_attr.attr, >> NULL, >> }; >> @@ -499,7 +501,7 @@ static struct attribute *ad7992_scan_el_attrs[] >> = > { >> &iio_const_attr_in0_index.dev_attr.attr, >> &iio_scan_el_in1.dev_attr.attr, >> &iio_const_attr_in1_index.dev_attr.attr, >> - &iio_dev_attr_in_precision.dev_attr.attr, >> + &iio_dev_attr_in_type.dev_attr.attr, >> NULL, >> }; >> @@ -543,7 +545,7 @@ static struct attribute >> *ad7997_8_scan_el_attrs[] > = { >> &iio_const_attr_in6_index.dev_attr.attr, >> &iio_scan_el_in7.dev_attr.attr, >> &iio_const_attr_in7_index.dev_attr.attr, >> - &iio_dev_attr_in_precision.dev_attr.attr, >> + &iio_dev_attr_in_type.dev_attr.attr, >> NULL, >> }; >> @@ -671,6 +673,8 @@ static const struct ad799x_chip_info > ad799x_chip_info_tbl[] = { >> [ad7991] = { >> .num_inputs = 4, >> .bits = 12, >> + .storagebits = 16, > Why have storage bits if it is always 16? It's also a characteristic of > the buffer elements of the driver rather than the chip so it doesn't > really make sense. We use storagebits in in_type. In a different driver that I'm currently preparing for RFC. I also use storagebits to characterize the buffer elements. But here you're right it doesn't make too much sense. >I'd prefer to see it generated as a round up integer > multiple of 8 from the bits value. Well that doesn't always work. There are devices that are 8-bit but require an 16-bit buffer. I think you remember the need for the in_type shift extension. They are type u8/16>>4 > That way the driver will nicely > extend to 8 bit or 17bit+ devices should any exist. >> + .sign = IIO_SCAN_EL_TYPE_UNSIGNED, >> .int_vref_mv = 4096, >> .dev_attrs = &ad7991_5_9_3_4_dev_attr_group, >> .scan_attrs = &ad7991_5_9_3_4_scan_el_group, @@ -679,6 > +683,8 @@ >> static const struct ad799x_chip_info ad799x_chip_info_tbl[] = { >> [ad7995] = { >> .num_inputs = 4, >> .bits = 10, >> + .storagebits = 16, >> + .sign = IIO_SCAN_EL_TYPE_UNSIGNED, >> .int_vref_mv = 1024, >> .dev_attrs = &ad7991_5_9_3_4_dev_attr_group, >> .scan_attrs = &ad7991_5_9_3_4_scan_el_group, @@ -687,6 > +693,8 @@ >> static const struct ad799x_chip_info ad799x_chip_info_tbl[] = { >> [ad7999] = { >> .num_inputs = 4, >> .bits = 10, >> + .storagebits = 16, >> + .sign = IIO_SCAN_EL_TYPE_UNSIGNED, >> .int_vref_mv = 1024, >> .dev_attrs = &ad7991_5_9_3_4_dev_attr_group, >> .scan_attrs = &ad7991_5_9_3_4_scan_el_group, @@ -695,6 > +703,8 @@ >> static const struct ad799x_chip_info ad799x_chip_info_tbl[] = { >> [ad7992] = { >> .num_inputs = 2, >> .bits = 12, >> + .storagebits = 16, >> + .sign = IIO_SCAN_EL_TYPE_UNSIGNED, >> .int_vref_mv = 4096, >> .monitor_mode = true, >> .default_config = AD7998_ALERT_EN, @@ -706,6 +716,8 @@ static >> const struct ad799x_chip_info > ad799x_chip_info_tbl[] = { >> [ad7993] = { >> .num_inputs = 4, >> .bits = 10, >> + .storagebits = 16, >> + .sign = IIO_SCAN_EL_TYPE_UNSIGNED, >> .int_vref_mv = 1024, >> .monitor_mode = true, >> .default_config = AD7998_ALERT_EN, @@ -717,6 +729,8 @@ static >> const struct ad799x_chip_info > ad799x_chip_info_tbl[] = { >> [ad7994] = { >> .num_inputs = 4, >> .bits = 12, >> + .storagebits = 16, >> + .sign = IIO_SCAN_EL_TYPE_UNSIGNED, >> .int_vref_mv = 4096, >> .monitor_mode = true, >> .default_config = AD7998_ALERT_EN, @@ -728,6 +742,8 @@ static >> const struct ad799x_chip_info > ad799x_chip_info_tbl[] = { >> [ad7997] = { >> .num_inputs = 8, >> .bits = 10, >> + .storagebits = 16, >> + .sign = IIO_SCAN_EL_TYPE_UNSIGNED, >> .int_vref_mv = 1024, >> .monitor_mode = true, >> .default_config = AD7998_ALERT_EN, @@ -739,6 +755,8 @@ static >> const struct ad799x_chip_info > ad799x_chip_info_tbl[] = { >> [ad7998] = { >> .num_inputs = 8, >> .bits = 12, >> + .storagebits = 16, >> + .sign = IIO_SCAN_EL_TYPE_UNSIGNED, >> .int_vref_mv = 4096, >> .monitor_mode = true, >> .default_config = AD7998_ALERT_EN, diff --git >> a/drivers/staging/iio/adc/ad799x_ring.c >> b/drivers/staging/iio/adc/ad799x_ring.c >> index d0217f8..c6871fa 100644 >> --- a/drivers/staging/iio/adc/ad799x_ring.c >> +++ b/drivers/staging/iio/adc/ad799x_ring.c >> @@ -53,7 +53,7 @@ int ad799x_single_channel_from_ring(struct > ad799x_state *st, long mask) >> mask >>= 1; >> } >> - ret = be16_to_cpu(ring_data[count]) & 0xFFF; >> + ret = be16_to_cpu(ring_data[count]); >> >> error_free_ring_data: >> kfree(ring_data); Greetings, Michael Analog Devices GmbH Wilhelm-Wagenfeld-Str. 6 80807 Muenchen Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 4036 Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif -- 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