Moi,
On Fri, Dec 20, 2024 at 03:19:09PM +0200, Tomi Valkeinen wrote:
> Hi,
Thanks for the review.
>
> On 17/12/2024 23:54, Sakari Ailus wrote:
> > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-17x.c b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> > index 380c99321030..cc93f79179dd 100644
> > --- a/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> > +++ b/drivers/media/platform/qcom/camss/camss-vfe-17x.c
> > @@ -443,16 +443,18 @@ static int vfe_enable_output(struct vfe_line *line)
> > struct vfe_device *vfe = to_vfe(line);
> > struct vfe_output *output = &line->output;
> > const struct vfe_hw_ops *ops = vfe->res->hw_ops;
> > - struct media_entity *sensor;
> > + struct media_pad *sensor;
> > unsigned long flags;
> > unsigned int frame_skip = 0;
> > unsigned int i;
> > sensor = camss_find_sensor(&line->subdev.entity);
> > if (sensor) {
> > - struct v4l2_subdev *subdev = media_entity_to_v4l2_subdev(sensor);
> > + struct v4l2_subdev *subdev =
> > + media_entity_to_v4l2_subdev(sensor->entity);
> > - v4l2_subdev_call(subdev, sensor, g_skip_frames, &frame_skip);
> > + v4l2_subdev_call(subdev, sensor, g_skip_frames,
> > + &frame_skip);
>
> This looks a bit odd... The sensor parameter was media_entity, but now it's
> media_pad, but I don't think the called op has changed. Is that right?
"sensor" in this case is the name of the operations struct, it's unrelated
to the variable with the same name. :-)
But the second line wrap is unnecessary, I'll fix it.
>
> > /* Max frame skip is 29 frames */
> > if (frame_skip > VFE_FRAME_DROP_VAL - 1)
> > frame_skip = VFE_FRAME_DROP_VAL - 1;
> > diff --git a/drivers/media/platform/qcom/camss/camss-vfe-gen1.c b/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
> > index eb33c03df27e..1970f7aa6d4d 100644
> > --- a/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
> > +++ b/drivers/media/platform/qcom/camss/camss-vfe-gen1.c
> > @@ -170,7 +170,7 @@ static int vfe_enable_output(struct vfe_line *line)
> > struct vfe_device *vfe = to_vfe(line);
> > struct vfe_output *output = &line->output;
> > const struct vfe_hw_ops *ops = vfe->res->hw_ops;
> > - struct media_entity *sensor;
> > + struct media_pad *sensor;
> > unsigned long flags;
> > unsigned int frame_skip = 0;
> > unsigned int i;
> > @@ -182,9 +182,11 @@ static int vfe_enable_output(struct vfe_line *line)
> > sensor = camss_find_sensor(&line->subdev.entity);
> > if (sensor) {
> > - struct v4l2_subdev *subdev = media_entity_to_v4l2_subdev(sensor);
> > + struct v4l2_subdev *subdev =
> > + media_entity_to_v4l2_subdev(sensor->entity);
> > - v4l2_subdev_call(subdev, sensor, g_skip_frames, &frame_skip);
> > + v4l2_subdev_call(subdev, sensor, g_skip_frames,
> > + &frame_skip);
>
> Here too.
Ditto.
>
> > /* Max frame skip is 29 frames */
> > if (frame_skip > VFE_FRAME_DROP_VAL - 1)
> > frame_skip = VFE_FRAME_DROP_VAL - 1;
> > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> > index 004a74f6b2f6..e86de4b59723 100644
> > --- a/drivers/media/platform/qcom/camss/camss.c
> > +++ b/drivers/media/platform/qcom/camss/camss.c
> > @@ -1996,12 +1996,12 @@ void camss_disable_clocks(int nclocks, struct camss_clock *clock)
> > }
> > /*
> > - * camss_find_sensor - Find a linked media entity which represents a sensor
> > + * camss_find_sensor - Find the media pad via which the sensor is linked
> > * @entity: Media entity to start searching from
> > *
> > * Return a pointer to sensor media entity or NULL if not found
> > */
> > -struct media_entity *camss_find_sensor(struct media_entity *entity)
> > +struct media_pad *camss_find_sensor(struct media_entity *entity)
>
> I don't like this change. Maybe rename the function to
> camss_find_sensor_pad(), and rename the "sensor" variables to "sensor_pad".
> That would make the diff more readable, too, to show all the places where we
> used to have a media_entity, but now have media_pad.
>
> But even then, after this change, I think mostly the callers of
> camss_find_sensor() will just end up changing it back to entity. Maybe
> that's fine, but is there a way to keep the camss_find_sensor as it is now,
> and just do something extra when calling v4l2_get_link_freq() to get the
> media_pad...
There are just a few callers of this function, we don't need a specific
helper for that reason within a driver. I'll rename the function.
--
Ystävällisin terveisin,
Sakari Ailus
