Re: [PATCH V3 2/2] regulator: tps80031: add regulator driver fortps80031

[Guennadi Liakhovetski]

Hi Laxman

A couple of nit-picks below.

On Sun, 11 Nov 2012, Laxman Dewangan wrote:

> Add regulator driver for Texas Instrument TPS80031/TPS80032 device.
> TPS80031/ TPS80032 Fully Integrated Power Management with Power
> Path and Battery Charger. It has 5 configurable step-down
> converters, 11 general purpose LDOs, VBUS generator and digital
> output to control regulators.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@xxxxxxxxxx>
> ---
> Changes from V1: 
> - Rewrite dcdc get voltage apis.
> - Remove -1 and +1 in vsel.
> - Correct the vbus ops for sw and hw based.
> 
> Changes from V2:
> - Move header file changes to first patch so easy to apply on subsystem tree.
> 
>  drivers/regulator/Kconfig              |    9 +
>  drivers/regulator/Makefile             |    1 +
>  drivers/regulator/tps80031-regulator.c |  789 ++++++++++++++++++++++++++++++++
>  3 files changed, 799 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/regulator/tps80031-regulator.c
> 

[snip]

> diff --git a/drivers/regulator/tps80031-regulator.c b/drivers/regulator/tps80031-regulator.c
> new file mode 100644
> index 0000000..0484478
> --- /dev/null
> +++ b/drivers/regulator/tps80031-regulator.c
> @@ -0,0 +1,789 @@

[snip]

> +static inline struct device *to_tps80031_dev(struct regulator_dev *rdev)

"inline" is not needed, the compiler will decide by itself.

> +{
> +	return rdev_get_dev(rdev)->parent->parent;
> +}

[snip]

> +static int tps80031_dcdc_get_voltage_sel(struct regulator_dev *rdev)
> +{
> +	struct tps80031_regulator *ri = rdev_get_drvdata(rdev);
> +	struct device *parent = to_tps80031_dev(rdev);
> +	uint8_t vsel = 0;

don't think you need to initialise vsel - if tps80031_read() returns 
success, vsel will be set.

> +	int ret;
> +
> +	if (ri->rinfo->force_reg) {
> +		ret = tps80031_read(parent, ri->rinfo->volt_id,
> +						ri->rinfo->force_reg, &vsel);
> +		if (ret < 0) {
> +			dev_err(ri->dev, "reg 0x%02x read failed, e = %d\n",
> +					ri->rinfo->force_reg, ret);
> +			return ret;
> +		}
> +
> +		if (!(vsel & SMPS_CMD_MASK))
> +			return vsel & SMPS_VSEL_MASK;
> +	}
> +	ret = tps80031_read(parent, ri->rinfo->volt_id,
> +				ri->rinfo->volt_reg, &vsel);
> +	if (ret < 0) {
> +		dev_err(ri->dev, "reg 0x%02x read failed, e = %d\n",
> +			ri->rinfo->volt_reg, ret);
> +		return ret;
> +	}
> +	return vsel & SMPS_VSEL_MASK;
> +}

[snip]

> +static int tps80031_vbus_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct tps80031_regulator *ri = rdev_get_drvdata(rdev);
> +	struct device *parent = to_tps80031_dev(rdev);
> +	int ret = -EIO;
> +	uint8_t ctrl1 = 0;
> +	uint8_t ctrl3 = 0;

No need to initialise ret, ctrl1, ctrl3

> +
> +	ret = tps80031_read(parent, SLAVE_ID2,
> +			TPS80031_CHARGERUSB_CTRL1, &ctrl1);
> +	if (ret < 0) {
> +		dev_err(ri->dev, "reg 0x%02x read failed, e = %d\n",
> +			TPS80031_CHARGERUSB_CTRL1, ret);
> +		return ret;
> +	}
> +	ret = tps80031_read(parent, SLAVE_ID2,
> +				TPS80031_CHARGERUSB_CTRL3, &ctrl3);
> +	if (ret < 0) {
> +		dev_err(ri->dev, "reg 0x%02x read failed, e = %d\n",
> +			TPS80031_CHARGERUSB_CTRL1, ret);
> +		return ret;
> +	}
> +	if ((ctrl1 & OPA_MODE_EN) && (ctrl3 & BOOST_HW_PWR_EN))
> +		return 1;
> +	return ret;
> +}
> +
> +static int tps80031_vbus_enable(struct regulator_dev *rdev)
> +{
> +	struct tps80031_regulator *ri = rdev_get_drvdata(rdev);
> +	struct device *parent = to_tps80031_dev(rdev);
> +	int ret;
> +
> +	ret = tps80031_set_bits(parent, SLAVE_ID2,
> +				TPS80031_CHARGERUSB_CTRL1, OPA_MODE_EN);
> +	if (ret < 0) {
> +		dev_err(ri->dev, "reg 0x%02x read failed, e = %d\n",
> +					TPS80031_CHARGERUSB_CTRL1, ret);
> +		return ret;
> +	}
> +
> +	ret = tps80031_set_bits(parent, SLAVE_ID2,
> +				TPS80031_CHARGERUSB_CTRL3, BOOST_HW_PWR_EN);
> +	if (ret < 0) {
> +		dev_err(ri->dev, "reg 0x%02x read failed, e = %d\n",
> +			TPS80031_CHARGERUSB_CTRL3, ret);
> +		return ret;
> +	}
> +	return ret;
> +}
> +
> +static int tps80031_vbus_disable(struct regulator_dev *rdev)
> +{
> +	struct tps80031_regulator *ri = rdev_get_drvdata(rdev);
> +	struct device *parent = to_tps80031_dev(rdev);
> +	int ret = 0;

Ditto for ret.

> +
> +	if (ri->config_flags & VBUS_DISCHRG_EN_PDN) {
> +		ret = tps80031_write(parent, SLAVE_ID2,
> +			USB_VBUS_CTRL_SET, VBUS_DISCHRG);
> +		if (ret < 0) {
> +			dev_err(ri->dev, "reg 0x%02x write failed, e = %d\n",
> +				USB_VBUS_CTRL_SET, ret);
> +			return ret;
> +		}
> +	}
> +
> +	ret = tps80031_clr_bits(parent, SLAVE_ID2,
> +			TPS80031_CHARGERUSB_CTRL1,  OPA_MODE_EN);
> +	if (ret < 0) {
> +		dev_err(ri->dev, "reg 0x%02x clearbit failed, e = %d\n",
> +				TPS80031_CHARGERUSB_CTRL1, ret);
> +		return ret;
> +	}
> +
> +	ret = tps80031_clr_bits(parent, SLAVE_ID2,
> +				TPS80031_CHARGERUSB_CTRL3, BOOST_HW_PWR_EN);
> +	if (ret < 0) {
> +		dev_err(ri->dev, "reg 0x%02x clearbit failed, e = %d\n",
> +				TPS80031_CHARGERUSB_CTRL3, ret);
> +		return ret;
> +	}
> +
> +	mdelay(DIV_ROUND_UP(ri->rinfo->desc.enable_time, 1000));
> +	if (ri->config_flags & VBUS_DISCHRG_EN_PDN) {
> +		ret = tps80031_write(parent, SLAVE_ID2,
> +			USB_VBUS_CTRL_CLR, VBUS_DISCHRG);
> +		if (ret < 0) {
> +			dev_err(ri->dev, "reg 0x%02x write failed, e = %d\n",
> +					USB_VBUS_CTRL_CLR, ret);
> +			return ret;
> +		}
> +	}
> +	return ret;
> +}

[snip]

> +static int tps80031_power_req_config(struct device *parent,
> +		struct tps80031_regulator *ri,
> +		struct tps80031_regulator_platform_data *tps80031_pdata)
> +{
> +	int ret = 0;

Ditto for ret.

> +
> +	if (ri->rinfo->preq_bit < 0)
> +		goto skip_pwr_req_config;
> +
> +	ret = tps80031_ext_power_req_config(parent, ri->ext_ctrl_flag,
> +			ri->rinfo->preq_bit, ri->rinfo->state_reg,
> +			ri->rinfo->trans_reg);
> +	if (ret < 0) {
> +		dev_err(ri->dev, "ext powerreq config failed, err = %d\n", ret);
> +		return ret;
> +	}
> +
> +skip_pwr_req_config:
> +	if (tps80031_pdata->ext_ctrl_flag & PWR_ON_ON_SLEEP) {
> +		ret = tps80031_update(parent, SLAVE_ID1, ri->rinfo->trans_reg,
> +				TRANS_SLEEP_ON, TRANS_SLEEP_MASK);
> +		if (ret < 0) {
> +			dev_err(ri->dev, "Reg 0x%02x update failed, e %d\n",
> +					ri->rinfo->trans_reg, ret);
> +			return ret;
> +		}
> +	}
> +	return ret;
> +}
> +
> +static int tps80031_regulator_config(struct device *parent,
> +		struct tps80031_regulator *ri,
> +		struct tps80031_regulator_platform_data *tps80031_pdata)
> +{
> +	int ret = 0;

Ditto

> +
> +	switch (ri->rinfo->desc.id) {
> +	case TPS80031_REGULATOR_LDOUSB:
> +		if (ri->config_flags &
> +			(USBLDO_INPUT_VSYS | USBLDO_INPUT_PMID)) {
> +			unsigned val = 0;

Ditto

> +			if (ri->config_flags & USBLDO_INPUT_VSYS)
> +				val = MISC2_LDOUSB_IN_VSYS;
> +			else
> +				val = MISC2_LDOUSB_IN_PMID;
> +
> +			ret = tps80031_update(parent, SLAVE_ID1,
> +				TPS80031_MISC2, val, MISC2_LDOUSB_IN_MASK);
> +			if (ret < 0) {
> +				dev_err(ri->dev,
> +					"LDOUSB config failed, e= %d\n", ret);
> +				return ret;
> +			}
> +		}
> +		break;
> +
> +	case TPS80031_REGULATOR_LDO3:
> +		if (ri->config_flags & LDO3_OUTPUT_VIB) {
> +			ret = tps80031_update(parent, SLAVE_ID1,
> +				TPS80031_MISC2, MISC2_LDO3_SEL_VIB_VAL,
> +				MISC2_LDO3_SEL_VIB_MASK);
> +			if (ret < 0) {
> +				dev_err(ri->dev,
> +					"LDO3 config failed, e = %d\n", ret);
> +				return ret;
> +			}
> +		}
> +		break;
> +
> +	case TPS80031_REGULATOR_VBUS:
> +		/* Provide SW control Ops if VBUS is SW control */
> +		if (!(ri->config_flags & VBUS_SW_ONLY))
> +			ri->rinfo->desc.ops = &tps80031_vbus_sw_ops;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	/* Configure Active state to ON, SLEEP to OFF and OFF_state to OFF */
> +	ret = tps80031_update(parent, SLAVE_ID1, ri->rinfo->trans_reg,
> +		TRANS_ACTIVE_ON | TRANS_SLEEP_OFF | TRANS_OFF_OFF,
> +		TRANS_ACTIVE_MASK | TRANS_SLEEP_MASK | TRANS_OFF_MASK);
> +	if (ret < 0) {
> +		dev_err(ri->dev, "trans reg update failed, e %d\n", ret);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int check_smps_mode_mult(struct device *parent,
> +	struct tps80031_regulator *ri)
> +{
> +	int mult_offset;
> +	int ret;
> +	u8 smps_offset;
> +	u8 smps_mult;
> +
> +	ret = tps80031_read(parent, SLAVE_ID1,
> +			TPS80031_SMPS_OFFSET, &smps_offset);
> +	if (ret < 0) {
> +		dev_err(parent, "Error in reading smps offset register\n");
> +		return ret;
> +	}
> +
> +	ret = tps80031_read(parent, SLAVE_ID1,
> +			TPS80031_SMPS_MULT, &smps_mult);
> +	if (ret < 0) {
> +		dev_err(parent, "Error in reading smps mult register\n");
> +		return ret;
> +	}
> +
> +	switch (ri->rinfo->desc.id) {
> +	case TPS80031_REGULATOR_VIO:
> +		mult_offset = SMPS_MULTOFFSET_VIO;
> +		break;
> +	case TPS80031_REGULATOR_SMPS1:
> +		mult_offset = SMPS_MULTOFFSET_SMPS1;
> +		break;
> +	case TPS80031_REGULATOR_SMPS2:
> +		mult_offset = SMPS_MULTOFFSET_SMPS2;
> +		break;
> +	case TPS80031_REGULATOR_SMPS3:
> +		mult_offset = SMPS_MULTOFFSET_SMPS3;
> +		break;
> +	case TPS80031_REGULATOR_SMPS4:
> +		mult_offset = SMPS_MULTOFFSET_SMPS4;
> +		break;
> +	case TPS80031_REGULATOR_LDO2:
> +		ri->device_flags = smps_mult & BIT(5) ? TRACK_MODE_ENABLE : 0;
> +		/* TRACK mode the ldo2 varies from 600mV to 1300mV */
> +		if (ri->device_flags & TRACK_MODE_ENABLE) {
> +			ri->rinfo->desc.min_uV = 600000;
> +			ri->rinfo->desc.uV_step = 12500;
> +			ri->rinfo->desc.n_voltages = 57;
> +			ri->rinfo->desc.vsel_mask = LDO_TRACK_VSEL_MASK;
> +		}
> +		return 0;
> +	default:
> +		return 0;
> +	}
> +
> +	ri->device_flags = (smps_offset & mult_offset) ? DCDC_OFFSET_EN : 0;
> +	ri->device_flags |= (smps_mult & mult_offset) ? DCDC_EXTENDED_EN : 0;
> +	switch (ri->device_flags) {
> +	case 0:
> +		ri->rinfo->desc.min_uV = 607700;
> +		ri->rinfo->desc.uV_step = 12660;
> +		break;
> +	case DCDC_OFFSET_EN:
> +		ri->rinfo->desc.min_uV = 700000;
> +		ri->rinfo->desc.uV_step = 12500;
> +		break;
> +	case DCDC_EXTENDED_EN:
> +		ri->rinfo->desc.min_uV = 1852000;
> +		ri->rinfo->desc.uV_step = 38600;
> +		break;
> +	case DCDC_OFFSET_EN | DCDC_EXTENDED_EN:
> +		ri->rinfo->desc.min_uV = 2161000;
> +		ri->rinfo->desc.uV_step = 38600;
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int __devinit tps80031_regulator_probe(struct platform_device *pdev)
> +{
> +	struct tps80031_platform_data *pdata;
> +	struct tps80031_regulator_platform_data *tps_pdata;
> +	struct tps80031_regulator_info *rinfo;
> +	struct tps80031_regulator *ri;
> +	struct tps80031_regulator *pmic;
> +	struct regulator_dev *rdev;
> +	struct regulator_config config = { };
> +	int ret;
> +	int num;
> +
> +	pdata = dev_get_platdata(pdev->dev.parent);
> +
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "No platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	pmic = devm_kzalloc(&pdev->dev,
> +			TPS80031_REGULATOR_MAX * sizeof(*pmic), GFP_KERNEL);
> +	if (!pmic) {
> +		dev_err(&pdev->dev, "mem alloc for pmic failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	for (num = 0; num < TPS80031_REGULATOR_MAX; ++num) {
> +		tps_pdata = pdata->regulator_pdata[num];
> +		rinfo = &tps80031_rinfo[num];
> +		ri = &pmic[num];
> +		ri->rinfo = rinfo;
> +		ri->dev = &pdev->dev;
> +
> +		check_smps_mode_mult(pdev->dev.parent, ri);

You might want to check check_smps_mode_mult() return value.

> +		config.dev = &pdev->dev;
> +		config.init_data = NULL;
> +		config.driver_data = ri;
> +		if (tps_pdata) {
> +			config.init_data = tps_pdata->reg_init_data;
> +			ri->config_flags = tps_pdata->config_flags;
> +			ri->ext_ctrl_flag = tps_pdata->ext_ctrl_flag;
> +			ret = tps80031_regulator_config(pdev->dev.parent,
> +					ri, tps_pdata);
> +			if (ret < 0) {
> +				dev_err(&pdev->dev,
> +					"regulator config failed, e %d\n", ret);
> +				goto fail;
> +			}
> +
> +			ret = tps80031_power_req_config(pdev->dev.parent,
> +					ri, tps_pdata);
> +			if (ret < 0) {
> +				dev_err(&pdev->dev,
> +					"pwr_req config failed, err %d\n", ret);
> +				goto fail;
> +			}
> +		}
> +		rdev = regulator_register(&ri->rinfo->desc, &config);
> +		if (IS_ERR_OR_NULL(rdev)) {
> +			dev_err(&pdev->dev,
> +				"register regulator failed %s\n",
> +					ri->rinfo->desc.name);
> +			ret = PTR_ERR(rdev);
> +			goto fail;
> +		}
> +		ri->rdev = rdev;
> +	}
> +
> +	platform_set_drvdata(pdev, pmic);
> +	return 0;
> +fail:
> +	while (--num >= 0) {
> +		ri = &pmic[num];
> +		regulator_unregister(ri->rdev);
> +	}
> +	return ret;
> +}
> +
> +static int __devexit tps80031_regulator_remove(struct platform_device *pdev)
> +{
> +	struct tps80031_regulator *pmic = platform_get_drvdata(pdev);
> +	struct tps80031_regulator *ri = NULL;
> +	int num;
> +
> +	for (num = 0; num < TPS80031_REGULATOR_MAX; ++num) {
> +		ri = &pmic[num];
> +		regulator_unregister(ri->rdev);
> +	}
> +	return 0;
> +}
> +
> +static struct platform_driver tps80031_regulator_driver = {
> +	.driver	= {
> +		.name	= "tps80031-pmic",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= tps80031_regulator_probe,
> +	.remove		= __devexit_p(tps80031_regulator_remove),
> +};
> +
> +static int __init tps80031_regulator_init(void)
> +{
> +	return platform_driver_register(&tps80031_regulator_driver);
> +}
> +subsys_initcall(tps80031_regulator_init);
> +
> +static void __exit tps80031_regulator_exit(void)
> +{
> +	platform_driver_unregister(&tps80031_regulator_driver);
> +}
> +module_exit(tps80031_regulator_exit);
> +
> +MODULE_ALIAS("platform:tps80031-regulator");
> +MODULE_DESCRIPTION("Regulator Driver for TI TPS80031 PMIC");
> +MODULE_AUTHOR("Laxman Dewangan <ldewangan@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.7.1.1

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/