Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 4 Jun 2020 14:39:43 +0100
From:   Mark Brown <broonie@kernel.org>
To:     Gene Chen <gene.chen.richtek@gmail.com>
Cc:     matthias.bgg@gmail.com, lgirdwood@gmail.com,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
        gene_chen@richtek.com, Wilma.Wu@mediatek.com,
        shufan_lee@richtek.com, cy_huang@richtek.com,
        benjamin.chao@mediatek.com
Subject: Re: [PATCH] regulator: mt6360: Add support for MT6360 regulator
Message-ID: <20200604133943.GE6644@sirena.org.uk>
References: <1591254387-10304-1-git-send-email-gene.chen.richtek@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="WK3l2KTTmXPVedZ6"
Content-Disposition: inline
In-Reply-To: <1591254387-10304-1-git-send-email-gene.chen.richtek@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--WK3l2KTTmXPVedZ6
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Jun 04, 2020 at 03:06:27PM +0800, Gene Chen wrote:

This looks nice and simple, a few fairly small comments below but high
level it's basically fine.

> --- /dev/null
> +++ b/drivers/regulator/mt6360-regulator.c
> @@ -0,0 +1,571 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 MediaTek Inc.

Please make the entire comment a C++ one so things look more
intentional.

> +	for (i = 0; i < devdata->num_irq_descs; i++) {
> +		irq_desc = devdata->irq_descs + i;
> +		if (unlikely(!irq_desc->name))
> +			continue;

Do we really need an unlikely here?  This shouldn't be a hot path.

> +static int mt6360_regulator_set_mode(
> +				  struct regulator_dev *rdev, unsigned int mode)
> +{

> +	switch (1 << (ffs(mode) - 1)) {
> +	case REGULATOR_MODE_NORMAL:

I don't understand why this isn't just a straight switch on mode?

> +static unsigned int mt6360_regulator_get_mode(struct regulator_dev *rdev)
> +{
> +	const struct mt6360_regulator_desc *desc =
> +			       (const struct mt6360_regulator_desc *)rdev->desc;
> +	int shift = ffs(desc->mode_get_mask) - 1, ret;
> +	unsigned int val = 0;
> +
> +	default:
> +		ret = 0;
> +	}

If we can't parse a valid value from the hardware then that's an error.

> +static int mt6360_regulator_reg_write(void *context,
> +				      unsigned int reg, unsigned int val)
> +{
> +	struct mt6360_regulator_data *mrd = context;
> +	u8 chunk[4] = {0};
> +
> +	/* chunk 0 ->i2c addr, 1 -> reg_addr, 2 -> reg_val 3-> crc8 */
> +	chunk[0] = (mrd->i2c->addr & 0x7f) << 1;
> +	chunk[1] = reg & 0x3f;
> +	chunk[2] = (u8)val;
> +	chunk[3] = crc8(mrd->crc8_table, chunk, 3, 0);
> +	/* also dummy one byte */
> +	return i2c_smbus_write_i2c_block_data(mrd->i2c, chunk[1], 3, chunk + 2);
> +}

Oh, wow - that's a fun I/O interface!

> +static const struct of_device_id __maybe_unused mt6360_regulator_of_id[] = {
> +	{
> +		.compatible = "mediatek,mt6360_pmic",
> +		.data = (void *)&mt6360_pmic_devdata,
> +	},
> +	{
> +		.compatible = "mediatek,mt6360_ldo",
> +		.data = (void *)&mt6360_ldo_devdata,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mt6360_regulator_of_id);

I don't see any DT bindings documentation for this, documentation is
required for all new bindings.

> +	mrd->regmap = devm_regmap_init(&(mrd->i2c->dev),
> +				       NULL, mrd, devdata->regmap_config);
> +	if (IS_ERR(mrd->regmap)) {
> +		dev_err(&pdev->dev, "Failed to register regmap\n");
> +		return PTR_ERR(mrd->regmap);
> +	}

This looks like a MFD so it's surprising to see us defining a regmap at
this level.  Why are we doing this?

--WK3l2KTTmXPVedZ6
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAl7Y+Z8ACgkQJNaLcl1U
h9DRUgf/Q07BCU3NVKHL/ESM60sLn9muFqxQNcAmFlGBR5bDLKsDToWTKNlTLQuv
W2REKAvgWpj9dLzCIWOnMEJob8jsw3SSx5fzUNDq+WJLlzhf4MPQkG+/B5kOs7A8
aCE35WQGvrfMs8zbQmA9VJxUVOHqnyoaUK86E73iUPAWShBSxH2AaVsi1WUqN53g
LPnVfJ2MvnJLE3z6nmcMof9PXxdA48vrM2Wes0XL3rVUQkuxz9xPKThmtGUUqDoe
Rx7F5rOP4r8Rq1lT1ac/U1X3AvqMyTK+9jniD9efE6zlLzlquzOjHyMxul1Z6Ok1
MusCOVNvor+RxHuCDiY2mnZXflShXQ==
=4cA0
-----END PGP SIGNATURE-----

--WK3l2KTTmXPVedZ6--