Hi Hans, On Wed, Mar 19, 2025 at 03:59:31PM +0100, Hans de Goede wrote: > Hi Sakari, > > On 14-Mar-25 1:46 PM, Sakari Ailus wrote: > > Hi Hans, > > > > Thanks for the update. > > > > On Fri, Mar 14, 2025 at 11:11:25AM +0100, Hans de Goede wrote: > >> From: Heimir Thor Sverrisson <heimir.sverrisson@xxxxxxxxx> > >> > >> Add a new driver for the Omnivision OV02C10 camera sensor. This is based > >> on the out of tree driver by Hao Yao <hao.yao@xxxxxxxxx> from: > >> https://github.com/intel/ipu6-drivers/blob/master/drivers/media/i2c/ov02c10.c > >> > >> This has been tested on a Dell XPS 9440 together with the IPU6 isys CSI > >> driver and the libcamera software ISP code. > >> > >> Tested-by: Ingvar Hagelund <ingvar@xxxxxxxxxxxxxxxxxx> > >> Signed-off-by: Heimir Thor Sverrisson <heimir.sverrisson@xxxxxxxxx> > >> Co-developed-by: Hans de Goede <hdegoede@xxxxxxxxxx> > >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > > <snip> > > >> diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c > >> new file mode 100644 > >> index 000000000000..5626aa2fe62c > >> --- /dev/null > >> +++ b/drivers/media/i2c/ov02c10.c > >> @@ -0,0 +1,1012 @@ > > <snip> > > >> +static const struct reg_sequence sensor_1928x1092_30fps_setting[] = { > > > > Could you use struct cci_reg_sequence for this? > > > >> + {0x0301, 0x08}, > > > > { CCI_REG8(0x0301), 0x08 }, > > > > etc. > > > > That would also enable using human-readable names in this list. > > TBH I would rather not, adding the CCI_REGx everywhere will make > it harder to keep the out of tree code in sync with this, using > raw struct reg_sequence combined with regmap_multi_reg_write() > is sort of the norm for drivers converted to the CCI helpers. > > Also while using symbolic register names (defines) greatly > improves the readability of other code accessing the registers, > I find it unhelpful for these large just send a bunch of > register values to the sensor cases, especially since for many > of the addresses we've no clue what we're sending / what > the register does. That's true for the undocumented ones (and this being an OV sensor, there are many) but for the rest it's helpful. I presume there's no need for updating downstream drivers after we have one in upstream? > > > > >> + {0x0303, 0x06}, > >> + {0x0304, 0x01}, > >> + {0x0305, 0xe0}, > > <snip> > > >> +}; > >> + > >> +static const struct reg_sequence sensor_1928x1092_30fps_1lane_setting[] = { > >> + {0x301b, 0xd2}, > >> + {0x3027, 0xe1}, > >> + {0x380c, 0x08}, > >> + {0x380d, 0xe8}, > >> + {0x380e, 0x04}, > >> + {0x380f, 0x8c}, > >> + {0x394e, 0x0b}, > >> + {0x4800, 0x24}, > >> + {0x5000, 0xf5}, > >> + /* plls */ > >> + {0x0303, 0x05}, > >> + {0x0305, 0x90}, > >> + {0x0316, 0x90}, > >> + {0x3016, 0x12}, > >> +}; > >> + > >> +static const struct reg_sequence sensor_1928x1092_30fps_2lane_setting[] = { > >> + {0x301b, 0xf0}, > >> + {0x3027, 0xf1}, > >> + {0x380c, 0x04}, > >> + {0x380d, 0x74}, > >> + {0x380e, 0x09}, > >> + {0x380f, 0x18}, > >> + {0x394e, 0x0a}, > >> + {0x4041, 0x20}, > >> + {0x4884, 0x04}, > >> + {0x4800, 0x64}, > >> + {0x4d00, 0x03}, > >> + {0x4d01, 0xd8}, > >> + {0x4d02, 0xba}, > >> + {0x4d03, 0xa0}, > >> + {0x4d04, 0xb7}, > >> + {0x4d05, 0x34}, > >> + {0x4d0d, 0x00}, > >> + {0x5000, 0xfd}, > >> + {0x481f, 0x30}, > > > > It's surprising to have more registers when the number of lanes is 2. I > > guess for 1 lane case it's using defaults? > > I guess so. At your request I split the registers which were > different for the 1 / 2 lane cases into 2 separate registerlists > and some register writes where only in the long 2 lane register list > of the previous version. Ok. Adding further modes later on will require addressing this. > > >> +static int ov02c10_test_pattern(struct ov02c10 *ov02c10, int pattern) > >> +{ > >> + int ret = 0; > >> + > >> + if (!pattern) > >> + return cci_update_bits(ov02c10->regmap, OV02C10_REG_TEST_PATTERN, > >> + BIT(7), 0, NULL); > >> + > >> + cci_update_bits(ov02c10->regmap, OV02C10_REG_TEST_PATTERN, > >> + 0x03, pattern - 1, &ret); > >> + cci_update_bits(ov02c10->regmap, OV02C10_REG_TEST_PATTERN, > >> + BIT(7), OV02C10_TEST_PATTERN_ENABLE, &ret); > > > > I'd do here: > > > > return cci_update_bits(...); > > Since we are relying on the error preserving / propagating > behavior of the CCI helpers here that feels weird / inconsistent, > I would expect either: > > ret = cci_update_bits(ov02c10->regmap, OV02C10_REG_TEST_PATTERN, > 0x03, pattern - 1, NULL); > if (ret) > return ret; > > return cci_update_bits(ov02c10->regmap, OV02C10_REG_TEST_PATTERN, > BIT(7), OV02C10_TEST_PATTERN_ENABLE, NULL); > } > > or: > > cci_update_bits(ov02c10->regmap, OV02C10_REG_TEST_PATTERN, > 0x03, pattern - 1, &ret); > cci_update_bits(ov02c10->regmap, OV02C10_REG_TEST_PATTERN, > BIT(7), OV02C10_TEST_PATTERN_ENABLE, &ret); > return ret; > } > > not a mix of the 2, with my preference going to the latter. Works for me, too, although I expect someone, will come some day and submit a cleanup patch for this. :-) I'm ok with v10, I guess further cleanup can be done if / when more functionality is added. -- Regards, Sakari Ailus
