[PATCH 0/3] staging: iio: adc: ad7280a: channel and attr init refactor

[PATCH 0/3] staging: iio: adc: ad7280a: channel and attr init refactor

[Slawomir Stepien]

This small series enhances the readability of the code by refactoring the
ad7280_channel_init and ad7280_attr_init functions.

The readability is enhanced by splitting the functions into more functions that
do more specific operations. The last patch adds braces to two complex for
loops.

Slawomir Stepien (3):
  staging: iio: adc: ad7280a: split ad7280_channel_init() to more
    functions
  staging: iio: adc: ad7280a: split ad7280_attr_init() to more functions
  staging: iio: adc: ad7280a: add braces to complex for loops

 drivers/staging/iio/adc/ad7280a.c | 204 +++++++++++++++++++-----------
 1 file changed, 129 insertions(+), 75 deletions(-)

-- 
2.19.2

[PATCH 1/3] staging: iio: adc: ad7280a: split ad7280_channel_init() to more functions

[Slawomir Stepien]

The ad7280_channel_init function has been split into more specific
functions to increase the code readability.

Signed-off-by: Slawomir Stepien <sst@poczta.fm>
---
 drivers/staging/iio/adc/ad7280a.c | 119 +++++++++++++++++++-----------
 1 file changed, 77 insertions(+), 42 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
index 7a0ba26f9fd9..a6b1bdd9f372 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -496,6 +496,75 @@ static const struct attribute_group ad7280_attrs_group = {
 	.attrs = ad7280_attributes,
 };
 
+static void ad7280_voltage_channel_init(struct ad7280_state *st, int cnt,
+					int dev, int ch)
+{
+	struct iio_chan_spec *chan = &st->channels[cnt];
+
+	chan->type = IIO_VOLTAGE;
+	chan->differential = 1;
+	chan->channel = (dev * 6) + ch;
+	chan->channel2 = chan->channel + 1;
+}
+
+static void ad7280_temp_channel_init(struct ad7280_state *st, int cnt, int dev,
+				     int ch)
+{
+	struct iio_chan_spec *chan = &st->channels[cnt];
+
+	chan->type = IIO_TEMP;
+	chan->channel = (dev * 6) + ch - 6;
+}
+
+static void ad7280_common_fields_init(struct ad7280_state *st, int cnt, int dev,
+				      int ch)
+{
+	struct iio_chan_spec *chan = &st->channels[cnt];
+
+	chan->indexed = 1;
+	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
+	chan->address = ad7280a_devaddr(dev) << 8 | ch;
+	chan->scan_index = cnt;
+	chan->scan_type.sign = 'u';
+	chan->scan_type.realbits = 12;
+	chan->scan_type.storagebits = 32;
+	chan->scan_type.shift = 0;
+}
+
+static void ad7280_all_voltage_channel_init(struct ad7280_state *st, int cnt,
+					    int dev)
+{
+	struct iio_chan_spec *chan = &st->channels[cnt];
+
+	chan->type = IIO_VOLTAGE;
+	chan->differential = 1;
+	chan->channel = 0;
+	chan->channel2 = dev * 6;
+	chan->address = AD7280A_ALL_CELLS;
+	chan->indexed = 1;
+	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
+	chan->scan_index = cnt;
+	chan->scan_type.sign = 'u';
+	chan->scan_type.realbits = 32;
+	chan->scan_type.storagebits = 32;
+	chan->scan_type.shift = 0;
+}
+
+static void ad7280_timestamp_channel_init(struct ad7280_state *st, int cnt)
+{
+	struct iio_chan_spec *chan = &st->channels[cnt];
+
+	chan->type = IIO_TIMESTAMP;
+	chan->channel = -1;
+	chan->scan_index = cnt;
+	chan->scan_type.sign = 's';
+	chan->scan_type.realbits = 64;
+	chan->scan_type.storagebits = 64;
+	chan->scan_type.shift = 0;
+}
+
 static int ad7280_channel_init(struct ad7280_state *st)
 {
 	int dev, ch, cnt;
@@ -508,51 +577,17 @@ static int ad7280_channel_init(struct ad7280_state *st)
 	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
 		for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_AUX_ADC_6;
 			ch++, cnt++) {
-			if (ch < AD7280A_AUX_ADC_1) {
-				st->channels[cnt].type = IIO_VOLTAGE;
-				st->channels[cnt].differential = 1;
-				st->channels[cnt].channel = (dev * 6) + ch;
-				st->channels[cnt].channel2 =
-					st->channels[cnt].channel + 1;
-			} else {
-				st->channels[cnt].type = IIO_TEMP;
-				st->channels[cnt].channel = (dev * 6) + ch - 6;
-			}
-			st->channels[cnt].indexed = 1;
-			st->channels[cnt].info_mask_separate =
-				BIT(IIO_CHAN_INFO_RAW);
-			st->channels[cnt].info_mask_shared_by_type =
-				BIT(IIO_CHAN_INFO_SCALE);
-			st->channels[cnt].address =
-				ad7280a_devaddr(dev) << 8 | ch;
-			st->channels[cnt].scan_index = cnt;
-			st->channels[cnt].scan_type.sign = 'u';
-			st->channels[cnt].scan_type.realbits = 12;
-			st->channels[cnt].scan_type.storagebits = 32;
-			st->channels[cnt].scan_type.shift = 0;
+			if (ch < AD7280A_AUX_ADC_1)
+				ad7280_voltage_channel_init(st, cnt, dev, ch);
+			else
+				ad7280_temp_channel_init(st, cnt, dev, ch);
+
+			ad7280_common_fields_init(st, cnt, dev, ch);
 		}
 
-	st->channels[cnt].type = IIO_VOLTAGE;
-	st->channels[cnt].differential = 1;
-	st->channels[cnt].channel = 0;
-	st->channels[cnt].channel2 = dev * 6;
-	st->channels[cnt].address = AD7280A_ALL_CELLS;
-	st->channels[cnt].indexed = 1;
-	st->channels[cnt].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
-	st->channels[cnt].info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
-	st->channels[cnt].scan_index = cnt;
-	st->channels[cnt].scan_type.sign = 'u';
-	st->channels[cnt].scan_type.realbits = 32;
-	st->channels[cnt].scan_type.storagebits = 32;
-	st->channels[cnt].scan_type.shift = 0;
+	ad7280_all_voltage_channel_init(st, cnt, dev);
 	cnt++;
-	st->channels[cnt].type = IIO_TIMESTAMP;
-	st->channels[cnt].channel = -1;
-	st->channels[cnt].scan_index = cnt;
-	st->channels[cnt].scan_type.sign = 's';
-	st->channels[cnt].scan_type.realbits = 64;
-	st->channels[cnt].scan_type.storagebits = 64;
-	st->channels[cnt].scan_type.shift = 0;
+	ad7280_timestamp_channel_init(st, cnt);
 
 	return cnt + 1;
 }
-- 
2.19.2

Re: [PATCH 1/3] staging: iio: adc: ad7280a: split ad7280_channel_init() to more functions

[Jonathan Cameron]

On Sun,  2 Dec 2018 12:58:28 +0100
Slawomir Stepien <sst@poczta.fm> wrote:

> The ad7280_channel_init function has been split into more specific
> functions to increase the code readability.
> 
> Signed-off-by: Slawomir Stepien <sst@poczta.fm>
Hi Slawomir,

A few comments inline.  I agree with the basic idea, but suggest
a slight different balance in where things are handled.

Jonathan

> ---
>  drivers/staging/iio/adc/ad7280a.c | 119 +++++++++++++++++++-----------
>  1 file changed, 77 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> index 7a0ba26f9fd9..a6b1bdd9f372 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -496,6 +496,75 @@ static const struct attribute_group ad7280_attrs_group = {
>  	.attrs = ad7280_attributes,
>  };
>  
> +static void ad7280_voltage_channel_init(struct ad7280_state *st, int cnt,
> +					int dev, int ch)
> +{
> +	struct iio_chan_spec *chan = &st->channels[cnt];

Pass the chan spec in here instead of the _state structure.
It will give cleaner code and better visibility on the scope of
this function where it is called.

> +
> +	chan->type = IIO_VOLTAGE;
> +	chan->differential = 1;
> +	chan->channel = (dev * 6) + ch;

This offset does seem more than a little random buried away in this function.
I would pass it in explicitly as a parameter so that the we have all the
channel number manipulations visible in one place.

> +	chan->channel2 = chan->channel + 1;
> +}
> +
> +static void ad7280_temp_channel_init(struct ad7280_state *st, int cnt, int dev,
> +				     int ch)
> +{
> +	struct iio_chan_spec *chan = &st->channels[cnt];
> +
> +	chan->type = IIO_TEMP;
> +	chan->channel = (dev * 6) + ch - 6;
This one is even more obscure when found in here. Pass it in as an additional parameter.
> +}
> +
> +static void ad7280_common_fields_init(struct ad7280_state *st, int cnt, int dev,
> +				      int ch)

It's not really the common fields given the voltage version is different.
Perhaps there is a better name for this function?


> +{
> +	struct iio_chan_spec *chan = &st->channels[cnt];
> +
> +	chan->indexed = 1;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> +	chan->address = ad7280a_devaddr(dev) << 8 | ch;
> +	chan->scan_index = cnt;
> +	chan->scan_type.sign = 'u';
> +	chan->scan_type.realbits = 12;
> +	chan->scan_type.storagebits = 32;
> +	chan->scan_type.shift = 0;

I doubt there is any need to set shift and 0 is the obvious default
so no 'documentation' benefit in doing it here.

> +}
> +
> +static void ad7280_all_voltage_channel_init(struct ad7280_state *st, int cnt,
> +					    int dev)
Total voltage perhaps rather than 'all'? 
> +{
> +	struct iio_chan_spec *chan = &st->channels[cnt];
> +
> +	chan->type = IIO_VOLTAGE;
> +	chan->differential = 1;
> +	chan->channel = 0;
> +	chan->channel2 = dev * 6;
> +	chan->address = AD7280A_ALL_CELLS;
> +	chan->indexed = 1;
> +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> +	chan->scan_index = cnt;
> +	chan->scan_type.sign = 'u';
> +	chan->scan_type.realbits = 32;
> +	chan->scan_type.storagebits = 32;
> +	chan->scan_type.shift = 0;
> +}
> +
> +static void ad7280_timestamp_channel_init(struct ad7280_state *st, int cnt)
> +{
> +	struct iio_chan_spec *chan = &st->channels[cnt];
> +
> +	chan->type = IIO_TIMESTAMP;
> +	chan->channel = -1;
> +	chan->scan_index = cnt;
> +	chan->scan_type.sign = 's';
> +	chan->scan_type.realbits = 64;
> +	chan->scan_type.storagebits = 64;
> +	chan->scan_type.shift = 0;
> +}
> +
>  static int ad7280_channel_init(struct ad7280_state *st)
>  {
>  	int dev, ch, cnt;
> @@ -508,51 +577,17 @@ static int ad7280_channel_init(struct ad7280_state *st)
>  	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
>  		for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_AUX_ADC_6;
>  			ch++, cnt++) {
> -			if (ch < AD7280A_AUX_ADC_1) {
> -				st->channels[cnt].type = IIO_VOLTAGE;
> -				st->channels[cnt].differential = 1;
> -				st->channels[cnt].channel = (dev * 6) + ch;
> -				st->channels[cnt].channel2 =
> -					st->channels[cnt].channel + 1;
> -			} else {
> -				st->channels[cnt].type = IIO_TEMP;
> -				st->channels[cnt].channel = (dev * 6) + ch - 6;
> -			}
> -			st->channels[cnt].indexed = 1;
> -			st->channels[cnt].info_mask_separate =
> -				BIT(IIO_CHAN_INFO_RAW);
> -			st->channels[cnt].info_mask_shared_by_type =
> -				BIT(IIO_CHAN_INFO_SCALE);
> -			st->channels[cnt].address =
> -				ad7280a_devaddr(dev) << 8 | ch;
> -			st->channels[cnt].scan_index = cnt;
> -			st->channels[cnt].scan_type.sign = 'u';
> -			st->channels[cnt].scan_type.realbits = 12;
> -			st->channels[cnt].scan_type.storagebits = 32;
> -			st->channels[cnt].scan_type.shift = 0;
> +			if (ch < AD7280A_AUX_ADC_1)
> +				ad7280_voltage_channel_init(st, cnt, dev, ch);
> +			else
> +				ad7280_temp_channel_init(st, cnt, dev, ch);
> +
> +			ad7280_common_fields_init(st, cnt, dev, ch);
>  		}
>  
> -	st->channels[cnt].type = IIO_VOLTAGE;
> -	st->channels[cnt].differential = 1;
> -	st->channels[cnt].channel = 0;
> -	st->channels[cnt].channel2 = dev * 6;
> -	st->channels[cnt].address = AD7280A_ALL_CELLS;
> -	st->channels[cnt].indexed = 1;
> -	st->channels[cnt].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> -	st->channels[cnt].info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> -	st->channels[cnt].scan_index = cnt;
> -	st->channels[cnt].scan_type.sign = 'u';
> -	st->channels[cnt].scan_type.realbits = 32;
> -	st->channels[cnt].scan_type.storagebits = 32;
> -	st->channels[cnt].scan_type.shift = 0;
> +	ad7280_all_voltage_channel_init(st, cnt, dev);
>  	cnt++;
> -	st->channels[cnt].type = IIO_TIMESTAMP;
> -	st->channels[cnt].channel = -1;
> -	st->channels[cnt].scan_index = cnt;
> -	st->channels[cnt].scan_type.sign = 's';
> -	st->channels[cnt].scan_type.realbits = 64;
> -	st->channels[cnt].scan_type.storagebits = 64;
> -	st->channels[cnt].scan_type.shift = 0;
> +	ad7280_timestamp_channel_init(st, cnt);
>  
>  	return cnt + 1;
>  }

Re: [PATCH 1/3] staging: iio: adc: ad7280a: split ad7280_channel_init() to more functions

[Slawomir Stepien]

On gru 02, 2018 15:45, Jonathan Cameron wrote:
> On Sun,  2 Dec 2018 12:58:28 +0100
> Slawomir Stepien <sst@poczta.fm> wrote:
> 
> > The ad7280_channel_init function has been split into more specific
> > functions to increase the code readability.
> > 
> > Signed-off-by: Slawomir Stepien <sst@poczta.fm>
> Hi Slawomir,

Hi

> A few comments inline.  I agree with the basic idea, but suggest
> a slight different balance in where things are handled.

Thank you for the review. I agree with the comments, but please see one remark
below.

> > ---
> >  drivers/staging/iio/adc/ad7280a.c | 119 +++++++++++++++++++-----------
> >  1 file changed, 77 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> > index 7a0ba26f9fd9..a6b1bdd9f372 100644
> > --- a/drivers/staging/iio/adc/ad7280a.c
> > +++ b/drivers/staging/iio/adc/ad7280a.c
> > @@ -496,6 +496,75 @@ static const struct attribute_group ad7280_attrs_group = {
> >  	.attrs = ad7280_attributes,
> >  };
> >  
> > +static void ad7280_voltage_channel_init(struct ad7280_state *st, int cnt,
> > +					int dev, int ch)
> > +{
> > +	struct iio_chan_spec *chan = &st->channels[cnt];
> 
> Pass the chan spec in here instead of the _state structure.
> It will give cleaner code and better visibility on the scope of
> this function where it is called.
> 
> > +
> > +	chan->type = IIO_VOLTAGE;
> > +	chan->differential = 1;
> > +	chan->channel = (dev * 6) + ch;
> 
> This offset does seem more than a little random buried away in this function.
> I would pass it in explicitly as a parameter so that the we have all the
> channel number manipulations visible in one place.
> 
> > +	chan->channel2 = chan->channel + 1;
> > +}
> > +
> > +static void ad7280_temp_channel_init(struct ad7280_state *st, int cnt, int dev,
> > +				     int ch)
> > +{
> > +	struct iio_chan_spec *chan = &st->channels[cnt];
> > +
> > +	chan->type = IIO_TEMP;
> > +	chan->channel = (dev * 6) + ch - 6;
> This one is even more obscure when found in here. Pass it in as an additional parameter.
> > +}
> > +
> > +static void ad7280_common_fields_init(struct ad7280_state *st, int cnt, int dev,
> > +				      int ch)
> 
> It's not really the common fields given the voltage version is different.
> Perhaps there is a better name for this function?

I don't understand. The fields of the chan in this function are set to the same
values for volt and temp channels (this function is called at the end of the
for loop regardless of the AD type of the channel). I do not see how the voltage
version is different.

> > +{
> > +	struct iio_chan_spec *chan = &st->channels[cnt];
> > +
> > +	chan->indexed = 1;
> > +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> > +	chan->address = ad7280a_devaddr(dev) << 8 | ch;
> > +	chan->scan_index = cnt;
> > +	chan->scan_type.sign = 'u';
> > +	chan->scan_type.realbits = 12;
> > +	chan->scan_type.storagebits = 32;
> > +	chan->scan_type.shift = 0;
> 
> I doubt there is any need to set shift and 0 is the obvious default
> so no 'documentation' benefit in doing it here.
> 
> > +}
> > +
> > +static void ad7280_all_voltage_channel_init(struct ad7280_state *st, int cnt,
> > +					    int dev)
> Total voltage perhaps rather than 'all'? 
> > +{
> > +	struct iio_chan_spec *chan = &st->channels[cnt];
> > +
> > +	chan->type = IIO_VOLTAGE;
> > +	chan->differential = 1;
> > +	chan->channel = 0;
> > +	chan->channel2 = dev * 6;
> > +	chan->address = AD7280A_ALL_CELLS;
> > +	chan->indexed = 1;
> > +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> > +	chan->scan_index = cnt;
> > +	chan->scan_type.sign = 'u';
> > +	chan->scan_type.realbits = 32;
> > +	chan->scan_type.storagebits = 32;
> > +	chan->scan_type.shift = 0;
> > +}
> > +
> > +static void ad7280_timestamp_channel_init(struct ad7280_state *st, int cnt)
> > +{
> > +	struct iio_chan_spec *chan = &st->channels[cnt];
> > +
> > +	chan->type = IIO_TIMESTAMP;
> > +	chan->channel = -1;
> > +	chan->scan_index = cnt;
> > +	chan->scan_type.sign = 's';
> > +	chan->scan_type.realbits = 64;
> > +	chan->scan_type.storagebits = 64;
> > +	chan->scan_type.shift = 0;
> > +}
> > +
> >  static int ad7280_channel_init(struct ad7280_state *st)
> >  {
> >  	int dev, ch, cnt;
> > @@ -508,51 +577,17 @@ static int ad7280_channel_init(struct ad7280_state *st)
> >  	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
> >  		for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_AUX_ADC_6;
> >  			ch++, cnt++) {
> > -			if (ch < AD7280A_AUX_ADC_1) {
> > -				st->channels[cnt].type = IIO_VOLTAGE;
> > -				st->channels[cnt].differential = 1;
> > -				st->channels[cnt].channel = (dev * 6) + ch;
> > -				st->channels[cnt].channel2 =
> > -					st->channels[cnt].channel + 1;
> > -			} else {
> > -				st->channels[cnt].type = IIO_TEMP;
> > -				st->channels[cnt].channel = (dev * 6) + ch - 6;
> > -			}
> > -			st->channels[cnt].indexed = 1;
> > -			st->channels[cnt].info_mask_separate =
> > -				BIT(IIO_CHAN_INFO_RAW);
> > -			st->channels[cnt].info_mask_shared_by_type =
> > -				BIT(IIO_CHAN_INFO_SCALE);
> > -			st->channels[cnt].address =
> > -				ad7280a_devaddr(dev) << 8 | ch;
> > -			st->channels[cnt].scan_index = cnt;
> > -			st->channels[cnt].scan_type.sign = 'u';
> > -			st->channels[cnt].scan_type.realbits = 12;
> > -			st->channels[cnt].scan_type.storagebits = 32;
> > -			st->channels[cnt].scan_type.shift = 0;
> > +			if (ch < AD7280A_AUX_ADC_1)
> > +				ad7280_voltage_channel_init(st, cnt, dev, ch);
> > +			else
> > +				ad7280_temp_channel_init(st, cnt, dev, ch);
> > +
> > +			ad7280_common_fields_init(st, cnt, dev, ch);
> >  		}
> >  
> > -	st->channels[cnt].type = IIO_VOLTAGE;
> > -	st->channels[cnt].differential = 1;
> > -	st->channels[cnt].channel = 0;
> > -	st->channels[cnt].channel2 = dev * 6;
> > -	st->channels[cnt].address = AD7280A_ALL_CELLS;
> > -	st->channels[cnt].indexed = 1;
> > -	st->channels[cnt].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > -	st->channels[cnt].info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> > -	st->channels[cnt].scan_index = cnt;
> > -	st->channels[cnt].scan_type.sign = 'u';
> > -	st->channels[cnt].scan_type.realbits = 32;
> > -	st->channels[cnt].scan_type.storagebits = 32;
> > -	st->channels[cnt].scan_type.shift = 0;
> > +	ad7280_all_voltage_channel_init(st, cnt, dev);
> >  	cnt++;
> > -	st->channels[cnt].type = IIO_TIMESTAMP;
> > -	st->channels[cnt].channel = -1;
> > -	st->channels[cnt].scan_index = cnt;
> > -	st->channels[cnt].scan_type.sign = 's';
> > -	st->channels[cnt].scan_type.realbits = 64;
> > -	st->channels[cnt].scan_type.storagebits = 64;
> > -	st->channels[cnt].scan_type.shift = 0;
> > +	ad7280_timestamp_channel_init(st, cnt);
> >  
> >  	return cnt + 1;
> >  }
> 

-- 
Slawomir Stepien

Re: [PATCH 1/3] staging: iio: adc: ad7280a: split ad7280_channel_init() to more functions

[Jonathan Cameron]

On Mon, 3 Dec 2018 21:06:41 +0100
Slawomir Stepien <sst@poczta.fm> wrote:

> On gru 02, 2018 15:45, Jonathan Cameron wrote:
> > On Sun,  2 Dec 2018 12:58:28 +0100
> > Slawomir Stepien <sst@poczta.fm> wrote:
> >   
> > > The ad7280_channel_init function has been split into more specific
> > > functions to increase the code readability.
> > > 
> > > Signed-off-by: Slawomir Stepien <sst@poczta.fm>  
> > Hi Slawomir,  
> 
> Hi
> 
> > A few comments inline.  I agree with the basic idea, but suggest
> > a slight different balance in where things are handled.  
> 
> Thank you for the review. I agree with the comments, but please see one remark
> below.
> 
> > > ---
> > >  drivers/staging/iio/adc/ad7280a.c | 119 +++++++++++++++++++-----------
> > >  1 file changed, 77 insertions(+), 42 deletions(-)
> > > 
> > > diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> > > index 7a0ba26f9fd9..a6b1bdd9f372 100644
> > > --- a/drivers/staging/iio/adc/ad7280a.c
> > > +++ b/drivers/staging/iio/adc/ad7280a.c
> > > @@ -496,6 +496,75 @@ static const struct attribute_group ad7280_attrs_group = {
> > >  	.attrs = ad7280_attributes,
> > >  };
> > >  
> > > +static void ad7280_voltage_channel_init(struct ad7280_state *st, int cnt,
> > > +					int dev, int ch)
> > > +{
> > > +	struct iio_chan_spec *chan = &st->channels[cnt];  
> > 
> > Pass the chan spec in here instead of the _state structure.
> > It will give cleaner code and better visibility on the scope of
> > this function where it is called.
> >   
> > > +
> > > +	chan->type = IIO_VOLTAGE;
> > > +	chan->differential = 1;
> > > +	chan->channel = (dev * 6) + ch;  
> > 
> > This offset does seem more than a little random buried away in this function.
> > I would pass it in explicitly as a parameter so that the we have all the
> > channel number manipulations visible in one place.
> >   
> > > +	chan->channel2 = chan->channel + 1;
> > > +}
> > > +
> > > +static void ad7280_temp_channel_init(struct ad7280_state *st, int cnt, int dev,
> > > +				     int ch)
> > > +{
> > > +	struct iio_chan_spec *chan = &st->channels[cnt];
> > > +
> > > +	chan->type = IIO_TEMP;
> > > +	chan->channel = (dev * 6) + ch - 6;  
> > This one is even more obscure when found in here. Pass it in as an additional parameter.  
> > > +}
> > > +
> > > +static void ad7280_common_fields_init(struct ad7280_state *st, int cnt, int dev,
> > > +				      int ch)  
> > 
> > It's not really the common fields given the voltage version is different.
> > Perhaps there is a better name for this function?  
> 
> I don't understand. The fields of the chan in this function are set to the same
> values for volt and temp channels (this function is called at the end of the
> for loop regardless of the AD type of the channel). I do not see how the voltage
> version is different.

It's the all voltage one that is different.  My mistake.

> 
> > > +{
> > > +	struct iio_chan_spec *chan = &st->channels[cnt];
> > > +
> > > +	chan->indexed = 1;
> > > +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > > +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> > > +	chan->address = ad7280a_devaddr(dev) << 8 | ch;
> > > +	chan->scan_index = cnt;
> > > +	chan->scan_type.sign = 'u';
> > > +	chan->scan_type.realbits = 12;
> > > +	chan->scan_type.storagebits = 32;
> > > +	chan->scan_type.shift = 0;  
> > 
> > I doubt there is any need to set shift and 0 is the obvious default
> > so no 'documentation' benefit in doing it here.
> >   
> > > +}
> > > +
> > > +static void ad7280_all_voltage_channel_init(struct ad7280_state *st, int cnt,
> > > +					    int dev)  
> > Total voltage perhaps rather than 'all'?   
> > > +{
> > > +	struct iio_chan_spec *chan = &st->channels[cnt];
> > > +
> > > +	chan->type = IIO_VOLTAGE;
> > > +	chan->differential = 1;
> > > +	chan->channel = 0;
> > > +	chan->channel2 = dev * 6;
> > > +	chan->address = AD7280A_ALL_CELLS;
> > > +	chan->indexed = 1;
> > > +	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > > +	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> > > +	chan->scan_index = cnt;
> > > +	chan->scan_type.sign = 'u';
> > > +	chan->scan_type.realbits = 32;
> > > +	chan->scan_type.storagebits = 32;
> > > +	chan->scan_type.shift = 0;
> > > +}
> > > +
> > > +static void ad7280_timestamp_channel_init(struct ad7280_state *st, int cnt)
> > > +{
> > > +	struct iio_chan_spec *chan = &st->channels[cnt];
> > > +
> > > +	chan->type = IIO_TIMESTAMP;
> > > +	chan->channel = -1;
> > > +	chan->scan_index = cnt;
> > > +	chan->scan_type.sign = 's';
> > > +	chan->scan_type.realbits = 64;
> > > +	chan->scan_type.storagebits = 64;
> > > +	chan->scan_type.shift = 0;
> > > +}
> > > +
> > >  static int ad7280_channel_init(struct ad7280_state *st)
> > >  {
> > >  	int dev, ch, cnt;
> > > @@ -508,51 +577,17 @@ static int ad7280_channel_init(struct ad7280_state *st)
> > >  	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
> > >  		for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_AUX_ADC_6;
> > >  			ch++, cnt++) {
> > > -			if (ch < AD7280A_AUX_ADC_1) {
> > > -				st->channels[cnt].type = IIO_VOLTAGE;
> > > -				st->channels[cnt].differential = 1;
> > > -				st->channels[cnt].channel = (dev * 6) + ch;
> > > -				st->channels[cnt].channel2 =
> > > -					st->channels[cnt].channel + 1;
> > > -			} else {
> > > -				st->channels[cnt].type = IIO_TEMP;
> > > -				st->channels[cnt].channel = (dev * 6) + ch - 6;
> > > -			}
> > > -			st->channels[cnt].indexed = 1;
> > > -			st->channels[cnt].info_mask_separate =
> > > -				BIT(IIO_CHAN_INFO_RAW);
> > > -			st->channels[cnt].info_mask_shared_by_type =
> > > -				BIT(IIO_CHAN_INFO_SCALE);
> > > -			st->channels[cnt].address =
> > > -				ad7280a_devaddr(dev) << 8 | ch;
> > > -			st->channels[cnt].scan_index = cnt;
> > > -			st->channels[cnt].scan_type.sign = 'u';
> > > -			st->channels[cnt].scan_type.realbits = 12;
> > > -			st->channels[cnt].scan_type.storagebits = 32;
> > > -			st->channels[cnt].scan_type.shift = 0;
> > > +			if (ch < AD7280A_AUX_ADC_1)
> > > +				ad7280_voltage_channel_init(st, cnt, dev, ch);
> > > +			else
> > > +				ad7280_temp_channel_init(st, cnt, dev, ch);
> > > +
> > > +			ad7280_common_fields_init(st, cnt, dev, ch);
> > >  		}
> > >  
> > > -	st->channels[cnt].type = IIO_VOLTAGE;
> > > -	st->channels[cnt].differential = 1;
> > > -	st->channels[cnt].channel = 0;
> > > -	st->channels[cnt].channel2 = dev * 6;
> > > -	st->channels[cnt].address = AD7280A_ALL_CELLS;
> > > -	st->channels[cnt].indexed = 1;
> > > -	st->channels[cnt].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > > -	st->channels[cnt].info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> > > -	st->channels[cnt].scan_index = cnt;
> > > -	st->channels[cnt].scan_type.sign = 'u';
> > > -	st->channels[cnt].scan_type.realbits = 32;
> > > -	st->channels[cnt].scan_type.storagebits = 32;
> > > -	st->channels[cnt].scan_type.shift = 0;
> > > +	ad7280_all_voltage_channel_init(st, cnt, dev);
> > >  	cnt++;
> > > -	st->channels[cnt].type = IIO_TIMESTAMP;
> > > -	st->channels[cnt].channel = -1;
> > > -	st->channels[cnt].scan_index = cnt;
> > > -	st->channels[cnt].scan_type.sign = 's';
> > > -	st->channels[cnt].scan_type.realbits = 64;
> > > -	st->channels[cnt].scan_type.storagebits = 64;
> > > -	st->channels[cnt].scan_type.shift = 0;
> > > +	ad7280_timestamp_channel_init(st, cnt);
> > >  
> > >  	return cnt + 1;
> > >  }  
> >   
> 

[PATCH 2/3] staging: iio: adc: ad7280a: split ad7280_attr_init() to more functions

[Slawomir Stepien]

The ad7280_attr_init function has been split into more specific
functions to increase the code readability.

Signed-off-by: Slawomir Stepien <sst@poczta.fm>
---
 drivers/staging/iio/adc/ad7280a.c | 79 +++++++++++++++++++------------
 1 file changed, 48 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
index a6b1bdd9f372..6faa85bf6f7e 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -592,11 +592,49 @@ static int ad7280_channel_init(struct ad7280_state *st)
 	return cnt + 1;
 }
 
+static int ad7280_balance_switch_attr_init(struct ad7280_state *st, int cnt,
+					   int dev, int ch)
+{
+	struct iio_dev_attr *attr = &st->iio_attr[cnt];
+
+	attr->address = ad7280a_devaddr(dev) << 8 | ch;
+	attr->dev_attr.attr.mode = 0644;
+	attr->dev_attr.show = ad7280_show_balance_sw;
+	attr->dev_attr.store = ad7280_store_balance_sw;
+	attr->dev_attr.attr.name = devm_kasprintf(&st->spi->dev, GFP_KERNEL,
+						  "in%d-in%d_balance_switch_en",
+						  ch, ch + 1);
+	if (!attr->dev_attr.attr.name)
+		return -ENOMEM;
+
+	ad7280_attributes[cnt] = &attr->dev_attr.attr;
+
+	return 0;
+}
+
+static int ad7280_balance_timer_attr_init(struct ad7280_state *st, int cnt,
+					  int dev, int ch)
+{
+	struct iio_dev_attr *attr = &st->iio_attr[cnt];
+
+	attr->address = ad7280a_devaddr(dev) << 8 | (AD7280A_CB1_TIMER + ch);
+	attr->dev_attr.attr.mode = 0644;
+	attr->dev_attr.show = ad7280_show_balance_timer;
+	attr->dev_attr.store = ad7280_store_balance_timer;
+	attr->dev_attr.attr.name = devm_kasprintf(&st->spi->dev, GFP_KERNEL,
+						  "in%d-in%d_balance_timer",
+						  ch, ch + 1);
+	if (!attr->dev_attr.attr.name)
+		return -ENOMEM;
+
+	ad7280_attributes[cnt] = &attr->dev_attr.attr;
+
+	return 0;
+}
+
 static int ad7280_attr_init(struct ad7280_state *st)
 {
-	int dev, ch, cnt;
-	unsigned int index;
-	struct iio_dev_attr *iio_attr;
+	int dev, ch, cnt, ret;
 
 	st->iio_attr = devm_kcalloc(&st->spi->dev, 2, sizeof(*st->iio_attr) *
 				    (st->slave_num + 1) * AD7280A_CELLS_PER_DEV,
@@ -607,35 +645,14 @@ static int ad7280_attr_init(struct ad7280_state *st)
 	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
 		for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_CELL_VOLTAGE_6;
 			ch++, cnt++) {
-			iio_attr = &st->iio_attr[cnt];
-			index = dev * AD7280A_CELLS_PER_DEV + ch;
-			iio_attr->address = ad7280a_devaddr(dev) << 8 | ch;
-			iio_attr->dev_attr.attr.mode = 0644;
-			iio_attr->dev_attr.show = ad7280_show_balance_sw;
-			iio_attr->dev_attr.store = ad7280_store_balance_sw;
-			iio_attr->dev_attr.attr.name =
-				devm_kasprintf(&st->spi->dev, GFP_KERNEL,
-					       "in%d-in%d_balance_switch_en",
-					       index, index + 1);
-			if (!iio_attr->dev_attr.attr.name)
-				return -ENOMEM;
-
-			ad7280_attributes[cnt] = &iio_attr->dev_attr.attr;
+			ret = ad7280_balance_switch_attr_init(st, cnt, dev, ch);
+			if (ret < 0)
+				return ret;
+
 			cnt++;
-			iio_attr = &st->iio_attr[cnt];
-			iio_attr->address = ad7280a_devaddr(dev) << 8 |
-				(AD7280A_CB1_TIMER + ch);
-			iio_attr->dev_attr.attr.mode = 0644;
-			iio_attr->dev_attr.show = ad7280_show_balance_timer;
-			iio_attr->dev_attr.store = ad7280_store_balance_timer;
-			iio_attr->dev_attr.attr.name =
-				devm_kasprintf(&st->spi->dev, GFP_KERNEL,
-					       "in%d-in%d_balance_timer",
-					       index, index + 1);
-			if (!iio_attr->dev_attr.attr.name)
-				return -ENOMEM;
-
-			ad7280_attributes[cnt] = &iio_attr->dev_attr.attr;
+			ret = ad7280_balance_timer_attr_init(st, cnt, dev, ch);
+			if (ret < 0)
+				return ret;
 		}
 
 	ad7280_attributes[cnt] = NULL;
-- 
2.19.2

Re: [PATCH 2/3] staging: iio: adc: ad7280a: split ad7280_attr_init() to more functions

[Jonathan Cameron]

On Sun,  2 Dec 2018 12:58:29 +0100
Slawomir Stepien <sst@poczta.fm> wrote:

> The ad7280_attr_init function has been split into more specific
> functions to increase the code readability.
> 
> Signed-off-by: Slawomir Stepien <sst@poczta.fm>
> ---
>  drivers/staging/iio/adc/ad7280a.c | 79 +++++++++++++++++++------------
>  1 file changed, 48 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> index a6b1bdd9f372..6faa85bf6f7e 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -592,11 +592,49 @@ static int ad7280_channel_init(struct ad7280_state *st)
>  	return cnt + 1;
>  }
>  
> +static int ad7280_balance_switch_attr_init(struct ad7280_state *st, int cnt,
> +					   int dev, int ch)
> +{
> +	struct iio_dev_attr *attr = &st->iio_attr[cnt];

I would pass this in to the function (and the dev you need below) to make the
scope of what the function is doing more obvious.

> +
> +	attr->address = ad7280a_devaddr(dev) << 8 | ch;
> +	attr->dev_attr.attr.mode = 0644;
> +	attr->dev_attr.show = ad7280_show_balance_sw;
> +	attr->dev_attr.store = ad7280_store_balance_sw;
> +	attr->dev_attr.attr.name = devm_kasprintf(&st->spi->dev, GFP_KERNEL,
> +						  "in%d-in%d_balance_switch_en",
> +						  ch, ch + 1);
> +	if (!attr->dev_attr.attr.name)
> +		return -ENOMEM;
> +
> +	ad7280_attributes[cnt] = &attr->dev_attr.attr;
> +
> +	return 0;
> +}
> +
> +static int ad7280_balance_timer_attr_init(struct ad7280_state *st, int cnt,
> +					  int dev, int ch)
> +{
> +	struct iio_dev_attr *attr = &st->iio_attr[cnt];
> +
> +	attr->address = ad7280a_devaddr(dev) << 8 | (AD7280A_CB1_TIMER + ch);
> +	attr->dev_attr.attr.mode = 0644;
> +	attr->dev_attr.show = ad7280_show_balance_timer;
> +	attr->dev_attr.store = ad7280_store_balance_timer;
> +	attr->dev_attr.attr.name = devm_kasprintf(&st->spi->dev, GFP_KERNEL,
> +						  "in%d-in%d_balance_timer",
> +						  ch, ch + 1);
> +	if (!attr->dev_attr.attr.name)
> +		return -ENOMEM;
> +
> +	ad7280_attributes[cnt] = &attr->dev_attr.attr;
> +
> +	return 0;
> +}
> +
>  static int ad7280_attr_init(struct ad7280_state *st)
>  {
> -	int dev, ch, cnt;
> -	unsigned int index;
> -	struct iio_dev_attr *iio_attr;
> +	int dev, ch, cnt, ret;
>  
>  	st->iio_attr = devm_kcalloc(&st->spi->dev, 2, sizeof(*st->iio_attr) *
>  				    (st->slave_num + 1) * AD7280A_CELLS_PER_DEV,
> @@ -607,35 +645,14 @@ static int ad7280_attr_init(struct ad7280_state *st)
>  	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
>  		for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_CELL_VOLTAGE_6;
>  			ch++, cnt++) {
> -			iio_attr = &st->iio_attr[cnt];
> -			index = dev * AD7280A_CELLS_PER_DEV + ch;
> -			iio_attr->address = ad7280a_devaddr(dev) << 8 | ch;
> -			iio_attr->dev_attr.attr.mode = 0644;
> -			iio_attr->dev_attr.show = ad7280_show_balance_sw;
> -			iio_attr->dev_attr.store = ad7280_store_balance_sw;
> -			iio_attr->dev_attr.attr.name =
> -				devm_kasprintf(&st->spi->dev, GFP_KERNEL,
> -					       "in%d-in%d_balance_switch_en",
> -					       index, index + 1);
> -			if (!iio_attr->dev_attr.attr.name)
> -				return -ENOMEM;
> -
> -			ad7280_attributes[cnt] = &iio_attr->dev_attr.attr;
> +			ret = ad7280_balance_switch_attr_init(st, cnt, dev, ch);
> +			if (ret < 0)
> +				return ret;
> +
>  			cnt++;
> -			iio_attr = &st->iio_attr[cnt];
> -			iio_attr->address = ad7280a_devaddr(dev) << 8 |
> -				(AD7280A_CB1_TIMER + ch);
> -			iio_attr->dev_attr.attr.mode = 0644;
> -			iio_attr->dev_attr.show = ad7280_show_balance_timer;
> -			iio_attr->dev_attr.store = ad7280_store_balance_timer;
> -			iio_attr->dev_attr.attr.name =
> -				devm_kasprintf(&st->spi->dev, GFP_KERNEL,
> -					       "in%d-in%d_balance_timer",
> -					       index, index + 1);
> -			if (!iio_attr->dev_attr.attr.name)
> -				return -ENOMEM;
> -
> -			ad7280_attributes[cnt] = &iio_attr->dev_attr.attr;
> +			ret = ad7280_balance_timer_attr_init(st, cnt, dev, ch);
> +			if (ret < 0)
> +				return ret;
>  		}
>  
>  	ad7280_attributes[cnt] = NULL;

[PATCH 3/3] staging: iio: adc: ad7280a: add braces to complex for loops

[Slawomir Stepien]

These for loops have just only one instruction as its body. However this
body is complex, so add braces to them.

Signed-off-by: Slawomir Stepien <sst@poczta.fm>
---
 drivers/staging/iio/adc/ad7280a.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
index 6faa85bf6f7e..6ec5a0d2a438 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -574,7 +574,7 @@ static int ad7280_channel_init(struct ad7280_state *st)
 	if (!st->channels)
 		return -ENOMEM;
 
-	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
+	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++) {
 		for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_AUX_ADC_6;
 			ch++, cnt++) {
 			if (ch < AD7280A_AUX_ADC_1)
@@ -584,6 +584,7 @@ static int ad7280_channel_init(struct ad7280_state *st)
 
 			ad7280_common_fields_init(st, cnt, dev, ch);
 		}
+	}
 
 	ad7280_all_voltage_channel_init(st, cnt, dev);
 	cnt++;
@@ -642,7 +643,7 @@ static int ad7280_attr_init(struct ad7280_state *st)
 	if (!st->iio_attr)
 		return -ENOMEM;
 
-	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
+	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++) {
 		for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_CELL_VOLTAGE_6;
 			ch++, cnt++) {
 			ret = ad7280_balance_switch_attr_init(st, cnt, dev, ch);
@@ -654,6 +655,7 @@ static int ad7280_attr_init(struct ad7280_state *st)
 			if (ret < 0)
 				return ret;
 		}
+	}
 
 	ad7280_attributes[cnt] = NULL;
 
-- 
2.19.2

Re: [PATCH 3/3] staging: iio: adc: ad7280a: add braces to complex for loops

[Jonathan Cameron]

On Sun,  2 Dec 2018 12:58:30 +0100
Slawomir Stepien <sst@poczta.fm> wrote:

> These for loops have just only one instruction as its body. However this
> body is complex, so add braces to them.
> 
> Signed-off-by: Slawomir Stepien <sst@poczta.fm>
This is fine.  I'll pick it up when 1 and 2 are updated.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/adc/ad7280a.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
> index 6faa85bf6f7e..6ec5a0d2a438 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -574,7 +574,7 @@ static int ad7280_channel_init(struct ad7280_state *st)
>  	if (!st->channels)
>  		return -ENOMEM;
>  
> -	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
> +	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++) {
>  		for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_AUX_ADC_6;
>  			ch++, cnt++) {
>  			if (ch < AD7280A_AUX_ADC_1)
> @@ -584,6 +584,7 @@ static int ad7280_channel_init(struct ad7280_state *st)
>  
>  			ad7280_common_fields_init(st, cnt, dev, ch);
>  		}
> +	}
>  
>  	ad7280_all_voltage_channel_init(st, cnt, dev);
>  	cnt++;
> @@ -642,7 +643,7 @@ static int ad7280_attr_init(struct ad7280_state *st)
>  	if (!st->iio_attr)
>  		return -ENOMEM;
>  
> -	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++)
> +	for (dev = 0, cnt = 0; dev <= st->slave_num; dev++) {
>  		for (ch = AD7280A_CELL_VOLTAGE_1; ch <= AD7280A_CELL_VOLTAGE_6;
>  			ch++, cnt++) {
>  			ret = ad7280_balance_switch_attr_init(st, cnt, dev, ch);
> @@ -654,6 +655,7 @@ static int ad7280_attr_init(struct ad7280_state *st)
>  			if (ret < 0)
>  				return ret;
>  		}
> +	}
>  
>  	ad7280_attributes[cnt] = NULL;
>