From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from saturn.retrosnub.co.uk ([178.18.118.26]:35645 "EHLO saturn.retrosnub.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753000AbbJZHV4 (ORCPT ); Mon, 26 Oct 2015 03:21:56 -0400 In-Reply-To: <20151026032945.GD2104@Ubuntu-D830> References: <20151023165418.GA18718@Ubuntu-D830> <562CB4E5.8010503@kernel.org> <20151026032945.GD2104@Ubuntu-D830> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Subject: Re: [PATCH] staging: iio: dummy: clean up the define and usage of dummy_scan_elements From: Jonathan Cameron Date: Mon, 26 Oct 2015 07:21:49 +0000 To: Alison Schofield , Jonathan Cameron , gregkh@linuxfoundation.org CC: outreachy-kernel@googlegroups.com, linux-iio@vger.kernel.org Message-ID: <9FFA453E-8F87-4B4C-9E10-D0641523A032@jic23.retrosnub.co.uk> Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org On 26 October 2015 03:29:46 GMT+00:00, Alison Schofield 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 >> > --- >> > 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.