Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Mon, 6 Nov 2023 08:45:55 +0000
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Cc:     Sebastian Reichel <sre@kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        linux-media@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 4/4] media: i2c: gc0308: new driver
Message-ID: <ZUinwwRVwqnIuueJ@kekkonen.localdomain>
References: <20231027011417.2174658-1-sre@kernel.org>
 <20231027011417.2174658-5-sre@kernel.org>
 <zcei5x4omoqamuap42imybtbsax4bw43juymald4l7ccq766lb@2wknnhmawdbt>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <zcei5x4omoqamuap42imybtbsax4bw43juymald4l7ccq766lb@2wknnhmawdbt>
Precedence: bulk

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@ideasonboard.com>
> 
> 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