Re: [PATCH 1/2] ACPI: Get acpi_device's parent from the parent field

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Nov 10, 2021 at 02:02:52PM +0200, Sakari Ailus wrote:
> On Wed, Nov 10, 2021 at 10:56:42AM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 10, 2021 at 10:21:36AM +0200, Sakari Ailus wrote:
> > > On Wed, Nov 10, 2021 at 10:18:20AM +0200, Andy Shevchenko wrote:
> > > > On Wed, Nov 10, 2021 at 10:09:04AM +0200, Sakari Ailus wrote:
> > > > > On Tue, Nov 09, 2021 at 02:19:13PM +0200, Andy Shevchenko wrote:
> > > > > > On Tue, Nov 09, 2021 at 01:19:34PM +0200, Sakari Ailus wrote:

...

> > > > > > > -	} else if (is_acpi_device_node(fwnode)) {
> > > > > > > +	}
> > > > > > 
> > > > > > > +	if (is_acpi_device_node(fwnode)) {
> > > > > > 
> > > > > > Unneeded change. Yes I know that 'else' here can be skipped. But in such cases
> > > > > > it's a trade-off between changes, code readability and maintenance. Since here
> > > > > > it's a fix, backporting concerns are also play role.
> > > > > 
> > > > > The patch applies cleanly to 5.5, the oldest kernel where it's needed.
> > > > 
> > > > Why? I don't see how this affects the workflow.
> > > > 
> > > > > Do you prefer another patch to remove the else clause?
> > > > 
> > > > Nope.
> > > > 
> > > > > I think it's a bit overkill...
> > > > 
> > > > Exactly, that's why the question is why have you split the if-else-if to
> > > > two if:s?
> > > 
> > > The else clause is useless, I think the code simply looks better without
> > > it.
> > 
> > I see a contradiction here:
> > 
> > Statement 1: 'else' is useless.
> > Statement 2: patch to remove it is overkill.
> 
> There's no contradiction.
> 
> I argue doing that in a separate patch is waste of everyone's time. As
> simple as that.

And this is precisely my point. But my other point is that doing this in the
fix patch is a churn. Bottom line, this part of the change shouldn't be here.

Also, it increases LOC counts. You may submit a separate patch to fix all of
the redundant 'else':s and we will see the necessity of them. But it's does
not belong to the patch you provided here.

Hope this clarifies my point.

-- 
With Best Regards,
Andy Shevchenko