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