Hi Jacopo, Sebastian,
On Mon, Oct 30, 2023 at 09:37:08AM +0100, Jacopo Mondi wrote:
> > +static bool gc0308_is_valid_format(u32 code)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(gc0308_formats); i++) {
> > + if (gc0308_formats[i].code == code)
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static int gc0308_enum_frame_size(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > + if (fse->index >= ARRAY_SIZE(gc0308_frame_sizes))
> > + return -EINVAL;
> > +
> > + if (!gc0308_is_valid_format(fse->code))
> > + return -EINVAL;
> > +
> > + fse->min_width = gc0308_frame_sizes[fse->index].width;
> > + fse->max_width = gc0308_frame_sizes[fse->index].width;
> > + fse->min_height = gc0308_frame_sizes[fse->index].height;
> > + fse->max_height = gc0308_frame_sizes[fse->index].height;
> > +
> > + return 0;
> > +}
> > +
> > +static void gc0308_update_pad_format(const struct gc0308_frame_size *mode,
> > + struct v4l2_mbus_framefmt *fmt, u32 code)
> > +{
> > + fmt->width = mode->width;
> > + fmt->height = mode->height;
> > + fmt->code = code;
> > + fmt->field = V4L2_FIELD_NONE;
> > + fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > +}
> > +
> > +static int gc0308_set_format(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_state *sd_state,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct gc0308 *gc0308 = to_gc0308(sd);
> > + const struct gc0308_frame_size *mode;
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(gc0308_formats); i++) {
> > + if (fmt->format.code == gc0308_formats[i].code)
> > + break;
> > + }
> > +
> > + if (i >= ARRAY_SIZE(gc0308_formats)) {
> > + dev_warn(gc0308->dev, "unsupported format code: %08x\n",
> > + fmt->format.code);
This isn't supposed to generate a kernel log message. I'd drop it
altogether.
> > + i = 0;
> > + }
>
> This looks very similar to gc0308_is_valid_format()
Could gc0308_is_valid_format() return a pointer to the format array or
NULL? Then you could check for NULL here and use the first entry in that
case.
>
> > +
> > + mode = v4l2_find_nearest_size(gc0308_frame_sizes,
> > + ARRAY_SIZE(gc0308_frame_sizes), width,
> > + height, fmt->format.width,
> > + fmt->format.height);
> > +
> > + gc0308_update_pad_format(mode, &fmt->format, gc0308_formats[i].code);
> > + *v4l2_subdev_get_pad_format(sd, sd_state, fmt->pad) = fmt->format;
> > +
> > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> > + return 0;
> > +
> > + gc0308->mode.out_format = gc0308_formats[i].regval;
> > + gc0308->mode.subsample = mode->subsample;
> > + gc0308->mode.width = mode->width;
> > + gc0308->mode.height = mode->height;
> > +
> > + return 0;
> > +}
...
> > + ret = v4l2_async_register_subdev(&gc0308->sd);
> > + if (ret) {
> > + dev_err_probe(dev, ret, "failed to register v4l subdev\n");
> > + goto fail_power_off;
> > + }
> > +
> > + pm_runtime_set_active(dev);
> > + pm_runtime_enable(dev);
> > + pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> > + pm_runtime_use_autosuspend(&client->dev);
> > + pm_runtime_idle(dev);
This will effective power off the device immediately, without a delay. But
I guess that's ok.
Note that enabling runtime PM needs to take place before registering the
sub-device. I'd move all these calls before that.
> > +
> > + return 0;
> > +
> > +fail_power_off:
> > + gc0308_power_off(dev);
> > +fail_subdev_cleanup:
> > + v4l2_subdev_cleanup(&gc0308->sd);
> > +fail_media_entity_cleanup:
> > + media_entity_cleanup(&gc0308->sd.entity);
> > +fail_ctrl_hdl_cleanup:
> > + v4l2_ctrl_handler_free(&gc0308->hdl);
> > + return ret;
> > +}
> > +
> > +static void gc0308_remove(struct i2c_client *client)
> > +{
> > + struct gc0308 *gc0308 = i2c_get_clientdata(client);
> > + struct device *dev = &client->dev;
> > +
> > + pm_runtime_get_sync(dev);
>
> Uh, I've never seen this call in a _remove before. Is it intentional ?
I think disabling runtime PM and marking the device suspended are enough
here runtime PM-wise.
>
> Apart these two nits the rest is good! thanks for addressing the
> comments received on the previous version.
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
>
> Thanks
> j
>
> > +
> > + v4l2_async_unregister_subdev(&gc0308->sd);
> > + v4l2_ctrl_handler_free(&gc0308->hdl);
> > +
> > + pm_runtime_disable(dev);
> > + pm_runtime_set_suspended(dev);
> > + pm_runtime_put_noidle(dev);
> > + gc0308_power_off(dev);
> > +}
--
Regards,
Sakari Ailus
