| 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: <20250624190018.GF20757@pendragon.ideasonboard.com>
Date: Tue, 24 Jun 2025 22:00:18 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: "Nirujogi, Pratap" <pnirujog@....com>
Cc: Pratap Nirujogi <pratap.nirujogi@....com>, mchehab@...nel.org,
sakari.ailus@...ux.intel.com, 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,
Mehdi Djait <mehdi.djait@...ux.intel.com>
Subject: Re: [PATCH v3 RESEND] media: i2c: Add OV05C10 camera sensor driver
On Tue, Jun 24, 2025 at 02:49:36PM -0400, Nirujogi, Pratap wrote:
> On 6/23/2025 8:19 PM, Laurent Pinchart wrote:
> > On Mon, Jun 23, 2025 at 07:28:46PM -0400, Nirujogi, Pratap wrote:
> >> On 6/23/2025 6:05 PM, 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 ?
> >>
> >> Its because the i2c_client device is initialized with swnode in the
> >> x86/platform driver.
> >>
> >> https://github.com/torvalds/linux/blob/master/drivers/platform/x86/amd/amd_isp4.c#L235
> >
> > So there's no information provided in the _DSD for the sensor ?
>
> yes, camera device was not properly described in the current model, we
> are going to address this issue for future models following the MIPI
> DisCo Imaging spec.
Any idea how far in the future that will be ? I suppose it won't be done
overnight, how many new machine models will get to the market with a
need for a platform driver ?
Will the Windows driver team also switch to MIPI DisCo Imaging ?
> > Looking at that platform driver, it matches the device based on the
> > sensor ACPI HID only ("OMNI5C10"). That doesn't seem quite right, I
> > think you need a DMI match as well. You can't assume that OMNI5C10,
> > which identifies the sensor, will always map to specific platform
> > integration data (connected to an AMD ISP, using a particular link
> > frequency, ...), can you ?
>
> Initally we had dmi checks, but as the driver matured through review
> iterations, we switched to ACPI driver approach and felt the bus
> traversal to find the matching HID device and dmi checks are no longer
> required. The (_HID, "OMNI5C10") used is specific to this platform and
> shouldn't be impacting other platform even though the dmi checks are
> skipped. Please see [A].
How do you guarantee that the same sensor will not be used with the
OMNI5C10 ACPI HID on another AMD-based laptop that would require a
different link frequency or use a different number of data lanes ?
> [A] https://lore.kernel.org/lkml/8d892845-e134-4553-a6af-55d785c1ae98@amd.com/
>
> >>>> Please refer
> >>>> the changes below and let us know if these should be submitted as a
> >>>> separate patch.
> >>>
> >>> Mehdi, do you have any comment ?
> >>>
> >>>> ---
> >>>> @@ -645,16 +645,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 is_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));
> >>>> + is_node = is_acpi_node(dev_fwnode(dev)) || is_software_node(dev_fwnode(dev));
> >>>>
> >>>> if (clk) {
> >>>> - if (!ret && acpi_node) {
> >>>> + if (!ret && is_node) {
> >>>> ret = clk_set_rate(clk, rate);
> >>>> if (ret) {
> >>>> dev_err(dev, "Failed to set clock rate: %u\n",
> >>>> @@ -668,7 +668,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) || !is_node)
> >>>> return ERR_PTR(-ENOENT);
> >>>>
> >>>> if (!id) {
> >>>> ----
--
Regards,
Laurent Pinchart
Powered by blists - more mailing lists