| 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: <db804aaf-3842-471b-874a-08d5374a76bf@amd.com>
Date: Tue, 24 Jun 2025 16:01:51 -0400
From: "Nirujogi, Pratap" <pnirujog@....com>
To: Mehdi Djait <mehdi.djait@...ux.intel.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
Pratap Nirujogi <pratap.nirujogi@....com>, mchehab@...nel.org,
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
Subject: Re: [PATCH v3 RESEND] media: i2c: Add OV05C10 camera sensor driver
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@...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 ?
>>>>>>>
>>>>>>>> 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@....com/
>
>> - software_node can also be used on DT systems
>>
>> [1] https://lore.kernel.org/lkml/0d801367-
>> da24-4596-83d9-08ccd89ca670@...hat.com/
>>
>> 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
>
Powered by blists - more mailing lists