| 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
| ||
|
Message-ID: <aFqQEwdzSY123xps@kekkonen.localdomain>
Date: Tue, 24 Jun 2025 11:46:27 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Mehdi Djait <mehdi.djait@...ux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
"Nirujogi, Pratap" <pnirujog@....com>,
Pratap Nirujogi <pratap.nirujogi@....com>, mchehab@...nel.org,
hverkuil@...all.nl, bryan.odonoghue@...aro.org, krzk@...nel.org,
dave.stevenson@...pberrypi.com, hdegoede@...hat.com,
jai.luthra@...asonboard.com, tomi.valkeinen@...asonboard.com,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
benjamin.chan@....com, bin.du@....com, grosikop@....com,
king.li@....com, dantony@....com, vengutta@....com,
Svetoslav.Stoilov@....com, Yana.Zheleva@....com
Subject: Re: [PATCH v3 RESEND] media: i2c: Add OV05C10 camera sensor driver
Hi Mehdi,
On Tue, Jun 24, 2025 at 01:27:03PM +0200, Mehdi Djait wrote:
> Hi Laurent, Hi Sakari,
>
> On Tue, Jun 24, 2025 at 01:27:45PM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 24, 2025 at 10:20:34AM +0000, Sakari Ailus wrote:
> > > On Tue, Jun 24, 2025 at 10:19:35AM +0000, Sakari Ailus wrote:
> > > > On Tue, Jun 24, 2025 at 10:35:18AM +0200, Mehdi Djait wrote:
> > > > > On Tue, Jun 24, 2025 at 01:05:03AM +0300, Laurent Pinchart wrote:
> > > > > > On Mon, Jun 23, 2025 at 05:51:48PM -0400, Nirujogi, Pratap wrote:
> > > > > > > On 6/16/2025 6:49 PM, Nirujogi, Pratap wrote:
> > > > > > > >>> +static int ov05c10_probe(struct i2c_client *client)
> > > > > > > >>> +{
> > > > > > > >>> + struct ov05c10 *ov05c10;
> > > > > > > >>> + u32 clkfreq;
> > > > > > > >>> + int ret;
> > > > > > > >>> +
> > > > > > > >>> + ov05c10 = devm_kzalloc(&client->dev, sizeof(*ov05c10),
> > > > > > > >>> GFP_KERNEL);
> > > > > > > >>> + if (!ov05c10)
> > > > > > > >>> + return -ENOMEM;
> > > > > > > >>> +
> > > > > > > >>> + struct fwnode_handle *fwnode = dev_fwnode(&client->dev);
> > > > > > > >>> +
> > > > > > > >>> + ret = fwnode_property_read_u32(fwnode, "clock-frequency",
> > > > > > > >>> &clkfreq);
> > > > > > > >>> + if (ret)
> > > > > > > >>> + return dev_err_probe(&client->dev, -EINVAL,
> > > > > > > >>> + "fail to get clock freq\n");
> > > > > > > >>
> > > > > > > >> Let's try to land
> > > > > > > >> https://lore.kernel.org/linux-media/20250521104115.176950-1-
> > > > > > > >> mehdi.djait@...ux.intel.com/
> > > > > > > >> and replace the code above with devm_v4l2_sensor_clk_get().
> > > > > > > >>
> > > > > > > > Ok, we will verify on our side.
> > > > > > >
> > > > > > > We tried using devm_v4l2_sensor_clk_get() and found its required to add
> > > > > > > support for software_node to make it work with this driver.
> > > > > >
> > > > > > Why is that ?
> > > > > >
> > > > > > > Please refer
> > > > > > > the changes below and let us know if these should be submitted as a
> > > > > > > separate patch.
> > > > >
> > > > > The helper is still not merged, so no patch is required.
> > > > >
> > > > > I will see if a change is needed from the helper side or the OV05C10 side.
> > > >
> > > > I wonder if there's a better way to figure out if you're running on a DT or
> > > > ACPI based system than getting the device's parents and checking which one
> > > > you find first, DT or ACPI. I think that should work for now at least.
> > >
> > > Or, rather, checking for non-OF node here would probably work the best. I
> > > wouldn't expect these to be software node based on DT systems ever.
> >
> > Until it happens :-) And we'll handle it then.
>
> So we have the following:
>
> - The problem with this driver is due to lack of proper ACPI
> description. HW is already shipping and AMD will work on better ACPI
> description for future models. See [1]
>
> - software_node can also be used on DT systems
>
> [1] https://lore.kernel.org/lkml/0d801367-da24-4596-83d9-08ccd89ca670@redhat.com/
>
> Now going back to the helper. If we want to support this case:
>
> Approach 1: software_node || acpi
>
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -682,16 +682,17 @@ struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> const char *clk_id __free(kfree) = NULL;
> struct clk_hw *clk_hw;
> struct clk *clk;
> - bool acpi_node;
> + bool acpi_sw_node;
> u32 rate;
> int ret;
>
> clk = devm_clk_get_optional(dev, id);
> ret = device_property_read_u32(dev, "clock-frequency", &rate);
> - acpi_node = is_acpi_node(dev_fwnode(dev));
> + acpi_sw_node = is_acpi_node(dev_fwnode(dev)) ||
> + is_software_node(dev_fwnode(dev));
>
> if (clk) {
> - if (!ret && acpi_node) {
> + if (!ret && acpi_sw_node) {
> ret = clk_set_rate(clk, rate);
> if (ret) {
> dev_err(dev, "Failed to set clock rate: %u\n",
> @@ -705,7 +706,7 @@ struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> if (ret)
> return ERR_PTR(ret);
>
> - if (!IS_ENABLED(CONFIG_COMMON_CLK) || !acpi_node)
> + if (!IS_ENABLED(CONFIG_COMMON_CLK) || !acpi_sw_node)
> return ERR_PTR(-ENOENT);
>
> if (!id) {
>
>
> Approach 2: of_node
>
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -682,16 +682,16 @@ struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> const char *clk_id __free(kfree) = NULL;
> struct clk_hw *clk_hw;
> struct clk *clk;
> - bool acpi_node;
> + bool of_node;
> u32 rate;
> int ret;
>
> clk = devm_clk_get_optional(dev, id);
> ret = device_property_read_u32(dev, "clock-frequency", &rate);
> - acpi_node = is_acpi_node(dev_fwnode(dev));
> + of_node = is_of_node(dev_fwnode(dev));
>
> if (clk) {
> - if (!ret && acpi_node) {
> + if (!ret && !of_node) {
> ret = clk_set_rate(clk, rate);
> if (ret) {
> dev_err(dev, "Failed to set clock rate: %u\n",
> @@ -705,7 +705,7 @@ struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> if (ret)
> return ERR_PTR(ret);
>
> - if (!IS_ENABLED(CONFIG_COMMON_CLK) || !acpi_node)
> + if (!IS_ENABLED(CONFIG_COMMON_CLK) || of_node)
> return ERR_PTR(-ENOENT);
>
> if (!id) {
I'm in favour of the latter but both should be workable.
Speaking of return values, devm_clk_get_optional() may also return
-EPROBE_DEFER. That needs to be handled.
And further on -EPROBE_DEFER, I think the helper should return
-EPROBE_DEFER if the "clock-frequency" property doesn't exist on non-OF
nodes. That signals the required software nodes required on Intel Windows
definitions/ipu-bridge or AMD systems aren't in place yet so really probing
should be deferred. This would allow removing the hacks that return
-EPROBE_DEFER in sensor drivers when no graph endpoint is found.
--
Regards,
Sakari Ailus
Powered by blists - more mailing lists