| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAE+NS36MVA=9e0Ev73gpJ-gOcY+_aNveTr+DhquD6iqY-GKXCQ@mail.gmail.com>
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