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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3272 bytes --]

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.

-- 
 i.