Hi Sakari, On 19-Mar-25 4:16 PM, Sakari Ailus wrote: > 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? Well Intel's camera team has a habit of adding downstream changes to drivers after they have been mainlined to e.g. support a different CSI lane count. Or even just downstream patches to driver which they never had in their downstream repo, see e.g. the patches for ov13b10 and ov08x40 here: https://github.com/intel/ipu6-drivers/tree/master/patch/v6.12 >>>> + {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 register lists >> 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. That is good to hear, thank you. Regards, Hans
