Re: [PATCH] staging: iio: dummy: clean up the define and usage of dummy_scan_elements

[Alison Schofield <amsfield22@gmail.com>]

On Sun, Oct 25, 2015 at 10:54:29AM +0000, Jonathan Cameron wrote:
> On 23/10/15 17:54, Alison Schofield wrote:
> > Cleanups related to the usage of dummy_scan_elements in the
> > iio dummy driver:
> > - change enum names to upper case INDEX_*
> > - use the enum for IIO_CHAN_SOFT_TIMESTAMP (rm hard coded value)
> > - comment edited to match changes & follow preferred kernel style
> > 
> As Greg observed this would be better as three separate patches.
> Comment inline as well.
> 
> Jonathan

Greg, Jonathan,

Thanks for the review.  I understand it needs to be a set. 

I only see 2 patches:
1) change enum names to be upper case.
2) replace hardcoded scan_index in IIO_CHAN_SOFT_TIMESTAMP

I don't see the comments as a 3rd, they go with each change.
ie. when I change the enum names, I update the comments
right above it to match.  Same with Patch 2.

OK?

alison

> > Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> > ---
> >  drivers/staging/iio/iio_simple_dummy.c        | 22 ++++++++++++----------
> >  drivers/staging/iio/iio_simple_dummy.h        | 20 +++++++++++---------
> >  drivers/staging/iio/iio_simple_dummy_buffer.c |  9 +++++----
> >  3 files changed, 28 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/iio_simple_dummy.c b/drivers/staging/iio/iio_simple_dummy.c
> > index 381f90f..5e19068 100644
> > --- a/drivers/staging/iio/iio_simple_dummy.c
> > +++ b/drivers/staging/iio/iio_simple_dummy.c
> > @@ -1,4 +1,4 @@
> > -/**
> > +/*
> >   * Copyright (c) 2011 Jonathan Cameron
> >   *
> >   * This program is free software; you can redistribute it and/or modify it
> > @@ -137,7 +137,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
> >  		 */
> >  		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> >  		/* The ordering of elements in the buffer via an enum */
> > -		.scan_index = voltage0,
> > +		.scan_index = INDEX_VOLTAGE_0,
> >  		.scan_type = { /* Description of storage in buffer */
> >  			.sign = 'u', /* unsigned */
> >  			.realbits = 13, /* 13 bits */
> > @@ -176,7 +176,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
> >  		 * sampling_frequency
> >  		 * The frequency in Hz at which the channels are sampled
> >  		 */
> > -		.scan_index = diffvoltage1m2,
> > +		.scan_index = INDEX_DIFFVOLTAGE_1M2,
> >  		.scan_type = { /* Description of storage in buffer */
> >  			.sign = 's', /* signed */
> >  			.realbits = 12, /* 12 bits */
> > @@ -194,7 +194,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
> >  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> >  		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > -		.scan_index = diffvoltage3m4,
> > +		.scan_index = INDEX_DIFFVOLTAGE_3M4,
> >  		.scan_type = {
> >  			.sign = 's',
> >  			.realbits = 11,
> > @@ -221,7 +221,7 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
> >  		BIT(IIO_CHAN_INFO_CALIBSCALE) |
> >  		BIT(IIO_CHAN_INFO_CALIBBIAS),
> >  		.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > -		.scan_index = accelx,
> > +		.scan_index = INDEX_ACCELX,
> >  		.scan_type = { /* Description of storage in buffer */
> >  			.sign = 's', /* signed */
> >  			.realbits = 16, /* 16 bits */
> > @@ -230,10 +230,10 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
> >  		},
> >  	},
> >  	/*
> > -	 * Convenience macro for timestamps. 4 is the index in
> > -	 * the buffer.
> > +	 * Convenience macro for timestamps.
> > +	 * INDEX_TIMESTAMP is the index in the buffer.
> >  	 */
> > -	IIO_CHAN_SOFT_TIMESTAMP(4),
> > +	IIO_CHAN_SOFT_TIMESTAMP(INDEX_TIMESTAMP),
> >  	/* DAC channel out_voltage0_raw */
> >  	{
> >  		.type = IIO_VOLTAGE,
> > @@ -364,8 +364,10 @@ static int iio_dummy_read_raw(struct iio_dev *indio_dev,
> >  				ret = IIO_VAL_INT_PLUS_MICRO;
> >  				break;
> >  			case 1:
> > -				/* all differential adc channels ->
> > -				 * 0.000001344 */
> > +				/*
> > +				 * all differential adc
> > +				 * channels -> 0.000001344
> > +				 */
> >  				*val = 0;
> >  				*val2 = 1344;
> >  				ret = IIO_VAL_INT_PLUS_NANO;
> > diff --git a/drivers/staging/iio/iio_simple_dummy.h b/drivers/staging/iio/iio_simple_dummy.h
> > index 5c2f4d0..31b62ad 100644
> > --- a/drivers/staging/iio/iio_simple_dummy.h
> > +++ b/drivers/staging/iio/iio_simple_dummy.h
> > @@ -96,20 +96,22 @@ iio_simple_dummy_events_unregister(struct iio_dev *indio_dev)
> >  
> >  #endif /* CONFIG_IIO_SIMPLE_DUMMY_EVENTS*/
> >  
> > -/**
> > +/*
> >   * enum iio_simple_dummy_scan_elements - scan index enum
> > - * @voltage0:		the single ended voltage channel
> > - * @diffvoltage1m2:	first differential channel
> > - * @diffvoltage3m4:	second differenial channel
> > - * @accelx:		acceleration channel
> > + * @INDEX_VOLTAGE_0:		the single ended voltage channel
> > + * @INDEX_DIFFVOLTAGE_1M2:	first differential channel
> > + * @INDEX_DIFFVOLTAGE_3M4:	second differential channel
> > + * @INDEX_ACCELX:		acceleration channel
> > + * @INDEX_TIMESTAMP:		timestamp channel
> >   *
> >   * Enum provides convenient numbering for the scan index.
> >   */
> >  enum iio_simple_dummy_scan_elements {
> > -	voltage0,
> > -	diffvoltage1m2,
> > -	diffvoltage3m4,
> > -	accelx,
> > +	INDEX_VOLTAGE_0,
> Even better would be to prefix these to make it clear they
> are driver specific.  With such generic names one might
> suspect they come from somewhere in the core.
> 
> Perhaps
> DUMMY_INDEX_...
> ?
> > +	INDEX_DIFFVOLTAGE_1M2,
> > +	INDEX_DIFFVOLTAGE_3M4,
> > +	INDEX_ACCELX,
> > +	INDEX_TIMESTAMP,
> >  };
> >  
> >  #ifdef CONFIG_IIO_SIMPLE_DUMMY_BUFFER
> > diff --git a/drivers/staging/iio/iio_simple_dummy_buffer.c b/drivers/staging/iio/iio_simple_dummy_buffer.c
> > index 00ed774..7516835 100644
> > --- a/drivers/staging/iio/iio_simple_dummy_buffer.c
> > +++ b/drivers/staging/iio/iio_simple_dummy_buffer.c
> > @@ -27,10 +27,11 @@
> >  /* Some fake data */
> >  
> >  static const s16 fakedata[] = {
> > -	[voltage0] = 7,
> > -	[diffvoltage1m2] = -33,
> > -	[diffvoltage3m4] = -2,
> > -	[accelx] = 344,
> > +	[INDEX_VOLTAGE_0] = 7,
> > +	[INDEX_DIFFVOLTAGE_1M2] = -33,
> > +	[INDEX_DIFFVOLTAGE_3M4] = -2,
> > +	[INDEX_ACCELX] = 344,
> > +	[INDEX_TIMESTAMP] = 50,
> >  };
> >  
> >  /**
> > 
>