[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <07898a21-2e8f-4413-90a1-076460cd55b8@amd.com>
Date: Thu, 8 May 2025 19:00:24 -0400
From: "Nirujogi, Pratap" <pnirujog@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Ilya K <me@...ti.me>, Pratap Nirujogi <pratap.nirujogi@....com>,
Hans de Goede <hdegoede@...hat.com>, W_Armin@....de,
mario.limonciello@....com, platform-driver-x86@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>, benjamin.chan@....com, bin.du@....com,
gjorgji.rosikopulos@....com, king.li@....com, dantony@....com
Subject: Re: [PATCH v12] platform/x86: Add AMD ISP platform config for OV05C10
Hi Ilpo,
On 5/8/2025 4:52 AM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Wed, 7 May 2025, Nirujogi, Pratap wrote:
>> On 5/6/2025 8:53 AM, Ilpo Järvinen wrote:
>>> Caution: This message originated from an External Source. Use proper caution
>>> when opening attachments, clicking links, or responding.
>>>
>>>
>>> On Tue, 6 May 2025, Ilya K wrote:
>>>
>>>>> +#define AMDISP_OV05C10_I2C_ADDR 0x10
>>>>> +#define AMDISP_OV05C10_PLAT_NAME "amdisp_ov05c10_platform"
>>>>> +#define AMDISP_OV05C10_HID "OMNI5C10"
>>>>> +#define AMDISP_OV05C10_REMOTE_EP_NAME "ov05c10_isp_4_1_1"
>>>>> +#define AMD_ISP_PLAT_DRV_NAME "amd-isp4"
>>>>
>>>> Hey folks, I know v12 might be a bit too late for this one, but I've got
>>>> another device here (Asus GZ302EA tablet) with a very similar camera
>>>> setup, but a different sensor (OV13B10), and it looks like this driver
>>>> just assumes a certain hardcoded configuration... I wonder if it makes
>>>> sense to reorganize the code so that more sensor configurations can be
>>>> added without making a separate module? I'd be happy to help with
>>>> refactoring/testing/etc, if people are interested.
>>>
>>> v12 is not too late, and besides, v9..v12 has happened within 5 days
>>> which is rather short time (hint to the submitter that there's no need
>>> to burn patch series version numbers at that speed :-)).
>>>
>>> I'll give folks some time to sort this out if you need to add e.g., some
>>> driver_data instead.
>>>
>>> --
>>> i.
>>>
>> Hi Ilya, Ilpo,
>>
>> I agree with the suggestion, but how about taking-up the refactoring part in a
>> separate patch. Yes this patch focussed on supporting OV05C10 and even the
>> code review proceeded with this understanding. Refactoring now for generic
>> support would require changes that would undo some of the recent review
>> feedback (especially related to global variables usage). Please let us know
>> what do you think.
>
> Hi,
>
> If you refer to comments given to v7 that resulted in removal of swnodes
> field from struct amdisp_platform (and some other fields along with it), I
> don't think the comment was given to mean that you could not have
> platform info structure (const struct amdisp_platform_info that never was
> been there) but that it should be separate struct from the runtime one
> (struct amdisp_platform). The runtime struct can have a pointer to the
> info struct if it's content is needed after probe.
>
> When the platform info struct exists, pointer to it can be put into
> driver_data in amdisp_sensor_ids. I don't expect you to necessarily add
> the other sensor there but I'd like to see this adapted such that it can
> be relatively easily added which likely requires having that separate
> struct for platform info.
>
> So in a sense, it undoes some of the changes done after v7 but looking
> into history of this patch, it looks the post v7 patches went slightly
> into wrong direction which makes adding next device harder than it needs
> to be (I'm sorry I didn't realize this sooner). TBH, I don't think adding
> the info struct is that much extra effort for you given what you had in
> v7, the info just needs another struct separate from struct
> amdisp_platform but the ingrediments kind of where there already.
>
Thanks for clarifying and providing direction. I agree adding the new
amdisp_platform_info structure should be straightforward. I will
implement the changes and submit the new patch v13.
Thanks,
Pratap
> --
> i.
Powered by blists - more mailing lists