Skip to contentSkip to conversation

staging:iio:ad2s1210: Rework read/write operation for fclkin and fexin

PATCH Superseded Rodrigo Siqueira 2 participants 8 messages Mar 12, 2018 – Mar 13, 2018

Subsystems (4)
Staging IIO Analog Devices IIO drivers Staging - industrial io

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Rodrigo Siqueira

author
#0 lore raw

cover staging:iio:ad2s1210: Rework read/write operation for fclkin and fexin

This patchset changes the way that frequency of clock input (fclkin) and the excitation frequency (fexcit) are exposed to the userspace. The original code uses the IIO_DEVICE_ATTR to export the configuration of fclkin and fexcit to the userspace which is not in agreement with the current ABI. This patchset adds one channel per clock configuration, updating the read_raw function by adding a handler for the new channels, and introduces the write_raw function. Furthermore, this patchset removes the legacy code responsible for the read and write operation related to fclkin and fexcit. Finally, this patchset uses parts of the original code for handling frequency configuration as an attempt to avoid problems.

Rodrigo Siqueira (3):
staging:iio:ad2s1210: Add channel for fclkin and fexcit staging:iio:ad2s1210: Add frequency handler in read_raw staging:iio:ad2s1210: Add write_raw to handle frequency

 drivers/staging/iio/resolver/ad2s1210.c | 201 ++++++++++++++++----------------
 1 file changed, 102 insertions(+), 99 deletions(-)
Signature · 1 line
2.16.2
author
#1 lore raw

PATCH 1/3 staging:iio:ad2s1210: Add channel for fclkin and fexcit

The ad2s1210 does not contain any channel for the fclkin and fexcit. As a result, it uses IIO_DEVICE_ATTR to expose this information. This patch adds one channel for fclkin and another for fexcit. It also adds an enum to easily address the correct channel.

Signed-off-by Rodrigo Siqueira <[email protected]>
---
 drivers/staging/iio/resolver/ad2s1210.c | 43 ++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 14 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +29 -14
index ac13b99bd9cb..28c3fd439663 100644
@@ -67,6 +67,11 @@ enum ad2s1210_mode {
 	MOD_RESERVED,
 };
 
+enum ad2s1210_frequency_channel {
+	FCLKIN = 0,
+	FEXCIT,
+};
+
 static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 };
 
 struct ad2s1210_state {

@@ -88,6 +93,30 @@ static const int ad2s1210_mode_vals[4][2] = {
 	[MOD_CONFIG] = { 1, 0 },
 };
 
+static const struct iio_chan_spec ad2s1210_channels[] = {
+	{
+		.type = IIO_ANGL,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_ANGL_VEL,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_CHAN_INFO_FREQUENCY,
+		.indexed = 1,
+		.channel = FCLKIN,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		.type = IIO_CHAN_INFO_FREQUENCY,
+		.indexed = 1,
+		.channel = FEXCIT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	},
+};
+
 static inline void ad2s1210_set_mode(enum ad2s1210_mode mode,
 				     struct ad2s1210_state *st)
 {

@@ -552,20 +581,6 @@ static IIO_DEVICE_ATTR(lot_low_thrd, 0644,
 		       ad2s1210_show_reg, ad2s1210_store_reg,
 		       AD2S1210_REG_LOT_LOW_THRD);
 
-static const struct iio_chan_spec ad2s1210_channels[] = {
-	{
-		.type = IIO_ANGL,
-		.indexed = 1,
-		.channel = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-	}, {
-		.type = IIO_ANGL_VEL,
-		.indexed = 1,
-		.channel = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-	}
-};
-
 static struct attribute *ad2s1210_attributes[] = {
 	&iio_dev_attr_fclkin.dev_attr.attr,
 	&iio_dev_attr_fexcit.dev_attr.attr,
Signature · 1 line
2.16.2
author
#2 lore raw

PATCH 2/3 staging:iio:ad2s1210: Add frequency handler in read_raw

In reply to Rodrigo Siqueira

Read data from fclkin and fexcit does not utilize the ad2s1210_read_raw function. This patch, append the required handler in the ad2s1210_read_raw to return the correct value. Also, this patch removes the legacy code related to the read function

Signed-off-by Rodrigo Siqueira <[email protected]>
---
 drivers/staging/iio/resolver/ad2s1210.c | 45 +++++++++++++++------------------
 1 file changed, 20 insertions(+), 25 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +20 -25
index 28c3fd439663..27a42ed10fcd 100644
@@ -210,15 +210,6 @@ static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
 	return ad2s1210_config_write(st, 0x0);
 }
 
-static ssize_t ad2s1210_show_fclkin(struct device *dev,
-				    struct device_attribute *attr,
-				    char *buf)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-
-	return sprintf(buf, "%u\n", st->fclkin);
-}
-
 static ssize_t ad2s1210_store_fclkin(struct device *dev,
 				     struct device_attribute *attr,
 				     const char *buf,

@@ -249,15 +240,6 @@ static ssize_t ad2s1210_store_fclkin(struct device *dev,
 	return ret < 0 ? ret : len;
 }
 
-static ssize_t ad2s1210_show_fexcit(struct device *dev,
-				    struct device_attribute *attr,
-				    char *buf)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-
-	return sprintf(buf, "%u\n", st->fexcit);
-}
-
 static ssize_t ad2s1210_store_fexcit(struct device *dev,
 				     struct device_attribute *attr,
 				     const char *buf, size_t len)

@@ -486,7 +468,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int *val,
 			     int *val2,
-			     long m)
+			     long mask)
 {
 	struct ad2s1210_state *st = iio_priv(indio_dev);
 	u16 negative;

@@ -535,23 +517,36 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 		*val = vel;
 		ret = IIO_VAL_INT;
 		break;
+	case IIO_CHAN_INFO_FREQUENCY:
+		ret = IIO_VAL_INT;
+		switch (chan->channel) {
+		case FCLKIN:
+			*val = st->fclkin;
+			goto unlock_mutex;
+		case FEXCIT:
+			*val = st->fexcit;
+			goto unlock_mutex;
+		default:
+			ret = -EINVAL;
+			goto unlock_mutex;
+		}
+		break;
 	default:
-		mutex_unlock(&st->lock);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto unlock_mutex;
 	}
 
 error_ret:
 	gpio_set_value(st->pdata->sample, 1);
 	/* delay (2 * tck + 20) nano seconds */
 	udelay(1);
+unlock_mutex:
 	mutex_unlock(&st->lock);
 	return ret;
 }
 
-static IIO_DEVICE_ATTR(fclkin, 0644,
-		       ad2s1210_show_fclkin, ad2s1210_store_fclkin, 0);
-static IIO_DEVICE_ATTR(fexcit, 0644,
-		       ad2s1210_show_fexcit,	ad2s1210_store_fexcit, 0);
+static IIO_DEVICE_ATTR(fclkin, 0644, NULL, ad2s1210_store_fclkin, 0);
+static IIO_DEVICE_ATTR(fexcit, 0644, NULL, ad2s1210_store_fexcit, 0);
 static IIO_DEVICE_ATTR(control, 0644,
 		       ad2s1210_show_control, ad2s1210_store_control, 0);
 static IIO_DEVICE_ATTR(bits, 0644,
Signature · 1 line
2.16.2
author
#3 lore raw

PATCH 3/3 staging:iio:ad2s1210: Add write_raw to handle frequency

In reply to Rodrigo Siqueira

The write interface of AD2S1210 utilizes IIO_DEVICE_ATTR, which violate the official IIO ABI. This patch, add the write_raw function responsible for handling the fclkin and fexcit channel; also it removes the use of IIO_DEVICE_ATTR for fclkin and fexcit.

Signed-off-by Rodrigo Siqueira <[email protected]>
---
 drivers/staging/iio/resolver/ad2s1210.c | 117 +++++++++++++++-----------------
 1 file changed, 55 insertions(+), 62 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +55 -62
index 27a42ed10fcd..ea6ade4e563c 100644
@@ -60,6 +60,8 @@
 
 #define AD2S1210_DEF_EXCIT	10000
 
+#define ERROR_MESSAGE "ad2s1210: %s out of range\n"
+
 enum ad2s1210_mode {
 	MOD_POS = 0,
 	MOD_VEL,

@@ -210,64 +212,6 @@ static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
 	return ad2s1210_config_write(st, 0x0);
 }
 
-static ssize_t ad2s1210_store_fclkin(struct device *dev,
-				     struct device_attribute *attr,
-				     const char *buf,
-				     size_t len)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-	unsigned int fclkin;
-	int ret;
-
-	ret = kstrtouint(buf, 10, &fclkin);
-	if (ret)
-		return ret;
-	if (fclkin < AD2S1210_MIN_CLKIN || fclkin > AD2S1210_MAX_CLKIN) {
-		dev_err(dev, "ad2s1210: fclkin out of range\n");
-		return -EINVAL;
-	}
-
-	mutex_lock(&st->lock);
-	st->fclkin = fclkin;
-
-	ret = ad2s1210_update_frequency_control_word(st);
-	if (ret < 0)
-		goto error_ret;
-	ret = ad2s1210_soft_reset(st);
-error_ret:
-	mutex_unlock(&st->lock);
-
-	return ret < 0 ? ret : len;
-}
-
-static ssize_t ad2s1210_store_fexcit(struct device *dev,
-				     struct device_attribute *attr,
-				     const char *buf, size_t len)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-	unsigned int fexcit;
-	int ret;
-
-	ret = kstrtouint(buf, 10, &fexcit);
-	if (ret < 0)
-		return ret;
-	if (fexcit < AD2S1210_MIN_EXCIT || fexcit > AD2S1210_MAX_EXCIT) {
-		dev_err(dev,
-			"ad2s1210: excitation frequency out of range\n");
-		return -EINVAL;
-	}
-	mutex_lock(&st->lock);
-	st->fexcit = fexcit;
-	ret = ad2s1210_update_frequency_control_word(st);
-	if (ret < 0)
-		goto error_ret;
-	ret = ad2s1210_soft_reset(st);
-error_ret:
-	mutex_unlock(&st->lock);
-
-	return ret < 0 ? ret : len;
-}
-
 static ssize_t ad2s1210_show_control(struct device *dev,
 				     struct device_attribute *attr,
 				     char *buf)

@@ -545,8 +489,58 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static IIO_DEVICE_ATTR(fclkin, 0644, NULL, ad2s1210_store_fclkin, 0);
-static IIO_DEVICE_ATTR(fexcit, 0644, NULL, ad2s1210_store_fexcit, 0);
+static int ad2s1210_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, int val,
+			      int val2, long mask)
+{
+	struct ad2s1210_state *st = iio_priv(indio_dev);
+	unsigned int clk = val;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_FREQUENCY:
+		switch (chan->channel) {
+		case FCLKIN:
+			if (clk < AD2S1210_MIN_CLKIN ||
+			    clk > AD2S1210_MAX_CLKIN) {
+				dev_err(&indio_dev->dev, ERROR_MESSAGE,
+					"fclkin");
+				ret = -EINVAL;
+				goto error_ret;
+			}
+			mutex_lock(&st->lock);
+			st->fclkin = clk;
+			break;
+		case FEXCIT:
+			if (clk < AD2S1210_MIN_EXCIT ||
+			    clk > AD2S1210_MAX_EXCIT) {
+				dev_err(&indio_dev->dev, ERROR_MESSAGE,
+					"excitation frequency");
+				ret = -EINVAL;
+				goto error_ret;
+			}
+			mutex_lock(&st->lock);
+			st->fexcit = clk;
+			break;
+		default:
+			ret = -EINVAL;
+			goto error_ret;
+		}
+		break;
+	default:
+		ret = -EINVAL;
+		goto error_ret;
+	}
+	ret = ad2s1210_update_frequency_control_word(st);
+	if (ret < 0)
+		goto error_unlock_mutex;
+	ret = ad2s1210_soft_reset(st);
+error_unlock_mutex:
+	mutex_unlock(&st->lock);
+error_ret:
+	return ret;
+}
+
 static IIO_DEVICE_ATTR(control, 0644,
 		       ad2s1210_show_control, ad2s1210_store_control, 0);
 static IIO_DEVICE_ATTR(bits, 0644,

@@ -577,8 +571,6 @@ static IIO_DEVICE_ATTR(lot_low_thrd, 0644,
 		       AD2S1210_REG_LOT_LOW_THRD);
 
 static struct attribute *ad2s1210_attributes[] = {
-	&iio_dev_attr_fclkin.dev_attr.attr,
-	&iio_dev_attr_fexcit.dev_attr.attr,
 	&iio_dev_attr_control.dev_attr.attr,
 	&iio_dev_attr_bits.dev_attr.attr,
 	&iio_dev_attr_fault.dev_attr.attr,

@@ -635,6 +627,7 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
 
 static const struct iio_info ad2s1210_info = {
 	.read_raw = ad2s1210_read_raw,
+	.write_raw = ad2s1210_write_raw,
 	.attrs = &ad2s1210_attribute_group,
 };
Signature · 1 line
2.16.2
#4 lore raw

Re: [PATCH 3/3] staging:iio:ad2s1210: Add write_raw to handle frequency

Quoted 15 lines · Rodrigo Siqueira

The write interface of AD2S1210 utilizes IIO_DEVICE_ATTR, which violate the official IIO ABI. This patch, add the write_raw function responsible for handling the fclkin and fexcit channel; also it removes the use of IIO_DEVICE_ATTR for fclkin and fexcit.

Signed-off-by Rodrigo Siqueira <[email protected]>
---
 drivers/staging/iio/resolver/ad2s1210.c | 117 +++++++++++++++-----------------
 1 file changed, 55 insertions(+), 62 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +0 -0
index 27a42ed10fcd..ea6ade4e563c 100644

drivers/staging/iio/resolver/ad2s1210.c line 64

Quoted 5 lines
drivers/staging/iio/resolver/ad2s1210.c +2 -0
@@ -60,6 +60,8 @@
 
 #define AD2S1210_DEF_EXCIT	10000
 
+#define ERROR_MESSAGE "ad2s1210: %s out of range\n"
+

Don't make this a define. That's horrible. It's a pointless layer of abstraction.

Quoted 75 lines
drivers/staging/iio/resolver/ad2s1210.c +1 -60
 enum ad2s1210_mode {
 	MOD_POS = 0,
 	MOD_VEL,

@@ -210,64 +212,6 @@ static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
 	return ad2s1210_config_write(st, 0x0);
 }
 
-static ssize_t ad2s1210_store_fclkin(struct device *dev,
-				     struct device_attribute *attr,
-				     const char *buf,
-				     size_t len)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-	unsigned int fclkin;
-	int ret;
-
-	ret = kstrtouint(buf, 10, &fclkin);
-	if (ret)
-		return ret;
-	if (fclkin < AD2S1210_MIN_CLKIN || fclkin > AD2S1210_MAX_CLKIN) {
-		dev_err(dev, "ad2s1210: fclkin out of range\n");
-		return -EINVAL;
-	}
-
-	mutex_lock(&st->lock);
-	st->fclkin = fclkin;
-
-	ret = ad2s1210_update_frequency_control_word(st);
-	if (ret < 0)
-		goto error_ret;
-	ret = ad2s1210_soft_reset(st);
-error_ret:
-	mutex_unlock(&st->lock);
-
-	return ret < 0 ? ret : len;
-}
-
-static ssize_t ad2s1210_store_fexcit(struct device *dev,
-				     struct device_attribute *attr,
-				     const char *buf, size_t len)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-	unsigned int fexcit;
-	int ret;
-
-	ret = kstrtouint(buf, 10, &fexcit);
-	if (ret < 0)
-		return ret;
-	if (fexcit < AD2S1210_MIN_EXCIT || fexcit > AD2S1210_MAX_EXCIT) {
-		dev_err(dev,
-			"ad2s1210: excitation frequency out of range\n");
-		return -EINVAL;
-	}
-	mutex_lock(&st->lock);
-	st->fexcit = fexcit;
-	ret = ad2s1210_update_frequency_control_word(st);
-	if (ret < 0)
-		goto error_ret;
-	ret = ad2s1210_soft_reset(st);
-error_ret:
-	mutex_unlock(&st->lock);
-
-	return ret < 0 ? ret : len;
-}
-
 static ssize_t ad2s1210_show_control(struct device *dev,
 				     struct device_attribute *attr,
 				     char *buf)

@@ -545,8 +489,58 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static IIO_DEVICE_ATTR(fclkin, 0644, NULL, ad2s1210_store_fclkin, 0);
-static IIO_DEVICE_ATTR(fexcit, 0644, NULL, ad2s1210_store_fexcit, 0);
+static int ad2s1210_write_raw(struct iio_dev *indio_dev,

drivers/staging/iio/resolver/ad2s1210.c static int ad2s1210_read_raw(struct iio_dev *indio_dev, · line 502

Quoted 10 lines
drivers/staging/iio/resolver/ad2s1210.c +10 -0
+			      struct iio_chan_spec const *chan, int val,
+			      int val2, long mask)
+{
+	struct ad2s1210_state *st = iio_priv(indio_dev);
+	unsigned int clk = val;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_FREQUENCY:
+		switch (chan->channel) {

This is a switch statement with only one case. Make it an if statement and chop out some indenting.

	if (mask != IIO_CHAN_INFO_FREQUENCY)
		return -EINVAL;

drivers/staging/iio/resolver/ad2s1210.c static int ad2s1210_read_raw(struct iio_dev *indio_dev, · line 506

Quoted 4 lines
drivers/staging/iio/resolver/ad2s1210.c +4 -0
+		case FCLKIN:
+			if (clk < AD2S1210_MIN_CLKIN ||
+			    clk > AD2S1210_MAX_CLKIN) {
+				dev_err(&indio_dev->dev, ERROR_MESSAGE,

Ah... I see why you did the ERROR_MESSAGE define, to get around the 80 character limit. Don't do that. Just go over 80 characters if you need to.

drivers/staging/iio/resolver/ad2s1210.c static int ad2s1210_read_raw(struct iio_dev *indio_dev, · line 509

Quoted 3 lines
drivers/staging/iio/resolver/ad2s1210.c +3 -0
+					"fclkin");
+				ret = -EINVAL;
+				goto error_ret;

Direct returns are better. Less chance of bugs statistically.

regards,
dan carpenter

author
#5 lore raw

Re: [PATCH 3/3] staging:iio:ad2s1210: Add write_raw to handle frequency

Quoted 115 lines · Dan Carpenter
Quoted 15 lines · Rodrigo Siqueira

The write interface of AD2S1210 utilizes IIO_DEVICE_ATTR, which violate the official IIO ABI. This patch, add the write_raw function responsible for handling the fclkin and fexcit channel; also it removes the use of IIO_DEVICE_ATTR for fclkin and fexcit.

Signed-off-by Rodrigo Siqueira <[email protected]>
---
 drivers/staging/iio/resolver/ad2s1210.c | 117 +++++++++++++++-----------------
 1 file changed, 55 insertions(+), 62 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +0 -0
index 27a42ed10fcd..ea6ade4e563c 100644

drivers/staging/iio/resolver/ad2s1210.c line 64

Quoted 5 lines
drivers/staging/iio/resolver/ad2s1210.c +2 -0
@@ -60,6 +60,8 @@
 
 #define AD2S1210_DEF_EXCIT	10000
 
+#define ERROR_MESSAGE "ad2s1210: %s out of range\n"
+

Don't make this a define. That's horrible. It's a pointless layer of abstraction.

Quoted 75 lines
drivers/staging/iio/resolver/ad2s1210.c +1 -60
 enum ad2s1210_mode {
 	MOD_POS = 0,
 	MOD_VEL,

@@ -210,64 +212,6 @@ static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
 	return ad2s1210_config_write(st, 0x0);
 }
 
-static ssize_t ad2s1210_store_fclkin(struct device *dev,
-				     struct device_attribute *attr,
-				     const char *buf,
-				     size_t len)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-	unsigned int fclkin;
-	int ret;
-
-	ret = kstrtouint(buf, 10, &fclkin);
-	if (ret)
-		return ret;
-	if (fclkin < AD2S1210_MIN_CLKIN || fclkin > AD2S1210_MAX_CLKIN) {
-		dev_err(dev, "ad2s1210: fclkin out of range\n");
-		return -EINVAL;
-	}
-
-	mutex_lock(&st->lock);
-	st->fclkin = fclkin;
-
-	ret = ad2s1210_update_frequency_control_word(st);
-	if (ret < 0)
-		goto error_ret;
-	ret = ad2s1210_soft_reset(st);
-error_ret:
-	mutex_unlock(&st->lock);
-
-	return ret < 0 ? ret : len;
-}
-
-static ssize_t ad2s1210_store_fexcit(struct device *dev,
-				     struct device_attribute *attr,
-				     const char *buf, size_t len)
-{
-	struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-	unsigned int fexcit;
-	int ret;
-
-	ret = kstrtouint(buf, 10, &fexcit);
-	if (ret < 0)
-		return ret;
-	if (fexcit < AD2S1210_MIN_EXCIT || fexcit > AD2S1210_MAX_EXCIT) {
-		dev_err(dev,
-			"ad2s1210: excitation frequency out of range\n");
-		return -EINVAL;
-	}
-	mutex_lock(&st->lock);
-	st->fexcit = fexcit;
-	ret = ad2s1210_update_frequency_control_word(st);
-	if (ret < 0)
-		goto error_ret;
-	ret = ad2s1210_soft_reset(st);
-error_ret:
-	mutex_unlock(&st->lock);
-
-	return ret < 0 ? ret : len;
-}
-
 static ssize_t ad2s1210_show_control(struct device *dev,
 				     struct device_attribute *attr,
 				     char *buf)

@@ -545,8 +489,58 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static IIO_DEVICE_ATTR(fclkin, 0644, NULL, ad2s1210_store_fclkin, 0);
-static IIO_DEVICE_ATTR(fexcit, 0644, NULL, ad2s1210_store_fexcit, 0);
+static int ad2s1210_write_raw(struct iio_dev *indio_dev,

drivers/staging/iio/resolver/ad2s1210.c static int ad2s1210_read_raw(struct iio_dev *indio_dev, · line 502

Quoted 10 lines
drivers/staging/iio/resolver/ad2s1210.c +10 -0
+			      struct iio_chan_spec const *chan, int val,
+			      int val2, long mask)
+{
+	struct ad2s1210_state *st = iio_priv(indio_dev);
+	unsigned int clk = val;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_FREQUENCY:
+		switch (chan->channel) {

This is a switch statement with only one case. Make it an if statement and chop out some indenting.

Quoted 7 lines
	if (mask != IIO_CHAN_INFO_FREQUENCY)
		return -EINVAL;

drivers/staging/iio/resolver/ad2s1210.c static int ad2s1210_read_raw(struct iio_dev *indio_dev, · line 506

Quoted 4 lines
drivers/staging/iio/resolver/ad2s1210.c +4 -0
+		case FCLKIN:
+			if (clk < AD2S1210_MIN_CLKIN ||
+			    clk > AD2S1210_MAX_CLKIN) {
+				dev_err(&indio_dev->dev, ERROR_MESSAGE,

Hi Dan,

Just a note, I did it because I will add more things to this function. However, I agree with you, and I will follow your recommendation for keep things simple and focused on the present.

Quoted 10 lines

Ah... I see why you did the ERROR_MESSAGE define, to get around the 80 character limit. Don't do that. Just go over 80 characters if you need to.

drivers/staging/iio/resolver/ad2s1210.c static int ad2s1210_read_raw(struct iio_dev *indio_dev, · line 509

Quoted 3 lines
drivers/staging/iio/resolver/ad2s1210.c +3 -0
+					"fclkin");
+				ret = -EINVAL;
+				goto error_ret;

Direct returns are better. Less chance of bugs statistically.

I totally get your point here, and I will fix it. However, just for curiosity, why goto in this situation has more chance to generate bugs statically?

I will send a v2 with your recommendantions.
Thanks for the review and feedbacks :)

Quoted 3 lines

regards,
dan carpenter

#6 lore raw

Re: [PATCH 3/3] staging:iio:ad2s1210: Add write_raw to handle frequency

Quoted 13 lines · Rodrigo Siqueira
Quoted 10 lines · Dan Carpenter

Ah... I see why you did the ERROR_MESSAGE define, to get around the 80 character limit. Don't do that. Just go over 80 characters if you need to.

drivers/staging/iio/resolver/ad2s1210.c static int ad2s1210_read_raw(struct iio_dev *indio_dev, · line 509

Quoted 3 lines
drivers/staging/iio/resolver/ad2s1210.c +3 -0
+					"fclkin");
+				ret = -EINVAL;
+				goto error_ret;

Direct returns are better. Less chance of bugs statistically.

Quoted 4 lines

I totally get your point here, and I will fix it. However, just for curiosity, why goto in this situation has more chance to generate bugs statically?

This is a do-nothing goto. I normally consider do-nothing gotos and do-everything gotos basically cousins but in this case it's probably unfair since it already has other labels.

Do-everything gotos are the most error prone way of doing error handling. I've reviewed a lot of static checker warnings and it really is true. I can't give you like a percent figure but do-everything error handling is a lot buggier than normal kernel style.

This style of error handling is supposed to prevent returning without unlocking bugs. I once looked through the git log and counted missing unlock bugs and my conclusion was that it basically doesn't work for preventing bugs. The kind of people who just add random returns will do it regardless of the error handling style. There was even one driver that indented locked code like this:

	lock(); {
		blah blah blah;
	} unlock();

When the driver was first submitted, it already had a missing unlock bug. I don't think style helps slow people down who are in a hurry.

The other thing about do-nothing gotos is that you introduce the possibility of "forgot to set the error code" bugs which wasn't there before.

regards,
dan carpenter

So actually "error_ret" seems like a pretty reasonable name for a do-nothing goto. I no

I've looked at a lot of error handling and this kind of error handling is more error prone. The single exit path thing is supposed to prevent bugs like not dropping the lock on exit and I've looked through the logs and counted bugs to see if it works and I don't think it does. The people who forget to unlock will forget to unlock regardless of the error handling style.

Quoted 10 lines

I will send a v2 with your recommendantions.
Thanks for the review and feedbacks :)

Quoted 3 lines

regards,
dan carpenter

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

author
#7 lore raw

Re: [PATCH 3/3] staging:iio:ad2s1210: Add write_raw to handle frequency

Quoted 59 lines · Dan Carpenter
Quoted 13 lines · Rodrigo Siqueira
Quoted 10 lines · Dan Carpenter

Ah... I see why you did the ERROR_MESSAGE define, to get around the 80 character limit. Don't do that. Just go over 80 characters if you need to.

drivers/staging/iio/resolver/ad2s1210.c static int ad2s1210_read_raw(struct iio_dev *indio_dev, · line 509

Quoted 3 lines
drivers/staging/iio/resolver/ad2s1210.c +3 -0
+					"fclkin");
+				ret = -EINVAL;
+				goto error_ret;

Direct returns are better. Less chance of bugs statistically.

Quoted 4 lines

I totally get your point here, and I will fix it. However, just for curiosity, why goto in this situation has more chance to generate bugs statically?

This is a do-nothing goto. I normally consider do-nothing gotos and do-everything gotos basically cousins but in this case it's probably unfair since it already has other labels.

Do-everything gotos are the most error prone way of doing error handling. I've reviewed a lot of static checker warnings and it really is true. I can't give you like a percent figure but do-everything error handling is a lot buggier than normal kernel style.

This style of error handling is supposed to prevent returning without unlocking bugs. I once looked through the git log and counted missing unlock bugs and my conclusion was that it basically doesn't work for preventing bugs. The kind of people who just add random returns will do it regardless of the error handling style. There was even one driver that indented locked code like this:

	lock(); {
		blah blah blah;
	} unlock();

When the driver was first submitted, it already had a missing unlock bug. I don't think style helps slow people down who are in a hurry.

The other thing about do-nothing gotos is that you introduce the possibility of "forgot to set the error code" bugs which wasn't there before.

regards,
dan carpenter

So actually "error_ret" seems like a pretty reasonable name for a do-nothing goto. I no

Quoted 7 lines

I've looked at a lot of error handling and this kind of error handling is more error prone. The single exit path thing is supposed to prevent bugs like not dropping the lock on exit and I've looked through the logs and counted bugs to see if it works and I don't think it does. The people who forget to unlock will forget to unlock regardless of the error handling style.

Thanks for the great explanation :)

Quoted 13 lines
Quoted 13 lines

I will send a v2 with your recommendantions.
Thanks for the review and feedbacks :)

Quoted 3 lines

regards,
dan carpenter

_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel