Hi Laurent,
thank you for the review!
On Fri, Mar 21, 2025 at 12:11:24PM +0200, Laurent Pinchart wrote:
> Hi Mehdi,
>
> Thank you for the patch.
>
> On Fri, Mar 21, 2025 at 10:38:14AM +0100, Mehdi Djait wrote:
SNIP
> > +
> > +struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> > +{
> > + struct clk_hw *clk_hw;
> > + struct clk *clk;
> > + u32 rate;
> > + int ret;
> > +
> > + clk = devm_clk_get_optional(dev, id);
> > + if (clk)
> > + return clk;
> > +
> > +#ifdef CONFIG_COMMON_CLK
>
> This patch will cause warnings when CONFIG_COMMON_CLK is disabled. Could
> you use
>
> if (IS_REACHABLE(CONFIG_COMMON_CLK)) {
> ...
> }
>
> instead ? It will also ensure that all code gets compile-tested, even
> when CONFIG_COMMON_CLK is disabled ?
>
> If you want to minimize implementation, you could write
>
> if (!IS_REACHABLE(CONFIG_COMMON_CLK))
> return ERR_PTR(-ENOENT);
>
> and keep the code below as-is.
>
this is indeed way better! I will change this in the v3
> > + if (!is_acpi_node(dev_fwnode(dev)))
> > + return ERR_PTR(-ENOENT);
> > +
> > + ret = device_property_read_u32(dev, "clock-frequency", &rate);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + if (!id) {
> > + id = devm_kasprintf(dev, GFP_KERNEL, "clk-%s", dev_name(dev));
>
> As far as I understand, the name doesn't need to stay valid after
> devm_clk_hw_register_fixed_rate() returns. You can call kasprintf here,
> and call kfree after devm_clk_hw_register_fixed_rate(). You could use
> __free to manage the memory life time:
>
> const char *clk_id __free(kfree) = NULL;
>
> if (!id) {
> clk_id = kasprintf(GFP_KERNEL, "clk-%s", dev_name(dev));
> if (!clk_id)
> return ERR_PTR(-ENOMEM);
> id = clk_id;
> }
>
Ack.
--
Kind Regards
Mehdi Djait
