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

["Nirujogi, Pratap" <pnirujog@amd.com>]

Hi Laurent,

On 6/23/2025 8:19 PM, Laurent Pinchart wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> 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 ?
> 
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.

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

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