Re: [PATCH] soc/qcom: add OCMEM driver

From: Stanimir Varbanov <svarbanov@mm-sol.com>
To: Rob Clark <robdclark@gmail.com>, linux-arm-msm@vger.kernel.org
Cc: Stephen Boyd <sboyd@codeaurora.org>,
	Bjorn Andersson <bjorn.andersson@sonymobile.com>
Subject: Re: [PATCH] soc/qcom: add OCMEM driver
Date: Tue, 6 Oct 2015 11:35:42 +0300	[thread overview]
Message-ID: <561387DE.70501@mm-sol.com> (raw)
In-Reply-To: <1444059886-2427-1-git-send-email-robdclark@gmail.com>

Hi Rob,

<snip>

> +
> +struct ocmem_config {
> +	uint8_t  num_regions;

u8

> +	uint32_t macro_size;

u32

> +static const struct ocmem_config ocmem_8974_config = {
> +	.num_regions = 3, .macro_size = SZ_128K,
> +};
> +
> +static const struct of_device_id ocmem_dt_match[] = {
> +	{ .compatible = "qcom,ocmem-msm8974", .data = &ocmem_8974_config },

Just a suggestion. As we have REG_OCMEM_HW_PROFILE and
REG_OCMEM_HW_VERSION register why not distinguish by version and hw
profile instead of SoC?

> +	{}
> +};
> +
> +static int ocmem_dev_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct ocmem_config *config = NULL;
> +	const struct of_device_id *match;
> +	struct resource *res;
> +	uint32_t reg, num_banks, region_size;
> +	int i, j, ret;
> +
> +	/* we need scm to be available: */
> +	if (!qcom_scm_is_available())
> +		return -EPROBE_DEFER;
> +
> +	match = of_match_device(ocmem_dt_match, dev);
> +	if (match)
> +		config = match->data;
> +
> +	if (!config) {
> +		dev_err(dev, "unknown config: %s\n", dev->of_node->name);
> +		return -ENXIO;
> +	}
> +
> +	ocmem = devm_kzalloc(dev, sizeof(*ocmem), GFP_KERNEL);
> +	if (!ocmem)
> +		return -ENOMEM;
> +
> +	ocmem->dev = dev;
> +	ocmem->config = config;
> +
> +	ocmem->core_clk = devm_clk_get(dev, "core_clk");
> +	if (IS_ERR(ocmem->core_clk)) {
> +		dev_err(dev, "Unable to get the core clock\n");

this dev_err will flood console in EPROBE_DEFER case, IMO should be removed.

> +		return PTR_ERR(ocmem->core_clk);
> +	}
> +
> +	ocmem->iface_clk = devm_clk_get(dev, "iface_clk");
> +	if (IS_ERR_OR_NULL(ocmem->iface_clk)) {
> +		ret = PTR_ERR(ocmem->iface_clk);
> +		ocmem->iface_clk = NULL;
> +		/* in probe-defer case, propagate error up and try again later: */
> +		if (ret == -EPROBE_DEFER)
> +			goto fail;

Why error path of the iface clock is different comparing with core clk.
Also I failed to found a case when common clk framework will return NULL?

if (IS_ERR(ocmem->iface_clk)
	return PTR_ERR(ocmem->iface_clk);

this should be enough.

> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +			"ocmem_ctrl_physical");
> +	if (!res) {
> +		dev_err(&pdev->dev, "failed to get memory resource\n");
> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +
> +	ocmem->mmio = devm_ioremap_nocache(&pdev->dev, res->start,
> +			resource_size(res));

I think you could use following to simplify ioremap:

	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
			"ocmem_ctrl_physical");
	ocmem->mmio = devm_ioremap_resource(dev, res);
	if (IS_ERR(ocmem->mmio))
		return PTR_ERR(ocmem->mmio);

> +	if (IS_ERR(ocmem->mmio)) {
> +		dev_err(&pdev->dev, "failed to ioremap memory resource\n");
> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +
> +	ocmem->ocmem_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +			"ocmem_physical");
> +	if (!ocmem->ocmem_mem) {
> +		dev_err(dev, "could not get OCMEM region\n");
> +		return -ENXIO;
> +	}
> +

<snip>

-- 
regards,
Stan