Re: [PATCH v3 RESEND] media: i2c: Add OV05C10 camera sensor driver
From: Nirujogi, Pratap
Date: Tue Jun 24 2025 - 16:02:14 EST
On 6/24/2025 2:26 PM, Nirujogi, Pratap wrote:
Hi Mehdi, Sakari, Laurent,
On 6/24/2025 7:27 AM, Mehdi Djait wrote:
Caution: This message originated from an External Source. Use proper
caution when opening attachments, clicking links, or responding.
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@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 ?
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]
Thanks Mehdi for clarifying and providing the reference from the
associated x86/platform driver patch.
yes, thats true we have to add software_nodes to mitigate the issue
caused by incomplete description of camera device in ACPI tables.
For future models we are working on a plan to address this issue
following the MIPI DisCo Imaging Spec suggested by Sakari to properly
describe the camera device in ACPI. Please see [A]
Once again thanks everyone for the support!
[A] https://lore.kernel.org/
lkml/2a9ba94e-7985-4ba9-88c6-45b8cf4d001f@xxxxxxx/
- software_node can also be used on DT systems
[1] https://lore.kernel.org/lkml/0d801367-
da24-4596-83d9-08ccd89ca670@xxxxxxxxxx/
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) {
Thanks for proposing "approach 2 using !swnode", I verified and confirm
it works at my end.
sorry, fixing the typo - I meant "approach 2 using !of_node".
Thanks,
Pratap
Thanks,
Pratap
--
Kind Regards
Mehdi Djait