Re: [PATCH 5/7] regulator: add driver for mtcmos voltage regulator on hi6220 SoC

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2836 bytes --]

On Thu, Nov 05, 2015 at 09:34:46PM +0800, Chen Feng wrote:
> Add driver to support mtcmos on hi6220

I just noticed that these patches are all being posted to the IOMMU list
- that seems a bit odd?

> +static int hi6220_mtcmos_is_on(struct hi6220_mtcmos *mtcmos,
> +			       unsigned int regs, unsigned int mask, int shift)
> +{
> +	unsigned int ret;
> +
> +	ret = readl(mtcmos->sc_on_regs + regs);
> +	ret &= (mask << shift);
> +
> +	return ret;
> +}
> +
> +static int hi6220_mtcmos_is_enabled(struct regulator_dev *rdev)
> +{
> +	int ret;
> +	struct hi6220_mtcmos_info *sreg = rdev_get_drvdata(rdev);
> +	struct platform_device *pdev =
> +		container_of(rdev->dev.parent, struct platform_device, dev);
> +	struct hi6220_mtcmos *mtcmos = platform_get_drvdata(pdev);
> +	struct hi6220_mtcmos_ctrl_regs *ctrl_regs = &sreg->ctrl_regs;
> +	struct hi6220_mtcmos_ctrl_data *ctrl_data = &sreg->ctrl_data;
> +
> +	ret = hi6220_mtcmos_is_on(mtcmos, ctrl_regs->status_reg,
> +				  ctrl_data->mask, ctrl_data->shift);
> +	return ret;
> +}

That's a *lot* of code for checking if a single bit is set, the same
thinng applies to the rest of the driver.  Unless this is for some
reason very performance critical I'd recommend just using regmap-mmio
and the regmap helpers, that will enable you to remove almost all the
code here.  Even if you can't do that at least removing the extra level
of helper function would help.

> +	sc_on_regs = of_get_property(np, "hisilicon,mtcmos-sc-on-base", NULL);
> +	if (sc_on_regs) {
> +		regs = ioremap(be32_to_cpu(*sc_on_regs), SZ_4K);
> +		mtcmos->sc_on_regs = regs;
> +	} else
> +		return -ENODEV;

{ } on both sides of the if statement.  You should also use normal
reg resource specifiers for register blocks the you need rather than
open coding some custom properties with absolute addresses.

> +	for (i = 0; i < HI6220_RG_MAX; i++) {
> +		init_data = hi6220_mtcmos_matches[i].init_data;
> +		if (!init_data)
> +			continue;

No, you should register all regulators on the device not just those with
init_data - you should just let the core do the DT parsing for you using
the standard of_match feature in the regulator_desc.

> +		mtcmos->rdev[i] = regulator_register(&sreg->rdesc, &config);
> +		if (IS_ERR(mtcmos->rdev[i])) {

devm_regulator_register().

> +static const struct of_device_id of_hi6220_mtcmos_match_tbl[] = {
> +	{ .compatible = "hisilicon,hi6220-mtcmos-driver", },

Why is -driver part of the compatible?

> +	.driver = {
> +		.name = "hisi_hi6220_mtcmos",

Linux generally uses - not _ in names.

> +static int __init hi6220_mtcmos_init(void)
> +{
> +	return platform_driver_register(&mtcmos_driver);
> +}
> +
> +static void __exit hi6220_mtcmos_exit(void)
> +{
> +	platform_driver_unregister(&mtcmos_driver);
> +}
> +
> +fs_initcall(hi6220_mtcmos_init);

Why is this at fs_initcall?!

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]