Re: [PATCH 4/6] soc/tegra: pmc: Register PMC child devices as platform device

From: Jon Hunter
Date: Tue May 03 2016 - 11:27:08 EST



On 02/05/16 13:17, Laxman Dewangan wrote:
> Power Management Controller(PMC) of Tegra does the multiple chip
> power related functionality for internal and IO interfacing.
> Some of the functionalities are power gating of IP blocks, IO pads
> voltage and power state configuration, system power state configurations,
> wakeup controls etc.
>
> Different functionalities of the PMC are provided through different
> framework like IO pads control can be provided through pinctrl framework,
> IO power control is via misc driver etc. All sub functionalities are
> represented as PMC child devices.
>
> Register the PMC child devices as platform device and fill the child
> devices table for Tegra210.
>
> Signed-off-by: Laxman Dewangan <ldewangan@xxxxxxxxxx>
>
> ---
> Changes from V1:
> Reworked on DT for having flat entry and register all child devices
> as simple platform device instead of of_populate_device().
> ---
> drivers/soc/tegra/pmc.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 54 insertions(+)
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index b3be4b9..625167e 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -132,6 +132,8 @@ struct tegra_pmc_soc {
> const u8 *cpu_powergates;
> const struct tegra_io_pads_control *io_pads_control;
> unsigned int num_io_pads;
> + const char **sub_devs_name;
> + unsigned int num_sub_devs;
> bool has_tsense_reset;
> bool has_gpu_clamps;
> };
> @@ -158,6 +160,8 @@ struct tegra_pmc_soc {
> * @lp0_vec_size: size of the LP0 warm boot code
> * @powergates_available: Bitmap of available power gates
> * @powergates_lock: mutex for power gate register access
> + * @pdevs: Platform device for PMC child devices.
> + * @num_pdevs: Number of platform devices.
> */
> struct tegra_pmc {
> struct device *dev;
> @@ -184,6 +188,9 @@ struct tegra_pmc {
> DECLARE_BITMAP(powergates_available, TEGRA_POWERGATE_MAX);
>
> struct mutex powergates_lock;
> +
> + struct platform_device **pdevs;
> + unsigned int num_pdevs;

We already have num_sub_devs in the soc data, do we really need this?

> };
>
> static struct tegra_pmc *pmc = &(struct tegra_pmc) {
> @@ -1379,6 +1386,43 @@ out:
> of_node_put(np);
> }
>
> +static int tegra_pmc_init_sub_devs(struct tegra_pmc *pmc)
> +{
> + int ret, i;
> +
> + if (!pmc->soc->num_sub_devs)
> + return 0;
> +
> + pmc->pdevs = devm_kzalloc(pmc->dev, sizeof(**pmc->pdevs), GFP_KERNEL);
> + if (!pmc->pdevs)
> + return -ENOMEM;
> +
> + for (i = 0; i < pmc->soc->num_sub_devs; ++i) {
> + pmc->pdevs[i] = platform_device_register_data(pmc->dev,
> + pmc->soc->sub_devs_name[i],
> + 0, NULL, 0);
> + if (IS_ERR(pmc->pdevs[i])) {
> + ret = PTR_ERR(pmc->pdevs[i]);
> + dev_err(pmc->dev,
> + "Failed to register platform device for %s: %d\n",
> + pmc->soc->sub_devs_name[i], ret);
> + goto pdev_cleanups;
> + }
> + pmc->num_pdevs++;

I would get rid of pmc->num_pdevs because isn't this always equal to i here?

> + }
> +
> + return 0;
> +
> +pdev_cleanups:
> + for (i = pmc->num_pdevs; i > 0; i--) {

Here I think this could be ...

while (--i) {

> + platform_device_unregister(pmc->pdevs[i - 1]);
> + pmc->pdevs[i - 1] = NULL;
> + }
> + pmc->num_pdevs = 0;
> +
> + return ret;
> +}

Cheers
Jon