| 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: <20200729101244.GH2419169@dell>
Date: Wed, 29 Jul 2020 11:12:44 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Gene Chen <gene.chen.richtek@...il.com>
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 v2 9/9] mfd: mt6360: Merge different sub-devices i2c
read/write
On Wed, 29 Jul 2020, Gene Chen wrote:
> Lee Jones <lee.jones@...aro.org> 於 2020年7月27日 週一 下午8:43寫道：
> >
> > On Fri, 17 Jul 2020, Gene Chen wrote:
> >
> > > From: Gene Chen <gene_chen@...htek.com>
> > >
> > > Remove unuse register definition.
> >
> > This should not be in here.
> >
> > > Merge different sub-devices i2c read/write function into one regmap,
> >
> > "I2C", "functions", "Regmap".
> >
>
> ACK
>
> > > because pmic and ldo part need crc bits for access protection.
> >
> > "PMIC", "LDO", "CRC".
> >
>
> ACK
>
> > > Signed-off-by: Gene Chen <gene_chen@...htek.com>
> > > ---
> > > drivers/mfd/Kconfig | 1 +
> > > drivers/mfd/mt6360-core.c | 229 +++++++++++++++++++++++++++++++++++++-----
> > > include/linux/mfd/mt6360.h | 240 ---------------------------------------------
> > > 3 files changed, 204 insertions(+), 266 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 3186a7c..97ef1ad 100644
> > > --- a/drivers/mfd/mt6360-core.c
> > > +++ b/drivers/mfd/mt6360-core.c
> > > @@ -14,7 +14,46 @@
> > > #include <linux/interrupt.h>
> > > #include <linux/mfd/core.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_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;mt6360_data
> > > + u8 crc8_tbl[CRC8_TABLE_SIZE];
> > > +};
> >
> > Make sure all of these entries are still used.
> >
> > > +#define MT6360_PMU_SLAVEID 0x34
> > > +#define MT6360_PMIC_SLAVEID 0x1A
> > > +#define MT6360_LDO_SLAVEID 0x64
> > > +#define MT6360_TCPC_SLAVEID 0x4E
> >
> > Can these be placed into ID order?
> >
>
> ACK
>
> > > +#define MT6360_REG_TCPCSTART 0x00
> > > +#define MT6360_REG_TCPCEND 0xFF
> > > +#define MT6360_REG_PMICSTART 0x100
> > > +#define MT6360_REG_PMICEND 0x13B
> > > +#define MT6360_REG_LDOSTART 0x200
> > > +#define MT6360_REG_LDOEND 0x21C
> > > +#define MT6360_REG_PMUSTART 0x300
> > > +#define MT6360_PMU_DEV_INFO 0x300
> > > +#define MT6360_PMU_CHG_IRQ1 0x3D0
> > > +#define MT6360_PMU_CHG_MASK1 0x3F0
> > > +#define MT6360_REG_PMUEND 0x3FF
> > > +
> > > +/* from 0x3D0 to 0x3DF */
> >
> > We don't need this in here.
> >
>
> ACK
>
> > > +#define MT6360_PMU_IRQ_REGNUM 16
> > > +
> > > +#define CHIP_VEN_MASK 0xF0
> > > +#define CHIP_VEN_MT6360 0x50
> > > +#define CHIP_REV_MASK 0x0F
> > >
> > > /* reg 0 -> 0 ~ 7 */
> > > #define MT6360_CHG_TREG_EVT 4
> > > @@ -220,12 +259,6 @@ static const struct regmap_irq_chip mt6360_irq_chip = {
> > > .use_ack = true,
> > > };
> > >
> > > -static const struct regmap_config mt6360_pmu_regmap_config = {
> > > - .reg_bits = 8,
> > > - .val_bits = 8,
> > > - .max_register = MT6360_PMU_MAXREG,
> > > -};
> > > -
> > > static const struct resource mt6360_adc_resources[] = {
> > > DEFINE_RES_IRQ_NAMED(MT6360_ADC_DONEI, "adc_donei"),
> > > };
> > > @@ -310,11 +343,153 @@ static int mt6360_check_vendor_info(struct mt6360_data *data)
> > > return 0;
> > > }
> > >
> > > -static const unsigned short mt6360_slave_addr[MT6360_SLAVE_MAX] = {
> > > - MT6360_PMU_SLAVEID,
> > > +static const u16 mt6360_slave_addrs[MT6360_SLAVE_MAX] = {
> > > + MT6360_TCPC_SLAVEID,
> > > MT6360_PMIC_SLAVEID,
> > > MT6360_LDO_SLAVEID,
> > > - MT6360_TCPC_SLAVEID,
> > > + MT6360_PMU_SLAVEID,
> > > +};
> > > +
> > > +static int mt6360_xlate_pmicldo_addr(u8 *addr, int rw_size)
> > > +{
> > > + u8 flags[4] = { 0x00, 0x40, 0x80, 0xc0 };
> > > +
> > > + if (rw_size < 1 || rw_size > 4)
> > > + return -EINVAL;
> > > +
> > > + *addr &= 0x3f;
> > > + *addr |= flags[rw_size - 1];
> > > +
> > > + return 0;
> > > +}
> >
> > You need some comments in here to explain what's going on.
> >
>
> ACK
>
> Is this comment readable?
>
> /*
> * When access sud-device PMIC and LDO part which only addressed
> 0x00~0x3F, read and write action need crc for protection.
>
> * Addr[5:0] is real access real register address.
> * Addr[7:6] use to store size, maximum 4 bytes.
>
> * When received the Addr, ic can interpret real register address and size to calculate or check crc
> * /
Don't you think this reads better?
No need for comments then:
#define MT6360_ADDRESS_MASK 0x3f
#define MT6360_DATA_SIZE_1_BYTE 0x00
#define MT6360_DATA_SIZE_2_BYTES 0x40
#define MT6360_DATA_SIZE_3_BYTES 0x80
#define MT6360_DATA_SIZE_4_BYTES 0xC0
static int mt6360_xlate_pmicldo_addr(u8 *addr, int rw_size)
{
/* Address is already in encoded [5:0] */
*addr &= MT6360_ADDRESS_MASK;
/* Encode size [7:6] */
switch (rw_size) {
case 1:
*addr |= MT6360_DATA_SIZE_1_BYTE
break;
case 2:
*addr |= MT6360_DATA_SIZE_2_BYTES
break;
case 3:
*addr |= MT6360_DATA_SIZE_3_BYTES
break;
case 4:
*addr |= MT6360_DATA_SIZE_4_BYTES
break;
default:
return -EINVAL;
}
return 0;
}
> /*
> * CRC calculation
> * total size is 2 byte and number of access bytes
> * 2 bytes include i2c device address, r/w bit and address which want to access
> * others for read or write data
> * /
>
> > > +static int mt6360_regmap_read(void *context, const void *reg, size_t reg_size,
> > > + void *val, size_t val_size)
> > > +{
> > > + struct mt6360_data *data = context;
> > > + u8 bank = *(u8 *)reg, reg_addr = *(u8 *)(reg + 1);
> > > + struct i2c_client *i2c = data->i2c[bank];
> > > + bool crc_needed = false;
> > > + u8 *buf;
> > > + /* first two is i2c_addr + reg_addr , last is crc8 */
> > > + int alloc_size = 2 + val_size + 1, read_size = val_size;
> > > + u8 crc;
> > > + int ret;
> > > +
> > > + if (bank == MT6360_SLAVE_PMIC || bank == MT6360_SLAVE_LDO) {
> > > + crc_needed = true;
> > > + ret = mt6360_xlate_pmicldo_addr(®_addr, val_size);
> > > + if (ret < 0)
> > > + return ret;
> > > + read_size += 1;
> > > + }
> > > +
> > > + buf = kzalloc(alloc_size, GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > +
> > > + /* 7 bit slave addr + read bit */
> > > + buf[0] = ((i2c->addr & 0x7f) << 1) + 1;
> > > + buf[1] = reg_addr;
> > > +
> > > + ret = i2c_smbus_read_i2c_block_data(i2c, reg_addr, read_size, buf + 2);
> > > +
> > > + if (ret == read_size) {
> > > + memcpy(val, buf + 2, val_size);
> > > + if (crc_needed) {
> > > + crc = crc8(data->crc8_tbl, buf, val_size + 2, 0);
> > > + if (crc != buf[val_size + 2])
> > > + ret = -EIO;
> > > + }
> > > + }
> > > +
> > > + kfree(buf);
> > > +
> > > + if (ret < 0)
> > > + return ret;
> > > + else if (ret != read_size)
> > > + return -EIO;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mt6360_regmap_write(void *context, const void *val, size_t val_size)
> > > +{
> > > + struct mt6360_data *data = context;
> > > + u8 bank = *(u8 *)val, reg_addr = *(u8 *)(val + 1);
> > > + struct i2c_client *i2c = data->i2c[bank];
> > > + bool crc_needed = false;
> > > + u8 *buf;
> > > + /* first two is i2c_addr + reg_addr , last crc8 + dummy */
> > > + int alloc_size = 2 + val_size + 2, write_size = val_size - 2;
> > > + int ret;
> > > +
> > > + if (bank == MT6360_SLAVE_PMIC || bank == MT6360_SLAVE_LDO) {
> > > + crc_needed = true;
> > > + ret = mt6360_xlate_pmicldo_addr(®_addr, val_size - 2);
> > > + if (ret < 0)
> > > + return ret;
> > > + }
> > > +
> > > + buf = kzalloc(alloc_size, GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > +
> > > + /* 7 bit slave addr + write bit */
> > > + buf[0] = ((i2c->addr & 0x7f) << 1);
> > > + buf[1] = reg_addr;
> > > + /* val need to minus regaddr 16bit */
> > > + memcpy(buf + 2, val + 2, write_size);
> > > +
> > > + if (crc_needed) {
> > > + buf[val_size] = crc8(data->crc8_tbl, buf, val_size, 0);
> > > + write_size += 2;
> > > + }
> > > +
> > > + ret = i2c_smbus_write_i2c_block_data(i2c,
> > > + reg_addr, write_size, buf + 2);
> > > +
> > > + kfree(buf);
> > > +
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct regmap_bus mt6360_regmap_bus = {
> > > + .read = mt6360_regmap_read,
> > > + .write = mt6360_regmap_write,
> > > +
> > > + /* due to pmic and ldo crc access size limit */
> > > + .max_raw_read = 4,
> > > + .max_raw_write = 4,
> > > +};
> >
> > Why isn't all of the above in a Regmap driver?
> >
>
> Do you means split out like drivers/base/regmap/regmap-ac97.c?
Yes, I do.
[...]
> > > + i2c_set_clientdata(client, data);
> >
> > Where is this used?
>
> I can use device to get chip_rev from dev_get_drvdata.
> According to different chip_rev, I may need apply different way to do.
>
> > Didn't you just move the definition into this file?
>
> ACK, I will seperate move definition into this file to new patch
That's not the point I'm making.
You can't use 'data' outside of this file, so why are you setting it
inside the clientdata area?
--
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