[PATCH 0/3] staging:iio:ad2s1210: Rework read/write operation for fclkin and fexin

[PATCH 0/3] staging:iio:ad2s1210: Rework read/write operation for fclkin and fexin

[Rodrigo Siqueira]

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(-)

-- 
2.16.2

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

[Rodrigo Siqueira]

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 <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1210.c | 43 ++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index ac13b99bd9cb..28c3fd439663 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -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,
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[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 <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1210.c | 45 +++++++++++++++------------------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 28c3fd439663..27a42ed10fcd 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -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,
-- 
2.16.2

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

[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 <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/resolver/ad2s1210.c | 117 +++++++++++++++-----------------
 1 file changed, 55 insertions(+), 62 deletions(-)

diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 27a42ed10fcd..ea6ade4e563c 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -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,
 };
 
-- 
2.16.2

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

[Dan Carpenter]

On Mon, Mar 12, 2018 at 03:21:52PM -0300, Rodrigo Siqueira wrote:
> 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 <rodrigosiqueiramelo@gmail.com>
> ---
>  drivers/staging/iio/resolver/ad2s1210.c | 117 +++++++++++++++-----------------
>  1 file changed, 55 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 27a42ed10fcd..ea6ade4e563c 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -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.


>  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) {

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;

> +		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.


> +					"fclkin");
> +				ret = -EINVAL;
> +				goto error_ret;

Direct returns are better.  Less chance of bugs statistically.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Rodrigo Siqueira]

On 03/13, Dan Carpenter wrote:
> On Mon, Mar 12, 2018 at 03:21:52PM -0300, Rodrigo Siqueira wrote:
> > 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 <rodrigosiqueiramelo@gmail.com>
> > ---
> >  drivers/staging/iio/resolver/ad2s1210.c | 117 +++++++++++++++-----------------
> >  1 file changed, 55 insertions(+), 62 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> > index 27a42ed10fcd..ea6ade4e563c 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -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.
> 
> 
> >  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) {
> 
> 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;
> 
> > +		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. 

> 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.
> 
> 
> > +					"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 :)
 
> regards,
> dan carpenter
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Dan Carpenter]

On Tue, Mar 13, 2018 at 10:06:29AM -0300, Rodrigo Siqueira wrote:
> On 03/13, Dan Carpenter wrote:
>
> > 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.
> > 
> > 
> > > +					"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?
> 

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.





> I will send a v2 with your recommendantions.
> Thanks for the review and feedbacks :)
>  
> > regards,
> > dan carpenter
> > 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Rodrigo Siqueira]

On 03/13, Dan Carpenter wrote:
> On Tue, Mar 13, 2018 at 10:06:29AM -0300, Rodrigo Siqueira wrote:
> > On 03/13, Dan Carpenter wrote:
> >
> > > 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.
> > > 
> > > 
> > > > +					"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?
> > 
> 
> 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.
> 

Thanks for the great explanation :)
 
> 
> 
> 
> > I will send a v2 with your recommendantions.
> > Thanks for the review and feedbacks :)
> >  
> > > regards,
> > > dan carpenter
> > > 
> > _______________________________________________
> > devel mailing list
> > devel@linuxdriverproject.org
> > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel