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

From: Rob Herring <robh@kernel.org>
To: Benoit Parrot <bparrot@ti.com>
Cc: Hans Verkuil <hverkuil@xs4all.nl>,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [Patch 03/19] media: ti-vpe: cal: Add per platform data support
Date: Tue, 29 Oct 2019 08:18:55 -0500	[thread overview]
Message-ID: <20191029131855.GA27597@bogus> (raw)
In-Reply-To: <20191018153437.20614-4-bparrot@ti.com>

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@ti.com>
> ---
>  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.

> +	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.

> +			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);