From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id AE1F8253F2A for ; Mon, 14 Jul 2025 14:10:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=213.167.242.64 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752502224; cv=none; b=driOPv2MfOg3E8FumVHRugYcFtQFU1IXe1HbKWkR/FCgpulPGCuzW8HVT/pkYCc5okBo+jGEmznPM7qUZbJsXKH6ge27plK+EJ845fSHRqdNiLnqfhJj81QoU38Y/DXRrB5PdRuvTHe6vsoVzq6+MYxbjBrcEivtITKOPTde3q8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752502224; c=relaxed/simple; bh=cqkWXI/TVI37kqLHNe0y1a2QaCXn55xsIGy3ygUkTGo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=c/23YX2Olw7M2E7khTbQ4G4gJazy6ljlgS91O1vmIV6kVv1cK9dhUrJPeZ4IVboHp6uCkOh+NDZsjG5WB035iiTEInrtCQCQh5sbXqss0/l4V7xzECo2SAt0JC2LU6uw1byXhnnwQ/w7M9HQIu2wRTD5VODK/L7UL4txl4Ghjr8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com; spf=pass smtp.mailfrom=ideasonboard.com; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b=XSJgQt/9; arc=none smtp.client-ip=213.167.242.64 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ideasonboard.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="XSJgQt/9" Received: from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi [81.175.209.231]) by perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 8C305166D; Mon, 14 Jul 2025 16:09:48 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1752502188; bh=cqkWXI/TVI37kqLHNe0y1a2QaCXn55xsIGy3ygUkTGo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=XSJgQt/9fbG0UI/FvJEt3gATfSowUaqkG1P3gzZY/SbJfg4uApjn0zwYZnAYStK/S t+4Fyz7lq0TFU+wXJYrXGnlqXFpqZk4xYQ3RsoWyotPvSAXwqi9TYLF7DmePr9PxmL c1JLI86527f0CqN2RzuHE4IOYzNqvcIWeSJyWU3M= Date: Mon, 14 Jul 2025 17:09:47 +0300 From: Laurent Pinchart To: Mehdi Djait Cc: linux-media@vger.kernel.org, Sakari Ailus , Hans Verkuil , Nicolas Dufresne , Tomi Valkeinen , Jonas Karlman , Matthew Majewski , Niklas =?utf-8?Q?S=C3=B6derlund?= Subject: Re: [PATCH 55/72] media: v4l2-common: Add legacy camera sensor clock helper Message-ID: <20250714140947.GG8243@pendragon.ideasonboard.com> References: <20250710174808.5361-1-laurent.pinchart@ideasonboard.com> <20250710174808.5361-56-laurent.pinchart@ideasonboard.com> <3rs65tdti5p7qdrj4f5fx5rkdfk5xz5xcjbr6aupdhd6zpu5xe@fm5qpcvy3ab7> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <3rs65tdti5p7qdrj4f5fx5rkdfk5xz5xcjbr6aupdhd6zpu5xe@fm5qpcvy3ab7> Hi Mehdi, On Mon, Jul 14, 2025 at 03:12:28PM +0200, Mehdi Djait wrote: > On Mon, Jul 14, 2025 at 02:55:02PM +0200, Mehdi Djait wrote: > > On Thu, Jul 10, 2025 at 08:47:51PM +0300, Laurent Pinchart wrote: > > > The recently introduced devm_v4l2_sensor_clk_get() helper aims at > > > simplifying sensor drivers by centralizing clock handling code, as well > > > as reducing cargo-cult and deprecated behaviour. > > > > > > A set of drivers implement external clock handling in a non-standard > > > way. This can't be changed as there is a high risk of breaking existing > > > platforms, but keeping the code as-is creates a risk of new drivers > > > copying deprecated behaviour. > > > > > > To fix this, introduce a new devm_v4l2_sensor_clk_get_legacy() helper > > > and use it in those driver. Compared to devm_v4l2_sensor_clk_get(), the > > > new helper takes the "clock-frequency" property into account and sets > > > the external clock rate on OF platforms, and adds the ability to specify > > > a fixed default or fallback clock rate in case the "clock-frequency" > > > property is not present. > > > > > > Signed-off-by: Laurent Pinchart > > > --- > > > drivers/media/v4l2-core/v4l2-common.c | 39 +++++++++++++++++++------ > > > include/media/v4l2-common.h | 41 ++++++++++++++++++++++++++- > > > 2 files changed, 70 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > > > index cfc78ea6276c..6f140a78e683 100644 > > > --- a/drivers/media/v4l2-core/v4l2-common.c > > > +++ b/drivers/media/v4l2-core/v4l2-common.c > > > @@ -703,24 +703,40 @@ int v4l2_link_freq_to_bitmap(struct device *dev, const u64 *fw_link_freqs, > > > } > > > EXPORT_SYMBOL_GPL(v4l2_link_freq_to_bitmap); > > > > > > -struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id) > > > +struct clk *__devm_v4l2_sensor_clk_get(struct device *dev, const char *id, > > > + bool legacy, bool fixed_rate, > > > + unsigned long clk_rate) > > > { > > > + bool of_node = is_of_node(dev_fwnode(dev)); > > > const char *clk_id __free(kfree) = NULL; > > > struct clk_hw *clk_hw; > > > struct clk *clk; > > > - bool of_node; > > > - u32 rate; > > > - int ret; > > > + u32 rate = clk_rate; > > > > [..] > > > > > + int ret = 0; > > > > > > clk = devm_clk_get_optional(dev, id); > > > if (IS_ERR(clk)) > > > return clk; > > > > > > - ret = device_property_read_u32(dev, "clock-frequency", &rate); > > > - of_node = is_of_node(dev_fwnode(dev)); > > > + /* > > > + * If the caller didn't request a fixed rate, retrieve it from the > > > + * clock-frequency property. -EINVAL indicates the property is absent, > > > + * and is not a failure. Other errors, or success with a clock-frequency > > > + * value of 0, are hard failures. > > > + */ > > > + if (!fixed_rate || !clk_rate) { > > > + ret = device_property_read_u32(dev, "clock-frequency", &rate); > > > + if (ret != -EINVAL || !rate) > > so according to the above comment, I think it should be: > if (ret != -EINVAL || (!rate && !ret)) You're right, there's an issue here. We need to return an error iif - clock-frequency can't be read for a reason different than the property being absent (to support ACPI platforms where the property could be created dynamically at a later time) ; or - clock-frequency can be read, and returns a 0 rate That translates to if ((ret && ret != -EINVAL) || (!ret && !rate)) Omitting the initial ret test would return -EINVAL when clock-frequency can be read correctly, which is not right. Do you agree with that ? > > > > [..] > > > > Let's take the case of camera sensor on a DT-system: > > > > it will call the following: > > devm_v4l2_sensor_clk_get(dev, NULL); > > -> __devm_v4l2_sensor_clk_get(dev, id, false, false, 0); > > --> with fixed_rate = 0 and clk_rate = 0 which will be assigned to > > u32 rate > > > > so it will go into the if statement and try to read the > > "clock-frequency" property, which does not exist. > > > > ret from device_property_read_u32() is -EINVAL and that is handled but > > rate will still be 0 so we will return ERR_PTR(-EINVAL) > > > > > + return ERR_PTR(-EINVAL); > > > + } > > > > > > if (clk) { > > > - if (!ret && !of_node) { > > > + /* > > > + * On non-OF platforms, or when legacy behaviour is requested, > > > + * set the clock rate if a rate has been specified by the caller > > > + * of by the clock-frequency property. > > | > > nit: +-> or ? Oops. I'll fix the typo. > > > + */ > > > + if (rate && (!of_node || legacy)) { > > > ret = clk_set_rate(clk, rate); > > > if (ret) { > > > dev_err(dev, "Failed to set clock rate: %u\n", > > > @@ -731,9 +747,14 @@ struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id) > > > return clk; > > > } > > > > > > - if (!IS_ENABLED(CONFIG_COMMON_CLK) || of_node) > > > + /* > > > + * Register a dummy fixed clock on non-OF platforms or when legacy > > > + * behaviour is requested. This required the common clock framework. > > > + */ > > > + if (!IS_ENABLED(CONFIG_COMMON_CLK) || (of_node && !legacy)) > > > return ERR_PTR(-ENOENT); > > > > > > + /* We need a rate to create a clock. */ > > > if (ret) > > > return ERR_PTR(ret == -EINVAL ? -EPROBE_DEFER : ret); > > > > > > @@ -750,4 +771,4 @@ struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id) > > > > > > return clk_hw->clk; > > > } > > > -EXPORT_SYMBOL_GPL(devm_v4l2_sensor_clk_get); > > > +EXPORT_SYMBOL_GPL(__devm_v4l2_sensor_clk_get); -- Regards, Laurent Pinchart