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;
MIME-Version: 1.0
References: <1597661277-27862-1-git-send-email-gene.chen.richtek@gmail.com>
 <1597661277-27862-10-git-send-email-gene.chen.richtek@gmail.com>
 <20200828104053.GM1826686@dell> <CAE+NS37uFoDhWyGkw0WTu+tR+_85EwzYRqecNMG6nK6b2J=9jg@mail.gmail.com>
 <20200908114819.GO4400@dell>
In-Reply-To: <20200908114819.GO4400@dell>
From:   Gene Chen <gene.chen.richtek@gmail.com>
Date:   Wed, 9 Sep 2020 07:09:44 +0800
Message-ID: <CAE+NS36MVA=9e0Ev73gpJ-gOcY+_aNveTr+DhquD6iqY-GKXCQ@mail.gmail.com>
Subject: Re: [PATCH v4 9/9] mfd: mt6360: Merge different sub-devices I2C read/write
To:     Lee Jones <lee.jones@linaro.org>
Cc:     Matthias Brugger <matthias.bgg@gmail.com>,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
        Gene Chen <gene_chen@richtek.com>, benjamin.chao@mediatek.com,
        shufan_lee@richtek.com, cy_huang@richtek.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Lee Jones <lee.jones@linaro.org> =E6=96=BC 2020=E5=B9=B49=E6=9C=888=E6=97=
=A5 =E9=80=B1=E4=BA=8C =E4=B8=8B=E5=8D=887:48=E5=AF=AB=E9=81=93=EF=BC=9A
>
> On Tue, 01 Sep 2020, Gene Chen wrote:
>
> > Lee Jones <lee.jones@linaro.org> =E6=96=BC 2020=E5=B9=B48=E6=9C=8828=E6=
=97=A5 =E9=80=B1=E4=BA=94 =E4=B8=8B=E5=8D=886:40=E5=AF=AB=E9=81=93=EF=BC=9A
> > >
> > > On Mon, 17 Aug 2020, Gene Chen wrote:
> > >
> > > > From: Gene Chen <gene_chen@richtek.com>
> > > >
> > > > Remove unuse register definition.
> > >
> > > This should be in a separate patch.
> > >
> > > > Merge different sub-devices I2C read/write functions into one Regma=
p,
> > > > because PMIC and LDO part need CRC bits for access protection.
> > > >
> > > > Signed-off-by: Gene Chen <gene_chen@richtek.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 =3D 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 i=
t
> > > 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_da=
ta.
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] =
=3D {
> > > > -     MT6360_PMU_SLAVEID,
> > > > +static const u16 mt6360_slave_addrs[MT6360_SLAVE_MAX] =3D {
> > >
> > > 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 *cli=
ent)
> > > >               return -ENOMEM;
> > > >
> > > >       ddata->dev =3D &client->dev;
> > > > -     i2c_set_clientdata(client, ddata);
> > > >
> > > > -     ddata->regmap =3D devm_regmap_init_i2c(client, &mt6360_pmu_re=
gmap_config);
> > > > +     for (i =3D 0; i < MT6360_SLAVE_MAX - 1; i++) {
> > > > +             ddata->i2c[i] =3D devm_i2c_new_dummy_device(&client->=
dev,
> > > > +                                                       client->ada=
pter,
> > > > +                                                       mt6360_slav=
e_addrs[i]);
> > > > +             if (IS_ERR(ddata->i2c[i])) {
> > > > +                     dev_err(&client->dev,
> > > > +                             "Failed to get new dummy I2C device f=
or 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 [=E6=9D=8E=E7=90=BC=E6=96=AF]
> Senior Technical Lead - Developer Services
> Linaro.org =E2=94=82 Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog