Re: [Patch 03/19] media: ti-vpe: cal: Add per platform data support

[Benoit Parrot]

Rob Herring <robh@xxxxxxxxxx> wrote on Tue [2019-Oct-29 08:18:55 -0500]:
> On Fri, Oct 18, 2019 at 10:34:21AM -0500, Benoit Parrot wrote:
> > First this patch adds a method to access the CTRL_CORE_CAMERRX_CONTROL
> > register to use the syscon mechanism. For backward compatibility we also
> > handle using the existing camerrx_control "reg" entry if a syscon node
> > is not found.
> > 
> > In addition the register bit layout for the CTRL_CORE_CAMERRX_CONTROL
> > changes depending on the device. In order to support this we need to use
> > a register access scheme based on data configuration instead of using
> > static macro.
> > 
> > In this case we make use of the regmap facility and create data set
> > based on the various device and phy available.
> > 
> > Signed-off-by: Benoit Parrot <bparrot@xxxxxx>
> > ---
> >  drivers/media/platform/ti-vpe/cal.c | 281 +++++++++++++++++++++-------
> >  1 file changed, 212 insertions(+), 69 deletions(-)
> 
> 
> > @@ -1816,6 +1911,18 @@ static int cal_probe(struct platform_device *pdev)
> >  	if (!dev)
> >  		return -ENOMEM;
> >  
> > +	match = of_match_device(of_match_ptr(cal_of_match), &pdev->dev);
> 
> Use of_device_get_match_data() instead.

Ok I'll change that.

> 
> > +	if (!match)
> > +		return -ENODEV;
> > +
> > +	if (match->data) {
> > +		dev->data = (struct cal_data *)match->data;
> > +		dev->flags = dev->data->flags;
> > +	} else {
> > +		dev_err(&pdev->dev, "Could not get feature data based on compatible version\n");
> > +		return -ENODEV;
> > +	}
> > +
> >  	/* set pseudo v4l2 device name so we can use v4l2_printk */
> >  	strscpy(dev->v4l2_dev.name, CAL_MODULE_NAME,
> >  		sizeof(dev->v4l2_dev.name));
> > @@ -1823,6 +1930,43 @@ static int cal_probe(struct platform_device *pdev)
> >  	/* save pdev pointer */
> >  	dev->pdev = pdev;
> >  
> > +	if (parent && of_property_read_bool(parent, "syscon-camerrx")) {
> > +		syscon_camerrx =
> > +			syscon_regmap_lookup_by_phandle(parent,
> > +							"syscon-camerrx");
> > +		if (IS_ERR(syscon_camerrx)) {
> > +			dev_err(&pdev->dev, "failed to get syscon-camerrx regmap\n");
> > +			return PTR_ERR(syscon_camerrx);
> > +		}
> > +
> > +		if (of_property_read_u32_index(parent, "syscon-camerrx", 1,
> > +					       &syscon_camerrx_offset)) {
> 
> Kind of odd to read the property twice and using functions that don't 
> match the type. We have functions to retrieve phandle and args.

Yeah, I wanted to make a distinction between the node being present and
any other kind of errors, so we can have a little more precise error
message.

> 
> > +			dev_err(&pdev->dev, "failed to get syscon-camerrx offset\n");
> > +			return -EINVAL;
> > +		}
> > +	} else {
> > +		/*
> > +		 * Backward DTS compatibility.
> > +		 * If syscon entry is not present then check if the
> > +		 * camerrx_control resource is present.
> > +		 */
> > +		syscon_camerrx = cal_get_camerarx_regmap(dev);
> > +		if (IS_ERR(syscon_camerrx)) {
> > +			dev_err(&pdev->dev, "failed to get camerrx_control regmap\n");
> > +			return PTR_ERR(syscon_camerrx);
> > +		}
> > +		/* In this case the base already point to the direct
> > +		 * CM register so no need for an offset
> > +		 */
> > +		syscon_camerrx_offset = 0;
> > +	}
> > +
> > +	dev->syscon_camerrx = syscon_camerrx;
> > +	dev->syscon_camerrx_offset = syscon_camerrx_offset;
> > +	ret = cal_camerarx_regmap_init(dev);
> > +	if (ret)
> > +		return ret;
> > +
> >  	dev->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> >  						"cal_top");
> >  	dev->base = devm_ioremap_resource(&pdev->dev, dev->res);
>