Re: [PATCH 1/2] mfd: da9062: enable being a system-power-controller

[Helmut Grohne <helmut.grohne@intenta.de>]

Hi,

Thank you for reviewing the code.

On Thu, Jan 23, 2020 at 04:51:37PM +0100, Adam Thomson wrote:
> 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.

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