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@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.
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);
