Re: [PATCH 3/6] iommu/tegra124: smmu: add support platform data

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 12/05/2013 05:25 AM, Hiroshi Doyu wrote:
> The later Tegra SoC(>= T124) has more registers for
> MC_SMMU_TRANSLATION_ENABLE_*. Now those info is provided as platfrom
> data. If those varies a lot on SoCs in the future, we can consider
> putting them into DT later.

This shouldn't be called "platform data", since that phrase already has
an existing different meaning within Linux. Instead, perhaps
"SoC-specific configuration" or "HW-specific configuration" (or
s/configuration/parameters/).

> diff --git a/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt b/Documentation/devicetree/bindings/iommu/nvidia,tegra30-smmu.txt

>  Required properties in the IOMMU node:
> -- compatible : "nvidia,tegra30-smmu"
> -- reg : Should contain 3 register banks(address and length) for each
> +- compatible : "nvidia,tegra124-smmu", "nvidia,tegra30-smmu"
> +- reg : Can contain multiple register banks(address and length) for each

Not all DTs should include both. I would phrase this as:

- compatible : One of the following:
  - nvidia,tegra30-smmu
  - nvidia,tegra124-smmu

> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c

> +struct smmu_platform_data {
> +	int asids;		/* number of asids */
> +	int nr_xlats;		/* number of translation_enable registers */
> +};

Shouldn't both or neither of those fields be prefixed with "nr_" for
consistency?

> +static const struct smmu_platform_data tegra124_smmu_pdata = {
> +	.asids = 128,
> +	.nr_xlats = 4,
> +};
> +
> +static struct of_device_id tegra_smmu_of_match[] = {
> +	{ .compatible = "nvidia,tegra124-smmu", .data = &tegra124_smmu_pdata, },
> +	{ .compatible = "nvidia,tegra30-smmu", },

Where is tegra30_smmu_pdata, and why doesn't this table entry point at
it? That would avoid conditional code elsewhere.

> @@ -1250,20 +1269,29 @@ static int tegra_smmu_probe(struct platform_device *pdev)

> +	smmu->nr_xlats = nr_xlats;

Why not just:

	smmu->pdata = pdata;

That would avoid having to change this code to copy new fields every
time they get added.