linux-kernel - Re: [RFC PATCH v3] media: v4l2-common: Add a helper for obtaining the clock producer

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ryzkubgzndeyufi2i4vuwmgaekyapol36oln237ki4m3fh32yl@mpr7g5woiqcf>
Date: Fri, 21 Mar 2025 13:30:43 +0100
From: Mehdi Djait <mehdi.djait@...ux.intel.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: sakari.ailus@...ux.intel.com, tomi.valkeinen@...asonboard.com, 
	jacopo.mondi@...asonboard.com, hverkuil@...all.nl, kieran.bingham@...asonboard.com, 
	naush@...pberrypi.com, mchehab@...nel.org, hdegoede@...hat.com, 
	dave.stevenson@...pberrypi.com, linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v3] media: v4l2-common: Add a helper for obtaining
 the clock producer

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