Re: [PATCH V4 4/4] media: ti: vpe: Add the VIP driver

[Krzysztof Kozlowski <krzk@kernel.org>]

On 15/10/2025 07:40, Yemike Abhilash Chandra wrote:
> +static int vip_probe_complete(struct platform_device *pdev)
> +{
> +	struct vip_shared *shared = platform_get_drvdata(pdev);
> +	struct vip_ctrl_module *ctrl = NULL;
> +	struct vip_port *port;
> +	struct vip_dev *dev;
> +	struct device_node *parent = pdev->dev.of_node;
> +	struct fwnode_handle *ep = NULL;
> +	struct of_phandle_args args;
> +	int ret, i, slice_id, port_id, p;
> +
> +	/* Allocate ctrl before using it */
> +	ctrl = devm_kzalloc(&pdev->dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	if (parent && of_property_present(parent, "ti,ctrl-module")) {
> +		ctrl->syscon_pol = syscon_regmap_lookup_by_phandle(parent,
> +								   "ti,ctrl-module");
> +		if (IS_ERR(ctrl->syscon_pol))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(ctrl->syscon_pol),
> +				     "Failed to get ti,ctrl-module regmap\n");
> +	}
> +
> +	ret = of_parse_phandle_with_fixed_args(parent, "ti,ctrl-module",
> +					       5, 0, &args);

1. You leak parent.
2. Why you can't just just take it from syscon call? This is not only
leaking, but actually duplicated and unnecessary.

> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to parse ti,ctrl-module\n");
> +		return ret;
> +	}
> +
> +	ctrl->syscon_offset = args.args[0];
> +
> +	for (i = 0; i < ARRAY_SIZE(ctrl->syscon_bit_field); i++)
> +		ctrl->syscon_bit_field[i] = args.args[i + 1];
> +
> +	for (p = 0; p < (VIP_NUM_PORTS * VIP_NUM_SLICES); p++) {
> +		ep = fwnode_graph_get_next_endpoint_by_regs(of_fwnode_handle(parent),
> +							    p, 0);
> +		if (!ep)
> +			continue;
> +
> +		switch (p) {
> +		case 0:
> +			slice_id = VIP_SLICE1;	port_id = VIP_PORTA;
> +			break;
> +		case 1:
> +			slice_id = VIP_SLICE2;	port_id = VIP_PORTA;
> +			break;
> +		case 2:
> +			slice_id = VIP_SLICE1;	port_id = VIP_PORTB;
> +			break;
> +		case 3:
> +			slice_id = VIP_SLICE2;	port_id = VIP_PORTB;
> +			break;
> +		default:
> +			dev_err(&pdev->dev, "Unknown port reg=<%d>\n", p);
> +			continue;
> +		}
> +
> +		ret = alloc_port(shared->devs[slice_id], port_id);
> +		if (ret < 0)
> +			continue;
> +
> +		dev = shared->devs[slice_id];
> +		dev->syscon = ctrl;
> +		port = dev->ports[port_id];
> +
> +		vip_register_subdev_notif(port, ep);
> +		fwnode_handle_put(ep);
> +	}
> +	return 0;
> +}
> +
> +static int vip_probe_slice(struct platform_device *pdev, int slice)
> +{
> +	struct vip_shared *shared = platform_get_drvdata(pdev);
> +	struct vip_dev *dev;
> +	struct vip_parser_data *parser;
> +	struct sc_data *sc;
> +	struct csc_data *csc;
> +	int ret;
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->irq = platform_get_irq(pdev, slice);
> +	if (dev->irq < 0)
> +		return dev->irq;
> +
> +	ret = devm_request_irq(&pdev->dev, dev->irq, vip_irq,
> +			       0, VIP_MODULE_NAME, dev);
> +	if (ret < 0)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&dev->slock);
> +	mutex_init(&dev->mutex);
> +
> +	dev->slice_id = slice;
> +	dev->pdev = pdev;
> +	dev->res = shared->res;
> +	dev->base = shared->base;
> +	dev->v4l2_dev = shared->v4l2_dev;
> +
> +	dev->shared = shared;
> +	shared->devs[slice] = dev;
> +
> +	vip_top_reset(dev);
> +	vip_set_slice_path(dev, VIP_MULTI_CHANNEL_DATA_SELECT, 1);
> +
> +	parser = devm_kzalloc(&pdev->dev, sizeof(*dev->parser), GFP_KERNEL);
> +	if (!parser)
> +		return PTR_ERR(parser);
> +
> +	parser->base = dev->base + (slice ? VIP_SLICE1_PARSER : VIP_SLICE0_PARSER);
> +	if (IS_ERR(parser->base))
> +		return PTR_ERR(parser->base);
> +
> +	parser->pdev = pdev;
> +	dev->parser = parser;
> +
> +	dev->sc_assigned = VIP_NOT_ASSIGNED;
> +	sc = devm_kzalloc(&pdev->dev, sizeof(*dev->sc), GFP_KERNEL);
> +	if (!sc)
> +		return PTR_ERR(sc);
> +
> +	sc->base = dev->base + (slice ? VIP_SLICE1_SC : VIP_SLICE0_SC);
> +	if (IS_ERR(sc->base))
> +		return PTR_ERR(sc->base);
> +
> +	sc->pdev = pdev;
> +	dev->sc = sc;
> +
> +	dev->csc_assigned = VIP_NOT_ASSIGNED;
> +	csc = devm_kzalloc(&pdev->dev, sizeof(*dev->csc), GFP_KERNEL);
> +	if (!csc)
> +		return PTR_ERR(csc);
> +
> +	csc->base = dev->base + (slice ? VIP_SLICE1_CSC : VIP_SLICE0_CSC);
> +	if (IS_ERR(csc->base))
> +		return PTR_ERR(csc->base);
> +
> +	csc->pdev = pdev;
> +	dev->csc = csc;
> +
> +	return 0;
> +}
> +
> +static int vip_probe(struct platform_device *pdev)
> +{
> +	struct vip_shared *shared;
> +	struct pinctrl *pinctrl;
> +	int ret, slice = VIP_SLICE1;
> +	u32 tmp, pid;
> +
> +	/* If there are no endpoint defined there is nothing to do */
> +	if (!vip_endpoint_scan(pdev)) {
> +		dev_err(&pdev->dev, "%s: No sensor", __func__);
> +		return -ENODEV;
> +	}
> +
> +	ret = dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"32-bit consistent DMA enable failed\n");
> +		return ret;
> +	}
> +
> +	shared = devm_kzalloc(&pdev->dev, sizeof(*shared), GFP_KERNEL);
> +	if (!shared)
> +		return -ENOMEM;
> +
> +	shared->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	shared->base = devm_ioremap_resource(&pdev->dev, shared->res);

Use proper wrapper for these two.

> +	if (IS_ERR(shared->base))
> +		return PTR_ERR(shared->base);
> +
> +	vip_init_format_info(&pdev->dev);
> +
> +	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ret = pm_runtime_get_sync(&pdev->dev);
> +	if (ret)
> +		goto err_runtime_disable;
> +
> +	/* Make sure H/W module has the right functionality */
> +	pid = reg_read(shared, VIP_PID);
> +	tmp = get_field(pid, VIP_PID_FUNC_MASK, VIP_PID_FUNC_SHIFT);
> +
> +	if (tmp != VIP_PID_FUNC) {
> +		dev_info(&pdev->dev, "vip: unexpected PID function: 0x%x\n",
> +			 tmp);
> +		ret = -ENODEV;
> +		goto err_runtime_put;
> +	}
> +
> +	ret = v4l2_device_register(&pdev->dev, &shared->v4l2_dev);
> +	if (ret)
> +		goto err_runtime_put;
> +
> +	/* enable clocks, so the firmware will load properly */
> +	vip_shared_set_clock_enable(shared, 1);
> +	vip_top_vpdma_reset(shared);
> +
> +	platform_set_drvdata(pdev, shared);
> +
> +	v4l2_ctrl_handler_init(&shared->ctrl_handler, 11);
> +	shared->v4l2_dev.ctrl_handler = &shared->ctrl_handler;
> +
> +	for (slice = VIP_SLICE1; slice < VIP_NUM_SLICES; slice++) {
> +		ret = vip_probe_slice(pdev, slice);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Creating slice failed");
> +			goto err_dev_unreg;
> +		}
> +	}
Best regards,
Krzysztof