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

[Yemike Abhilash Chandra]

Hi Krzysztof,
Thanks for the review.

On 16/07/25 19:39, Krzysztof Kozlowski wrote:
> On 16/07/2025 13:19, Yemike Abhilash Chandra wrote:
>
> > +static int vip_probe_complete(struct platform_device *pdev)
> > +{
> > +	struct vip_shared *shared = platform_get_drvdata(pdev);
> > +	struct regmap *syscon_pol = NULL;
> > +	u32 syscon_pol_offset = 0;
> > +	struct vip_port *port;
> > +	struct vip_dev *dev;
> > +	struct device_node *parent = pdev->dev.of_node;
> > +	struct fwnode_handle *ep = NULL;
> > +	int ret, slice_id, port_id, p;
> > +
> > +	if (parent && of_property_read_bool(parent, "ti,vip-clk-polarity")) {
> > +		syscon_pol = syscon_regmap_lookup_by_phandle(parent,
> > +							     "ti,vip-clk-polarity");
> > +		if (IS_ERR(syscon_pol)) {
> > +			dev_err(&pdev->dev, "failed to get ti,vip-clk-polarity regmap\n");
> > +			return PTR_ERR(syscon_pol);
>
> Syntax is return dev_err_probe. If this is not probe path, then this has
> to be fixed.

I will fix this in v3.

> > +		}
> > +
> > +		if (of_property_read_u32_index(parent, "ti,vip-clk-polarity",
> > +					       1, &syscon_pol_offset)) {
> > +			dev_err(&pdev->dev, "failed to get ti,vip-clk-polarity offset\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	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_pol = syscon_pol;
> > +		dev->syscon_pol_offset = syscon_pol_offset;
> > +		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, int instance_id)
> > +{
> > +	struct vip_shared *shared = platform_get_drvdata(pdev);
> > +	struct vip_dev *dev;
> > +	struct vip_parser_data *parser;
> > +	u32 vin_id;
> > +	int ret;
> > +
> > +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> > +	if (!dev)
> > +		return -ENOMEM;
> > +
> > +	dev->instance_id = instance_id;
> > +	vin_id = 1 + ((dev->instance_id - 1) * 2) + slice;
> > +	snprintf(dev->name, sizeof(dev->name), "vin%d", vin_id);
> > +
> > +	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, dev->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->res = platform_get_resource_byname(pdev,
> > +						   IORESOURCE_MEM,
> > +						   (slice == 0) ?
> > +						   "parser0" :
> > +						   "parser1");
> > +	parser->base = devm_ioremap_resource(&pdev->dev, parser->res);
> > +	if (IS_ERR(parser->base))
> > +		return PTR_ERR(parser->base);
> > +
> > +	parser->pdev = pdev;
> > +	dev->parser = parser;
> > +
> > +	dev->sc_assigned = VIP_NOT_ASSIGNED;
> > +	dev->sc = sc_create(pdev, (slice == 0) ? "sc0" : "sc1");
> > +	if (IS_ERR(dev->sc))
> > +		return PTR_ERR(dev->sc);
> > +
> > +	dev->csc_assigned = VIP_NOT_ASSIGNED;
> > +	dev->csc = csc_create(pdev, (slice == 0) ? "csc0" : "csc1");
> > +	if (IS_ERR(dev->sc))
> > +		return PTR_ERR(dev->sc);
> > +
> > +	return 0;
> > +}
> > +
> > +static int vip_probe(struct platform_device *pdev)
> > +{
> > +	struct vip_shared *shared;
> > +	struct pinctrl *pinctrl;
> > +	int ret, slice = VIP_SLICE1;
> > +	int instance_id;
> > +	u32 tmp, pid;
> > +	const char *label;
> > +
> > +	if (!of_property_read_string(pdev->dev.of_node, "label", &label)) {
> > +		if (strcmp(label, "vip1") == 0)
> > +			instance_id = 1;
> > +		else if (strcmp(label, "vip2") == 0)
> > +			instance_id = 2;
> > +		else if (strcmp(label, "vip3") == 0)
>
>
> Heh, nice try. You cannot encode instance ID as different property (and
> instance ID is not allowed, see writing bindings in next).
>
> And how does it work with label called "krzk"? Your binding said that
> "krzk" is a perfectly correct label.
>
> You need to think about such cases.
>
>
> > +			instance_id = 3;
>
> And past here you use uninitialized instance_id, because you did not
> consider "krzk".

Understood. I will avoid this in v3

Thanks and Regards,
Yemike Abhilash Chandra

> Best regards,
> Krzysztof