Re: [PATCH] media: i2c: Add OV05C camera sensor driver

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

On 3/29/2025 12:25 AM, Krzysztof Kozlowski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On 28/03/2025 23:19, Nirujogi, Pratap wrote:
>> Hi Krzysztof,
>>
>> Thanks for reviewing and extremely sorry for the delayed response.
>>
>> We have submitted V2 patch based on the review feedback. Can you please
>> help to review latest V2 patch and let us know your feedback.
>>
>> Thanks,
>> Pratap
>>
>> On 3/1/2025 8:30 AM, Krzysztof Kozlowski wrote:
>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>
>>>
>>> On 28/02/2025 17:53, Pratap Nirujogi wrote:
>>>> Add driver for OmniVision 5.2M OV05C10 sensor. This driver
>>>> supports only the full size normal 2888x1808@30fps 2-lane
>>>> sensor profile.
>>>>
>>>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com>
>>>> ---
>>>>    drivers/media/i2c/Kconfig  |   10 +
>>>>    drivers/media/i2c/Makefile |    1 +
>>>>    drivers/media/i2c/ov05c.c  | 1031 ++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 1042 insertions(+)
>>>>    create mode 100644 drivers/media/i2c/ov05c.c
>>>>
>>>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>>>> index 8ba096b8ebca..fd160feabc41 100644
>>>> --- a/drivers/media/i2c/Kconfig
>>>> +++ b/drivers/media/i2c/Kconfig
>>>> @@ -337,6 +337,16 @@ config VIDEO_OG01A1B
>>>>           To compile this driver as a module, choose M here: the
>>>>           module will be called og01a1b.
>>>>
>>>> +config VIDEO_OV05C
>>>> +     tristate "OmniVision OV05 sensor support"
>>>> +     select V4L2_CCI_I2C
>>>> +     help
>>>> +       This is a Video4Linux2 sensor driver for the OmniVision
>>>> +       OV05C camera.
>>>> +
>>>> +       To compile this driver as a module, choose M here: the
>>>> +       module will be called OV05C.
>>>> +
>>>>    config VIDEO_OV01A10
>>>>         tristate "OmniVision OV01A10 sensor support"
>>>>         help
>>>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>>>> index fbb988bd067a..08bfc2d59be2 100644
>>>> --- a/drivers/media/i2c/Makefile
>>>> +++ b/drivers/media/i2c/Makefile
>>>> @@ -80,6 +80,7 @@ obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o
>>>>    obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o
>>>>    obj-$(CONFIG_VIDEO_MT9V111) += mt9v111.o
>>>>    obj-$(CONFIG_VIDEO_OG01A1B) += og01a1b.o
>>>> +obj-$(CONFIG_VIDEO_OV05C) += ov05c.o
>>>>    obj-$(CONFIG_VIDEO_OV01A10) += ov01a10.o
>>>>    obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o
>>>>    obj-$(CONFIG_VIDEO_OV08D10) += ov08d10.o
>>>> diff --git a/drivers/media/i2c/ov05c.c b/drivers/media/i2c/ov05c.c
>>>> new file mode 100644
>>>> index 000000000000..96c4f74af4a9
>>>> --- /dev/null
>>>> +++ b/drivers/media/i2c/ov05c.c
>>>> @@ -0,0 +1,1031 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +/*
>>>> + * Copyright (C) 2025 Advanced Micro Devices, Inc. All rights reserved.
>>>> + * All Rights Reserved.
>>>> + *
>>>> + * Permission is hereby granted, free of charge, to any person obtaining a
>>>> + * copy of this software and associated documentation files (the
>>>> + * "Software"), to deal in the Software without restriction, including
>>>> + * without limitation the rights to use, copy, modify, merge, publish,
>>>> + * distribute, sub license, and/or sell copies of the Software, and to
>>>> + * permit persons to whom the Software is furnished to do so, subject to
>>>> + * the following conditions:
>>>> + *
>>>> + * The above copyright notice and this permission notice (including the
>>>> + * next paragraph) shall be included in all copies or substantial portions
>>>> + * of the Software.
>>>> + *
>>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
>>>> + * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
>>>> + * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
>>>> + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
>>>> + * USE OR OTHER DEALINGS IN THE SOFTWARE.
>>>
>>> What's with AMD? Second patch that day, same issues.
>>>
>>> Drop license boilerplate.
>>>
>> Done. Updated copyright header and license in V2.
>>
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/acpi.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/units.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +#include <linux/gpio.h>
>>>> +#include <media/v4l2-ctrls.h>
>>>> +#include <media/v4l2-device.h>
>>>> +#include <media/v4l2-fwnode.h>
>>>> +#include <media/v4l2-cci.h>
>>>
>>>
>>> ...
>>>
>>>> +
>>>> +static int ov05c_probe(struct i2c_client *client)
>>>> +{
>>>> +     struct ov05c *ov05c;
>>>> +     int i, ret;
>>>> +
>>>> +     ov05c = devm_kzalloc(&client->dev, sizeof(*ov05c), GFP_KERNEL);
>>>> +     if (!ov05c)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     client->dev.init_name = DRV_NAME;
>>>> +
>>>> +     /* create sensor enable gpio control */
>>>> +     ov05c->enable_gpio = devm_gpiod_get(&client->dev, "sensor0_enable", GPIOD_OUT_LOW);
>>>
>>>
>>> s/sensor0_enable/enable/
>>>
>> Is it okay to use "sensor0_enabled" as connection id? We used this name
>> to differentiate the two GPIO PINs that has to be programmed for RGB
>> streaming to work with this sensor.
> 
> How much time did you give me to respond here? 20 minutes. In the middle
> of my night. And then you send v2, without waiting for my answer.
> 
> That's not acceptable for me.
> 
My apologies for the inconvenience caused.

> Best regards,
> Krzysztof