Re: [PATCH v6 2/3] media: rockchip: Add a driver for Rockhip's camera interface

[Krzysztof Kozlowski]

On 29/09/2023 12:08, Mehdi Djait wrote:
> Introduce a driver for the camera interface on some Rockchip platforms.
> 
...

> +static const struct of_device_id rk_vip_plat_of_match[] = {
> +	{
> +		.compatible = "rockchip,px30-vip",
> +		.data = &px30_vip_match_data,
> +	},
> +	{},
> +};
> +
> +void rk_vip_soft_reset(struct rk_vip_device *vip_dev)
> +{
> +	reset_control_assert(vip_dev->vip_rst);
> +
> +	udelay(5);
> +
> +	reset_control_deassert(vip_dev->vip_rst);
> +}
> +
> +static int rk_vip_plat_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct v4l2_device *v4l2_dev;
> +	struct rk_vip_device *vip_dev;
> +	const struct vip_match_data *data;
> +	struct resource *res;
> +	int i, ret, irq;
> +
> +	match = of_match_node(rk_vip_plat_of_match, node);
> +	if (IS_ERR(match))

of_match_node does no return ERR_PTR.

> +		return PTR_ERR(match);
> +
> +	vip_dev = devm_kzalloc(dev, sizeof(*vip_dev), GFP_KERNEL);
> +	if (!vip_dev)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, vip_dev);
> +	vip_dev->dev = dev;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_irq(dev, irq, rk_vip_irq_pingpong, IRQF_SHARED,

Shared? With devm? This is error-prone pattern (as discussed few times
on the list). Are you 100% sure that all your use-cases are correct?

> +			       dev_driver_string(dev), dev);
> +	if (ret < 0) {
> +		dev_err(dev, "request irq failed: %d\n", ret);

return dev_err_probe

> +		return ret;
> +	}
> +
> +	vip_dev->irq = irq;
> +	data = match->data;
> +	vip_dev->chip_id = data->chip_id;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	vip_dev->base_addr = devm_ioremap_resource(dev, res);

Use wrapper for these two.

> +
> +	if (IS_ERR(vip_dev->base_addr))
> +		return PTR_ERR(vip_dev->base_addr);
> +
> +	for (i = 0; i < data->clks_num; i++)
> +		vip_dev->clks[i].id = data->clks[i];
> +
> +	vip_dev->num_clk = data->clks_num;
> +
> +	ret = devm_clk_bulk_get(dev, vip_dev->num_clk, vip_dev->clks);
> +	if (ret)
> +		return ret;
> +
> +	vip_dev->vip_rst = devm_reset_control_array_get(dev, false, false);
> +	if (IS_ERR(vip_dev->vip_rst))
> +		return PTR_ERR(vip_dev->vip_rst);
> +
> +	/* Initialize the stream */
> +	rk_vip_stream_init(vip_dev);
> +	strscpy(vip_dev->media_dev.model, "rk_vip",
> +		sizeof(vip_dev->media_dev.model));
> +	vip_dev->media_dev.dev = &pdev->dev;
> +	v4l2_dev = &vip_dev->v4l2_dev;
> +	v4l2_dev->mdev = &vip_dev->media_dev;
> +	strscpy(v4l2_dev->name, "rk_vip", sizeof(v4l2_dev->name));
> +
> +	ret = v4l2_device_register(vip_dev->dev, &vip_dev->v4l2_dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	media_device_init(&vip_dev->media_dev);
> +
> +	ret = media_device_register(&vip_dev->media_dev);
> +	if (ret < 0) {
> +		v4l2_err(v4l2_dev, "Failed to register media device: %d\n",
> +			 ret);
> +		goto err_unreg_v4l2_dev;
> +	}
> +
> +	/* create & register platform subdev (from of_node) */
> +	ret = rk_vip_register_platform_subdevs(vip_dev);
> +	if (ret < 0)
> +		goto err_unreg_media_dev;
> +
> +	vip_dev->sensor.std = V4L2_STD_NTSC;
> +	rk_vip_set_default_format(vip_dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return 0;
> +
> +err_unreg_media_dev:
> +	media_device_unregister(&vip_dev->media_dev);
> +err_unreg_v4l2_dev:
> +	v4l2_device_unregister(&vip_dev->v4l2_dev);
> +	return ret;
> +}
> +
> +static int rk_vip_plat_remove(struct platform_device *pdev)
> +{
> +	struct rk_vip_device *vip_dev = platform_get_drvdata(pdev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	media_device_unregister(&vip_dev->media_dev);
> +	v4l2_device_unregister(&vip_dev->v4l2_dev);
> +	rk_vip_unregister_stream_vdev(vip_dev);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused rk_vip_runtime_suspend(struct device *dev)
> +{
> +	struct rk_vip_device *vip_dev = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable_unprepare(vip_dev->num_clk, vip_dev->clks);
> +
> +	return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int __maybe_unused rk_vip_runtime_resume(struct device *dev)
> +{
> +	struct rk_vip_device *vip_dev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pinctrl_pm_select_default_state(dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	return clk_bulk_prepare_enable(vip_dev->num_clk, vip_dev->clks);
> +}
> +
> +static const struct dev_pm_ops rk_vip_plat_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
> +	SET_RUNTIME_PM_OPS(rk_vip_runtime_suspend, rk_vip_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver rk_vip_plat_drv = {
> +	.driver = {
> +		   .name = VIP_DRIVER_NAME,
> +		   .of_match_table = of_match_ptr(rk_vip_plat_of_match),

drop of_match_ptr, causes warnings in your code.


Best regards,
Krzysztof