[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHp75VfP2AB45mn6gB3suCAO9iT3bOWZ=7m9U7E087Lac0P3gg@mail.gmail.com>
Date: Fri, 10 May 2024 16:15:43 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Johan Hovold <johan@...nel.org>
Cc: Johan Hovold <johan+linaro@...nel.org>, Lee Jones <lee@...nel.org>,
Mark Brown <broonie@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>, Das Srinagesh <quic_gurus@...cinc.com>,
Satya Priya <quic_c_skakit@...cinc.com>, Stephen Boyd <swboyd@...omium.org>,
linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH 11/13] mfd: pm8008: rework driver
On Thu, May 9, 2024 at 12:42 PM Johan Hovold <johan@...nel.org> wrote:
> On Mon, May 06, 2024 at 10:18:58PM +0300, Andy Shevchenko wrote:
> > Mon, May 06, 2024 at 05:08:28PM +0200, Johan Hovold kirjoitti:
..
> > > +static void devm_irq_domain_fwnode_release(void *res)
> > > +{
> >
> > > + struct fwnode_handle *fwnode = res;
> >
> > Unneeded line, can be
> >
> > static void devm_irq_domain_fwnode_release(void *fwnode)
> >
> > > + irq_domain_free_fwnode(fwnode);
> > > +}
>
> I think I prefer it this way for clarity and for type safety in the
> unlikely even that the argument to irq_domain_free_fwnode() would ever
> change.
If it ever changes, the allocation part most likely would need an
update and since devm_add_action() takes this type of function, I
don't believe the argument would ever change from void * to something
else. With this it just adds an additional burden on the conversion.
> > > + name = devm_kasprintf(dev, GFP_KERNEL, "%pOF-internal", dev->of_node);
> >
> > You are using fwnode for IRQ domain and IRQ domain core uses fwnode, why OF here?
> >
> > name = devm_kasprintf(dev, GFP_KERNEL, "%pfw-internal", dev_fwnode(dev));
>
> This driver only support OF so why bother.
Sure, but it makes a bit of inconsistency. Besides that dereferencing
of_node might also add a burden one day we want to get rid of it or
move it somewhere else, or convert to the list_head or so.
dev_of_node(dev) in this case prevents from looking into this case.
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists