* [PATCH v2] media: i2c: Add OV05C10 camera sensor driver @ 2025-03-28 21:42 ` Pratap Nirujogi 2025-03-29 1:18 ` Bryan O'Donoghue 2025-03-29 4:30 ` Krzysztof Kozlowski 0 siblings, 2 replies; 11+ messages in thread From: Pratap Nirujogi @ 2025-03-28 21:42 UTC (permalink / raw) To: mchehab, sakari.ailus, hverkuil, laurent.pinchart, dave.stevenson, krzk, dan.carpenter Cc: linux-media, linux-kernel, benjamin.chan, bin.du, grosikop, king.li, dantony, Bin Du, Venkata Narendra Kumar Gutta, Pratap Nirujogi From: Bin Du <Bin.Du@amd.com> Add driver for OmniVision 5.2M OV05C10 sensor. This driver supports only the full size normal 2888x1808@30fps 2-lane sensor profile. Co-developed-by: Venkata Narendra Kumar Gutta <vengutta@amd.com> Signed-off-by: Venkata Narendra Kumar Gutta <vengutta@amd.com> Co-developed-by: Bin Du <bin.du@amd.com> Signed-off-by: Bin Du <bin.du@amd.com> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@amd.com> --- v1 -> v2: - Fixed V4L2 compliance issues. Ensured 100% compliance for ioctls supported. - Implemented the .enable_streams() and .disable_streams(). - Implemented init_state() function instead of open() in ov05c10_internal_ops. - Replaced ov05c10_get_pad_format() with v4l2_subdev_get_fmt() for v4l2_subdev_pad_ops.get_fmt(). - Added helper function for sensor page switching. Implemented register read / write wrapper functions that checks the need for page switching before cci_read() and cci_write() operations. - Added support to parse v4l2 endpoint node using the fwnode handle from i2c_client driver. - Renamed driver as OV05C10 to reflect the complete sensor name. - Enabled PM runtime auto suspend / resume. - Fixed coding errors and other cosmetic corrections based on the review feedback. MAINTAINERS | 8 + drivers/media/i2c/Kconfig | 10 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov05c10.c | 1065 +++++++++++++++++++++++++++++++++++ 4 files changed, 1084 insertions(+) create mode 100755 drivers/media/i2c/ov05c10.c diff --git a/MAINTAINERS b/MAINTAINERS index c8f597f7e48a..a13a26903e7f 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17545,6 +17545,14 @@ T: git git://linuxtv.org/media.git F: Documentation/devicetree/bindings/media/i2c/ovti,ov02a10.yaml F: drivers/media/i2c/ov02a10.c +OMNIVISION OV05C10 SENSOR DRIVER +M: Nirujogi Pratap <pratap.nirujogi@amd.com> +M: Bin Du <bin.du@amd.com> +L: linux-media@vger.kernel.org +S: Maintained +T: git git://linuxtv.org/media.git +F: drivers/media/i2c/ov05c10.c + OMNIVISION OV08D10 SENSOR DRIVER M: Jimmy Su <jimmy.su@intel.com> L: linux-media@vger.kernel.org diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index e576b213084d..70336ee2494e 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -356,6 +356,16 @@ config VIDEO_OV02A10 To compile this driver as a module, choose M here: the module will be called ov02a10. +config VIDEO_OV05C10 + tristate "OmniVision OV05C10 sensor support" + select V4L2_CCI_I2C + help + This is a Video4Linux2 sensor driver for the OmniVision + OV05C10 camera. + + To compile this driver as a module, choose M here: the + module will be called OV05C10. + config VIDEO_OV08D10 tristate "OmniVision OV08D10 sensor support" help diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile index 6c23a4463527..a3ec22164389 100644 --- a/drivers/media/i2c/Makefile +++ b/drivers/media/i2c/Makefile @@ -83,6 +83,7 @@ obj-$(CONFIG_VIDEO_MT9V111) += mt9v111.o obj-$(CONFIG_VIDEO_OG01A1B) += og01a1b.o obj-$(CONFIG_VIDEO_OV01A10) += ov01a10.o obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o +obj-$(CONFIG_VIDEO_OV05C10) += ov05c10.o obj-$(CONFIG_VIDEO_OV08D10) += ov08d10.o obj-$(CONFIG_VIDEO_OV08X40) += ov08x40.o obj-$(CONFIG_VIDEO_OV13858) += ov13858.o diff --git a/drivers/media/i2c/ov05c10.c b/drivers/media/i2c/ov05c10.c new file mode 100755 index 000000000000..f48a8f15b360 --- /dev/null +++ b/drivers/media/i2c/ov05c10.c @@ -0,0 +1,1065 @@ +// SPDX-License-Identifier: GPL-2.0+ +// Copyright (C) 2025 Advanced Micro Devices, Inc. + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/gpio.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/pm_runtime.h> +#include <linux/units.h> +#include <media/v4l2-cci.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-device.h> +#include <media/v4l2-fwnode.h> + +#define DRV_NAME "ov05c10" +#define OV05C10_REF_CLK (24 * HZ_PER_MHZ) + +#define MODE_WIDTH 2888 +#define MODE_HEIGHT 1808 + +#define PAGE_NUM_MASK 0xff000000 +#define PAGE_NUM_SHIFT 24 +#define REG_ADDR_MASK 0x00ffffff + +#define OV05C10_SYSCTL_PAGE (0 << PAGE_NUM_SHIFT) +#define OV05C10_CISCTL_PAGE (1 << PAGE_NUM_SHIFT) +#define OV05C10_ISPCTL_PAGE (4 << PAGE_NUM_SHIFT) + +/* Chip ID */ +#define OV05C10_REG_CHIP_ID (CCI_REG24(0x00) | OV05C10_SYSCTL_PAGE) +#define OV05C10_CHIP_ID 0x43055610 + +/* Control registers */ +#define OV05C10_REG_TRIGGER (CCI_REG8(0x01) | OV05C10_CISCTL_PAGE) +#define OV05C_REG_TRIGGER_START BIT(0) + +/* Exposure control */ +#define OV05C10_REG_EXPOSURE (CCI_REG24(0x02) | OV05C10_CISCTL_PAGE) +#define OV05C10_EXPOSURE_MAX_MARGIN 33 +#define OV05C10_EXPOSURE_MIN 4 +#define OV05C10_EXPOSURE_STEP 1 +#define OV05C10_EXPOSURE_DEFAULT 0x40 + +/* V_TIMING internal */ +#define OV05C10_REG_VTS (CCI_REG16(0x05) | OV05C10_CISCTL_PAGE) +#define OV05C10_VTS_30FPS 1860 +#define OV05C10_VTS_MAX 0x7fff + +/* Test Pattern Control */ +#define OV05C10_REG_TEST_PATTERN (CCI_REG8(0x12) | OV05C10_ISPCTL_PAGE) +#define OV05C10_TEST_PATTERN_ENABLE BIT(0) +#define OV05C10_REG_TEST_PATTERN_CTL (CCI_REG8(0xf3) | OV05C10_ISPCTL_PAGE) +#define OV05C10_REG_TEST_PATTERN_XXX BIT(0) + +/* Digital gain control */ +#define OV05C10_REG_DGTL_GAIN_H (CCI_REG8(0x21) | OV05C10_CISCTL_PAGE) +#define OV05C10_REG_DGTL_GAIN_L (CCI_REG8(0x22) | OV05C10_CISCTL_PAGE) + +#define OV05C10_DGTL_GAIN_MIN 0x40 +#define OV05C10_DGTL_GAIN_MAX 0xff +#define OV05C10_DGTL_GAIN_DEFAULT 0x40 +#define OV05C10_DGTL_GAIN_STEP 1 + +#define OV05C10_DGTL_GAIN_L_MASK 0xff +#define OV05C10_DGTL_GAIN_H_SHIFT 8 +#define OV05C10_DGTL_GAIN_H_MASK 0xff00 + +/* Analog gain control */ +#define OV05C10_REG_ANALOG_GAIN (CCI_REG8(0x24) | OV05C10_CISCTL_PAGE) +#define OV05C10_ANA_GAIN_MIN 0x80 +#define OV05C10_ANA_GAIN_MAX 0x07c0 +#define OV05C10_ANA_GAIN_STEP 1 +#define OV05C10_ANA_GAIN_DEFAULT 0x80 + +/* H TIMING internal */ +#define OV05C10_REG_HTS (CCI_REG16(0x37) | OV05C10_CISCTL_PAGE) +#define OV05C10_HTS_30FPS 0x0280 + +/* Page selection */ +#define OV05C10_REG_PAGE_CTL CCI_REG8(0xfd) + +#define NUM_OF_PADS 1 + +#define OV05C10_GET_PAGE_NUM(reg) (((reg) & PAGE_NUM_MASK) >>\ + PAGE_NUM_SHIFT) +#define OV05C10_GET_REG_ADDR(reg) ((reg) & REG_ADDR_MASK) + +enum { + OV05C10_LINK_FREQ_900MHZ_INDEX, +}; + +struct ov05c10_reg_list { + u32 num_of_regs; + const struct cci_reg_sequence *regs; +}; + +/* Mode : resolution and related config&values */ +struct ov05c10_mode { + /* Frame width */ + u32 width; + /* Frame height */ + u32 height; + /* number of lanes */ + u32 lanes; + + /* V-timing */ + u32 vts_def; + u32 vts_min; + + /* HTS */ + u32 hts; + + /* Index of Link frequency config to be used */ + u32 link_freq_index; + + /* Default register values */ + struct ov05c10_reg_list reg_list; +}; + +static const s64 ov05c10_link_frequencies[] = { + 925 * HZ_PER_MHZ, +}; + +/* 2888x1808 30fps, 1800mbps, 2lane, 24mhz */ +static const struct cci_reg_sequence ov05c10_2888x1808_regs[] = { + { CCI_REG8(0xfd), 0x00 }, + { CCI_REG8(0x20), 0x00 }, + { CCI_REG8(0xfd), 0x00 }, + { CCI_REG8(0x20), 0x0b }, + { CCI_REG8(0xc1), 0x09 }, + { CCI_REG8(0x21), 0x06 }, + { CCI_REG8(0x14), 0x78 }, + { CCI_REG8(0xe7), 0x03 }, + { CCI_REG8(0xe7), 0x00 }, + { CCI_REG8(0x21), 0x00 }, + { CCI_REG8(0xfd), 0x01 }, + { CCI_REG8(0x03), 0x00 }, + { CCI_REG8(0x04), 0x06 }, + { CCI_REG8(0x05), 0x07 }, + { CCI_REG8(0x06), 0x44 }, + { CCI_REG8(0x07), 0x08 }, + { CCI_REG8(0x1b), 0x01 }, + { CCI_REG8(0x24), 0xff }, + { CCI_REG8(0x32), 0x03 }, + { CCI_REG8(0x42), 0x5d }, + { CCI_REG8(0x43), 0x08 }, + { CCI_REG8(0x44), 0x81 }, + { CCI_REG8(0x46), 0x5f }, + { CCI_REG8(0x48), 0x18 }, + { CCI_REG8(0x49), 0x04 }, + { CCI_REG8(0x5c), 0x18 }, + { CCI_REG8(0x5e), 0x13 }, + { CCI_REG8(0x70), 0x15 }, + { CCI_REG8(0x77), 0x35 }, + { CCI_REG8(0x79), 0x00 }, + { CCI_REG8(0x7b), 0x08 }, + { CCI_REG8(0x7d), 0x08 }, + { CCI_REG8(0x7e), 0x08 }, + { CCI_REG8(0x7f), 0x08 }, + { CCI_REG8(0x90), 0x37 }, + { CCI_REG8(0x91), 0x05 }, + { CCI_REG8(0x92), 0x18 }, + { CCI_REG8(0x93), 0x27 }, + { CCI_REG8(0x94), 0x05 }, + { CCI_REG8(0x95), 0x38 }, + { CCI_REG8(0x9b), 0x00 }, + { CCI_REG8(0x9c), 0x06 }, + { CCI_REG8(0x9d), 0x28 }, + { CCI_REG8(0x9e), 0x06 }, + { CCI_REG8(0xb2), 0x0f }, + { CCI_REG8(0xb3), 0x29 }, + { CCI_REG8(0xbf), 0x3c }, + { CCI_REG8(0xc2), 0x04 }, + { CCI_REG8(0xc4), 0x00 }, + { CCI_REG8(0xca), 0x20 }, + { CCI_REG8(0xcb), 0x20 }, + { CCI_REG8(0xcc), 0x28 }, + { CCI_REG8(0xcd), 0x28 }, + { CCI_REG8(0xce), 0x20 }, + { CCI_REG8(0xcf), 0x20 }, + { CCI_REG8(0xd0), 0x2a }, + { CCI_REG8(0xd1), 0x2a }, + { CCI_REG8(0xfd), 0x0f }, + { CCI_REG8(0x00), 0x00 }, + { CCI_REG8(0x01), 0xa0 }, + { CCI_REG8(0x02), 0x48 }, + { CCI_REG8(0x07), 0x8f }, + { CCI_REG8(0x08), 0x70 }, + { CCI_REG8(0x09), 0x01 }, + { CCI_REG8(0x0b), 0x40 }, + { CCI_REG8(0x0d), 0x07 }, + { CCI_REG8(0x11), 0x33 }, + { CCI_REG8(0x12), 0x77 }, + { CCI_REG8(0x13), 0x66 }, + { CCI_REG8(0x14), 0x65 }, + { CCI_REG8(0x15), 0x37 }, + { CCI_REG8(0x16), 0xbf }, + { CCI_REG8(0x17), 0xff }, + { CCI_REG8(0x18), 0xff }, + { CCI_REG8(0x19), 0x12 }, + { CCI_REG8(0x1a), 0x10 }, + { CCI_REG8(0x1c), 0x77 }, + { CCI_REG8(0x1d), 0x77 }, + { CCI_REG8(0x20), 0x0f }, + { CCI_REG8(0x21), 0x0f }, + { CCI_REG8(0x22), 0x0f }, + { CCI_REG8(0x23), 0x0f }, + { CCI_REG8(0x2b), 0x20 }, + { CCI_REG8(0x2c), 0x20 }, + { CCI_REG8(0x2d), 0x04 }, + { CCI_REG8(0xfd), 0x03 }, + { CCI_REG8(0x9d), 0x0f }, + { CCI_REG8(0x9f), 0x40 }, + { CCI_REG8(0xfd), 0x00 }, + { CCI_REG8(0x20), 0x1b }, + { CCI_REG8(0xfd), 0x04 }, + { CCI_REG8(0x19), 0x60 }, + { CCI_REG8(0xfd), 0x02 }, + { CCI_REG8(0x75), 0x05 }, + { CCI_REG8(0x7f), 0x06 }, + { CCI_REG8(0x9a), 0x03 }, + { CCI_REG8(0xa2), 0x07 }, + { CCI_REG8(0xa3), 0x10 }, + { CCI_REG8(0xa5), 0x02 }, + { CCI_REG8(0xa6), 0x0b }, + { CCI_REG8(0xa7), 0x48 }, + { CCI_REG8(0xfd), 0x07 }, + { CCI_REG8(0x42), 0x00 }, + { CCI_REG8(0x43), 0x80 }, + { CCI_REG8(0x44), 0x00 }, + { CCI_REG8(0x45), 0x80 }, + { CCI_REG8(0x46), 0x00 }, + { CCI_REG8(0x47), 0x80 }, + { CCI_REG8(0x48), 0x00 }, + { CCI_REG8(0x49), 0x80 }, + { CCI_REG8(0x00), 0xf7 }, + { CCI_REG8(0xfd), 0x00 }, + { CCI_REG8(0xe7), 0x03 }, + { CCI_REG8(0xe7), 0x00 }, + { CCI_REG8(0xfd), 0x00 }, + { CCI_REG8(0x93), 0x18 }, + { CCI_REG8(0x94), 0xff }, + { CCI_REG8(0x95), 0xbd }, + { CCI_REG8(0x96), 0x1a }, + { CCI_REG8(0x98), 0x04 }, + { CCI_REG8(0x99), 0x08 }, + { CCI_REG8(0x9b), 0x10 }, + { CCI_REG8(0x9c), 0x3f }, + { CCI_REG8(0xa1), 0x05 }, + { CCI_REG8(0xa4), 0x2f }, + { CCI_REG8(0xc0), 0x0c }, + { CCI_REG8(0xc1), 0x08 }, + { CCI_REG8(0xc2), 0x00 }, + { CCI_REG8(0xb6), 0x20 }, + { CCI_REG8(0xbb), 0x80 }, + { CCI_REG8(0xfd), 0x00 }, + { CCI_REG8(0xa0), 0x01 }, + { CCI_REG8(0xfd), 0x01 }, +}; + +static const struct cci_reg_sequence mode_OV05C10_stream_on_regs[] = { + { CCI_REG8(0xfd), 0x01 }, + { CCI_REG8(0x33), 0x03 }, + { CCI_REG8(0x01), 0x02 }, + { CCI_REG8(0xfd), 0x00 }, + { CCI_REG8(0x20), 0x1f }, + { CCI_REG8(0xfd), 0x01 }, +}; + +static const struct cci_reg_sequence mode_OV05C10_stream_off_regs[] = { + { CCI_REG8(0xfd), 0x00 }, + { CCI_REG8(0x20), 0x5b }, + { CCI_REG8(0xfd), 0x01 }, + { CCI_REG8(0x33), 0x02 }, + { CCI_REG8(0x01), 0x02 }, +}; + +static const char * const ov05c10_test_pattern_menu[] = { + "Disabled", + "Vertical Color Bar Type 1", + "Vertical Color Bar Type 2", + "Vertical Color Bar Type 3", + "Vertical Color Bar Type 4" +}; + +/* Configurations for supported link frequencies */ +#define OV05C10_LINK_FREQ_900MHZ (900 * HZ_PER_MHZ) + +/* Number of lanes supported */ +#define OV05C10_DATA_LANES 2 + +/* Bits per sample of sensor output */ +#define OV05C10_BITS_PER_SAMPLE 10 + +/* + * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample + * data rate => double data rate; number of lanes => 2; bits per pixel => 10 + */ +static u64 link_freq_to_pixel_rate(u64 f, u32 lane_nr) +{ + f *= 2 * lane_nr; + do_div(f, OV05C10_BITS_PER_SAMPLE); + + return f; +} + +/* Menu items for LINK_FREQ V4L2 control */ +static const s64 ov05c10_link_freq_menu_items[] = { + OV05C10_LINK_FREQ_900MHZ, +}; + +/* Mode configs, currently, only support 1 mode */ +static const struct ov05c10_mode supported_mode = { + .width = MODE_WIDTH, + .height = MODE_HEIGHT, + .vts_def = OV05C10_VTS_30FPS, + .vts_min = OV05C10_VTS_30FPS, + .hts = 640, + .lanes = 2, + .reg_list = { + .num_of_regs = ARRAY_SIZE(ov05c10_2888x1808_regs), + .regs = ov05c10_2888x1808_regs, + }, + .link_freq_index = OV05C10_LINK_FREQ_900MHZ_INDEX, +}; + +struct ov05c10 { + struct v4l2_subdev sd; + struct media_pad pad; + + /* V4L2 control handler */ + struct v4l2_ctrl_handler ctrl_handler; + + /* V4L2 Controls */ + struct v4l2_ctrl *link_freq; + struct v4l2_ctrl *pixel_rate; + struct v4l2_ctrl *vblank; + struct v4l2_ctrl *hblank; + struct v4l2_ctrl *exposure; + + struct regmap *regmap; + + /* gpio descriptor */ + struct gpio_desc *enable_gpio; + + /* Current page for sensor register control */ + int cur_page; +}; + +#define to_ov05c10(_sd) container_of(_sd, struct ov05c10, sd) + +static int ov05c10_init_state(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state) +{ + struct v4l2_mbus_framefmt *frame_fmt; + struct v4l2_subdev_format fmt = { + .which = V4L2_SUBDEV_FORMAT_TRY, + .format = { + .width = MODE_WIDTH, + .height = MODE_HEIGHT, + .code = MEDIA_BUS_FMT_SGRBG10_1X10, + .field = V4L2_FIELD_NONE, + } + }; + + frame_fmt = v4l2_subdev_state_get_format(sd_state, 0); + *frame_fmt = fmt.format; + return 0; +} + +static int ov05c10_switch_page(struct ov05c10 *ov05c10, u32 page, int *err) +{ + int ret = 0; + + if (err && *err) + return *err; + + if (page != ov05c10->cur_page) { + cci_write(ov05c10->regmap, OV05C10_REG_PAGE_CTL, page, &ret); + if (!ret) + ov05c10->cur_page = page; + } + + if (err) + *err = ret; + + return ret; +} + +/* refer to the implementation of cci_read */ +static int ov05c10_reg_read(struct ov05c10 *ov05c10, u32 reg, + u64 *val, int *err) +{ + u32 page; + u32 addr; + int ret = 0; + + if (err && *err) + return *err; + + page = OV05C10_GET_PAGE_NUM(reg); + addr = OV05C10_GET_REG_ADDR(reg); + ov05c10_switch_page(ov05c10, page, &ret); + cci_read(ov05c10->regmap, addr, val, &ret); + if (err) + *err = ret; + + return ret; +} + +/* refer to the implementation of cci_write */ +static int ov05c10_reg_write(struct ov05c10 *ov05c10, u32 reg, + u64 val, int *err) +{ + u32 page; + u32 addr; + int ret = 0; + + if (err && *err) + return *err; + + page = OV05C10_GET_PAGE_NUM(reg); + addr = OV05C10_GET_REG_ADDR(reg); + ov05c10_switch_page(ov05c10, page, &ret); + cci_write(ov05c10->regmap, addr, val, &ret); + if (err) + *err = ret; + + return ret; +} + +static int ov05c10_update_vblank(struct ov05c10 *ov05c10, u32 vblank) +{ + const struct ov05c10_mode *mode = &supported_mode; + u64 val; + int ret = 0; + + val = mode->height + vblank; + ov05c10_reg_write(ov05c10, OV05C10_REG_VTS, val, &ret); + ov05c10_reg_write(ov05c10, OV05C10_REG_TRIGGER, + OV05C_REG_TRIGGER_START, &ret); + + return ret; +} + +static int ov05c10_update_exposure(struct ov05c10 *ov05c10, u32 exposure) +{ + int ret = 0; + + ov05c10_reg_write(ov05c10, OV05C10_REG_EXPOSURE, exposure, &ret); + ov05c10_reg_write(ov05c10, OV05C10_REG_TRIGGER, + OV05C_REG_TRIGGER_START, &ret); + + return ret; +} + +static int ov05c10_update_analog_gain(struct ov05c10 *ov05c10, u32 a_gain) +{ + int ret = 0; + + ov05c10_reg_write(ov05c10, OV05C10_REG_ANALOG_GAIN, a_gain, &ret); + ov05c10_reg_write(ov05c10, OV05C10_REG_TRIGGER, + OV05C_REG_TRIGGER_START, &ret); + + return ret; +} + +static int ov05c10_update_digital_gain(struct ov05c10 *ov05c10, u32 d_gain) +{ + u64 val; + int ret = 0; + + val = d_gain & OV05C10_DGTL_GAIN_L_MASK; + ov05c10_reg_write(ov05c10, OV05C10_REG_DGTL_GAIN_L, val, &ret); + + val = (d_gain & OV05C10_DGTL_GAIN_H_MASK) >> OV05C10_DGTL_GAIN_H_SHIFT; + ov05c10_reg_write(ov05c10, OV05C10_REG_DGTL_GAIN_H, val, &ret); + + ov05c10_reg_write(ov05c10, OV05C10_REG_TRIGGER, + OV05C_REG_TRIGGER_START, &ret); + + return ret; +} + +static int ov05c10_enable_test_pattern(struct ov05c10 *ov05c10, u32 pattern) +{ + u64 val; + int ret = 0; + + if (pattern) { + ov05c10_reg_read(ov05c10, OV05C10_REG_TEST_PATTERN_CTL, + &val, &ret); + ov05c10_reg_write(ov05c10, OV05C10_REG_TEST_PATTERN_CTL, + val | OV05C10_REG_TEST_PATTERN_XXX, &ret); + ov05c10_reg_read(ov05c10, OV05C10_REG_TEST_PATTERN, &val, &ret); + val |= OV05C10_TEST_PATTERN_ENABLE; + } else { + ov05c10_reg_read(ov05c10, OV05C10_REG_TEST_PATTERN, &val, &ret); + val &= ~OV05C10_TEST_PATTERN_ENABLE; + } + + ov05c10_reg_write(ov05c10, OV05C10_REG_TEST_PATTERN, val, &ret); + ov05c10_reg_write(ov05c10, OV05C10_REG_TRIGGER, + OV05C_REG_TRIGGER_START, &ret); + + return ret; +} + +static int ov05c10_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct ov05c10 *ov05c10 = container_of(ctrl->handler, + struct ov05c10, ctrl_handler); + struct i2c_client *client = v4l2_get_subdevdata(&ov05c10->sd); + const struct ov05c10_mode *mode = &supported_mode; + s64 max; + int ret = 0; + + /* Propagate change of current control to all related controls */ + if (ctrl->id == V4L2_CID_VBLANK) { + s64 cur_exp = ov05c10->exposure->cur.val; + + /* Update max exposure while meeting expected vblanking */ + max = mode->height + ctrl->val - OV05C10_EXPOSURE_MAX_MARGIN; + cur_exp = clamp(cur_exp, ov05c10->exposure->minimum, max); + ret = __v4l2_ctrl_modify_range(ov05c10->exposure, + ov05c10->exposure->minimum, + max, ov05c10->exposure->step, + cur_exp); + if (!ret) + return ret; + } + + /* + * Applying V4L2 control value only happens + * when power is up for streaming + */ + if (!pm_runtime_get_if_in_use(&client->dev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_ANALOGUE_GAIN: + ret = ov05c10_update_analog_gain(ov05c10, ctrl->val); + break; + case V4L2_CID_DIGITAL_GAIN: + ret = ov05c10_update_digital_gain(ov05c10, ctrl->val); + break; + case V4L2_CID_EXPOSURE: + ret = ov05c10_update_exposure(ov05c10, ctrl->val); + break; + case V4L2_CID_VBLANK: + ret = ov05c10_update_vblank(ov05c10, ctrl->val); + break; + case V4L2_CID_TEST_PATTERN: + ret = ov05c10_enable_test_pattern(ov05c10, ctrl->val); + break; + default: + ret = -ENOTTY; + dev_err(&client->dev, + "ctrl(id:0x%x,val:0x%x) is not handled\n", + ctrl->id, ctrl->val); + break; + } + + pm_runtime_put(&client->dev); + + return ret; +} + +static const struct v4l2_ctrl_ops ov05c10_ctrl_ops = { + .s_ctrl = ov05c10_set_ctrl, +}; + +static int ov05c10_enum_mbus_code(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_mbus_code_enum *code) +{ + /* Only one bayer order(GRBG) is supported */ + if (code->index > 0) + return -EINVAL; + + code->code = MEDIA_BUS_FMT_SGRBG10_1X10; + + return 0; +} + +static int ov05c10_enum_frame_size(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_frame_size_enum *fse) +{ + /* ov05c10 driver currently only supports 1 mode*/ + if (fse->index != 0) + return -EINVAL; + + if (fse->code != MEDIA_BUS_FMT_SGRBG10_1X10) + return -EINVAL; + + fse->min_width = supported_mode.width; + fse->max_width = fse->min_width; + fse->min_height = supported_mode.height; + fse->max_height = fse->min_height; + + return 0; +} + +static void ov05c10_update_pad_format(const struct ov05c10_mode *mode, + struct v4l2_subdev_format *fmt) +{ + fmt->format.width = mode->width; + fmt->format.height = mode->height; + fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10; + fmt->format.field = V4L2_FIELD_NONE; +} + +static int ov05c10_set_pad_format(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_format *fmt) +{ + struct v4l2_mbus_framefmt *framefmt; + struct ov05c10 *ov05c10 = to_ov05c10(sd); + const struct ov05c10_mode *mode; + s32 vblank_def; + s32 vblank_min; + s64 pixel_rate; + s64 link_freq; + s64 h_blank; + + /* Only one raw bayer(GRBG) order is supported */ + if (fmt->format.code != MEDIA_BUS_FMT_SGRBG10_1X10) + fmt->format.code = MEDIA_BUS_FMT_SGRBG10_1X10; + + mode = &supported_mode; + ov05c10_update_pad_format(mode, fmt); + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { + framefmt = v4l2_subdev_state_get_format(sd_state, fmt->pad); + *framefmt = fmt->format; + } else { + __v4l2_ctrl_s_ctrl(ov05c10->link_freq, mode->link_freq_index); + link_freq = ov05c10_link_freq_menu_items[mode->link_freq_index]; + pixel_rate = link_freq_to_pixel_rate(link_freq, + mode->lanes); + __v4l2_ctrl_s_ctrl_int64(ov05c10->pixel_rate, pixel_rate); + + /* Update limits and set FPS to default */ + vblank_def = mode->vts_def - mode->height; + vblank_min = mode->vts_min - mode->height; + __v4l2_ctrl_modify_range(ov05c10->vblank, vblank_min, + OV05C10_VTS_MAX - mode->height, + 1, vblank_def); + __v4l2_ctrl_s_ctrl(ov05c10->vblank, vblank_def); + h_blank = mode->hts; + __v4l2_ctrl_modify_range(ov05c10->hblank, h_blank, + h_blank, 1, h_blank); + } + + return 0; +} + +static int ov05c10_start_streaming(struct ov05c10 *ov05c10) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov05c10->sd); + const struct ov05c10_mode *mode = &supported_mode; + const struct ov05c10_reg_list *reg_list; + int ret = 0; + + /* Apply default values of current mode */ + reg_list = &mode->reg_list; + cci_multi_reg_write(ov05c10->regmap, reg_list->regs, + reg_list->num_of_regs, &ret); + if (ret) { + dev_err(&client->dev, "fail to set mode, ret: %d\n", ret); + + return ret; + } + + /* Apply customized values from user */ + ret = __v4l2_ctrl_handler_setup(ov05c10->sd.ctrl_handler); + if (ret) { + dev_err(&client->dev, "failed to setup v4l2 handler %d", ret); + + return ret; + } + + cci_multi_reg_write(ov05c10->regmap, mode_OV05C10_stream_on_regs, + ARRAY_SIZE(mode_OV05C10_stream_on_regs), &ret); + if (ret) + dev_err(&client->dev, "fail to start the streaming\n"); + + return ret; +} + +static int ov05c10_stop_streaming(struct ov05c10 *ov05c10) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov05c10->sd); + int ret = 0; + + cci_multi_reg_write(ov05c10->regmap, mode_OV05C10_stream_off_regs, + ARRAY_SIZE(mode_OV05C10_stream_off_regs), &ret); + if (ret) + dev_err(&client->dev, "fail to stop the streaming\n"); + + return ret; +} + +static void ov05c10_sensor_power_set(struct ov05c10 *ov05c10, bool on) +{ + if (on) { + gpiod_set_value(ov05c10->enable_gpio, 0); + usleep_range(10, 20); + + gpiod_set_value(ov05c10->enable_gpio, 1); + usleep_range(1000, 2000); + } else { + gpiod_set_value(ov05c10->enable_gpio, 0); + usleep_range(10, 20); + } +} + +static int ov05c10_enable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct i2c_client *client = v4l2_get_subdevdata(sd); + struct ov05c10 *ov05c10 = to_ov05c10(sd); + int ret = 0; + + ret = pm_runtime_resume_and_get(&client->dev); + if (ret < 0) + return ret; + + ov05c10->cur_page = -1; + + ret = ov05c10_start_streaming(ov05c10); + if (ret) + goto err_rpm_put; + + return 0; + +err_rpm_put: + pm_runtime_put(&client->dev); + return ret; +} + +static int ov05c10_disable_streams(struct v4l2_subdev *sd, + struct v4l2_subdev_state *state, u32 pad, + u64 streams_mask) +{ + struct i2c_client *client = v4l2_get_subdevdata(sd); + struct ov05c10 *ov05c10 = to_ov05c10(sd); + int ret = 0; + + ov05c10_stop_streaming(ov05c10); + pm_runtime_put(&client->dev); + + return ret; +} + +static const struct v4l2_subdev_video_ops ov05c10_video_ops = { + .s_stream = v4l2_subdev_s_stream_helper, +}; + +static const struct v4l2_subdev_pad_ops ov05c10_pad_ops = { + .enum_mbus_code = ov05c10_enum_mbus_code, + .get_fmt = v4l2_subdev_get_fmt, + .set_fmt = ov05c10_set_pad_format, + .enum_frame_size = ov05c10_enum_frame_size, + .enable_streams = ov05c10_enable_streams, + .disable_streams = ov05c10_disable_streams, +}; + +static const struct v4l2_subdev_ops ov05c10_subdev_ops = { + .video = &ov05c10_video_ops, + .pad = &ov05c10_pad_ops, +}; + +static const struct media_entity_operations ov05c10_subdev_entity_ops = { + .link_validate = v4l2_subdev_link_validate, +}; + +static const struct v4l2_subdev_internal_ops ov05c10_internal_ops = { + .init_state = ov05c10_init_state, +}; + +static int ov05c10_init_controls(struct ov05c10 *ov05c10) +{ + struct i2c_client *client = v4l2_get_subdevdata(&ov05c10->sd); + const struct ov05c10_mode *mode = &supported_mode; + struct v4l2_fwnode_device_properties props; + struct v4l2_ctrl_handler *ctrl_hdlr; + s64 pixel_rate_max; + s64 exposure_max; + s64 vblank_def; + s64 vblank_min; + u32 max_items; + s64 hblank; + int ret; + + ret = v4l2_ctrl_handler_init(&ov05c10->ctrl_handler, 10); + if (ret) + return ret; + + ctrl_hdlr = &ov05c10->ctrl_handler; + + max_items = ARRAY_SIZE(ov05c10_link_freq_menu_items) - 1; + ov05c10->link_freq = + v4l2_ctrl_new_int_menu(ctrl_hdlr, + NULL, + V4L2_CID_LINK_FREQ, + max_items, + 0, + ov05c10_link_freq_menu_items); + if (ov05c10->link_freq) + ov05c10->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + pixel_rate_max = + link_freq_to_pixel_rate(ov05c10_link_freq_menu_items[0], + supported_mode.lanes); + ov05c10->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, NULL, + V4L2_CID_PIXEL_RATE, + 0, pixel_rate_max, + 1, pixel_rate_max); + + vblank_def = mode->vts_def - mode->height; + vblank_min = mode->vts_min - mode->height; + ov05c10->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &ov05c10_ctrl_ops, + V4L2_CID_VBLANK, + vblank_min, + OV05C10_VTS_MAX - mode->height, + 1, vblank_def); + + hblank = max(0, mode->hts - mode->width); + ov05c10->hblank = v4l2_ctrl_new_std(ctrl_hdlr, NULL, + V4L2_CID_HBLANK, + hblank, hblank, 1, hblank); + if (ov05c10->hblank) + ov05c10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; + + exposure_max = mode->vts_def - OV05C10_EXPOSURE_MAX_MARGIN; + ov05c10->exposure = v4l2_ctrl_new_std(ctrl_hdlr, &ov05c10_ctrl_ops, + V4L2_CID_EXPOSURE, + OV05C10_EXPOSURE_MIN, + exposure_max, + OV05C10_EXPOSURE_STEP, + exposure_max); + + v4l2_ctrl_new_std(ctrl_hdlr, &ov05c10_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, + OV05C10_ANA_GAIN_MIN, OV05C10_ANA_GAIN_MAX, + OV05C10_ANA_GAIN_STEP, OV05C10_ANA_GAIN_DEFAULT); + + v4l2_ctrl_new_std(ctrl_hdlr, &ov05c10_ctrl_ops, V4L2_CID_DIGITAL_GAIN, + OV05C10_DGTL_GAIN_MIN, OV05C10_DGTL_GAIN_MAX, + OV05C10_DGTL_GAIN_STEP, OV05C10_DGTL_GAIN_DEFAULT); + + v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &ov05c10_ctrl_ops, + V4L2_CID_TEST_PATTERN, + ARRAY_SIZE(ov05c10_test_pattern_menu) - 1, + 0, 0, ov05c10_test_pattern_menu); + + if (ctrl_hdlr->error) { + ret = ctrl_hdlr->error; + dev_err(&client->dev, "V4L2 control init failed (%d)", ret); + goto err_hdl_free; + } + + ret = v4l2_fwnode_device_parse(&client->dev, &props); + if (ret) + goto err_hdl_free; + + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov05c10_ctrl_ops, + &props); + if (ret) + goto err_hdl_free; + + ov05c10->sd.ctrl_handler = ctrl_hdlr; + + return 0; + +err_hdl_free: + v4l2_ctrl_handler_free(ctrl_hdlr); + + return ret; +} + +static int ov05c10_parse_endpoint(struct device *dev, + struct fwnode_handle *fwnode) +{ + struct v4l2_fwnode_endpoint bus_cfg = { + .bus_type = V4L2_MBUS_CSI2_DPHY + }; + struct fwnode_handle *ep; + unsigned long bitmap; + int ret; + + ep = fwnode_graph_get_next_endpoint(fwnode, NULL); + if (!ep) { + dev_err(dev, "Failed to get next endpoint\n"); + return -ENXIO; + } + + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); + fwnode_handle_put(ep); + if (ret) + return ret; + + if (bus_cfg.bus.mipi_csi2.num_data_lanes != supported_mode.lanes) { + dev_err(dev, + "number of CSI2 data lanes %d is not supported\n", + bus_cfg.bus.mipi_csi2.num_data_lanes); + ret = -EINVAL; + goto err_endpoint_free; + } + + ret = v4l2_link_freq_to_bitmap(dev, bus_cfg.link_frequencies, + bus_cfg.nr_of_link_frequencies, + ov05c10_link_frequencies, + ARRAY_SIZE(ov05c10_link_frequencies), + &bitmap); + if (ret) + dev_err(dev, "v4l2_link_freq_to_bitmap fail with %d\n", ret); +err_endpoint_free: + v4l2_fwnode_endpoint_free(&bus_cfg); + + return ret; +} + +static int ov05c10_probe(struct i2c_client *client) +{ + struct ov05c10 *ov05c10; + u32 refclk; + int ret; + + ov05c10 = devm_kzalloc(&client->dev, sizeof(*ov05c10), GFP_KERNEL); + if (!ov05c10) + return -ENOMEM; + + struct fwnode_handle *fwnode = dev_fwnode(&client->dev); + + ret = fwnode_property_read_u32(fwnode, "refclk", &refclk); + if (ret) + return dev_err_probe(&client->dev, -EINVAL, + "fail to get refclk\n"); + if (refclk != OV05C10_REF_CLK) + return dev_err_probe(&client->dev, -EINVAL, + "failbad refclk %u, %lu expected\n", + refclk, OV05C10_REF_CLK); + + ret = ov05c10_parse_endpoint(&client->dev, fwnode); + if (ret) + return dev_err_probe(&client->dev, -EINVAL, + "fail to parse endpoint\n"); + + ov05c10->enable_gpio = devm_gpiod_get(&client->dev, "sensor0_enable", + GPIOD_OUT_LOW); + if (IS_ERR(ov05c10->enable_gpio)) + return dev_err_probe(&client->dev, + PTR_ERR(ov05c10->enable_gpio), + "fail to get enable gpio\n"); + + v4l2_i2c_subdev_init(&ov05c10->sd, client, &ov05c10_subdev_ops); + + ov05c10->regmap = devm_cci_regmap_init_i2c(client, 8); + if (IS_ERR(ov05c10->regmap)) + return dev_err_probe(&client->dev, PTR_ERR(ov05c10->regmap), + "fail to init cci\n"); + + ov05c10->cur_page = -1; + + ret = ov05c10_init_controls(ov05c10); + if (ret) + return dev_err_probe(&client->dev, ret, "fail to init ctl\n"); + + ov05c10->sd.internal_ops = &ov05c10_internal_ops; + ov05c10->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; + ov05c10->sd.entity.ops = &ov05c10_subdev_entity_ops; + ov05c10->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; + + ov05c10->pad.flags = MEDIA_PAD_FL_SOURCE; + + ret = media_entity_pads_init(&ov05c10->sd.entity, NUM_OF_PADS, + &ov05c10->pad); + if (ret) + goto err_hdl_free; + + ret = v4l2_subdev_init_finalize(&ov05c10->sd); + if (ret < 0) + goto err_media_entity_cleanup; + + ret = v4l2_async_register_subdev_sensor(&ov05c10->sd); + if (ret) + goto err_media_entity_cleanup; + + pm_runtime_set_active(&client->dev); + pm_runtime_enable(&client->dev); + pm_runtime_idle(&client->dev); + pm_runtime_set_autosuspend_delay(&client->dev, 1000); + pm_runtime_use_autosuspend(&client->dev); + return 0; + +err_media_entity_cleanup: + media_entity_cleanup(&ov05c10->sd.entity); + +err_hdl_free: + v4l2_ctrl_handler_free(ov05c10->sd.ctrl_handler); + + return ret; +} + +static void ov05c10_remove(struct i2c_client *client) +{ + struct v4l2_subdev *sd = i2c_get_clientdata(client); + struct ov05c10 *ov05c10 = to_ov05c10(sd); + + v4l2_async_unregister_subdev(sd); + media_entity_cleanup(&sd->entity); + v4l2_ctrl_handler_free(ov05c10->sd.ctrl_handler); + + pm_runtime_disable(&client->dev); + pm_runtime_set_suspended(&client->dev); +} + +static int ov05c10_runtime_resume(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct ov05c10 *ov05c10 = to_ov05c10(sd); + + ov05c10_sensor_power_set(ov05c10, true); + return 0; +} + +static int ov05c10_runtime_suspend(struct device *dev) +{ + struct v4l2_subdev *sd = dev_get_drvdata(dev); + struct ov05c10 *ov05c10 = to_ov05c10(sd); + + ov05c10_sensor_power_set(ov05c10, false); + return 0; +} + +static DEFINE_RUNTIME_DEV_PM_OPS(ov05c10_pm_ops, ov05c10_runtime_suspend, + ov05c10_runtime_resume, NULL); + +static const struct i2c_device_id ov05c10_id[] = { + {"ov05c10", 0 }, + { } +}; + +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"); -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] media: i2c: Add OV05C10 camera sensor driver 2025-03-28 21:42 ` [PATCH v2] media: i2c: Add OV05C10 camera sensor driver Pratap Nirujogi @ 2025-03-29 1:18 ` Bryan O'Donoghue 2025-03-31 19:17 ` Nirujogi, Pratap 2025-03-29 4:30 ` Krzysztof Kozlowski 1 sibling, 1 reply; 11+ messages in thread From: Bryan O'Donoghue @ 2025-03-29 1:18 UTC (permalink / raw) To: Pratap Nirujogi, mchehab, sakari.ailus, hverkuil, laurent.pinchart, dave.stevenson, krzk, dan.carpenter Cc: linux-media, linux-kernel, benjamin.chan, bin.du, grosikop, king.li, dantony, Venkata Narendra Kumar Gutta 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. > + > +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. --- bod ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] media: i2c: Add OV05C10 camera sensor driver 2025-03-29 1:18 ` Bryan O'Donoghue @ 2025-03-31 19:17 ` Nirujogi, Pratap 2025-04-02 1:20 ` Laurent Pinchart 2025-04-08 21:09 ` Nirujogi, Pratap 0 siblings, 2 replies; 11+ messages in thread From: Nirujogi, Pratap @ 2025-03-31 19:17 UTC (permalink / raw) To: Bryan O'Donoghue, Pratap Nirujogi, mchehab, sakari.ailus, hverkuil, laurent.pinchart, dave.stevenson, krzk, dan.carpenter Cc: linux-media, linux-kernel, benjamin.chan, bin.du, grosikop, king.li, dantony, Venkata Narendra Kumar Gutta Hi Bryan, Thanks for the review. Thanks, Pratap On 3/28/2025 9:18 PM, Bryan O'Donoghue wrote: > Caution: This message originated from an External Source. Use proper > caution when opening attachments, clicking links, or responding. > > > 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" }, {} }; >> + >> +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. > --- > bod ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] media: i2c: Add OV05C10 camera sensor driver 2025-03-31 19:17 ` Nirujogi, Pratap @ 2025-04-02 1:20 ` Laurent Pinchart 2025-04-02 22:04 ` Nirujogi, Pratap 2025-05-06 7:45 ` Sakari Ailus 2025-04-08 21:09 ` Nirujogi, Pratap 1 sibling, 2 replies; 11+ messages in thread From: Laurent Pinchart @ 2025-04-02 1:20 UTC (permalink / raw) To: Nirujogi, Pratap Cc: Bryan O'Donoghue, Pratap Nirujogi, mchehab, sakari.ailus, hverkuil, dave.stevenson, krzk, dan.carpenter, linux-media, linux-kernel, benjamin.chan, bin.du, grosikop, king.li, dantony, Venkata Narendra Kumar Gutta 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 ? > >> + > >> +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. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] media: i2c: Add OV05C10 camera sensor driver 2025-04-02 1:20 ` Laurent Pinchart @ 2025-04-02 22:04 ` Nirujogi, Pratap 2025-05-06 7:45 ` Sakari Ailus 1 sibling, 0 replies; 11+ messages in thread From: Nirujogi, Pratap @ 2025-04-02 22:04 UTC (permalink / raw) To: Laurent Pinchart Cc: Bryan O'Donoghue, Pratap Nirujogi, mchehab, sakari.ailus, hverkuil, dave.stevenson, krzk, dan.carpenter, linux-media, linux-kernel, benjamin.chan, bin.du, grosikop, king.li, dantony, Venkata Narendra Kumar Gutta Hi Laurent, Please see below v4l2-compliance test reports for: (a) ov05c10 device /dev/v4l-subdev0 (b) amd_isp_capture device /dev/video0 Compliance test for device /dev/v4l-subdev0: -------------------------------------------- atg@isp-pv-linux:~/v4l-utils/build/utils/v4l2-compliance$ sudo ./v4l2-compliance -u 0 v4l2-compliance 1.29.0-5348, 64 bits, 64-bit time_t v4l2-compliance SHA: 75e3f0e2c2cb 2025-03-17 18:12:17 Compliance test for device /dev/v4l-subdev0: Driver Info: Driver version : 6.14.0 Capabilities : 0x00000000 Client Capabilities: 0x0000000000000002 interval-uses-which Required ioctls: test VIDIOC_SUDBEV_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/v4l-subdev0 open: OK test VIDIOC_SUBDEV_QUERYCAP: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 11 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) test CREATE_BUFS maximum buffers: OK test VIDIOC_REMOVE_BUFS: OK test VIDIOC_EXPBUF: OK (Not Supported) test Requests: OK (Not Supported) test blocking wait: OK (Not Supported) Total for device /dev/v4l-subdev0: 46, Succeeded: 46, Failed: 0, Warnings: 0 atg@isp-pv-linux:~/v4l-utils/build/utils/v4l2-compliance$ --- Compliance test for amd_isp_capture device /dev/video0: ------------------------------------------------------- atg@isp-pv-linux:~/test$ sudo ./v4l2-compliance -d /dev/video0 [sudo] password for atg: v4l2-compliance 1.29.0-5348, 64 bits, 64-bit time_t v4l2-compliance SHA: 75e3f0e2c2cb 2025-03-17 18:12:17 Compliance test for amd_isp_capture device /dev/video0: Driver Info: Driver name : amd_isp_capture Card type : amd_isp_capture Bus info : platform:amd_isp_capture Driver version : 6.14.0 Capabilities : 0xa4200001 Video Capture I/O MC Streaming Extended Pix Format Device Capabilities Device Caps : 0x24200001 Video Capture I/O MC Streaming Extended Pix Format Media Driver Info: Driver name : amd_isp_capture Model : amd_isp41_mdev Serial : Bus info : platform:amd_isp_capture Media version : 6.14.0 Hardware revision: 0x00000000 (0) Driver version : 6.14.0 Interface Info: ID : 0x03000003 Type : V4L Video Entity Info: ID : 0x00000001 (1) Name : Preview Function : V4L2 I/O Pad 0x01000002 : 0: Sink Link 0x02000007: from remote pad 0x1000006 of entity 'ov05c10 99-0010' (Camera Sensor): Data, Enabled, Immutable Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/video0 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test CREATE_BUFS maximum buffers: OK test VIDIOC_REMOVE_BUFS: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) test blocking wait: OK Total for amd_isp_capture device /dev/video0: 49, Succeeded: 49, Failed: 0, Warnings: 0 atg@isp-pv-linux:~/test$ --- Thanks, Pratap On 4/1/2025 9:20 PM, Laurent Pinchart wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > 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 ? > Sure, will wait for Sakari's feedback. We will also check if the driver can work using acpi_device_id match instead of i2c_device_id. >>>> + >>>> +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. > > -- > Regards, > > Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] media: i2c: Add OV05C10 camera sensor driver 2025-04-02 1:20 ` Laurent Pinchart 2025-04-02 22:04 ` Nirujogi, Pratap @ 2025-05-06 7:45 ` Sakari Ailus 2025-05-06 11:29 ` Hans de Goede 1 sibling, 1 reply; 11+ messages in thread From: Sakari Ailus @ 2025-05-06 7:45 UTC (permalink / raw) To: Laurent Pinchart Cc: Nirujogi, Pratap, Bryan O'Donoghue, Pratap Nirujogi, mchehab, hverkuil, dave.stevenson, krzk, dan.carpenter, linux-media, linux-kernel, benjamin.chan, bin.du, grosikop, king.li, dantony, Venkata Narendra Kumar Gutta 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. DT bindings would of course be a plus. > > > >> + > > >> +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. > -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] media: i2c: Add OV05C10 camera sensor driver 2025-05-06 7:45 ` Sakari Ailus @ 2025-05-06 11:29 ` Hans de Goede 2025-05-07 20:25 ` Nirujogi, Pratap 0 siblings, 1 reply; 11+ messages in thread From: Hans de Goede @ 2025-05-06 11:29 UTC (permalink / raw) To: Sakari Ailus, Laurent Pinchart Cc: Nirujogi, Pratap, Bryan O'Donoghue, Pratap Nirujogi, mchehab, hverkuil, dave.stevenson, krzk, dan.carpenter, linux-media, linux-kernel, benjamin.chan, bin.du, grosikop, king.li, dantony, Venkata Narendra Kumar Gutta 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. >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] media: i2c: Add OV05C10 camera sensor driver 2025-05-06 11:29 ` Hans de Goede @ 2025-05-07 20:25 ` Nirujogi, Pratap 0 siblings, 0 replies; 11+ messages in thread From: Nirujogi, Pratap @ 2025-05-07 20:25 UTC (permalink / raw) To: Hans de Goede, Sakari Ailus, Laurent Pinchart Cc: Bryan O'Donoghue, Pratap Nirujogi, mchehab, hverkuil, dave.stevenson, krzk, dan.carpenter, linux-media, linux-kernel, benjamin.chan, bin.du, grosikop, king.li, dantony, Venkata Narendra Kumar Gutta On 5/6/2025 7:29 AM, Hans de Goede wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > 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 > > Thanks Hans for clarifying. > > > >>>>>> + >>>>>> +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. >>> >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] media: i2c: Add OV05C10 camera sensor driver 2025-03-31 19:17 ` Nirujogi, Pratap 2025-04-02 1:20 ` Laurent Pinchart @ 2025-04-08 21:09 ` Nirujogi, Pratap 1 sibling, 0 replies; 11+ messages in thread From: Nirujogi, Pratap @ 2025-04-08 21:09 UTC (permalink / raw) To: Bryan O'Donoghue, Pratap Nirujogi, mchehab, sakari.ailus, hverkuil, laurent.pinchart, dave.stevenson, krzk, dan.carpenter Cc: linux-media, linux-kernel, benjamin.chan, bin.du, grosikop, king.li, dantony, Venkata Narendra Kumar Gutta Hi Bryan, On 3/31/2025 3:17 PM, Nirujogi, Pratap wrote: > Hi Bryan, > > Thanks for the review. > > Thanks, > Pratap > > On 3/28/2025 9:18 PM, Bryan O'Donoghue wrote: >> Caution: This message originated from an External Source. Use proper >> caution when opening attachments, clicking links, or responding. >> >> >> 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" }, > {} > }; > I'm planning to submit v3 addressing the outstanding comments, but will not be including the ACPI device table for now. I will take care of this in the upcoming patch based on Sakari's feedback to accomodate both ACPI and I2C device tables. Thanks, Pratap >>> + >>> +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. > >> --- >> bod > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] media: i2c: Add OV05C10 camera sensor driver 2025-03-28 21:42 ` [PATCH v2] media: i2c: Add OV05C10 camera sensor driver Pratap Nirujogi 2025-03-29 1:18 ` Bryan O'Donoghue @ 2025-03-29 4:30 ` Krzysztof Kozlowski 2025-03-31 19:19 ` Nirujogi, Pratap 1 sibling, 1 reply; 11+ messages in thread From: Krzysztof Kozlowski @ 2025-03-29 4:30 UTC (permalink / raw) To: Pratap Nirujogi, mchehab, sakari.ailus, hverkuil, laurent.pinchart, dave.stevenson, dan.carpenter Cc: linux-media, linux-kernel, benjamin.chan, bin.du, grosikop, king.li, dantony, Venkata Narendra Kumar Gutta On 28/03/2025 22:42, Pratap Nirujogi wrote: > +static int ov05c10_probe(struct i2c_client *client) > +{ > + struct ov05c10 *ov05c10; > + u32 refclk; > + int ret; > + > + ov05c10 = devm_kzalloc(&client->dev, sizeof(*ov05c10), GFP_KERNEL); > + if (!ov05c10) > + return -ENOMEM; > + > + struct fwnode_handle *fwnode = dev_fwnode(&client->dev); > + > + ret = fwnode_property_read_u32(fwnode, "refclk", &refclk); Use existing properties, like clock-frequency. refclk means this is the clock, not it's frequency. > + if (ret) > + return dev_err_probe(&client->dev, -EINVAL, > + "fail to get refclk\n"); > + if (refclk != OV05C10_REF_CLK) > + return dev_err_probe(&client->dev, -EINVAL, > + "failbad refclk %u, %lu expected\n", > + refclk, OV05C10_REF_CLK); > + > + ret = ov05c10_parse_endpoint(&client->dev, fwnode); > + if (ret) > + return dev_err_probe(&client->dev, -EINVAL, > + "fail to parse endpoint\n"); > + > + ov05c10->enable_gpio = devm_gpiod_get(&client->dev, "sensor0_enable", > + GPIOD_OUT_LOW); Nothing improved and you did not bothered to wait for my feedback. You just sent v2. There is no second GPIO, otherwise would be present here. NAK. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] media: i2c: Add OV05C10 camera sensor driver 2025-03-29 4:30 ` Krzysztof Kozlowski @ 2025-03-31 19:19 ` Nirujogi, Pratap 0 siblings, 0 replies; 11+ messages in thread From: Nirujogi, Pratap @ 2025-03-31 19:19 UTC (permalink / raw) To: Krzysztof Kozlowski, Pratap Nirujogi, mchehab, sakari.ailus, hverkuil, laurent.pinchart, dave.stevenson, dan.carpenter Cc: linux-media, linux-kernel, benjamin.chan, bin.du, grosikop, king.li, dantony, Venkata Narendra Kumar Gutta Hi Krzysztof, Thanks for the review. Thanks, Pratap On 3/29/2025 12: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/03/2025 22:42, Pratap Nirujogi wrote: >> +static int ov05c10_probe(struct i2c_client *client) >> +{ >> + struct ov05c10 *ov05c10; >> + u32 refclk; >> + int ret; >> + >> + ov05c10 = devm_kzalloc(&client->dev, sizeof(*ov05c10), GFP_KERNEL); >> + if (!ov05c10) >> + return -ENOMEM; >> + >> + struct fwnode_handle *fwnode = dev_fwnode(&client->dev); >> + >> + ret = fwnode_property_read_u32(fwnode, "refclk", &refclk); > > Use existing properties, like clock-frequency. refclk means this is the > clock, not it's frequency. > sure, will replace "refclk" with "clock-frequency" variable in the next V3 patch. >> + if (ret) >> + return dev_err_probe(&client->dev, -EINVAL, >> + "fail to get refclk\n"); >> + if (refclk != OV05C10_REF_CLK) >> + return dev_err_probe(&client->dev, -EINVAL, >> + "failbad refclk %u, %lu expected\n", >> + refclk, OV05C10_REF_CLK); >> + >> + ret = ov05c10_parse_endpoint(&client->dev, fwnode); >> + if (ret) >> + return dev_err_probe(&client->dev, -EINVAL, >> + "fail to parse endpoint\n"); >> + >> + ov05c10->enable_gpio = devm_gpiod_get(&client->dev, "sensor0_enable", >> + GPIOD_OUT_LOW); > > Nothing improved and you did not bothered to wait for my feedback. You > just sent v2. > sorry, I should have waited for your feedback before submitting V2. I will use the suggested name "enable" in the next V3 submission. > There is no second GPIO, otherwise would be present here. > I rechecked both GPIOs. Only one is relevant for sensor driver and other one is for the ISP driver. Sorry for the confusion caused. > NAK. > > > > Best regards, > Krzysztof ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-05-07 20:25 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <MhUYQD7uWnfZQAPq7VslFWPHOmx2B2UfAIpbMhLq1-7GC_i5bI2hhns_-ov_AAVpEH_VmDDFYkS5aOKBwnY61g==@protonmail.internalid>
2025-03-28 21:42 ` [PATCH v2] media: i2c: Add OV05C10 camera sensor driver Pratap Nirujogi
2025-03-29 1:18 ` Bryan O'Donoghue
2025-03-31 19:17 ` Nirujogi, Pratap
2025-04-02 1:20 ` Laurent Pinchart
2025-04-02 22:04 ` Nirujogi, Pratap
2025-05-06 7:45 ` Sakari Ailus
2025-05-06 11:29 ` Hans de Goede
2025-05-07 20:25 ` Nirujogi, Pratap
2025-04-08 21:09 ` Nirujogi, Pratap
2025-03-29 4:30 ` Krzysztof Kozlowski
2025-03-31 19:19 ` Nirujogi, Pratap
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox