Re: [PATCH v3 RESEND] media: i2c: Add OV05C10 camera sensor driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@linux.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 ?

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 ?

> >> 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