Gene Chen <gene.chen.rich...@gmail.com> 於 2020年7月30日 週四 上午10:56寫道: > > Lee Jones <lee.jo...@linaro.org> 於 2020年7月29日 週三 下午6:12寫道: > > > > On Wed, 29 Jul 2020, Gene Chen wrote: > > > > > Lee Jones <lee.jo...@linaro.org> 於 2020年7月27日 週一 下午8:43寫道: > > > > > > > > On Fri, 17 Jul 2020, Gene Chen wrote: > > > > > > > > > From: Gene Chen <gene_c...@richtek.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_c...@richtek.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; > > } > > > > ACK. Thanks for your suggestions. > > > > /* > > > * 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. > > > > [...] > > > > ACK >
After I implement first version of regmap-mt6360.c, I found out ac97 is a intel standard codec. If device follow this spec, it can reuse this api. But regmap-mt6360.c may single use only. should I try sending patch once to regmap reviewer? > > > > > + 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? > > > > I see. It's my logical defect > I will remove set clientdata. > > > -- > > Lee Jones [李琼斯] > > Senior Technical Lead - Developer Services > > Linaro.org │ Open source software for Arm SoCs > > Follow Linaro: Facebook | Twitter | Blog