Re: [PATCH v3 RESEND] media: i2c: Add OV05C10 camera sensor driver
From: Nirujogi, Pratap
Date: Tue Jun 24 2025 - 15:49:36 EST
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@xxxxxxxxxxxxxxx/
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@xxxxxxx/
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