linux-kernel - Re: [PATCH v3 2/2] iio: frequency: adf4377: add clk provider support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHp75Vc7bmScSdR2R4US-1h9MwE+2tx_Rh+UosdWNk5J70xQpA@mail.gmail.com>
Date: Tue, 25 Nov 2025 18:22:40 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>, 
	Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Michael Turquette <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, 
	Dragos Bogdan <dragos.bogdan@...log.com>, linux-iio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-clk@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iio: frequency: adf4377: add clk provider support

On Tue, Nov 25, 2025 at 6:21 PM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
> On Tue, Nov 25, 2025 at 5:48 PM Antoniu Miclaus
> <antoniu.miclaus@...log.com> wrote:

...

> > +static int adf4377_clk_register(struct adf4377_state *st)
> > +{
> > +       struct spi_device *spi = st->spi;

Also consider to have

  struct device *dev = &spi->dev;

here, and use it below to make less LoCs and better to read code.

> > +       struct clk_init_data init;
> > +       struct clk_parent_data parent_data;
> > +       int ret;
> > +
> > +       if (!device_property_present(&spi->dev, "#clock-cells"))
> > +               return 0;
> > +
> > +       if (device_property_read_string(&spi->dev, "clock-output-names", &init.name)) {
>
> I would rather split this to two
>
>   ret = device_property_read_string(...);
>   if (ret) {
>     ...
>   }
>
> which makes the intention more explicit, otherwise it's not clear what
> the return values are and what you do with them (since I know it's
> int, the rule of thumb to assign to int variable and check it, for
> boolean it's fine to do if (boolean_fn(...)) in the code).
>
> > +               init.name = devm_kasprintf(&spi->dev, GFP_KERNEL, "%s-clk",
> > +                                          fwnode_get_name(dev_fwnode(&spi->dev)));
>
> %pfw / %pfwP
>
> > +               if (!init.name)
> > +                       return -ENOMEM;
> > +       }
> > +
> > +       parent_data.fw_name = "ref_in";
> > +
> > +       init.ops = &adf4377_clk_ops;
> > +       init.parent_data = &parent_data;
> > +       init.num_parents = 1;
> > +       init.flags = CLK_SET_RATE_PARENT;
> > +
> > +       st->hw.init = &init;
> > +       ret = devm_clk_hw_register(&spi->dev, &st->hw);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = devm_of_clk_add_hw_provider(&spi->dev, of_clk_hw_simple_get, &st->hw);
> > +       if (ret)
> > +               return ret;
> > +
> > +       st->clkout = st->hw.clk;
> > +
> > +       return 0;
> > +}
>
> Otherwise LGTM.
>
> --
> With Best Regards,
> Andy Shevchenko



-- 
With Best Regards,
Andy Shevchenko