Hi Laurent, On 6/7/23 17:51, Laurent Pinchart wrote: > Hi Hans, > > On Wed, Jun 07, 2023 at 10:53:54AM +0200, Hans de Goede wrote: >> On 6/6/23 22:53, Andy Shevchenko wrote: >>> On Tue, Jun 6, 2023 at 7:58 PM Hans de Goede wrote: >>>> >>>> Use the new comon CCI register access helpers to replace the private >>>> register access helpers in the ov2680 driver. >>>> >>>> While at it also switch to using the same register address defines >>>> as the standard drivers/media/i2c/ov2680.c driver to make merging >>>> the 2 drivers simpler. >>> >>> ... >>> >>>> + cci_write(sensor->regmap, OV2680_REG_SENSOR_CTRL_0A, sensor_ctrl_0a, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_START, sensor->mode.h_start, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_VERTICAL_START, sensor->mode.v_start, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_END, sensor->mode.h_end, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_VERTICAL_END, sensor->mode.v_end, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_OUTPUT_SIZE, >>>> + sensor->mode.h_output_size, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_VERTICAL_OUTPUT_SIZE, >>>> + sensor->mode.v_output_size, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_TIMING_HTS, sensor->mode.hts, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_TIMING_VTS, sensor->mode.vts, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_ISP_X_WIN, 0, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_ISP_Y_WIN, 0, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_X_INC, inc, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_Y_INC, inc, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_X_WIN, sensor->mode.h_output_size, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_Y_WIN, sensor->mode.v_output_size, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_FORMAT1, fmt1, &ret); >>>> + cci_write(sensor->regmap, OV2680_REG_FORMAT2, fmt2, &ret); >>> >>> I know that &ret thingy was discussed before and Laurent is keen to >>> have this, but has anybody actually tested how bad or not at all the >>> code generation becomes? >> >> The cci_write function is in another module, so it won't be inlined >> and as such I don't see how the code generation can become bad. We >> loose all the if (ret) return ret; checks here, so the code should >> become smaller. >> >> Or are you worried about having to pass the 1 extra parameter ? >> >>> ... >>> >>>> + struct device *dev; >>>> + struct regmap *regmap; >>> >>> Isn't the same device associated with regmap? If so, one of them >>> probably duplicates the other. >> >> You are right, but the entire atomisp-ov2680.c file is going away real >> soon now. I plan to post a series to get drivers/media/i2c/ov2680.c >> ready to replace it later today. >> >> So I'm not even sure if this patch should be merged, as I mentioned in >> the cover letter this one is mostly here to illustrate use of the new >> helpers. > > How about porting drivers/media/i2c/imx290.c ? That's a real-life > example that can be merged, which is good to serve as an example > showcasing the API usage in mainline. It will also help ensuring that > these helpers are a good fit for drivers that already encode the > register width in the macros. I prefer to port over drivers which I can actually test, at least for now. I already have converting ov5693.c (which also already has macros to encode to width) on my TODO list. I'll convert that for v2 of the series. And I also have a conversion of the "main" drivers/media/i2c/ov2680.c ready. I'll post that conversion as part of my big main ov2680 changes series which I'll post in a couple of minutes (just need to write a cover letter and then its ready). Regards, Hans >> I also wrote this patch to make porting recent atomisp-ov2680.c >> changes over to drivers/media/i2c/ov2680.c easier. Part of the series >> to get drivers/media/i2c/ov2680.c into shape is converting it to the >> new CCI helpers so that I could then easily copy over bits from the >> also converted atomisp-ov2680.c. >> >> So it might be interesting to still merge this so that the latest >> state of atomisp-ov2680.c is easier to compare to >> drivers/media/i2c/ov2680.c if the need arises. >
