| Message ID | ac00afab032039350d23cfc9752f8e9225537fd0.1680564468.git.william.gray@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Migrate STX104 to the regmap API | expand |
On Tue, Apr 04, 2023 at 10:12:00AM -0400, William Breathitt Gray wrote: > By bailing out early if chan->output is false for the IIO_CHAN_INFO_RAW, > indentation can be decreased by a tab and code readability improved. ... > + /* DAC can only accept up to a 16-bit value */ > + if ((unsigned int)val > 65535) > + return -EINVAL; While the patch is good per se, I don't like two things (which are also in the original code): - explicit casting (can it be avoided?) - would be good to have U16_MAX or ?.. instead of hard coded number Can it be addressed with (additional) patches?
On Wed, Apr 05, 2023 at 11:50:08AM +0300, Andy Shevchenko wrote: > On Tue, Apr 04, 2023 at 10:12:00AM -0400, William Breathitt Gray wrote: > > By bailing out early if chan->output is false for the IIO_CHAN_INFO_RAW, > > indentation can be decreased by a tab and code readability improved. > > ... > > > + /* DAC can only accept up to a 16-bit value */ > > + if ((unsigned int)val > 65535) > > + return -EINVAL; > > While the patch is good per se, I don't like two things (which are also in the > original code): > - explicit casting (can it be avoided?) We could explicitly check for negative instead: if (val < 0 || val > 65535) Would that be better? > - would be good to have U16_MAX or ?.. instead of hard coded number Fair point, I'll use U16_MAX then. > Can it be addressed with (additional) patches? Sure, I'll submit a separate patch to address this. William Breathitt Gray
diff --git a/drivers/iio/addac/stx104.c b/drivers/iio/addac/stx104.c index 8730b79e921c..9cc467469dde 100644 --- a/drivers/iio/addac/stx104.c +++ b/drivers/iio/addac/stx104.c @@ -180,20 +180,20 @@ static int stx104_write_raw(struct iio_dev *indio_dev, return 0; case IIO_CHAN_INFO_RAW: - if (chan->output) { - /* DAC can only accept up to a 16-bit value */ - if ((unsigned int)val > 65535) - return -EINVAL; + if (!chan->output) + return -EINVAL; - mutex_lock(&priv->lock); + /* DAC can only accept up to a 16-bit value */ + if ((unsigned int)val > 65535) + return -EINVAL; - priv->chan_out_states[chan->channel] = val; - iowrite16(val, &priv->reg->dac[chan->channel]); + mutex_lock(&priv->lock); - mutex_unlock(&priv->lock); - return 0; - } - return -EINVAL; + priv->chan_out_states[chan->channel] = val; + iowrite16(val, &priv->reg->dac[chan->channel]); + + mutex_unlock(&priv->lock); + return 0; } return -EINVAL;
By bailing out early if chan->output is false for the IIO_CHAN_INFO_RAW, indentation can be decreased by a tab and code readability improved. Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: William Breathitt Gray <william.gray@linaro.org> --- drivers/iio/addac/stx104.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)