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@xxxxxxxxxxxxxx>
> > ---
> > 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);
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
