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

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

On 26 October 2015 03:29:46 GMT+00:00, Alison Schofield <amsfield22@gmail.com> wrote:
>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?
There are a few other comment syntax changes that should really be in a third patch, trivial though they are.
>
>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,
>> >  };
>> >  
>> >  /**
>> > 
>> 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.