On Mon, 13 Jun 2022 19:11:39 +0800 ChiaEn Wu <peterwu.pub@xxxxxxxxx> wrote: > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx> > > Add Mediatek MT6370 MFD support. > > Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx> Hi. A few trivial things that probably overlap with other reviewer comments. > +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 0x8: /* RT5081 */ > + case 0xA: /* RT5081A */ > + case 0xE: /* MT6370 */ > + case 0xF: /* MT6371 */ > + case 0x9: /* MT6372P */ > + case 0xB: /* MT6372CP */ > + break; > + default: > + dev_err(info->dev, "Not invalid value 0x%02x\n", devinfo); Double negative... You mean "Invalid value" though you probably want to say something more specific such as "Unknown Vendor ID 0x%02x\n") > + return -ENODEV; > + } > + > + return 0; > +} > + > +static int mt6370_regmap_read(void *context, const void *reg_buf, > + size_t reg_size, void *val_buf, size_t val_size) > +{ > + struct mt6370_info *info = context; > + u8 bank_idx = *(u8 *)reg_buf, bank_addr = *(u8 *)(reg_buf + 1); > + int ret; > + > + ret = i2c_smbus_read_i2c_block_data(info->i2c[bank_idx], bank_addr, > + val_size, val_buf); > + if (ret != val_size) > + return ret; If it's positive , that is probably not what regmap expects. See handling in here https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-i2c.c#L222 > + > + return 0; > +} > + > +static int mt6370_probe(struct i2c_client *i2c) > +{ > + struct mt6370_info *info; > + struct i2c_client *usbc_i2c; > + int ret; > + > + info = devm_kzalloc(&i2c->dev, sizeof(*info), GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + > + info->dev = &i2c->dev; > + > + usbc_i2c = devm_i2c_new_dummy_device(&i2c->dev, i2c->adapter, > + MT6370_USBC_I2CADDR); > + if (IS_ERR(usbc_i2c)) { > + ret = PTR_ERR(usbc_i2c); > + dev_err(&i2c->dev, "Failed to register usbc i2c client %d\n", ret); Might as well use return dev_err_probe(&i2c->dev, PTR_ERR(usb_i2c), "Failed to register USBC I2C client\n"); Whilst the defer handling isn't relevant, it is safe to use throughout probe and tidier in this case at least. > + return ret; > + } > + > + /* Assign I2C client for PMU and TypeC */ > + info->i2c[MT6370_PMU_I2C] = i2c; > + info->i2c[MT6370_USBC_I2C] = usbc_i2c; > + > + info->regmap = devm_regmap_init(&i2c->dev, &mt6370_regmap_bus, info, > + &mt6370_regmap_config); > + if (IS_ERR(info->regmap)) { > + ret = PTR_ERR(info->regmap); > + dev_err(&i2c->dev, "Failed to register regmap (%d)\n", ret); as above dev_err_probe() will tidy this and following cases up for you. > + return ret; > + } > + > + ret = mt6370_check_vendor_info(info); > + if (ret) { > + dev_err(&i2c->dev, "Failed to check vendor info (%d)\n", ret); > + return ret; > + } > + > + ret = devm_regmap_add_irq_chip(&i2c->dev, info->regmap, i2c->irq, > + IRQF_ONESHOT, -1, &mt6370_irq_chip, > + &info->irq_data); > + if (ret) { > + dev_err(&i2c->dev, "Failed to add irq chip (%d)\n", ret); > + return ret; > + } > + > + return devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, > + mt6370_devices, ARRAY_SIZE(mt6370_devices), > + NULL, 0, > + regmap_irq_get_domain(info->irq_data)); > +}