by Gene Chen

[permalink] [raw]

Subject: Re: [PATCH v4 9/9] mfd: mt6360: Merge different sub-devices I2C read/write

Lee Jones <[email protected]> 於 2020年9月8日週二下午7:48寫道：
>
> On Tue, 01 Sep 2020, Gene Chen wrote:
>
> > Lee Jones <[email protected]> 於 2020年8月28日週五下午6:40寫道：
> > >
> > > On Mon, 17 Aug 2020, Gene Chen wrote:
> > >
> > > > From: Gene Chen <[email protected]>
> > > >
> > > > Remove unuse register definition.
> > >
> > > This should be in a separate patch.
> > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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;

> [...]
>
> > > > -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.

> > > > + MT6360_TCPC_SLAVEID,
> > > > MT6360_PMIC_SLAVEID,
> > > > MT6360_LDO_SLAVEID,
> > > > - MT6360_TCPC_SLAVEID,
> > > > + MT6360_PMU_SLAVEID,
> > > > +};
>
> [...]
>
> > > > 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

2020-09-09 07:41:06

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH v4 9/9] mfd: mt6360: Merge different sub-devices I2C read/write

On Wed, 09 Sep 2020, Gene Chen wrote:

> Lee Jones <[email protected]> 於 2020年9月8日週二下午7:48寫道：
> >
> > On Tue, 01 Sep 2020, Gene Chen wrote:
> >
> > > Lee Jones <[email protected]> 於 2020年8月28日週五下午6:40寫道：
> > > >
> > > > On Mon, 17 Aug 2020, Gene Chen wrote:
> > > >
> > > > > From: Gene Chen <[email protected]>
> > > > >
> > > > > Remove unuse register definition.
> > > >
> > > > This should be in a separate patch.
> > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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;

Oh, you've renamed it whilst moving it. That is probably not best
practise, as it causes this kind of confusion.

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