| Message ID | ae60ca5cf37d1678cb6b498450f75f93f3d498d2.1520878492.git.rodrigosiqueiramelo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 > -- 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
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 -- 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
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 -- 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
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, };
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(-)