Re: [PATCH v12] platform/x86: Add AMD ISP platform config for OV05C10

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

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.