Re: [PATCH 2/2] regulator: uniphier: add regulator driver for UniPhier SoC

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]

On Fri, Jun 29, 2018 at 05:22:13PM +0900, Kunihiko Hayashi wrote:

> +++ b/drivers/regulator/uniphier-regulator.c
> @@ -0,0 +1,251 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Regulator controller driver for UniPhier SoC
> + * Copyright 2018 Socionext Inc.
> + * Author: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> + */

Please make the entire header a C++ comment so that it looks intentional
rather than mixing C and C++ like this.

> +static int uniphier_regulator_enable(struct regulator_dev *rdev)
> +{
> +	struct uniphier_regulator_priv *priv = rdev_get_drvdata(rdev);
> +	u32 val;
> +
> +	val = readl_relaxed(priv->base + rdev->desc->enable_reg);
> +	val &= ~rdev->desc->enable_mask;
> +	val |= rdev->desc->enable_val;
> +	writel_relaxed(val, priv->base + rdev->desc->enable_reg);
> +
> +	return 0;
> +}

Could you use a MMIO regmap for this driver?  All the operations look
like they're just straight up operations of the sort the standard
helpers support them and it means the driver will be able to take
advantage of any improvements the core makes for free.

Otherwise this looks great, and the above two issues can be fixed as
followup patches so I'll apply.

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