regulator: Add regulator driver for the MC34708 PMIC

Martin Fuzzey <[email protected]> hat am 28. April 2015 um 16:17 geschrieben:

Signed-off-by Martin Fuzzey <[email protected]>

a commit message would be nice and the subject should specify dt-bindings instead of regulator subsystem.

Mark Brown <[email protected]> hat am 1. Mai 2015 um 13:01 geschrieben:

Quoted 2 lines · Stefan Wahren

a commit message would be nice and the subject should specify dt-bindings instead of regulator subsystem.

No, it shouldn't. DT bindings for a subsystem go through the subsystem and the subsystem maintainer needs to see them to read them.

Quoted 6 lines

Mark Brown <[email protected]> hat am 1. Mai 2015 um 13:01 geschrieben:

Quoted 2 lines · Stefan Wahren

a commit message would be nice and the subject should specify dt-bindings instead of regulator subsystem.

Quoted 2 lines

No, it shouldn't. DT bindings for a subsystem go through the subsystem and the subsystem maintainer needs to see them to read them.

sorry for my bad expressing.

I tought the subject line should be something like:

DT: add binding documentation for mc34708 regulator

Documentation/devicetree/bindings/regulator/mc34708-regulator.txt new file +4 -0

+
+
+Example:
+&i2c3 {

+MODULE_LICENSE("GPL v2");

Signed-off-by Martin Fuzzey <[email protected]>

drivers/regulator/mc34708-regulator.c new file +10 -0

+static int mc34708_sw_find_hw_mode_sel(
+	const struct mc34708_regulator_kind *kind,
+	unsigned int normal,
+	unsigned int standby)
+{
+	const struct mc34708_hw_mode *mode;
+	int i;
+
+	for (i = 0, mode = kind->hw_modes;
+			i < kind->num_hw_modes; i++, mode++) {

drivers/regulator/mc34708-regulator.c new file +13 -0

+static int mc34708_swbst_setup(struct mc34708_regulator *mc34708_reg)
+{
+	int val, mode;
+
+	val = mc34708_read_bits(mc34708_reg,
+				mc34708_reg->def->mode_reg,
+				mc34708_reg->def->mode_mask);
+	if (val < 0)
+		return val;
+
+	if (val > 0) {
+		mode = mc34708_swbst_hwmode_to_mode(val);
+		if (mode < 0)

+/*
+ * Setting some LDO standby states also requires changing the normal state.
+ * Therefore save the LDO configuration register on suspend and restore it
+ * on resume.
+ *
+ * This works because .set_suspend_X are called by the platform suspend handler
+ * AFTER device suspend
+ */

Quoted 1 line · Martin Fuzzey

Signed-off-by Martin Fuzzey <[email protected]>

Please use subject lines reflecting the style for the subsystem.

Quoted 7 lines

drivers/regulator/mc34708-regulator.c new file +7 -0

+static int mc34708_read_bits(struct mc34708_regulator *mc34708_reg,
+			     unsigned int reg, u32 mask)
+{
+	int ret;
+	u32 val;
+
+	ret = mc13xxx_reg_read(mc34708_reg->mc13xxx, reg, &val);

drivers/regulator/mc34708-regulator.c line 110

Quoted 5 lines

drivers/regulator/mc34708-regulator.c new file +5 -0

+	if (ret < 0)
+		return ret;
+
+	val &= mask;
+	val >>= ffs(mask) - 1;

Ick, this looks confusing - it's wrapping something which should hopefully be a regmap in multiple layer. The bitfield access helper does seem reasonable though.

drivers/regulator/mc34708-regulator.c line 120

Quoted 6 lines

drivers/regulator/mc34708-regulator.c new file +6 -0

+static int mc34708_update_bits(struct mc34708_regulator *mc34708_reg,
+			       unsigned int reg, u32 mask, u32 val)
+{
+	val <<= ffs(mask) - 1;
+
+	return mc13xxx_reg_rmw(mc34708_reg->mc13xxx, reg, mask, val);

This is a wrapper for the widely used _update_bits() interface which has a different interface - that's *definitely* confusing.

drivers/regulator/mc34708-regulator.c line 129

Quoted 7 lines

drivers/regulator/mc34708-regulator.c new file +7 -0

+static int mc34708_get_voltage_sel(struct regulator_dev *rdev)
+{
+	struct mc34708_regulator *mc34708_reg = rdev->reg_data;
+
+	return mc34708_read_bits(mc34708_reg,
+				rdev->desc->vsel_reg, rdev->desc->vsel_mask);
+}

Don't open code this, use the standard regmap helpers.

drivers/regulator/mc34708-regulator.c line 168

Quoted 8 lines

drivers/regulator/mc34708-regulator.c new file +7 -0

+	val = mc34708_read_bits(mc34708_reg,
+				mc34708_reg->def->mode_reg,
+				mc34708_reg->def->mode_mask);
+	if (val < 0)
+		return ERR_PTR(val);
+
+	BUG_ON(val >= mc34708_reg->def->kind->num_hw_modes);

This is too severe, could be a hardware error.

Quoted 15 lines

drivers/regulator/mc34708-regulator.c new file +15 -0

+static int mc34708_sw_find_hw_mode_sel(
+	const struct mc34708_regulator_kind *kind,
+	unsigned int normal,
+	unsigned int standby)
+{
+	const struct mc34708_hw_mode *mode;
+	int i;
+
+	for (i = 0, mode = kind->hw_modes;
+			i < kind->num_hw_modes; i++, mode++) {
+		if (mode->normal == -1 || mode->standby == -1)
+			continue;
+
+		if (mode->standby != standby)
+			continue;

drivers/regulator/mc34708-regulator.c line 197

Quoted 5 lines

drivers/regulator/mc34708-regulator.c new file +5 -0

+
+		if ((mode->normal == normal) ||
+		    (normal && (mode->alt_normal == normal)))
+			return i;
+	}

I honestly don't really know what the above is supposed to do. It's mapping something to something but what exactly is unclear... skipping most of the rest of the mode code.

drivers/regulator/mc34708-regulator.c line 448

Quoted 6 lines

drivers/regulator/mc34708-regulator.c new file +6 -0

+static int mc34708_swbst_mode_to_hwmode(unsigned int mode)
+{
+	switch (mode) {
+	case REGULATOR_MODE_IDLE:
+	case REGULATOR_MODE_STANDBY:
+		return  MC34708_SW_OPMODE_PFM;

No, this is broken - you're mapping two different modes to one hardware setting. If the device doesn't support something it just doesn't support it, let the upper layers work out how to handle that.

Also an extra space there.

Quoted 17 lines

drivers/regulator/mc34708-regulator.c new file +17 -0

+static int mc34708_swbst_setup(struct mc34708_regulator *mc34708_reg)
+{
+	int val, mode;
+
+	val = mc34708_read_bits(mc34708_reg,
+				mc34708_reg->def->mode_reg,
+				mc34708_reg->def->mode_mask);
+	if (val < 0)
+		return val;
+
+	if (val > 0) {
+		mode = mc34708_swbst_hwmode_to_mode(val);
+		if (mode < 0)
+			return mode;
+
+		mc34708_reg->req_mode_normal = mode;
+	} else {

drivers/regulator/mc34708-regulator.c line 331

Quoted 6 lines

drivers/regulator/mc34708-regulator.c new file +6 -0

+		/*
+		 * If regulator is intially off we don't know the mode
+		 * but we need a mode to be able to enable it later.
+		 */
+		mc34708_reg->req_mode_normal = REGULATOR_MODE_NORMAL;
+	}

I don't really understand what the above is supposed to do, some comments would probably help.

Quoted 4 lines

drivers/regulator/mc34708-regulator.c new file +4 -0

+static int mc34708_ldo_set_suspend_enable(struct regulator_dev *rdev)
+{
+	struct mc34708_regulator *mc34708_reg = rdev->reg_data;
+	int ret;

drivers/regulator/mc34708-regulator.c line 401

Quoted 6 lines

drivers/regulator/mc34708-regulator.c new file +6 -0

+
+	ret = mc34708_update_hw_mode(mc34708_reg,
+				     mc34708_reg->req_mode_normal,
+				     mc34708_reg->req_mode_standby);
+	if (!ret)
+		mc34708_reg->suspend_off = false;

This looks like it should be a noop. It also seems very familiar from some of the other code.

drivers/regulator/mc34708-regulator.c line 833

Quoted 8 lines

drivers/regulator/mc34708-regulator.c new file +8 -0

+static int mc34708_ldo_set_suspend_mode(struct regulator_dev *rdev,
+					unsigned int mode)
+{
+	struct mc34708_regulator *mc34708_reg = rdev->reg_data;
+	int ret;
+
+	if (!mc34708_ldo_has_mode_bit(mc34708_reg))
+		return 0;

Again, if the driver doesn't support something don't implement it.

drivers/regulator/mc34708-regulator.c line 943

Quoted 3 lines

drivers/regulator/mc34708-regulator.c new file +3 -0

+static const unsigned int mc34708_vgen1_volt_table[] = {
+	1200000, 1250000, 1300000, 1350000, 1400000, 1450000, 1500000, 1550000
+};

This looks like a linear mapping, use the standard helpers please.

Quoted 8 lines

+/*
+ * Setting some LDO standby states also requires changing the normal state.
+ * Therefore save the LDO configuration register on suspend and restore it
+ * on resume.
+ *
+ * This works because .set_suspend_X are called by the platform suspend handler
+ * AFTER device suspend
+ */

That's not something you can rely on, I suggest omitting this for now and doing it separately.

drivers/regulator/mc34708-regulator.c line 1170

Quoted 13 lines

drivers/regulator/mc34708-regulator.c new file +13 -0

+	num_regs = of_get_child_count(regs_np);
+	mc34708_data = devm_kzalloc(dev, sizeof(*mc34708_data) +
+					num_regs * sizeof(*mc34708_reg),
+					GFP_KERNEL);
+	if (!mc34708_data) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	dev_set_drvdata(dev, mc34708_data);
+
+	mc34708_reg = mc34708_data->regulators;
+	for_each_child_of_node(regs_np, reg_np) {

No, this is broken - your driver should like all the others register all the regulators on the device uncondtionally. It should also be using .of_match to allow the core to look up the init data.

Quoted 3 lines · Martin Fuzzey

Quoted 1 line

Signed-off-by Martin Fuzzey <[email protected]>

Please use subject lines reflecting the style for the subsystem.

You mean
regulator: mc34708: Add driver?

I ommitted the mc34708 part because it's a new driver

Quoted 3 lines

Ick, this looks confusing - it's wrapping something which should hopefully be a regmap in multiple layer. The bitfield access helper does seem reasonable though.

The reason for this wrapping stuff is that this is not a standalone driver, rather a "subdriver" of the mc13xxx MFD driver.
That already exists and supports the mc34708 (for the RTC for example) but not yet the regulator parts.
It does not expose regmap (although it does use it internally).

Would you be happier if I rename this to mc34708_set_bitfield() for example?

Then the way I handle all this is just use mc34708_sw_find_hw_mode_sel() to recalculate the hardware mode by walking the tables every time something needs to change.
The table is different depending on the regulator type.

This turned out to be *much* simpler that the initial open code approach I first tried.

Quoted 2 lines

I don't really understand what the above is supposed to do, some comments would probably help.

We need to store the desired modes (for normal and standby state) since .set_mode() and .set_suspend_mode() are not always called before enable() / disable().

Martin Fuzzey <[email protected]> hat am 28. April 2015 um 16:17 geschrieben:

[...]

drivers/regulator/mc34708-regulator.c new file +0 -0

new file mode 100644
index 0000000..b5ff727

Martin Fuzzey <[email protected]> hat am 28. April 2015 um 16:17 geschrieben:

Add regulator driver and associated DT bindings for the regulators in the Freescale MC34708 PMIC.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel