чт, 20 лип. 2023 р. о 22:43 Simon Glass <s...@chromium.org> пише: > > Hi Svyatoslav, > > On Thu, 20 Jul 2023 at 02:48, Svyatoslav Ryhel <clamo...@gmail.com> wrote: > > > > PMICs are responsible for device poweroff sequence so lets implement > > this function. > > > > Signed-off-by: Svyatoslav Ryhel <clamo...@gmail.com> > > --- > > drivers/power/pmic/pmic-uclass.c | 12 ++++++++++++ > > include/power/pmic.h | 13 +++++++++++++ > > 2 files changed, 25 insertions(+) > > > > diff --git a/drivers/power/pmic/pmic-uclass.c > > b/drivers/power/pmic/pmic-uclass.c > > index 0e2f5e1f41..23803bc96a 100644 > > --- a/drivers/power/pmic/pmic-uclass.c > > +++ b/drivers/power/pmic/pmic-uclass.c > > @@ -99,6 +99,18 @@ int pmic_get(const char *name, struct udevice **devp) > > return uclass_get_device_by_name(UCLASS_PMIC, name, devp); > > } > > > > +int pmic_poweroff(struct udevice *dev) > > +{ > > + const struct dm_pmic_ops *ops = dev_get_driver_ops(dev); > > + struct uc_pmic_priv *priv = dev_get_uclass_priv(dev); > > + > > + if (!ops || !ops->poweroff || > > + !priv->sys_pow_ctrl) > > + return -ENOSYS; > > + > > + return ops->poweroff(dev); > > +} > > + > > int pmic_reg_count(struct udevice *dev) > > { > > const struct dm_pmic_ops *ops = dev_get_driver_ops(dev); > > diff --git a/include/power/pmic.h b/include/power/pmic.h > > index 636221692d..d8dd1ceaa3 100644 > > --- a/include/power/pmic.h > > +++ b/include/power/pmic.h > > @@ -157,11 +157,13 @@ struct pmic { > > * Should be implemented by UCLASS_PMIC device drivers. The standard > > * device operations provides the I/O interface for it's childs. > > * > > + * @poweroff: perform poweroff sequence > > * @reg_count: device's register count > > * @read: read 'len' bytes at "reg" and store it into the 'buffer' > > * @write: write 'len' bytes from the 'buffer' to the register at > > 'reg' address > > */ > > struct dm_pmic_ops { > > + int (*poweroff)(struct udevice *dev); > > Please do remember to fully comment each method here too - see p2sb > set_hide() for an example. >
It is commented in description before ops + * @poweroff: perform poweroff sequence p2sb set_hide() description is nice but it does not fit this case since its description is set before ops. > > int (*reg_count)(struct udevice *dev); > > int (*read)(struct udevice *dev, uint reg, uint8_t *buffer, int > > len); > > int (*write)(struct udevice *dev, uint reg, const uint8_t *buffer, > > @@ -242,6 +244,16 @@ int pmic_bind_children(struct udevice *pmic, ofnode > > parent, > > */ > > int pmic_get(const char *name, struct udevice **devp); > > > > +/** > > + * pmic_poweroff: call the pmic poweroff sequence > > + * > > + * The required pmic device can be obtained by 'pmic_get()' > > + * > > + * @dev - pointer to the UCLASS_PMIC device > > + * Return: device turns off or negative value of errno. > > + */ > > +int pmic_poweroff(struct udevice *dev); > > + > > /** > > * pmic_reg_count: get the pmic register count > > * > > @@ -306,6 +318,7 @@ int pmic_clrsetbits(struct udevice *dev, uint reg, uint > > clr, uint set); > > */ > > struct uc_pmic_priv { > > uint trans_len; > > + bool sys_pow_ctrl; > > comment > > > }; > > > > #endif /* DM_PMIC */ > > -- > > 2.39.2 > > > > This needs a test addition to test/dm/pmic.c (perhaps you have it in > another patch). I knew you would ask for this and I have thought about implementing a dm test but I have faced the next issue: poweroff call (in this context) performs device poweroff on success and -err on failure. Iirc sandbox pmic is not dedicated for poweroff and it can not be "emulated" correctly on sandbox (emulation will be just a passing value like for example pmic_reg_count test). If this is mandatory then tell pls which behavior is expected in the test and I will try to replicate it. > Regards, > Simon