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

[Hans de Goede <hdegoede@redhat.com>]

Hi Sakari,

On 6-May-25 9:45 AM, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Wed, Apr 02, 2025 at 04:20:52AM +0300, Laurent Pinchart wrote:
>> On Mon, Mar 31, 2025 at 03:17:22PM -0400, Nirujogi, Pratap wrote:
>>> On 3/28/2025 9:18 PM, Bryan O'Donoghue wrote:
>>>> On 28/03/2025 21:42, Pratap Nirujogi wrote:
>>>>> From: Bin Du <Bin.Du@amd.com>
>>>>
>>>>> +static const struct i2c_device_id ov05c10_id[] = {
>>>>> +     {"ov05c10", 0 },
>>>>> +     { }
>>>>> +};
>>>>
>>>> There's an IPU6/IPU7 version of this driver.
>>>>
>>>> https://github.com/intel/ipu6-drivers/blob/master/drivers/media/i2c/ 
>>>> ov05c10.c
>>>>
>>>> Perhaps you could import the Intel ACPI name contained in there too.
>>>>
>>> sure, will add Intel ACPI names too in V3. To be specific, I'm going to 
>>> add the below table in addition to the existing "struct i2c_device_id 
>>> ov05c10_id[]" table:
>>>
>>> static const struct acpi_device_id ov05c10_acpi_ids[] = {
>>> 	{ "OVTI05C1" },
>>> 	{}
>>> };
>>
>> You could drop the i2c_device_id table if you added an OF device ID
>> table, but you'll need DT bindings for that. Sakari, any best practice
>> rule in this area ?
> 
> I don't think there should be a need for an I²C ID in any case, having just
> ACPI _HID is fine.

AMD laptops with MIPI cameras (where this sensor is used) do not
properly describe the sensor as an ACPI I2C device in their
ACPI tables. So instead this is going to rely on manual i2c-client
instantiation by some pdx86 glue code, see:

https://lore.kernel.org/platform-driver-x86/20250505171302.4177445-1-pratap.nirujogi@amd.com/

To have a driver bind to such manually instantiated
i2c-clients there must be an i2c_device_id table.

Regards,

Hans





>>>>> +
>>>>> +MODULE_DEVICE_TABLE(i2c, ov05c10_id);
>>>>> +
>>>>> +static struct i2c_driver ov05c10_i2c_driver = {
>>>>> +     .driver = {
>>>>> +             .name = DRV_NAME,
>>>>> +             .pm = pm_ptr(&ov05c10_pm_ops),
>>>>> +     },
>>>>> +     .id_table = ov05c10_id,
>>>>> +     .probe = ov05c10_probe,
>>>>> +     .remove = ov05c10_remove,
>>>>> +};
>>>>> +
>>>>> +module_i2c_driver(ov05c10_i2c_driver);
>>>>> +
>>>>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@amd.com>");
>>>>> +MODULE_AUTHOR("Venkata Narendra Kumar Gutta <vengutta@amd.com>");
>>>>> +MODULE_AUTHOR("Bin Du <bin.du@amd.com>");
>>>>> +MODULE_DESCRIPTION("OmniVision OV05C1010 sensor driver");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>>> Why v2 ? Checkpatch will complain about v2 and BTW the IPU6 driver above
>>>> is GPL not GPL v2.
>>>
>>> sure, will replace "GPL v2" with "GPL" in V3.
>>
>