On Mon, Oct 10, 2016 at 4:03 PM, Eva Rachel Retuya <eraretuya@xxxxxxxxx> wrote: > On Sun, Oct 09, 2016 at 07:36:45PM +0100, Jonathan Cameron wrote: >> >> >> On 9 October 2016 19:26:52 BST, sayli karnik <karniksayli1995@xxxxxxxxx> wrote: >> >Attributes that were once privately defined become standard with time >> >and >> >hence a special global define is used. Hence update driver ad7152 to >> >use >> >IIO_CHAN_INFO_SAMP_FREQ which is a global define instead of >> >IIO_DEV_ATTR_SAMP_FREQ. >> >Move functionality from IIO_DEV_ATTR_SAMP_FREQ attribute into >> >IIO_CHAN_INFO_SAMP_FREQ to implement the sampling_frequency attribute. >> >Modify ad7152_read_raw() and ad7152_write_raw() to allow reading and >> >writing the element as well. Also add a lock in the driver's private >> >data. >> > >> >Signed-off-by: sayli karnik <karniksayli1995@xxxxxxxxx> >> >> Sorry Sayli, >> >> I was not clear enough. The block to local lock change would make a good extra patch. >> >> It is a separate change so should be a separate patch. I have already applied v3 of the patch >> without the mlock change. >> >> https://git.kernel.org/cgit/linux/kernel/git/jic23/iio.git/log/?h=testing >> > > Good Day Sayli and Jonathan, > > While I was working on the v2 of another cdc driver I happen to check > the link above for the approach taken on using function handlers. I have > noticed that in the write_raw handler (ad7152_write_raw_samp_freq), val > was never used. > > for (i = 0; i < ARRAY_SIZE(ad7152_filter_rate_table); i++) > if (data >= ad7152_filter_rate_table[i][0]) > break; > > In place of 'data' is where I am expecting 'val' to be used. Any > comments regarding this? I apologize if I turn out to be wrong though. > Hey! Thanks Eva! I think I skipped this little bug. It should be val according to me too. Jonathan? thanks, sayli > Best, > Eva > >> Such an mlock replacing patch needs to replace all uses of mlock in the driver. Also make sure >> you initialise the mutex appropriately. >> >> Jonathan >> >> >> >--- >> >Changes in v4: >> >Replaced mlock with a local lock since mlock is intended to protect >> >'only' >> >switches between modes. Given this driver doesn't support more than one >> >mode >> >(sysfs polled reads only) >> > >> >Changes in v3: >> >Drop the unlock in ad7152_write_raw_samp_freq() and use the one at the >> >exit point >> >instead >> >Return ret immediately instead of using a goto statement in >> >ad7152_write_raw_samp_freq() >> > >> >Changes in v2: >> >Give a more detailed explanation >> >Remove null check for val in ad7152_write_raw_samp_freq() >> >Merged two stucture variable initialisations into one statement in >> >ad7152_read_raw_samp_freq() >> >Set ret to IIO_VAL_INT in ad7152_read_raw() when mask equals >> >IIO_CHAN_INFO_SAMP_FREQ and ret>=0 >> >drivers/staging/iio/cdc/ad7152.c | 117 >> >++++++++++++++++++++++----------------- >> > 1 file changed, 66 insertions(+), 51 deletions(-) >> > >> >diff --git a/drivers/staging/iio/cdc/ad7152.c >> >b/drivers/staging/iio/cdc/ad7152.c >> >index 485d0a5..0ce3849 100644 >> >--- a/drivers/staging/iio/cdc/ad7152.c >> >+++ b/drivers/staging/iio/cdc/ad7152.c >> >@@ -89,6 +89,7 @@ struct ad7152_chip_info { >> > */ >> > u8 filter_rate_setup; >> > u8 setup[2]; >> >+ struct mutex state_lock; /* protect hardware state */ >> > }; >> > >> > static inline ssize_t ad7152_start_calib(struct device *dev, >> >@@ -165,63 +166,12 @@ static const unsigned char >> >ad7152_filter_rate_table[][2] = { >> > {200, 5 + 1}, {50, 20 + 1}, {20, 50 + 1}, {17, 60 + 1}, >> > }; >> > >> >-static ssize_t ad7152_show_filter_rate_setup(struct device *dev, >> >- struct device_attribute *attr, >> >- char *buf) >> >-{ >> >- struct iio_dev *indio_dev = dev_to_iio_dev(dev); >> >- struct ad7152_chip_info *chip = iio_priv(indio_dev); >> >- >> >- return sprintf(buf, "%d\n", >> >- ad7152_filter_rate_table[chip->filter_rate_setup][0]); >> >-} >> >- >> >-static ssize_t ad7152_store_filter_rate_setup(struct device *dev, >> >- struct device_attribute *attr, >> >- const char *buf, >> >- size_t len) >> >-{ >> >- struct iio_dev *indio_dev = dev_to_iio_dev(dev); >> >- struct ad7152_chip_info *chip = iio_priv(indio_dev); >> >- u8 data; >> >- int ret, i; >> >- >> >- ret = kstrtou8(buf, 10, &data); >> >- if (ret < 0) >> >- return ret; >> >- >> >- for (i = 0; i < ARRAY_SIZE(ad7152_filter_rate_table); i++) >> >- if (data >= ad7152_filter_rate_table[i][0]) >> >- break; >> >- >> >- if (i >= ARRAY_SIZE(ad7152_filter_rate_table)) >> >- i = ARRAY_SIZE(ad7152_filter_rate_table) - 1; >> >- >> >- mutex_lock(&indio_dev->mlock); >> >- ret = i2c_smbus_write_byte_data(chip->client, >> >- AD7152_REG_CFG2, AD7152_CFG2_OSR(i)); >> >- if (ret < 0) { >> >- mutex_unlock(&indio_dev->mlock); >> >- return ret; >> >- } >> >- >> >- chip->filter_rate_setup = i; >> >- mutex_unlock(&indio_dev->mlock); >> >- >> >- return len; >> >-} >> >- >> >-static IIO_DEV_ATTR_SAMP_FREQ(S_IRUGO | S_IWUSR, >> >- ad7152_show_filter_rate_setup, >> >- ad7152_store_filter_rate_setup); >> >- >> > static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("200 50 20 17"); >> > >> > static IIO_CONST_ATTR(in_capacitance_scale_available, >> > "0.000061050 0.000030525 0.000015263 0.000007631"); >> > >> > static struct attribute *ad7152_attributes[] = { >> >- &iio_dev_attr_sampling_frequency.dev_attr.attr, >> > &iio_dev_attr_in_capacitance0_calibbias_calibration.dev_attr.attr, >> > &iio_dev_attr_in_capacitance1_calibbias_calibration.dev_attr.attr, >> > &iio_dev_attr_in_capacitance0_calibscale_calibration.dev_attr.attr, >> >@@ -247,6 +197,50 @@ static const int ad7152_scale_table[] = { >> > 30525, 7631, 15263, 61050 >> > }; >> > >> >+/** >> >+ * read_raw handler for IIO_CHAN_INFO_SAMP_FREQ >> >+ * >> >+ * lock must be held >> >+ **/ >> >+static int ad7152_read_raw_samp_freq(struct device *dev, int *val) >> >+{ >> >+ struct ad7152_chip_info *chip = iio_priv(dev_to_iio_dev(dev)); >> >+ >> >+ *val = ad7152_filter_rate_table[chip->filter_rate_setup][0]; >> >+ >> >+ return 0; >> >+} >> >+ >> >+/** >> >+ * write_raw handler for IIO_CHAN_INFO_SAMP_FREQ >> >+ * >> >+ * lock must be held >> >+ **/ >> >+static int ad7152_write_raw_samp_freq(struct device *dev, int val) >> >+{ >> >+ struct iio_dev *indio_dev = dev_to_iio_dev(dev); >> >+ struct ad7152_chip_info *chip = iio_priv(indio_dev); >> >+ u8 data; >> >+ int ret, i; >> >+ >> >+ for (i = 0; i < ARRAY_SIZE(ad7152_filter_rate_table); i++) >> >+ if (data >= ad7152_filter_rate_table[i][0]) >> >+ break; >> >+ >> >+ if (i >= ARRAY_SIZE(ad7152_filter_rate_table)) >> >+ i = ARRAY_SIZE(ad7152_filter_rate_table) - 1; >> >+ >> >+ mutex_lock(&chip->state_lock); >> >+ ret = i2c_smbus_write_byte_data(chip->client, >> >+ AD7152_REG_CFG2, AD7152_CFG2_OSR(i)); >> >+ if (ret < 0) >> >+ return ret; >> >+ >> >+ chip->filter_rate_setup = i; >> >+ mutex_unlock(&chip->state_lock); >> >+ >> >+ return ret; >> >+} >> > static int ad7152_write_raw(struct iio_dev *indio_dev, >> > struct iio_chan_spec const *chan, >> > int val, >> >@@ -309,6 +303,16 @@ static int ad7152_write_raw(struct iio_dev >> >*indio_dev, >> > >> > ret = 0; >> > break; >> >+ case IIO_CHAN_INFO_SAMP_FREQ: >> >+ if (val2) >> >+ return -EINVAL; >> >+ >> >+ ret = ad7152_write_raw_samp_freq(&indio_dev->dev, val); >> >+ if (ret < 0) >> >+ goto out; >> >+ >> >+ ret = 0; >> >+ break; >> > default: >> > ret = -EINVAL; >> > } >> >@@ -403,6 +407,13 @@ static int ad7152_read_raw(struct iio_dev >> >*indio_dev, >> > >> > ret = IIO_VAL_INT_PLUS_NANO; >> > break; >> >+ case IIO_CHAN_INFO_SAMP_FREQ: >> >+ ret = ad7152_read_raw_samp_freq(&indio_dev->dev, val); >> >+ if (ret < 0) >> >+ goto out; >> >+ >> >+ ret = IIO_VAL_INT; >> >+ break; >> > default: >> > ret = -EINVAL; >> > } >> >@@ -440,6 +451,7 @@ static const struct iio_chan_spec ad7152_channels[] >> >= { >> > BIT(IIO_CHAN_INFO_CALIBSCALE) | >> > BIT(IIO_CHAN_INFO_CALIBBIAS) | >> > BIT(IIO_CHAN_INFO_SCALE), >> >+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), >> > }, { >> > .type = IIO_CAPACITANCE, >> > .differential = 1, >> >@@ -450,6 +462,7 @@ static const struct iio_chan_spec ad7152_channels[] >> >= { >> > BIT(IIO_CHAN_INFO_CALIBSCALE) | >> > BIT(IIO_CHAN_INFO_CALIBBIAS) | >> > BIT(IIO_CHAN_INFO_SCALE), >> >+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), >> > }, { >> > .type = IIO_CAPACITANCE, >> > .indexed = 1, >> >@@ -458,6 +471,7 @@ static const struct iio_chan_spec ad7152_channels[] >> >= { >> > BIT(IIO_CHAN_INFO_CALIBSCALE) | >> > BIT(IIO_CHAN_INFO_CALIBBIAS) | >> > BIT(IIO_CHAN_INFO_SCALE), >> >+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), >> > }, { >> > .type = IIO_CAPACITANCE, >> > .differential = 1, >> >@@ -468,6 +482,7 @@ static const struct iio_chan_spec ad7152_channels[] >> >= { >> > BIT(IIO_CHAN_INFO_CALIBSCALE) | >> > BIT(IIO_CHAN_INFO_CALIBBIAS) | >> > BIT(IIO_CHAN_INFO_SCALE), >> >+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), >> > } >> > }; >> > /* >> >> -- >> Sent from my Android device with K-9 Mail. Please excuse my brevity. >> -- >> 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 -- 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
