Re: [PATCH v3] clock: max77686: Add driver for Maxim 77686 32KHz crystal oscillator

[Arnd Bergmann <arnd@arndb.de>]

On Monday 11 June 2012, Jonghwa Lee wrote:
> Maxim 77686 has three 32KHz clock outputs through the its crystal oscillator.
> This driver can control those ouputs by I2C bus. The clocks are used to supply
> to SOC and peripheral chips as a clock source. Clocks can be enabled/disabled only.
> It uses regmap interface to communicate with I2C bus.
> 
> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Thanks for fixing the issues pointed out in my first review, it
looks much better now.

> +struct clk *clk32khz_ap;
> +struct clk *clk32khz_cp;
> +struct clk *clk32khz_pmic;

As Felipe pointed out, you should not have global pointers
for these, not even static ones.

I think it's best if you use platform_set_drvdata() to attach
the array with the three max77686_clk structures to the platform
device.

> +char *max77686_clks[] = {
> +	"32khap",
> +	"32khcp",
> +	"p32kh",
> +};

These can just be open-coded in the place where the strings
are used, there is no need to have a separate symbol for them.

> +static __devinit int max77686_clk_probe(struct platform_device *pdev)
> +{
> +	struct max77686_dev *iodev = dev_get_drvdata(pdev->dev.parent);
> +	struct max77686_clk *max77686[MAX77686_CLKS_NUM];
> +	int i, ret;
> +
> +	for (i = 0; i < MAX77686_CLKS_NUM; i++) {
> +		max77686[i] = devm_kzalloc(&pdev->dev,
> +				 sizeof(struct max77686_clk), GFP_KERNEL);
> +		if (!max77686[i])
> +			return -ENOMEM;
> +
> +		max77686[i]->iodev = iodev;
> +		max77686[i]->mask = 1 << i;
> +		mutex_init(&max77686[i]->mutex);
> +	}
> +
> +
> +	clk32khz_ap = clk_register(&pdev->dev, max77686_clks[MAX77686_32KH_AP],
> +				&max77686_clk_ops,
> +				&max77686[MAX77686_32KH_AP]->hw,
> +				NULL, 0, CLK_IS_ROOT);
> +	if (IS_ERR(clk32khz_ap)) {
> +		ret = PTR_ERR(clk32khz_ap);
> +		goto err;
> +	}
> +
> +	clk32khz_cp = clk_register(&pdev->dev, max77686_clks[MAX77686_32KH_CP],
> +				&max77686_clk_ops,
> +				&max77686[MAX77686_32KH_CP]->hw,
> +				NULL, 0, CLK_IS_ROOT);
> +	if (IS_ERR(clk32khz_cp)) {
> +		ret = PTR_ERR(clk32khz_cp);
> +		goto err;
> +	}
> +
> +	clk32khz_pmic = clk_register(&pdev->dev, max77686_clks[MAX77686_P32KH],
> +				&max77686_clk_ops,
> +				&max77686[MAX77686_P32KH]->hw,
> +				NULL, 0, CLK_IS_ROOT);
> +	if (IS_ERR(clk32khz_pmic)) {
> +		ret = PTR_ERR(clk32khz_pmic);
> +		goto err;
> +	}

Note that the prototype for clk_register has changed recently, so this
will have to be rewritten. It might simplify the code if you pull the call
to clk_register into the loop, and add a helper function for the loop body,
like

	for (i = 0; i < MAX77686_CLKS_NUM; i++) {
		ret = max77686_clk_register(&pdev->dev, iodev, i);
		if (ret) {
			max77686_clk_remove(pdev);
			return ret;
		}
	}

> +static int __devexit max77686_clk_remove(struct platform_device *pdev)
> +{
> +	kfree(clk32khz_ap);
> +	kfree(clk32khz_cp);
> +	kfree(clk32khz_pmic);
> +	return 0;
> +}

As Mark mentioned, the kfree is not necessary, but the clk_unregister
is missing here and in the error path of the probe function.

	Arnd