Re: [PATCH] regulator: mt6360: Add support for MT6360 regulator

Loop MFD maintainer,

Mark Brown [off-list ref] 於 2020年6月4日 週四 下午9:39寫道：

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.

quoted

--- /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.

ACK

quoted

+     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.

ACK, we will remove it

quoted

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

quoted

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

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

ACK

quoted

+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.

ACK

quoted

+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!

quoted

+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.

ACK, we will update binding document later.

quoted

+     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?

Because MT6360 include PMU/PMIC/LDO/TYPEC part, only PMIC/LDO i2c R/W
need contain CRC format.
We will update new patch to conclude r/w in mfd driver.
According to different register address with bank number, we can
distinguish different part of module.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel