| Message ID | 20181106214308.10619-1-sst@poczta.fm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] staging: iio: adc: ad7280a: do not pass the copy of struct element | expand |
On Tue, 2018-11-06 at 22:43 +0100, Slawomir Stepien wrote: > There is no need to pass this parameter since is always a copy of > parameter that is available via st pointer. > Hey, Comments inline. Thanks Alex > Signed-off-by: Slawomir Stepien <sst@poczta.fm> > --- > Note: this has been rebased on kernel/git/jic23/iio.git testing branch. I > hope > that's OK! > > Changes since v1: > * Change my email address > --- > drivers/staging/iio/adc/ad7280a.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7280a.c > b/drivers/staging/iio/adc/ad7280a.c > index 89c2c2deca64..fc6237ddb460 100644 > --- a/drivers/staging/iio/adc/ad7280a.c > +++ b/drivers/staging/iio/adc/ad7280a.c > @@ -291,7 +291,7 @@ static int ad7280_read_channel(struct ad7280_state > *st, unsigned int devaddr, > return (tmp >> 11) & 0xFFF; > } > > -static int ad7280_read_all_channels(struct ad7280_state *st, unsigned > int cnt, > +static int ad7280_read_all_channels(struct ad7280_state *st, > unsigned int *array) > { [Generally speaking] When passing an array of elements to a function in C, it's a good idea to also pass the count of elements, so that you know how many elements to iterate over. So, this code should be fine as-is. I agree that the element count is stored on `st`, but having `cnt` as a parameter on the ad7280_read_all_channels() function keeps it neatly coupled with `array` and gives you the flexibility to read less than `cnt` channels. I don't feel this change (even though correct) is really worth the trouble, since it doesn't really add much value to the existing code. But if there are any opinions otherwise, I don't feel really strongly about doing it one way (before this patch) or the other (with this patch). > int i, ret; > @@ -312,7 +312,7 @@ static int ad7280_read_all_channels(struct > ad7280_state *st, unsigned int cnt, > > ad7280_delay(st); > > - for (i = 0; i < cnt; i++) { > + for (i = 0; i < st->scan_cnt; i++) { > ret = __ad7280_read32(st, &tmp); > if (ret) > return ret; > @@ -687,7 +687,7 @@ static irqreturn_t ad7280_event_handler(int irq, void > *private) > if (!channels) > return IRQ_HANDLED; > > - ret = ad7280_read_all_channels(st, st->scan_cnt, channels); > + ret = ad7280_read_all_channels(st, channels); > if (ret < 0) > goto out; > > @@ -791,7 +791,7 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, > case IIO_CHAN_INFO_RAW: > mutex_lock(&st->lock); > if (chan->address == AD7280A_ALL_CELLS) > - ret = ad7280_read_all_channels(st, st->scan_cnt, > NULL); > + ret = ad7280_read_all_channels(st, NULL); > else > ret = ad7280_read_channel(st, chan->address >> 8, > chan->address & 0xFF);
On lis 07, 2018 10:16, Ardelean, Alexandru wrote: > On Tue, 2018-11-06 at 22:43 +0100, Slawomir Stepien wrote: > > There is no need to pass this parameter since is always a copy of > > parameter that is available via st pointer. > > > > Hey, > > Comments inline. > > Thanks > Alex > > > Signed-off-by: Slawomir Stepien <sst@poczta.fm> > > --- > > Note: this has been rebased on kernel/git/jic23/iio.git testing branch. I > > hope > > that's OK! > > > > Changes since v1: > > * Change my email address > > --- > > drivers/staging/iio/adc/ad7280a.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/iio/adc/ad7280a.c > > b/drivers/staging/iio/adc/ad7280a.c > > index 89c2c2deca64..fc6237ddb460 100644 > > --- a/drivers/staging/iio/adc/ad7280a.c > > +++ b/drivers/staging/iio/adc/ad7280a.c > > @@ -291,7 +291,7 @@ static int ad7280_read_channel(struct ad7280_state > > *st, unsigned int devaddr, > > return (tmp >> 11) & 0xFFF; > > } > > > > -static int ad7280_read_all_channels(struct ad7280_state *st, unsigned > > int cnt, > > +static int ad7280_read_all_channels(struct ad7280_state *st, > > unsigned int *array) > > { > > [Generally speaking] When passing an array of elements to a function in C, > it's a good idea to also pass the count of elements, so that you know how > many elements to iterate over. > So, this code should be fine as-is. > > I agree that the element count is stored on `st`, but having `cnt` as a > parameter on the ad7280_read_all_channels() function keeps it neatly > coupled with `array` and gives you the flexibility to read less than `cnt` > channels. > > I don't feel this change (even though correct) is really worth the trouble, > since it doesn't really add much value to the existing code. Yes. You are right here. It's not worth the trouble. Thank you for this review Alex. > But if there are any opinions otherwise, I don't feel really strongly about > doing it one way (before this patch) or the other (with this patch). > > > int i, ret; > > @@ -312,7 +312,7 @@ static int ad7280_read_all_channels(struct > > ad7280_state *st, unsigned int cnt, > > > > ad7280_delay(st); > > > > - for (i = 0; i < cnt; i++) { > > + for (i = 0; i < st->scan_cnt; i++) { > > ret = __ad7280_read32(st, &tmp); > > if (ret) > > return ret; > > @@ -687,7 +687,7 @@ static irqreturn_t ad7280_event_handler(int irq, void > > *private) > > if (!channels) > > return IRQ_HANDLED; > > > > - ret = ad7280_read_all_channels(st, st->scan_cnt, channels); > > + ret = ad7280_read_all_channels(st, channels); > > if (ret < 0) > > goto out; > > > > @@ -791,7 +791,7 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, > > case IIO_CHAN_INFO_RAW: > > mutex_lock(&st->lock); > > if (chan->address == AD7280A_ALL_CELLS) > > - ret = ad7280_read_all_channels(st, st->scan_cnt, > > NULL); > > + ret = ad7280_read_all_channels(st, NULL); > > else > > ret = ad7280_read_channel(st, chan->address >> 8, > > chan->address & 0xFF);
diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c index 89c2c2deca64..fc6237ddb460 100644 --- a/drivers/staging/iio/adc/ad7280a.c +++ b/drivers/staging/iio/adc/ad7280a.c @@ -291,7 +291,7 @@ static int ad7280_read_channel(struct ad7280_state *st, unsigned int devaddr, return (tmp >> 11) & 0xFFF; } -static int ad7280_read_all_channels(struct ad7280_state *st, unsigned int cnt, +static int ad7280_read_all_channels(struct ad7280_state *st, unsigned int *array) { int i, ret; @@ -312,7 +312,7 @@ static int ad7280_read_all_channels(struct ad7280_state *st, unsigned int cnt, ad7280_delay(st); - for (i = 0; i < cnt; i++) { + for (i = 0; i < st->scan_cnt; i++) { ret = __ad7280_read32(st, &tmp); if (ret) return ret; @@ -687,7 +687,7 @@ static irqreturn_t ad7280_event_handler(int irq, void *private) if (!channels) return IRQ_HANDLED; - ret = ad7280_read_all_channels(st, st->scan_cnt, channels); + ret = ad7280_read_all_channels(st, channels); if (ret < 0) goto out; @@ -791,7 +791,7 @@ static int ad7280_read_raw(struct iio_dev *indio_dev, case IIO_CHAN_INFO_RAW: mutex_lock(&st->lock); if (chan->address == AD7280A_ALL_CELLS) - ret = ad7280_read_all_channels(st, st->scan_cnt, NULL); + ret = ad7280_read_all_channels(st, NULL); else ret = ad7280_read_channel(st, chan->address >> 8, chan->address & 0xFF);
There is no need to pass this parameter since is always a copy of parameter that is available via st pointer. Signed-off-by: Slawomir Stepien <sst@poczta.fm> --- Note: this has been rebased on kernel/git/jic23/iio.git testing branch. I hope that's OK! Changes since v1: * Change my email address --- drivers/staging/iio/adc/ad7280a.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)