On Thu, May 07, 2020 at 12:43:36AM +0200, mirq-test@rere.qmqm.pl wrote:
> On Wed, May 06, 2020 at 09:33:55PM +0200, Thierry Reding wrote:
> [...]
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -1769,10 +1769,14 @@ static int tegra_i2c_remove(struct platform_device *pdev)
> >  static int __maybe_unused tegra_i2c_suspend(struct device *dev)
> >  {
> >  	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
> > +	int err = 0;
> >  
> >  	i2c_mark_adapter_suspended(&i2c_dev->adapter);
> >  
> > -	return 0;
> > +	if (!pm_runtime_status_suspended(dev))
> > +		err = tegra_i2c_runtime_suspend(dev);
> > +
> > +	return err;
> >  }
> >  
> >  static int __maybe_unused tegra_i2c_resume(struct device *dev)
> > @@ -1788,9 +1792,11 @@ static int __maybe_unused tegra_i2c_resume(struct device *dev)
> >  	if (err)
> >  		return err;
> >  
> > -	err = tegra_i2c_runtime_suspend(dev);
> > -	if (err)
> > -		return err;
> > +	if (pm_runtime_status_suspended(dev)) {
> [...]
> Shouldn't this be negated as in suspend? I would assume that inbetween
> suspend and resume nothing changes the stored state.

I know this is confusing because I have now twice had the same doubts
after looking at the patch after I sent it out and thought I had sent
out a wrong version.

However, I think it starts to make more sense when you look at the
resulting code, which I'll reproduce below:

	static int __maybe_unused tegra_i2c_resume(struct device *dev)
	{
		struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
		int err;

		err = tegra_i2c_runtime_resume(dev);
		if (err)
			return err;

		err = tegra_i2c_init(i2c_dev, false);
		if (err)
			return err;

		if (pm_runtime_status_suspended(dev)) {
			err = tegra_i2c_runtime_suspend(dev);
			if (err)
				return err;
		}

		i2c_mark_adapter_resumed(&i2c_dev->adapter);

		return 0;
	}

So the purpose here is to runtime resume the I2C controller temporarily
so that the register context can be reprogrammed because it was lost
during suspend. Now, if the controller was runtime suspended prior to
system suspend, we want to put it back into suspend after the context
was loaded again. Conversely, if it was not runtime suspended, then we
want to keep it on.

If it helps I can sprinkle some comments throughout this function to try
and explain why exactly this is being done.

Thierry