Hi Laurent,
On Tue, Apr 23, 2024 at 03:50:36PM +0300, Laurent Pinchart wrote:
> > > > + return ccs_get_format(subdev, sd_state, fmt);
> > > > +
> > > > mutex_lock(&sensor->mutex);
> > >
> > > Is this needed, shouldn't the state lock be enough ?
> >
> > Not while the access to the device's state is serialised using the driver's
> > own mutex. This changes two patches later though.
>
> I realized that during the review, two patches later :-)
Yes, this was my expectation, too. ;)
>
> > > >
> > > > + if (subdev == &sensor->src->sd && fmt->stream == CCS_STREAM_META) {
> > > > + ccs_set_format_meta(subdev, sd_state, &fmt->format);
> > > > +
> > > > + mutex_unlock(&sensor->mutex);
> > > > +
> > > > + return 0;
> > > > + }
> > > > +
> > > > if (fmt->pad == ssd->source_pad) {
> > > > int rval;
> > > >
> > > > rval = ccs_set_format_source(subdev, sd_state, fmt);
> > > > + if (sensor->embedded_start != sensor->embedded_end)
> > >
> > > A ccs_sensor_has_embedded_data() (name bikeshedding allowed) inline
> > > helper could be nice to replace this manual check could be nice, as you
> > > do the same in many locations below.
> >
> > Sounds good.
> >
> > > > + ccs_set_format_meta(subdev, sd_state, NULL);
> > >
> > > This doesn't seem correct, you shouldn't set the metadata format on
> > > subdevs that are not the source subdev.
> >
> > Good point. I'll add a check.
> >
> > > A comment to explain how the metadata format is propagated would also be
> > > useful.
> >
> > I'll add this to the documentation patch which actually could be better
> > after this patch, not before.
>
> I meant comments in the source code, to make it easier to follow the
> code flow. Format propagation is error-prone, having comments explaining
> what the code does next to the code helps during review, and should also
> help during futher developments.
I'll add some here, too.
...
> > > > @@ -3109,6 +3407,14 @@ static const struct v4l2_subdev_pad_ops ccs_pad_ops = {
> > > > .set_fmt = ccs_set_format,
> > > > .get_selection = ccs_get_selection,
> > > > .set_selection = ccs_set_selection,
> > > > +};
> > > > +
> > > > +static const struct v4l2_subdev_pad_ops ccs_src_pad_ops = {
> > > > + .enum_mbus_code = ccs_enum_mbus_code,
> > > > + .get_fmt = ccs_get_format,
> > >
> > > I'm surprised you need to implement .get_fmt(). The
> > > v4l2_subdev_get_fmt() helper should have been enough.
> >
> > It should be possible to get rid of that now, too. I'll add a new patch for
> > this.
Now I remember why it's here. The controls affect the mbus code and
changing this is outside the scope for now (I'm not sure if someone would
complain about this changing).
Presumably such changes could be merged with the sensor API changes in
order to avoid several separate changes, so to be determined later on. The
same goes for this patch (post 6.10).
--
Regards,
Sakari Ailus
