mfd: da9062: enable being a system-power-controller

The DA9062 can be the device used to power the CPU. In that case, it can be used to power off the system. In the CONTROL_A register, the M_*_EN bits must be zero for the corresponding *_EN bits to have an effect. We zero them all to turn off the system.

Signed-off-by Helmut Grohne <[email protected]>

---
 drivers/mfd/da9062-core.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

This patch includes the functionality into the da9062-core driver. If that is not the preferred way to integrate it, it can be added as a mfd_cell instead. In that case, I can move the functionality to a new drivers/power/reset/da9062-poweroff.c. As far as I can see, doing so implies that we can no longer use the standard system-power-controller property though and must use a new compatible property dlg,da9062-poweroff. Please let me know your preference.

drivers/mfd/da9062-core.c +8 -0

index e69626867c26..a2b5dfee677f 100644
@@ -572,6 +572,23 @@ static const struct of_device_id da9062_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, da9062_dt_ids);
 
+/* Hold client since pm_power_off is global. */
+static struct i2c_client *da9062_i2c_client;
+
+static void da9062_power_off(void)
+{
+	struct da9062 *chip = i2c_get_clientdata(da9062_i2c_client);
+	const unsigned int mask = DA9062AA_SYSTEM_EN_MASK |
+		DA9062AA_POWER_EN_MASK | DA9062AA_POWER1_EN_MASK

|

drivers/mfd/da9062-core.c +1 -0

+		DA9062AA_M_SYSTEM_EN_MASK |

DA9062AA_M_POWER_EN_MASK |

drivers/mfd/da9062-core.c +2 -0

+		DA9062AA_M_POWER1_EN_MASK;
+	int ret = regmap_update_bits(chip->regmap, DA9062AA_CONTROL_A,

mask, 0);

drivers/mfd/da9062-core.c +20 -1

+
+	if (ret < 0)
+		dev_err(&da9062_i2c_client->dev,
+			"DA9062AA_CONTROL_A update failed, %d\n", ret);
+}
+
 static int da9062_i2c_probe(struct i2c_client *i2c,
 	const struct i2c_device_id *id)
 {

@@ -661,6 +678,15 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}
 
+	if (of_device_is_system_power_controller(i2c->dev.of_node)) {
+		if (!pm_power_off) {
+			da9062_i2c_client = i2c;
+			pm_power_off = da9062_power_off;
+		} else {
+			dev_warn(&i2c->dev, "Poweroff callback already
 assigned\n");
+		}
+	}
+
 	return ret;
 }

@@ -668,6 +694,11 @@ static int da9062_i2c_remove(struct i2c_client *i2c)
 {
 	struct da9062 *chip = i2c_get_clientdata(i2c);
 
+	if (pm_power_off == da9062_power_off)
+		pm_power_off = NULL;
+	if (da9062_i2c_client)
+		da9062_i2c_client = NULL;
+
 	mfd_remove_devices(chip->dev);
 	regmap_del_irq_chip(i2c->irq, chip->regmap_irq);
 
--

2.20.1

I have concerns about using regmap/I2C within the pm_power_off() callback function although I am aware there are other examples of this in the kernel. At the point that is called I believe IRQs are disabled so it would require a platform to have an atomic version of the I2C bus's xfer function. Don't know if there's a check to see if the bus supports this, but if not then maybe it's something worth adding? That way we can then only support the pm_power_off() approach on systems which can actually do it.

Hi,

Thank you for reviewing the code.

Quoted 6 lines · Adam Thomson

I have concerns about using regmap/I2C within the pm_power_off() callback function although I am aware there are other examples of this in the kernel. At the point that is called I believe IRQs are disabled so it would require a platform to have an atomic version of the I2C bus's xfer function. Don't know if there's a check to see if the bus supports this, but if not then maybe it's something worth adding? That way we can then only support the

pm_power_off()

Quoted 1 line

approach on systems which can actually do it.

On arm, machine_power_off calls the pm_power_off callback after issuing local_irq_disable() and smp_send_stop(). So I think your intuition is correct that we are running with only one CPU left with IRQs disabled.

I have tested this code on a board with an i2c-cadence bus. This driver seems to use IRQs for completion tracking with no fallback to polling. I'm now puzzled as to why this works at all. Given that I'm using regmap_update_bits on a volatile register, it would have to complete the read before performing the relevant write. Nevertheless, it reliably turns off here. So I'm starting to wonder whether there is a flaw in the analysis.

I also looked into whether linux/i2c.h would tell us about the availability of an atomic xfer function. Indeed, the i2c_algorithm structure has a master_xfer_atomic specifically for this purpose. The i2c core will automatically use this function when irqs are disabled. Unfortunately, very few buses implement this function. In particular, i2c-cadence lacks it.

So I could check for i2c_dev->adapter->algo->master_xfer_atomic != NULL indeed. Possibly this could be wrapped in a central inline function.

I concur that quite a few other drivers use a regmap/i2c from their pm_power_off hook. Examples include:
* arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c (i2c without regmap)
* drivers/mfd/axp20x.c (regmap without i2c)
* drivers/mfd/dm355evm_msp.c (i2c without regmap)
* drivers/mfd/max77620.c (regmap and i2c)
* drivers/mfd/max8907.c (regmap and i2c)
* drivers/mfd/palmas.c (regmap and i2c)
* drivers/mfd/retu-mfd.c (regmap and i2c)
* drivers/mfd/rn5t618.c (regmap and i2c)
* drivers/mfd/tps6586x.c (regmap and i2c)
* drivers/mfd/tps65910.c (regmap and i2c)
* drivers/mfd/tps80031.c (regmap and i2c)
* drivers/mfd/twl4030-power.c (i2c without regmap)
* drivers/regulator/act8865-regulator.c (regmap and i2c)

For this reason, I think the practice of using regmap/i2c within pm_power_off is well-established and should not be questioned for an individual device. In addition, the relevant functionality must be explicitly requested by modifying a board-specific device-tree. It can be assumed that an integrator would test whether the mfd actually works as a power controller when adding the relevant property. Given that we turned off other CPUs and IRQs, the behaviour should be fairly reliable.

I think that requiring atomic transfers for pm_power_off would be relatively easy to implement (for all mfds). However, I also think that it would break a fair number of boards, because so few buses implement atomic transfers. As such, I don't think we can actually require it before requiring all buses to implement atomic transfers. At that point, the check becomes useless, because the i2c core will automatically use atomic transfers during pm_power_off.

Given these reasons (consistency with other drivers, testing and "don't break"), I think that including the functionality without an additional check is a reasonable thing to do.

Helmut

Quoted 29 lines · Helmut Grohne

Quoted 6 lines · Adam Thomson

I have concerns about using regmap/I2C within the pm_power_off() callback function although I am aware there are other examples of this in the kernel. At the point that is called I believe IRQs are disabled so it would require a platform to have an atomic version of the I2C bus's xfer function. Don't know if there's a check to see if the bus supports this, but if not then maybe it's something worth adding? That way we can then only support the

pm_power_off()

Quoted 1 line

approach on systems which can actually do it.

On arm, machine_power_off calls the pm_power_off callback after issuing local_irq_disable() and smp_send_stop(). So I think your intuition is correct that we are running with only one CPU left with IRQs disabled.

I have tested this code on a board with an i2c-cadence bus. This driver seems to use IRQs for completion tracking with no fallback to polling. I'm now puzzled as to why this works at all. Given that I'm using regmap_update_bits on a volatile register, it would have to complete the read before performing the relevant write. Nevertheless, it reliably turns off here. So I'm starting to wonder whether there is a flaw in the analysis.

I also looked into whether linux/i2c.h would tell us about the availability of an atomic xfer function. Indeed, the i2c_algorithm structure has a master_xfer_atomic specifically for this purpose. The i2c core will automatically use this function when irqs are disabled. Unfortunately, very few buses implement this function. In particular, i2c-cadence lacks it.

Quoted 2 lines

So I could check for i2c_dev->adapter->algo->master_xfer_atomic != NULL indeed. Possibly this could be wrapped in a central inline function.

Yes, I'd be tempted to make this a nice wrapper function to hide the particulars, were someone to implement this.

Quoted 17 lines

I concur that quite a few other drivers use a regmap/i2c from their pm_power_off hook. Examples include:
* arch/powerpc/platforms/83xx/mcu_mpc8349emitx.c (i2c without regmap)
* drivers/mfd/axp20x.c (regmap without i2c)
* drivers/mfd/dm355evm_msp.c (i2c without regmap)
* drivers/mfd/max77620.c (regmap and i2c)
* drivers/mfd/max8907.c (regmap and i2c)
* drivers/mfd/palmas.c (regmap and i2c)
* drivers/mfd/retu-mfd.c (regmap and i2c)
* drivers/mfd/rn5t618.c (regmap and i2c)
* drivers/mfd/tps6586x.c (regmap and i2c)
* drivers/mfd/tps65910.c (regmap and i2c)
* drivers/mfd/tps80031.c (regmap and i2c)
* drivers/mfd/twl4030-power.c (i2c without regmap)
* drivers/regulator/act8865-regulator.c (regmap and i2c)

Quoted 7 lines

For this reason, I think the practice of using regmap/i2c within pm_power_off is well-established and should not be questioned for an individual device. In addition, the relevant functionality must be explicitly requested by modifying a board-specific device-tree. It can be assumed that an integrator would test whether the mfd actually works as a power controller when adding the relevant property. Given that we turned off other CPUs and IRQs, the behaviour should be fairly reliable.

I never like assumptions and they tend to catch people out. A lot of the time driver developers will use another as a template/example and so the same possible mistakes can be duplicated. I don't know for certain these are mistakes but the code seems to indicate that could be the case, and there's a good reason that atomic I2C transfer code has been added into the kernel. I also prefer code that helps people identify where a problem might lie so having a check for I2C atomic support would be useful to indicate if there is a problem.

Lee, do you have any further insight into any of this? Am I barking up the wrong tree here?