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.
> + 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?
> +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.
> +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!
> +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.
> + 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?
signature.asc
Description: PGP signature
