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

[Slawomir Stepien <sst@poczta.fm>]

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