Date:   Wed, 9 Sep 2020 07:09:44 +0800
From:   Gene Chen <gene.chen.richtek@...il.com>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Matthias Brugger <matthias.bgg@...il.com>,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Gene Chen <gene_chen@...htek.com>, benjamin.chao@...iatek.com,
        shufan_lee@...htek.com, cy_huang@...htek.com
Subject: Re: [PATCH v4 9/9] mfd: mt6360: Merge different sub-devices I2C read/write

Lee Jones <lee.jones@...aro.org> 於 2020年9月8日 週二 下午7:48寫道：
>
> On Tue, 01 Sep 2020, Gene Chen wrote:
>
> > Lee Jones <lee.jones@...aro.org> 於 2020年8月28日 週五 下午6:40寫道：
> > >
> > > On Mon, 17 Aug 2020, Gene Chen wrote:
> > >
> > > > From: Gene Chen <gene_chen@...htek.com>
> > > >
> > > > 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 <gene_chen@...htek.com>
> > > > ---
> > > >  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

Powered by blists - more mailing lists