Hi Andy, Thanks for your helpful comments! We have some questions below. Andy Shevchenko <andy.shevchenko@xxxxxxxxx> 於 2022年6月24日 週五 凌晨2:01寫道： > > On Thu, Jun 23, 2022 at 1:59 PM ChiaEn Wu <peterwu.pub@xxxxxxxxx> wrote: > > > > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx> > > > > Add Mediatek MT6370 MFD support. > > ... > > > +config MFD_MT6370 > > + tristate "Mediatek MT6370 SubPMIC" > > + select MFD_CORE > > + select REGMAP_I2C > > + select REGMAP_IRQ > > + depends on I2C > > + help > > + Say Y here to enable MT6370 SubPMIC functional support. > > + It consists of a single cell battery charger with ADC monitoring, RGB > > + LEDs, dual channel flashlight, WLED backlight driver, display bias > > + voltage supply, one general purpose LDO, and the USB Type-C & PD > > + controller complies with the latest USB Type-C and PD standards. > > What will be the module name in case it's chosen to be built as a module? OK, we will add related text in the next patch! Thanks! > > ... > > > obj-$(CONFIG_INTEL_SOC_PMIC_BXTWC) += intel_soc_pmic_bxtwc.o > > obj-$(CONFIG_INTEL_SOC_PMIC_CHTWC) += intel_soc_pmic_chtwc.o > > obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o > > obj-$(CONFIG_MFD_MT6360) += mt6360-core.o > > +obj-$(CONFIG_MFD_MT6370) += mt6370.o > > mt6397-objs := mt6397-core.o mt6397-irq.o mt6358-irq.o > > obj-$(CONFIG_MFD_MT6397) += mt6397.o > > obj-$(CONFIG_INTEL_SOC_PMIC_MRFLD) += intel_soc_pmic_mrfld.o > > This whole bunch of drivers is in the wrong place in Makefile. > > https://lore.kernel.org/all/20220616182524.7956-2-andriy.shevchenko@xxxxxxxxxxxxxxx/ > hmm... So shall we need to cherry-pick your this patch first, then modify the Makefile before the next submission?? > ... > > > +#define MT6370_REG_MAXADDR 0x1FF > > Wondering if (BIT(10) - 1) gives a better hint on how hardware limits > this (so it will be clear it's 10-bit address). well... This "0x1FF" is just a virtual mapping value to map the max address of the PMU bank(0x1XX). So, I feel its means is different from using (BIT(10) - 1) here. > > ... > > > +static int mt6370_check_vendor_info(struct mt6370_info *info) > > +{ > > + unsigned int devinfo; > > + int ret; > > + > > + ret = regmap_read(info->regmap, MT6370_REG_DEV_INFO, &devinfo); > > + if (ret) > > + return ret; > > + > > + switch (FIELD_GET(MT6370_VENID_MASK, devinfo)) { > > + case MT6370_VENID_RT5081: > > + case MT6370_VENID_RT5081A: > > + case MT6370_VENID_MT6370: > > + case MT6370_VENID_MT6371: > > + case MT6370_VENID_MT6372P: > > + case MT6370_VENID_MT6372CP: > > return 0; > > > + break; > > + default: > > + dev_err(info->dev, "Unknown Vendor ID 0x%02x\n", devinfo); > > + return -ENODEV; > > + } > > + > > + return 0; > > ...and drop these two lines? OK! We will refine it in the next patch! > > > +} > > ... > > > + bank_idx = *(u8 *)reg_buf; > > + bank_addr = *(u8 *)(reg_buf + 1); > > Why not > > const u8 *u8_buf = reg_buf; > > bank_idx = u8_buf[0]; > bank_addr = u8_buf[1]; > > ? We will refine it in the next patch! Thanks! > > ... > > > + if (ret < 0) > > + return ret; > > + else if (ret != val_size) > > Redundant 'else'. I'm not quite sure what you mean, so I made the following changes first. ------------------------------------ if (ret < 0) return ret; if (ret != val_size) return -EIO; ------------------------------------ I don't know if it meets your expectations?? > > > + return -EIO; > > ... > > > + bank_idx = *(u8 *)data; > > + bank_addr = *(u8 *)(data + 1); > > As per above. > > -- > With Best Regards, > Andy Shevchenko Best regards, ChiaEn Wu
