| 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: <6f5a9eda-c499-4b0b-805e-f75abea46098@amd.com>
Date: Tue, 24 Jun 2025 15:49:00 -0400
From: "Nirujogi, Pratap" <pnirujog@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.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
Hi Laurent,
On 6/24/2025 3:00 PM, Laurent Pinchart wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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 ?
>
We are aiming to have MIPI DisCo Imaging ready for models based on
ISP5.x. However, I don't have specific information on when the ISP5.x
based APU models will be released at this point of time. We may use the
current mitigation using the platform driver for the launches that are
based on ISP4.x. But, to be honest, I don't have visibility into future
ISP4.x launches as well.
> Will the Windows driver team also switch to MIPI DisCo Imaging ?
>
yes, even the windows team intends to switch to MIPI DisCo Imaging, they
shall need to coordinate with MSFT to finalize the plan.
Thanks,
Pratap
>>> 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 ?
>
Currently, no such scenario exists AFAIK. OMNI5C10 is tied to the models
based on "AMD RYZEN AI MAX PRO 385 w/ Radeon 8050S". We can for sure add
the DMI checks if really necessary for the current or future models.
Thanks,
Pratap
>> [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