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

[Yemike Abhilash Chandra <y-abhilashchandra@ti.com>]

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