(resending as plain text)
On Sun, Aug 30, 2020 at 4:43 PM Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
>
> Hi Dafna,
>
> On Thu, Jul 23, 2020 at 03:20:10PM +0200, Dafna Hirschfeld wrote:
> > Add a function 'rkisp1_cap_enum_mbus_codes' that receive
> > a pointer to 'v4l2_subdev_mbus_code_enum' and returns the
> > next supported mbus format of the capture. The function
> > assumes that pixel formats with identical 'mbus' are grouped
> > together in the hardcoded arrays, therefore the order of the
> > entries in the array 'rkisp1_sp_fmts' are adjusted.
> > This function is a helper for the media bus enumeration of
> > the source pad of the resizer entity.
> >
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> > ---
> > drivers/staging/media/rkisp1/rkisp1-capture.c | 77 ++++++++++++-------
> > drivers/staging/media/rkisp1/rkisp1-common.h | 8 ++
> > 2 files changed, 58 insertions(+), 27 deletions(-)
> >
>
> Thank you for the patch. Please see my comments inline.
>
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > index 5dd91b5f82a4..4dabd07a3da9 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > @@ -112,6 +112,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
> > .write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> > .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > },
> > + /* yuv400 */
> > + {
> > + .fourcc = V4L2_PIX_FMT_GREY,
> > + .uv_swap = 0,
> > + .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> > + .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > + },
> > /* yuv420 */
> > {
> > .fourcc = V4L2_PIX_FMT_NV21,
> > @@ -144,13 +151,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
> > .write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> > .mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> > },
> > - /* yuv400 */
> > - {
> > - .fourcc = V4L2_PIX_FMT_GREY,
> > - .uv_swap = 0,
> > - .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> > - .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > - },
> > /* raw */
> > {
> > .fourcc = V4L2_PIX_FMT_SRGGB8,
> > @@ -236,6 +236,26 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
> > .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> > .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > },
> > + /* yuv400 */
> > + {
> > + .fourcc = V4L2_PIX_FMT_GREY,
> > + .uv_swap = 0,
> > + .write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> > + .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> > + .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > + },
> > + /* rgb */
> > + {
> > + .fourcc = V4L2_PIX_FMT_XBGR32,
> > + .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > + .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> > + .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > + }, {
> > + .fourcc = V4L2_PIX_FMT_RGB565,
> > + .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > + .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> > + .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > + },
>
> Is there any reason to move RGB formats here rather than keeping them at
> the end of the list, after all YUV formats?
Hi,
yes, the new function 'rkisp1_cap_enum_mbus_codes' assumes that
pixelformats with
identical mbus codes are grouped together. It uses it to iterate the
formats and increment
a counter when encountering a new mbus.
>
> > /* yuv420 */
> > {
> > .fourcc = V4L2_PIX_FMT_NV21,
> > @@ -274,26 +294,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
> > .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> > .mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> > },
> > - /* yuv400 */
> > - {
> > - .fourcc = V4L2_PIX_FMT_GREY,
> > - .uv_swap = 0,
> > - .write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> > - .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> > - .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > - },
> > - /* rgb */
> > - {
> > - .fourcc = V4L2_PIX_FMT_XBGR32,
> > - .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > - .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> > - .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > - }, {
> > - .fourcc = V4L2_PIX_FMT_RGB565,
> > - .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > - .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> > - .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > - },
> > };
> >
> > static const struct rkisp1_capture_config rkisp1_capture_config_mp = {
> > @@ -334,6 +334,29 @@ rkisp1_vdev_to_node(struct video_device *vdev)
> > return container_of(vdev, struct rkisp1_vdev_node, vdev);
> > }
> >
> > +int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap,
> > + struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > + const struct rkisp1_capture_fmt_cfg *fmts = cap->config->fmts;
> > + u32 curr_mbus = fmts[0].mbus;
> > + int i, n = 0;
> > +
> > + if (code->index == 0) {
> > + code->code = fmts[0].mbus;
> > + return 0;
> > + }
> > +
> > + for (i = 1; i < cap->config->fmt_size; i++)
>
> How about just initializing curr_mbus to MEDIA_BUS_FMT_FIXED? This is not
> supposed to be found in the array, so is a safe initial value, which would
> allow removing the explicit handling of index == 0.
yes, I'll do that.
>
> > + if (fmts[i].mbus != curr_mbus) {
>
> As Helen mentioned, we could use the continue keyword to skip the iteration
> early and make it obvious that rest of the loop is entirely for the case
> where mbus != curr_mbus, removing one level of nesting.
>
that too,
> > + curr_mbus = fmts[i].mbus;
> > + if (++n == code->index) {
> > + code->code = curr_mbus;
> > + return 0;
> > + }
> > + }
>
> According to the kernel coding style guidelines [1], this for loop should
> have braces.
>
> [1] https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
>
thanks, I didn't know that.
Thanks,
Dafna
> Best regards,
> Tomasz
