Message-ID: <aF6_jVJYCXeKZfXo@smile.fi.intel.com>
Date: Fri, 27 Jun 2025 18:58:05 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Rahul Pathak <rpathak@...tanamicro.com>
Cc: Anup Patel <apatel@...tanamicro.com>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jassi Brar <jassisinghbrar@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Rafael J . Wysocki" <rafael@...nel.org>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Uwe Kleine-König <ukleinek@...nel.org>,
	Palmer Dabbelt <palmer@...belt.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Len Brown <lenb@...nel.org>, Sunil V L <sunilvl@...tanamicro.com>,
	Leyfoon Tan <leyfoon.tan@...rfivetech.com>,
	Atish Patra <atish.patra@...ux.dev>,
	Andrew Jones <ajones@...tanamicro.com>,
	Samuel Holland <samuel.holland@...ive.com>,
	Anup Patel <anup@...infault.org>, linux-clk@...r.kernel.org,
	devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 09/23] clk: Add clock driver for the RISC-V RPMI clock
 service group

On Fri, Jun 27, 2025 at 08:36:41PM +0530, Rahul Pathak wrote:

...

> > > > > +     if (ret || rx.status)
> > > > > +             return 0;
> > > >
> > > > Why rx.status can't be checked before calling to a sending message?
> > > > Sounds like the rpmi_mbox_init_send_with_response() links rx to msg somehow.
> > > > If this is the case, use msg here, otherwise move the check to be in the
> > > > correct place.
> > >
> > > Yes, the rpmi_mbox_init_send_with_response is a helper function which links
> > > the rx to msg. It's a very simple function which only performs assignments.
> > >
> > > Using msg instead of rx directly will require additional typecasting
> > > which will only clutter
> > > I can add a comment if that helps wherever the rpmi_mbox_init_send_with_response
> > > is used.
> >
> > This is besides harder-to-read code is kinda of layering violation.
> > If you afraid of a casting, add a helper to check for the status error.
> > Comment won't help much as making code better to begin with.
> 
> Why using rx is the issue in the first place when it's the same layer
> which links the rx with msg using the helper function and then
> uses it directly?  Infact using rx directly avoids unnecessary code
> which is only increasing redundant code which ultimately results in
> same thing. Even if I add a helper function that will require additional
> pointer passing with NULL checking which all is currently avoided.
> And, we are not just talking about rx.status but a lot of other fields.

Because it's simply bad code, look at the simplified model:

	int foo, bar;
	int ret;

	func_1(..., &foo, &bar);
	ret = func_2(&foo);
	if (ret || bar)
		...do something...

When one reads this code the immediate reaction will be like mine.
This is also (without deeper understanding) tempting to someone
who even thinks that the code can be simplified (w/o knowing that it
may not) to change it as

	func_1(..., &foo, &bar);
	if (bar)
		...do something...

	ret = func_2(&foo);
	if (ret)
		...do something...

Using msg is the right thing to do. In that way there is no questions asked
and everything is clear. Also why layering violation? Because the conditional
requires to know the guts of rx in the code which doesn't use rx that way
(or rather using it as semi-opaque object).

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists