Re: [PATCH 1/3] mfd: Add hi6421 PMIC core driver

[Mark Brown]

On Tue, Jun 04, 2013 at 10:28:41PM +0800, Guodong Xu wrote:

> +static struct of_device_id of_hi6421_pmic_child_match_tbl[] = {
> +	/* regulators */
> +	{
> +		.compatible = "hisilicon,hi6421-ldo",
> +	},
> +	{
> +		.compatible = "hisilicon,hi6421-buck012",
> +	},
> +	{
> +		.compatible = "hisilicon,hi6421-buck345",
> +	},
> +	{ /* end */ }
> +};

I would expect this to be in the regulator driver.

> +/*
> + * The PMIC register is only 8-bit.
> + * Hisilicon SoC use hardware to map PMIC register into SoC mapping.
> + * At here, we are accessing SoC register with 32-bit.
> + */
> +u32 hi6421_pmic_read(struct hi6421_pmic *pmic, int reg)
> +{
> +	unsigned long flags;
> +	u32 ret;
> +	spin_lock_irqsave(&pmic->lock, flags);
> +	ret = readl_relaxed(pmic->regs + (reg << 2));
> +	spin_unlock_irqrestore(&pmic->lock, flags);
> +	return ret;
> +}
> +EXPORT_SYMBOL(hi6421_pmic_read);

This all looks like you want to be using regmap MMIO.  This would save a
bit of code and get you access to infrastructure like tracepoints and
debugfs regiseter dumps as well as the regulator API helpers.

> +	if (!devm_request_mem_region(dev, pmic->res->start,
> +				     resource_size(pmic->res),
> +				     pdev->name)) {
> +		dev_err(dev, "cannot claim register memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	pmic->regs = devm_ioremap(dev, pmic->res->start,
> +				  resource_size(pmic->res));

devm_request_and_ioremap().

> +	/* populate sub nodes */
> +	of_platform_populate(np, of_hi6421_pmic_child_match_tbl, NULL, dev);

You should be using the MFD API for this.

> +static int hi6421_pmic_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct hi6421_pmic *pmic = platform_get_drvdata(pdev);
> +
> +	devm_iounmap(dev, pmic->regs);
> +	devm_release_mem_region(dev, pmic->res->start,
> +				resource_size(pmic->res));
> +	devm_kfree(dev, pmic);

No point in cleaning up devm_ stuff, a major goal of the API is to save
writing that code.  The only thing that's needed is to remove the MFD
children which you're not doing.

> +	platform_set_drvdata(pdev, NULL);

This isn't required, it's never OK to access the driver data unless from
code that set it.

> +#define	OCP_DEB_CTRL_REG		(0x51)
> +#define	OCP_DEB_SEL_MASK		(0x0C)
> +#define OCP_DEB_SEL_8MS			(0x00)
> +#define OCP_DEB_SEL_16MS		(0x04)
> +#define OCP_DEB_SEL_32MS		(0x08)
> +#define OCP_DEB_SEL_64MS		(0x0C)
> +#define OCP_EN_DEBOUNCE_MASK		(0x02)
> +#define OCP_EN_DEBOUNCE_ENABLE		(0x02)
> +#define OCP_AUTO_STOP_MASK		(0x01)
> +#define OCP_AUTO_STOP_ENABLE		(0x01)

These should be namespaced.

Attachment: signature.asc
Description: Digital signature