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.
signature.asc
Description: Digital signature
