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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130604164440.GE31367@sirena.org.uk>
Date:	Tue, 4 Jun 2013 17:44:40 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Guodong Xu <guodong.xu@...aro.org>
Cc:	sameo@...ux.intel.com, lgirdwood@...il.com,
	linux-kernel@...r.kernel.org, patches@...aro.org,
	Haojian Zhuang <haojian.zhuang@...aro.org>
Subject: Re: [PATCH 1/3] mfd: Add hi6421 PMIC core driver

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.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)