| Message ID | 20190917160925.9791-3-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ADC AD799x improvements | expand |
On Tue, 2019-09-17 at 18:09 +0200, Marco Felsch wrote: > [External] > Comments inline. > Factor out the configuration register update to reuse it during pm > resume operation. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/iio/adc/ad799x.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c > index f658012baad8..af5a2de9c22f 100644 > --- a/drivers/iio/adc/ad799x.c > +++ b/drivers/iio/adc/ad799x.c > @@ -167,6 +167,21 @@ static int ad799x_read_config(struct ad799x_state > *st) > } > } > > +static int ad799x_update_config(struct ad799x_state *st, u16 config) > +{ > + int ret; > + > + ret = ad799x_write_config(st, config); > + if (ret < 0) > + return ret; > + ret = ad799x_read_config(st); > + if (ret < 0) > + return ret; > + st->config = ret; > + > + return 0; > +} > + > /** > * ad799x_trigger_handler() bh of trigger launched polling to ring > buffer > * > @@ -808,13 +823,9 @@ static int ad799x_probe(struct i2c_client *client, > indio_dev->channels = st->chip_config->channel; > indio_dev->num_channels = chip_info->num_channels; > > - ret = ad799x_write_config(st, st->chip_config->default_config); > - if (ret < 0) > - goto error_disable_vref; > - ret = ad799x_read_config(st); > - if (ret < 0) > + ret = ad799x_update_config(st, st->chip_config->default_config); > + if (ret) > goto error_disable_vref; > - st->config = ret; I'm feeling this could go a bit further maybe. I'm noticing that patch 3 adds ad799x_suspend() & ad799x_resume(). It looks to me (I could be wrong), that this bit of code (with some minor re-ordering) is actually a ad799x_resume() call. Similarly, ad799x_suspend() could be added in ad799x_remove(). If that's the case, patch 2 & 3 could be squashed into a single patch that adds ad799x_suspend() & ad799x_resume() & also replaces them here and in the ad799x_remove() code. > > ret = iio_triggered_buffer_setup(indio_dev, NULL, > &ad799x_trigger_handler, NULL);
Hi, On 19-09-18 06:51, Ardelean, Alexandru wrote: > On Tue, 2019-09-17 at 18:09 +0200, Marco Felsch wrote: > > [External] > > > > Comments inline. > > > Factor out the configuration register update to reuse it during pm > > resume operation. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/iio/adc/ad799x.c | 23 +++++++++++++++++------ > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c > > index f658012baad8..af5a2de9c22f 100644 > > --- a/drivers/iio/adc/ad799x.c > > +++ b/drivers/iio/adc/ad799x.c > > @@ -167,6 +167,21 @@ static int ad799x_read_config(struct ad799x_state > > *st) > > } > > } > > > > +static int ad799x_update_config(struct ad799x_state *st, u16 config) > > +{ > > + int ret; > > + > > + ret = ad799x_write_config(st, config); > > + if (ret < 0) > > + return ret; > > + ret = ad799x_read_config(st); > > + if (ret < 0) > > + return ret; > > + st->config = ret; > > + > > + return 0; > > +} > > + > > /** > > * ad799x_trigger_handler() bh of trigger launched polling to ring > > buffer > > * > > @@ -808,13 +823,9 @@ static int ad799x_probe(struct i2c_client *client, > > indio_dev->channels = st->chip_config->channel; > > indio_dev->num_channels = chip_info->num_channels; > > > > - ret = ad799x_write_config(st, st->chip_config->default_config); > > - if (ret < 0) > > - goto error_disable_vref; > > - ret = ad799x_read_config(st); > > - if (ret < 0) > > + ret = ad799x_update_config(st, st->chip_config->default_config); > > + if (ret) > > goto error_disable_vref; > > - st->config = ret; > > I'm feeling this could go a bit further maybe. > I'm noticing that patch 3 adds ad799x_suspend() & ad799x_resume(). Hm.. I don't know. You're right that this is needed for the resume(). I wanted to keep the changes minimal to speed up the review process. As you mentioned below I can squash patch 2 & 3. > It looks to me (I could be wrong), that this bit of code (with some minor > re-ordering) is actually a ad799x_resume() call. I would keep them seperate at least resume() and the probe() path. Regards, Marco > Similarly, ad799x_suspend() could be added in ad799x_remove(). > > If that's the case, patch 2 & 3 could be squashed into a single patch that > adds ad799x_suspend() & ad799x_resume() & also replaces them here and in > the ad799x_remove() code. > > > > > ret = iio_triggered_buffer_setup(indio_dev, NULL, > > &ad799x_trigger_handler, NULL);
On Wed, 2019-09-18 at 10:52 +0200, Marco Felsch wrote: > [External] > > Hi, > > On 19-09-18 06:51, Ardelean, Alexandru wrote: > > On Tue, 2019-09-17 at 18:09 +0200, Marco Felsch wrote: > > > [External] > > > > > > > Comments inline. > > > > > Factor out the configuration register update to reuse it during pm > > > resume operation. > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > --- > > > drivers/iio/adc/ad799x.c | 23 +++++++++++++++++------ > > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c > > > index f658012baad8..af5a2de9c22f 100644 > > > --- a/drivers/iio/adc/ad799x.c > > > +++ b/drivers/iio/adc/ad799x.c > > > @@ -167,6 +167,21 @@ static int ad799x_read_config(struct > > > ad799x_state > > > *st) > > > } > > > } > > > > > > +static int ad799x_update_config(struct ad799x_state *st, u16 config) > > > +{ > > > + int ret; > > > + > > > + ret = ad799x_write_config(st, config); > > > + if (ret < 0) > > > + return ret; > > > + ret = ad799x_read_config(st); > > > + if (ret < 0) > > > + return ret; > > > + st->config = ret; > > > + > > > + return 0; > > > +} > > > + > > > /** > > > * ad799x_trigger_handler() bh of trigger launched polling to ring > > > buffer > > > * > > > @@ -808,13 +823,9 @@ static int ad799x_probe(struct i2c_client > > > *client, > > > indio_dev->channels = st->chip_config->channel; > > > indio_dev->num_channels = chip_info->num_channels; > > > > > > - ret = ad799x_write_config(st, st->chip_config->default_config); > > > - if (ret < 0) > > > - goto error_disable_vref; > > > - ret = ad799x_read_config(st); > > > - if (ret < 0) > > > + ret = ad799x_update_config(st, st->chip_config->default_config); > > > + if (ret) > > > goto error_disable_vref; > > > - st->config = ret; > > > > I'm feeling this could go a bit further maybe. > > I'm noticing that patch 3 adds ad799x_suspend() & ad799x_resume(). > > Hm.. I don't know. You're right that this is needed for the resume(). I > wanted to keep the changes minimal to speed up the review process. As > you mentioned below I can squash patch 2 & 3. > > > It looks to me (I could be wrong), that this bit of code (with some > > minor > > re-ordering) is actually a ad799x_resume() call. > > I would keep them seperate at least resume() and the probe() path. I would be inclined to reuse resume() & suspend() in probe() & remove(), but it's not a big deal. We can leave it separately. > > Regards, > Marco > > > Similarly, ad799x_suspend() could be added in ad799x_remove(). > > > > If that's the case, patch 2 & 3 could be squashed into a single patch > > that > > adds ad799x_suspend() & ad799x_resume() & also replaces them here and > > in > > the ad799x_remove() code. > > > > > > > > ret = iio_triggered_buffer_setup(indio_dev, NULL, > > > &ad799x_trigger_handler, NULL);
diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c index f658012baad8..af5a2de9c22f 100644 --- a/drivers/iio/adc/ad799x.c +++ b/drivers/iio/adc/ad799x.c @@ -167,6 +167,21 @@ static int ad799x_read_config(struct ad799x_state *st) } } +static int ad799x_update_config(struct ad799x_state *st, u16 config) +{ + int ret; + + ret = ad799x_write_config(st, config); + if (ret < 0) + return ret; + ret = ad799x_read_config(st); + if (ret < 0) + return ret; + st->config = ret; + + return 0; +} + /** * ad799x_trigger_handler() bh of trigger launched polling to ring buffer * @@ -808,13 +823,9 @@ static int ad799x_probe(struct i2c_client *client, indio_dev->channels = st->chip_config->channel; indio_dev->num_channels = chip_info->num_channels; - ret = ad799x_write_config(st, st->chip_config->default_config); - if (ret < 0) - goto error_disable_vref; - ret = ad799x_read_config(st); - if (ret < 0) + ret = ad799x_update_config(st, st->chip_config->default_config); + if (ret) goto error_disable_vref; - st->config = ret; ret = iio_triggered_buffer_setup(indio_dev, NULL, &ad799x_trigger_handler, NULL);
Factor out the configuration register update to reuse it during pm resume operation. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/iio/adc/ad799x.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)