Hi Bingbu,
For some reason my mail client decided to add double > > below. Hope it
is not too confusing.
On Tue, 2023-10-24 at 19:29 +0800, bingbu.cao@xxxxxxxxx wrote:
> > From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> >
> > Input system driver do basic isys hardware setup and irq handling
> > and work with fwnode and v4l2 to register the ISYS v4l2 devices.
> >
> > Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > Reported-by: Claus Stovgaard <claus.stovgaard@xxxxxxxxx>
> > ---
> > drivers/media/pci/intel/ipu6/ipu6-isys.c | 1345 >
> > ++++++++++++++++++++++
> > drivers/media/pci/intel/ipu6/ipu6-isys.h | 201 ++++
> > 2 files changed, 1546 insertions(+)
> > create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys.c
> > create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys.h
> >
...
> > +static int isys_register_video_devices(struct ipu6_isys *isys)
> > +{
> > + unsigned int i;
> > + int ret;
> > +
> > + for (i = 0; i < NR_OF_VIDEO_DEVICE; i++) {
> > + snprintf(isys->av[i].vdev.name, > sizeof(isys-
> > >av[i].vdev.name),
> > + IPU6_ISYS_ENTITY_PREFIX " ISYS Capture
> > %u", > i);
> > + isys->av[i].isys = isys;
> > + isys->av[i].aq.buf_prepare = ipu6_isys_buf_prepare;
> > + isys->av[i].aq.fill_frame_buf_set =
> > + ipu6_isys_buf_to_fw_frame_buf_pin;
> > + isys->av[i].aq.link_fmt_validate = >
> > ipu6_isys_link_fmt_validate;
These 3 function pointers are always set to the same value, is there
any point in that, or could they be called directly?
> > + isys->av[i].aq.vbq.buf_struct_size =
> > + sizeof(struct ipu6_isys_video_buffer);
> > + isys->av[i].pfmt = &ipu6_isys_pfmts[0];
> > +
> > + ret = ipu6_isys_video_init(&isys->av[i]);
> > + if (ret)
> > + goto fail;
> > + }
> > +
> > + return 0;
> > +
> > +fail:
> > + while (i--)
> > + ipu6_isys_video_cleanup(&isys->av[i]);
> > +
> > + return ret;
> > +}
...
> > + if (resp->error_info.error == >
> > IPU6_FW_ISYS_ERROR_STREAM_IN_SUSPENSION)
> > + /* Suspension is kind of special case: not enough >
> > buffers */
> > + dev_dbg(&adev->auxdev.dev,
> > + "FW error resp %02d %s, stream %u, error >
> > SUSPENSION, details %d, timestamp 0x%16.16llx, pin %d\n",
> > + resp->type,
> > + fw_msg[resp_type_to_index(resp->type)].msg,
> > + resp->stream_handle,
> > + resp->error_info.error_details,
> > + fw_msg[resp_type_to_index(resp-
> > >type)].valid_> ts ?
> > + ts : 0, resp->pin_id);
> > + else if (resp->error_info.error)
> > + dev_dbg(&adev->auxdev.dev,
> > + "FW error resp %02d %s, stream %u, error
> > %d, > details %d, timestamp 0x%16.16llx, pin %d\n",
> > + resp->type,
> > + fw_msg[resp_type_to_index(resp->type)].msg,
> > + resp->stream_handle,
> > + resp->error_info.error, > resp-
> > >error_info.error_details,
> > + fw_msg[resp_type_to_index(resp-
> > >type)].valid_> ts ?
> > + ts : 0, resp->pin_id);
> > + else
> > + dev_dbg(&adev->auxdev.dev,
> > + "FW resp %02d %s, stream %u, timestamp >
> > 0x%16.16llx, pin %d\n",
> > + resp->type,
> > + fw_msg[resp_type_to_index(resp->type)].msg,
> > + resp->stream_handle,
> > + fw_msg[resp_type_to_index(resp-
> > >type)].valid_> ts ?
> > + ts : 0, resp->pin_id);
fw_msg[resp_type_to_index(resp->type)] is duplicated 6 times above. It
would be a lot cleaner if you did the lookup once before the if/else
if/else block.
>
/Andreas
