On Mon, Jan 23, 2023 at 01:51:35PM +0100, Hans de Goede wrote:
> Looking at isp_subdev_link_setup(), this function can never work,
> it does a switch-case like this:
>
> switch (local->index | is_media_entity_v4l2_subdev(remote->entity))
>
> with cases like this:
>
> case ATOMISP_SUBDEV_PAD_SINK | MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN
>
> where ATOMISP_SUBDEV_PAD_SINK matches an index (0-4) and
> MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN is 0x00020000, but
> is_media_entity_v4l2_subdev(remote->entity) does not return
> MEDIA_ENT_F_* values, it return a bool, so 0 or 1 which means
> that non of the cases can ever match the input value.
>
> Looking at the rest of the function all it ever does (if it
> would actually hit one of the cases) is set the atomisp_sub_device
> struct's input member.
>
> And checking the rest of the atomisp code that member is never
> read. Also userspace does not actually setup media-controller
> links when using the atomisp /dev/video$ nodes since all the links
> are fixed. So isp_subdev_link_setup() never runs.
>
> Remove the unnecessary and broken isp_subdev_link_setup() function
> and also remove the unused atomisp_sub_device struct's input member.
(One of the) best patch(es) in the series!
Reviewed-by: Andy Shevchenko <andy@xxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> .../media/atomisp/pci/atomisp_subdev.c | 79 -------------------
> .../media/atomisp/pci/atomisp_subdev.h | 13 ---
> 2 files changed, 92 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> index eb8f319fca5c..52d1936b8c87 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> @@ -714,85 +714,8 @@ static void isp_subdev_init_params(struct atomisp_sub_device *asd)
> }
> }
>
> -/*
> -* isp_subdev_link_setup - Setup isp subdev connections
> -* @entity: ispsubdev media entity
> -* @local: Pad at the local end of the link
> -* @remote: Pad at the remote end of the link
> -* @flags: Link flags
> -*
> -* return -EINVAL or zero on success
> -*/
> -static int isp_subdev_link_setup(struct media_entity *entity,
> - const struct media_pad *local,
> - const struct media_pad *remote, u32 flags)
> -{
> - struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> - struct atomisp_sub_device *isp_sd = v4l2_get_subdevdata(sd);
> - struct atomisp_device *isp = isp_sd->isp;
> - unsigned int i;
> -
> - switch (local->index | is_media_entity_v4l2_subdev(remote->entity)) {
> - case ATOMISP_SUBDEV_PAD_SINK | MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN:
> - /* Read from the sensor CSI2-ports. */
> - if (!(flags & MEDIA_LNK_FL_ENABLED)) {
> - isp_sd->input = ATOMISP_SUBDEV_INPUT_NONE;
> - break;
> - }
> -
> - if (isp_sd->input != ATOMISP_SUBDEV_INPUT_NONE)
> - return -EBUSY;
> -
> - for (i = 0; i < ATOMISP_CAMERA_NR_PORTS; i++) {
> - if (remote->entity != &isp->csi2_port[i].subdev.entity)
> - continue;
> -
> - isp_sd->input = ATOMISP_SUBDEV_INPUT_CSI2_PORT1 + i;
> - return 0;
> - }
> -
> - return -EINVAL;
> -
> - case ATOMISP_SUBDEV_PAD_SINK | MEDIA_ENT_F_OLD_BASE:
> - /* read from memory */
> - if (flags & MEDIA_LNK_FL_ENABLED) {
> - if (isp_sd->input >= ATOMISP_SUBDEV_INPUT_CSI2_PORT1 &&
> - isp_sd->input < (ATOMISP_SUBDEV_INPUT_CSI2_PORT1
> - + ATOMISP_CAMERA_NR_PORTS))
> - return -EBUSY;
> - isp_sd->input = ATOMISP_SUBDEV_INPUT_MEMORY;
> - } else {
> - if (isp_sd->input == ATOMISP_SUBDEV_INPUT_MEMORY)
> - isp_sd->input = ATOMISP_SUBDEV_INPUT_NONE;
> - }
> - break;
> -
> - case ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW | MEDIA_ENT_F_OLD_BASE:
> - /* always write to memory */
> - break;
> -
> - case ATOMISP_SUBDEV_PAD_SOURCE_VF | MEDIA_ENT_F_OLD_BASE:
> - /* always write to memory */
> - break;
> -
> - case ATOMISP_SUBDEV_PAD_SOURCE_CAPTURE | MEDIA_ENT_F_OLD_BASE:
> - /* always write to memory */
> - break;
> -
> - case ATOMISP_SUBDEV_PAD_SOURCE_VIDEO | MEDIA_ENT_F_OLD_BASE:
> - /* always write to memory */
> - break;
> -
> - default:
> - return -EINVAL;
> - }
> -
> - return 0;
> -}
> -
> /* media operations */
> static const struct media_entity_operations isp_subdev_media_ops = {
> - .link_setup = isp_subdev_link_setup,
> .link_validate = v4l2_subdev_link_validate,
> /* .set_power = v4l2_subdev_set_power, */
> };
> @@ -1071,8 +994,6 @@ static int isp_subdev_init_entities(struct atomisp_sub_device *asd)
> struct media_entity *me = &sd->entity;
> int ret;
>
> - asd->input = ATOMISP_SUBDEV_INPUT_NONE;
> -
> v4l2_subdev_init(sd, &isp_subdev_v4l2_ops);
> sprintf(sd->name, "ATOMISP_SUBDEV_%d", asd->index);
> v4l2_set_subdevdata(sd, asd);
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.h b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
> index bd2872cbb50c..daa6077a83bd 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
> @@ -30,18 +30,6 @@
>
> /* EXP_ID's ranger is 1 ~ 250 */
> #define ATOMISP_MAX_EXP_ID (250)
> -enum atomisp_subdev_input_entity {
> - ATOMISP_SUBDEV_INPUT_NONE,
> - ATOMISP_SUBDEV_INPUT_MEMORY,
> - ATOMISP_SUBDEV_INPUT_CSI2,
> - /*
> - * The following enum for CSI2 port must go together in one row.
> - * Otherwise it breaks the code logic.
> - */
> - ATOMISP_SUBDEV_INPUT_CSI2_PORT1,
> - ATOMISP_SUBDEV_INPUT_CSI2_PORT2,
> - ATOMISP_SUBDEV_INPUT_CSI2_PORT3,
> -};
>
> #define ATOMISP_SUBDEV_PAD_SINK 0
> /* capture output for still frames */
> @@ -267,7 +255,6 @@ struct atomisp_sub_device {
> struct atomisp_pad_format fmt[ATOMISP_SUBDEV_PADS_NUM];
> u16 capture_pad; /* main capture pad; defines much of isp config */
>
> - enum atomisp_subdev_input_entity input;
> unsigned int output;
> struct atomisp_video_pipe video_out_capture; /* capture output */
> struct atomisp_video_pipe video_out_vf; /* viewfinder output */
> --
> 2.39.0
>
--
With Best Regards,
Andy Shevchenko
