Re: [RFC PATCH 3/5] regulator: drivers: Add TI TPS65219 PMIC regulators support
flat view
From: Mark Brown <broonie@kernel.org>
Date: 2022-06-13 17:42:20
Also in:
linux-devicetree, lkml
On Mon, Jun 13, 2022 at 11:06:02AM +0200, Jerome NEANNE wrote:
quoted hunk ↗ jump to hunk
+static int tps65219_pmic_enable(struct regulator_dev *dev) +{ + struct tps65219 *tps = rdev_get_drvdata(dev); + int rid = rdev_get_id(dev); + int ret; + + +
Lots of random blank space here.
quoted hunk ↗ jump to hunk
+ if (rid < TPS65219_BUCK_1 || rid > TPS65219_LDO_4) + return -EINVAL; +
If a regulator doesn't support an operation it shouldn't be providing it, no need to check here.
+ ret = tps65219_set_bits(tps, dev->desc->enable_reg, + dev->desc->enable_mask, dev->desc->enable_mask);
If you provide the regmap to the child devices then this can use the standard regmap helpers for most of the operations, meaning most of the operations in the driver can be removed.
+/* generic regulator_set_bypass_regmap does not match requirements use custom instead */
In what way? Please also try to keep within 80 columns normally.
quoted hunk ↗ jump to hunk
+ if (dev->desc->ops->is_enabled) { + dev_err(tps->dev, "%s LDO%d is enabled, should be shut down to set bypass ", + __func__, rid); + return -EBUSY; + }
This is testing if there is an enable operation, not if the regulator is enabled.
quoted hunk ↗ jump to hunk
+ if (enable) { + dev_dbg(tps->dev, "%s, LDO%d already in bypass mode", __func__, rid); + return ret; + }
This is not a problem, just silently succeed.
quoted hunk ↗ jump to hunk
+ /* Allocate memory for strobes */ + tps->strobes = devm_kcalloc(&pdev->dev, + TPS65219_NUM_REGULATOR, sizeof(u8), + GFP_KERNEL); + if (!tps->strobes) + return -ENOMEM;
These are never referenced anywhere else in the driver, no need to allocate them.
quoted hunk ↗ jump to hunk
+ ret = regmap_read(tps->regmap, regulators[i].bypass_reg, &val); + if (ret) { + dev_err(tps->dev, "dev_err failed to map register for %s regulator\n", + pdev->name); + return ret; + }
I'm not clear what this check is intended to do, it at least needs to be better documented.