Messages in this thread | | | From | Gene Chen <> | Date | Thu, 18 Jun 2020 19:35:24 +0800 | Subject | Re: [PATCH] regulator: mt6360: Add support for MT6360 regulator |
| |
Mark Brown <broonie@kernel.org> 於 2020年6月4日 週四 下午9:39寫道: > > On Thu, Jun 04, 2020 at 03:06:27PM +0800, Gene Chen wrote: > > This looks nice and simple, a few fairly small comments below but high > level it's basically fine. > > > --- /dev/null > > +++ b/drivers/regulator/mt6360-regulator.c > > @@ -0,0 +1,571 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2020 MediaTek Inc. > > Please make the entire comment a C++ one so things look more > intentional. >
ACK
> > + for (i = 0; i < devdata->num_irq_descs; i++) { > > + irq_desc = devdata->irq_descs + i; > > + if (unlikely(!irq_desc->name)) > > + continue; > > Do we really need an unlikely here? This shouldn't be a hot path. > > > +static int mt6360_regulator_set_mode( > > + struct regulator_dev *rdev, unsigned int mode) > > +{ > > > + switch (1 << (ffs(mode) - 1)) { > > + case REGULATOR_MODE_NORMAL: > > I don't understand why this isn't just a straight switch on mode? >
ACK, we will fix it
> > +static unsigned int mt6360_regulator_get_mode(struct regulator_dev *rdev) > > +{ > > + const struct mt6360_regulator_desc *desc = > > + (const struct mt6360_regulator_desc *)rdev->desc; > > + int shift = ffs(desc->mode_get_mask) - 1, ret; > > + unsigned int val = 0; > > + > > + default: > > + ret = 0; > > + } > > If we can't parse a valid value from the hardware then that's an error. >
ACK
> > +static int mt6360_regulator_reg_write(void *context, > > + unsigned int reg, unsigned int val) > > +{ > > + struct mt6360_regulator_data *mrd = context; > > + u8 chunk[4] = {0}; > > + > > + /* chunk 0 ->i2c addr, 1 -> reg_addr, 2 -> reg_val 3-> crc8 */ > > + chunk[0] = (mrd->i2c->addr & 0x7f) << 1; > > + chunk[1] = reg & 0x3f; > > + chunk[2] = (u8)val; > > + chunk[3] = crc8(mrd->crc8_table, chunk, 3, 0); > > + /* also dummy one byte */ > > + return i2c_smbus_write_i2c_block_data(mrd->i2c, chunk[1], 3, chunk + 2); > > +} > > Oh, wow - that's a fun I/O interface! >
MT6360 PMIC/LDO part use CRC to avoid i2c write mistaken
> > +static const struct of_device_id __maybe_unused mt6360_regulator_of_id[] = { > > + { > > + .compatible = "mediatek,mt6360_pmic", > > + .data = (void *)&mt6360_pmic_devdata, > > + }, > > + { > > + .compatible = "mediatek,mt6360_ldo", > > + .data = (void *)&mt6360_ldo_devdata, > > + }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, mt6360_regulator_of_id); > > I don't see any DT bindings documentation for this, documentation is > required for all new bindings. >
ACK, we will update binding document
> > + mrd->regmap = devm_regmap_init(&(mrd->i2c->dev), > > + NULL, mrd, devdata->regmap_config); > > + if (IS_ERR(mrd->regmap)) { > > + dev_err(&pdev->dev, "Failed to register regmap\n"); > > + return PTR_ERR(mrd->regmap); > > + } > > This looks like a MFD so it's surprising to see us defining a regmap at > this level. Why are we doing this?
because other sub-device (e.g. CHARGER/LED/ADC) no need CRC when i2c R/W we will merge remgap into mfd, and use "bank" strategy to distinguish different part
| |