Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko <andy@xxxxxxxxxx>]

On Thu, Sep 21, 2023 at 08:42:43PM +0200, Marek Behún wrote:
> On Tue, 19 Sep 2023 16:00:39 +0300
> Andy Shevchenko <andy@xxxxxxxxxx> wrote:

...

> > > +	mutex_lock(&mcu->lock);
> > > +
> > > +	if (ctl_mask)
> > > +		err = omnia_ctl_cmd_unlocked(mcu, CMD_GENERAL_CONTROL, ctl,
> > > +					     ctl_mask);  
> > 
> > > +	if (!err && ext_ctl_mask)
> > > +		err = omnia_ctl_cmd_unlocked(mcu, CMD_EXT_CONTROL, ext_ctl,
> > > +					     ext_ctl_mask);  
> > 
> > Can it be
> > 
> > 	if (err)
> > 		goto out_unlock;
> > 
> > 	if (_mask)
> > 		...
> > 
> > ?
> 
> Hi Andy,
> 
> so I am refactoring this to use guard(mutex), but now I have this:
> 
> 	guard(mutex, &mcu->lock);
> 
> 	if (ctl_mask) {
> 		err = ...;
> 		if (err)
> 			goto out_err;
> 	}
> 
> 	if (ext_ctl_mask) {
> 		err = ...;
> 		if (err)
> 			goto out_err;
> 	}
> 
> 	return;
> out_err:
> 	dev_err(dev, "Cannot set GPIOs: %d\n", err);
> 
> which clearly is not any better... or at least the original

...which rather means that the design of above is not so good, i.e.
why do you need the same message in the different situations?

>   if (!err && ext_ctl_mask)
> is better IMO.

I disagree.

> Compare with:
> 
> 	guard(mutex, &mcu->lock);
> 
> 	if (ctl_mask)
> 		err = ...;
> 
> 	if (!err && ext_ctl_mask)
> 		err = ...;
> 
> 	if (err)
> 		dev_err(dev, "Cannot set GPIOs: %d\n", err);
> 
> 
> Do you have a better suggestion?

Use different messages (if even needed) for different situations.

With cleanup.h in place you shouldn't supposed to have goto:s
(in simple cases like yours).

-- 
With Best Regards,
Andy Shevchenko