[PATCH v2 1/9] power: pmic-uclass: implement poweroff ops

[Svyatoslav Ryhel]

чт, 20 лип. 2023 р. о 22:43 Simon Glass <sjg at chromium.org> пише:
>
> Hi Svyatoslav,
>
> On Thu, 20 Jul 2023 at 02:48, Svyatoslav Ryhel <clamor95 at gmail.com> wrote:
> >
> > PMICs are responsible for device poweroff sequence so lets implement
> > this function.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95 at 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