Messages in this thread | | | From | Adam Thomson <> | Subject | RE: [PATCH 1/2] mfd: da9062: enable being a system-power-controller | Date | Thu, 23 Jan 2020 15:51:37 +0000 |
| |
On 07 January 2020 12:06, Helmut Grohne wrote:
> 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 <helmut.grohne@intenta.de> > --- > 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.
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.
> > diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c > index e69626867c26..a2b5dfee677f 100644 > --- a/drivers/mfd/da9062-core.c > +++ b/drivers/mfd/da9062-core.c > @@ -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 > | > + DA9062AA_M_SYSTEM_EN_MASK | > DA9062AA_M_POWER_EN_MASK | > + DA9062AA_M_POWER1_EN_MASK; > + int ret = regmap_update_bits(chip->regmap, DA9062AA_CONTROL_A, > mask, 0); > + > + 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
| |