Re: [PATCH 6/6] iio: imu: inv_icm42600: Avoid configuring if already pm_runtime suspended

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 14 Jul 2025 10:15:59 +0000
Sean Nyekjaer <sean@geanix.com> wrote:

> On Sun, Jul 13, 2025 at 03:32:27PM +0100, Jonathan Cameron wrote:
> > On Wed, 09 Jul 2025 14:35:14 +0200
> > Sean Nyekjaer <sean@geanix.com> wrote:
> >   
> > > Do as in suspend, skip resume configuration steps if the device is already
> > > pm_runtime suspended. This avoids reconfiguring a device that is already
> > > in the correct low-power state and ensures that pm_runtimeM handles the
> > > power state transitions properly.
> > >
> > > Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > ---  
> > 
> > Not really related to what you have here, but this code would really
> > benefit from using guard(mutex)()
> > 
> > Jonathan  
> 
> I have converted most of this driver to use guard(mutex).
> 
> Does it make sense to use guard(mutex) in functions that still relies on
> goto error out? Like...

No - general rule is never combine cleanup.h magic like guard with gotos.

If you do want to use it in cases like this, factor out the stuff
> 
> static int inv_icm42600_temp_read(struct inv_icm42600_state *st, s16 *temp)
> {
> 	struct device *dev = regmap_get_device(st->map);
> 	__be16 *raw;
> 	int ret;
> 
> 	pm_runtime_get_sync(dev);
> 	mutex_lock(&st->lock);
> 

from here

> 	ret = inv_icm42600_set_temp_conf(st, true, NULL);
> 	if (ret)
> 		goto exit;
> 
> 	raw = (__be16 *)&st->buffer[0];
> 	ret = regmap_bulk_read(st->map, INV_ICM42600_REG_TEMP_DATA, raw, sizeof(*raw));
> 	if (ret)
> 		goto exit;
> 
> 	*temp = (s16)be16_to_cpup(raw);
> 	if (*temp == INV_ICM42600_DATA_INVALID)
> 		ret = -EINVAL;
> 

to here as a utility function.  Then can use direct returns in that 
function and there are no gotos to worry about.

> exit:
> 	mutex_unlock(&st->lock);
> 	pm_runtime_mark_last_busy(dev);
> 	pm_runtime_put_autosuspend(dev);
> 
> 	return ret;
> }
> 
> 
> If I use guard_scoped(..) it creates a lot of diff lines...
The only way to use that here and avoid the issue would be with breaks
which is ugly.   If we are going to pay the cost of churn, then
factoring out the guts of the function is much cleaner.

May not be worth bothering though!

Jonathan

> 
> /Sean
>