Lee Jones [off-list ref] 於 2020年9月8日 週二 下午7:48寫道：

On Tue, 01 Sep 2020, Gene Chen wrote:

quoted

Lee Jones [off-list ref] 於 2020年8月28日 週五 下午6:40寫道：

quoted

On Mon, 17 Aug 2020, Gene Chen wrote:

quoted

From: Gene Chen <redacted>

Remove unuse register definition.

This should be in a separate patch.

quoted

Merge different sub-devices I2C read/write functions into one Regmap,
because PMIC and LDO part need CRC bits for access protection.

Signed-off-by: Gene Chen <redacted>
---
 drivers/mfd/Kconfig        |   1 +
 drivers/mfd/mt6360-core.c  | 260 +++++++++++++++++++++++++++++++++++++++------
 include/linux/mfd/mt6360.h | 240 -----------------------------------------
 3 files changed, 226 insertions(+), 275 deletions(-)
 delete mode 100644 include/linux/mfd/mt6360.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index a37d7d1..0684ddc 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig

@@ -913,6 +913,7 @@ config MFD_MT6360
      select MFD_CORE
      select REGMAP_I2C
      select REGMAP_IRQ
+     select CRC8
      depends on I2C
      help
        Say Y here to enable MT6360 PMU/PMIC/LDO functional support.

diff --git a/drivers/mfd/mt6360-core.c b/drivers/mfd/mt6360-core.c
index 677c974..e995220 100644
--- a/drivers/mfd/mt6360-core.c
+++ b/drivers/mfd/mt6360-core.c

@@ -14,7 +14,53 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>

-#include <linux/mfd/mt6360.h>
+enum {
+     MT6360_SLAVE_TCPC = 0,
+     MT6360_SLAVE_PMIC,
+     MT6360_SLAVE_LDO,
+     MT6360_SLAVE_PMU,
+     MT6360_SLAVE_MAX,
+};
+
+struct mt6360_ddata {
+     struct i2c_client *i2c[MT6360_SLAVE_MAX];
+     struct device *dev;
+     struct regmap *regmap;
+     struct regmap_irq_chip_data *irq_data;
+     unsigned int chip_rev;
+     u8 crc8_tbl[CRC8_TABLE_SIZE];
+};

This is not a new structure, right?  Where was this before?  Surely it
should be removed from wherever it was in the same patch that places
it here?

No, it is merge from header file to source code for unuse in other sub-module.

So where did it come from and why don't I see the removal in this
patch?

Change is in the bottom of this patch.
There is a little confuse part in "[PATCH v4 5/9] mfd: mt6360: Rename
mt6360_pmu_data by mt6360_ddata"
The "PATCH 5/9" change mt6360_pmu_data to mt6360_ddata instead of mt6360_data.
I will update PATCH v5 to fix it.

[PATCH v4 9/9]

diff --git a/include/linux/mfd/mt6360.h b/include/linux/mfd/mt6360.h
-struct mt6360_data {
-       struct i2c_client *i2c[MT6360_SLAVE_MAX];
-       struct device *dev;
-       struct regmap *regmap;
-       struct regmap_irq_chip_data *irq_data;
-       unsigned int chip_rev;
-};

[PATCH v4 5/9]

diff --git a/include/linux/mfd/mt6360.h b/include/linux/mfd/mt6360.h
-struct mt6360_pmu_data {
+struct mt6360_data {
        struct i2c_client *i2c[MT6360_SLAVE_MAX];
        struct device *dev;
        struct regmap *regmap;

[...]

quoted

quoted

quoted

-static const unsigned short mt6360_slave_addr[MT6360_SLAVE_MAX] = {
-     MT6360_PMU_SLAVEID,
+static const u16 mt6360_slave_addrs[MT6360_SLAVE_MAX] = {

Why are you changing the data type?

Easy to read.
I think it's the same?

It's an unrelated change and should not be in this patch.

Please separate patches into functional changes.

ACK. It's not very important change. I will revert it.

quoted

quoted

quoted

+     MT6360_TCPC_SLAVEID,
      MT6360_PMIC_SLAVEID,
      MT6360_LDO_SLAVEID,
-     MT6360_TCPC_SLAVEID,
+     MT6360_PMU_SLAVEID,
+};

[...]

quoted

quoted

quoted

 static int mt6360_probe(struct i2c_client *client)

@@ -329,9 +521,23 @@ static int mt6360_probe(struct i2c_client *client)
              return -ENOMEM;

      ddata->dev = &client->dev;
-     i2c_set_clientdata(client, ddata);

-     ddata->regmap = devm_regmap_init_i2c(client, &mt6360_pmu_regmap_config);
+     for (i = 0; i < MT6360_SLAVE_MAX - 1; i++) {
+             ddata->i2c[i] = devm_i2c_new_dummy_device(&client->dev,
+                                                       client->adapter,
+                                                       mt6360_slave_addrs[i]);
+             if (IS_ERR(ddata->i2c[i])) {
+                     dev_err(&client->dev,
+                             "Failed to get new dummy I2C device for address 0x%x",
+                             mt6360_slave_addrs[i]);
+                     return PTR_ERR(ddata->i2c[i]);

Do you have to free the new devices you just allocated?

Usually no need to free devm_i2c_new_dummy_device,
Should I use kfree(ddata->i2c[i]);?

You tell me.

I survey the upstream code e.q. drivers/mfd/tps80031.c
It' should not have to free the memory.

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

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