lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHp75VcHeiQgvZ5e+Dz+gpKghCo5RSTQLsyHGGSgdVQbVu2t+g@mail.gmail.com>
Date:   Wed, 7 Apr 2021 16:21:45 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc:     Matti Vaittinen <mazziesaccount@...il.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-power <linux-power@...rohmeurope.com>,
        linux-arm-msm@...r.kernel.org,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH v6 3/8] regulator: IRQ based event/error notification helpers

On Wed, Apr 7, 2021 at 1:04 PM Matti Vaittinen
<matti.vaittinen@...rohmeurope.com> wrote:
>
> Provide helper function for IC's implementing regulator notifications
> when an IRQ fires. The helper also works for IRQs which can not be acked.

> Helper can be set to disable the IRQ at handler and then re-enabling it
> on delayed work later. The helper also adds regulator_get_error_flags()
> errors in cache for the duration of IRQ disabling.

Thanks for an update, my comments below. After addressing them, feel free to add
Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>

> Signed-off-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
>
> ---
> v6 (fix issues noted by Andy):
> - remove unnecessary variable
> - use BIT(foo) instead of 1 << foo
> - use devm_add_action_or_reset()
> - do not check the irq parameter validity, leave that to
>   request_threaded_irq()
> - put resource-managed function in devres.c
> - fix the kerneldocs for the new IRQ helpers
> v5:
>  - fix the pr_emerg print
> v4:
>  - Comment block styling
>  - Added prints to point the potential HW failure before BUG()
>  - Corrected typo from kerneldoc
>  - added missing newlines
> ---
>  drivers/regulator/Makefile       |   2 +-
>  drivers/regulator/core.c         |  24 +-
>  drivers/regulator/devres.c       |  49 ++++
>  drivers/regulator/irq_helpers.c  | 396 +++++++++++++++++++++++++++++++
>  include/linux/regulator/driver.h | 135 +++++++++++
>  5 files changed, 602 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/regulator/irq_helpers.c
>
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 44d2f8bf4b74..e25f1c2d6c9b 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -4,7 +4,7 @@
>  #
>
>
> -obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o helpers.o devres.o
> +obj-$(CONFIG_REGULATOR) += core.o dummy.o fixed-helper.o helpers.o devres.o irq_helpers.o
>  obj-$(CONFIG_OF) += of_regulator.o
>  obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
>  obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 16114aea099a..fabc83288e1b 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -4388,20 +4388,37 @@ unsigned int regulator_get_mode(struct regulator *regulator)
>  }
>  EXPORT_SYMBOL_GPL(regulator_get_mode);
>
> +static int rdev_get_cached_err_flags(struct regulator_dev *rdev)
> +{
> +       int ret = 0;
> +
> +       if (rdev->use_cached_err) {
> +               spin_lock(&rdev->err_lock);
> +               ret = rdev->cached_err;
> +               spin_unlock(&rdev->err_lock);
> +       }
> +       return ret;

Not critical, but I would rather have it this way

if (! bla bla bla) // space after ! on visibility purpose
  return 0;

spin lock
ret =
spin unlock

return ret;

And drop ret = 0, the main reason why I pointed to this. We have a lot
of issues of hiding (potential) errors due to global assignments like
this. I saw some problematic patches that people unthoughtfully put
such an assignment to shut the compiler up about uninitialized
variables.

however it's not critical per se, can be refactored later.

> +}
> +
>  static int _regulator_get_error_flags(struct regulator_dev *rdev,
>                                         unsigned int *flags)
>  {
> -       int ret;
> +       int ret, tmpret;
>
>         regulator_lock(rdev);
>
> +       ret = rdev_get_cached_err_flags(rdev);
> +
>         /* sanity check */
> -       if (!rdev->desc->ops->get_error_flags) {
> +       if (rdev->desc->ops->get_error_flags) {
> +               tmpret = rdev->desc->ops->get_error_flags(rdev, flags);
> +               if (tmpret > 0)

> +                       ret |= tmpret;

Oh, I don't like this. Easy fix is to rename ret (okay, it's been used
elsewhere, so adding then) to something meaningful, like error_flags,
then you can easily understand that value should be positive and error
codes are negative.

> +       } else if (!rdev->use_cached_err) {
>                 ret = -EINVAL;
>                 goto out;
>         }
>
> -       ret = rdev->desc->ops->get_error_flags(rdev, flags);
>  out:
>         regulator_unlock(rdev);
>         return ret;
> @@ -5236,6 +5253,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
>                 goto rinse;
>         }
>         device_initialize(&rdev->dev);
> +       spin_lock_init(&rdev->err_lock);
>
>         /*
>          * Duplicate the config so the driver could override it after
> diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
> index 3091210889e3..127262dcd3f7 100644
> --- a/drivers/regulator/devres.c
> +++ b/drivers/regulator/devres.c
> @@ -481,3 +481,52 @@ void devm_regulator_unregister_notifier(struct regulator *regulator,
>                 WARN_ON(rc);
>  }
>  EXPORT_SYMBOL_GPL(devm_regulator_unregister_notifier);
> +
> +static void regulator_irq_helper_drop(void *res)
> +{
> +       regulator_irq_helper_cancel(&res);
> +}
> +
> +/**
> + * devm_regulator_irq_helper - resource managed registration of IRQ based
> + * regulator event/error notifier
> + *
> + * @dev:               device to which lifetime the helper's lifetime is
> + *                     bound.
> + * @d:                 IRQ helper descriptor.
> + * @irq:               IRQ used to inform events/errors to be notified.
> + * @irq_flags:         Extra IRQ flags to be OR's with the default IRQF_ONESHOT

OR'ed

> + *                     when requesting the (threaded) irq.
> + * @common_errs:       Errors which can be flagged by this IRQ for all rdevs.
> + *                     When IRQ is re-enabled these errors will be cleared
> + *                     from all associated regulators
> + * @per_rdev_errs:     Optional error flag array describing errors specific
> + *                     for only some of the regulators. These errors will be
> + *                     or'ed with common errors. If this is given the array
> + *                     should contain rdev_amount flags. Can be set to NULL
> + *                     if there is no regulator specific error flags for this
> + *                     IRQ.
> + * @rdev:              Array of regulators associated with this IRQ.
> + * @rdev_amount:       Amount of regulators associated wit this IRQ.

wit -> with

Can you describe, please, the return value meaning. It will be good
also to move detailed descriptions (expectations?) of the fields to
the Description section, here.

> + */
> +void *devm_regulator_irq_helper(struct device *dev,
> +                               const struct regulator_irq_desc *d, int irq,
> +                               int irq_flags, int common_errs,
> +                               int *per_rdev_errs,

> +                               struct regulator_dev **rdev, int rdev_amount)

I didn't get why you need the ** pointer instead of plain pointer.

> +{
> +       void *ptr;
> +       int ret;
> +
> +       ptr = regulator_irq_helper(dev, d, irq, irq_flags, common_errs,
> +                                   per_rdev_errs, rdev, rdev_amount);
> +       if (IS_ERR(ptr))
> +               return ptr;

> +       ret = devm_add_action_or_reset(dev, regulator_irq_helper_drop, ptr);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       return ptr;

> +}
> +EXPORT_SYMBOL_GPL(devm_regulator_irq_helper);
> diff --git a/drivers/regulator/irq_helpers.c b/drivers/regulator/irq_helpers.c
> new file mode 100644
> index 000000000000..374ff0f3c6d3
> --- /dev/null
> +++ b/drivers/regulator/irq_helpers.c
> @@ -0,0 +1,396 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2021 ROHM Semiconductors
> +// regulator IRQ based event notification helpers
> +//
> +// Logic has been partially adapted from qcom-labibb driver.
> +//
> +// Author: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>

> +#include <linux/of_irq.h>

Not sure how this header is used. I haven't found any direct users of
it. Perhaps you wanted interrupt.h?

> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>

+ Blank line? I would separate group of generic headers with
particular to the subsystem

> +#include <linux/regulator/driver.h>
> +
> +struct regulator_irq {
> +       struct regulator_irq_data rdata;
> +       struct regulator_irq_desc desc;
> +       int irq;
> +       int retry_cnt;
> +       struct delayed_work isr_work;
> +};
> +
> +/*
> + * Should only be called from threaded handler to prevent potential deadlock
> + */
> +static void rdev_flag_err(struct regulator_dev *rdev, int err)
> +{
> +       spin_lock(&rdev->err_lock);
> +       rdev->cached_err |= err;
> +       spin_unlock(&rdev->err_lock);
> +}
> +
> +static void rdev_clear_err(struct regulator_dev *rdev, int err)
> +{
> +       spin_lock(&rdev->err_lock);
> +       rdev->cached_err &= ~err;
> +       spin_unlock(&rdev->err_lock);
> +}
> +
> +static void die_loudly(const char *msg)

I feel that this is a too generic name, perhaps rdev_die_loudly().

> +{
> +       pr_emerg("%s\n", msg);
> +       BUG();
> +}
> +
> +static void regulator_notifier_isr_work(struct work_struct *work)
> +{
> +       struct regulator_irq *h;
> +       struct regulator_irq_desc *d;
> +       struct regulator_irq_data *rid;
> +       int ret = 0;
> +       int tmo, i;
> +       int num_rdevs;
> +
> +       h = container_of(work, struct regulator_irq,
> +                           isr_work.work);
> +       d = &h->desc;
> +       rid = &h->rdata;
> +       num_rdevs = rid->num_states;
> +
> +reread:
> +       if (d->fatal_cnt && h->retry_cnt > d->fatal_cnt) {
> +               if (d->die)
> +                       ret = d->die(rid);
> +               else
> +                       die_loudly("Regulator HW failure? - no IC recovery");
> +
> +               /*
> +                * If the 'last resort' IC recovery failed we will have
> +                * nothing else left to do...
> +                */
> +               if (ret)
> +                       die_loudly("Regulator HW failure? - IC recovery failed");

Looking at the above code this will be executed if and only if
d->die() is defined, correct?
In that case, why not

if (d->die) {
  ret = ...
  if (ret)
   rdev_die_loudly(...);
} else
   rdev_die_loudly(...);

?

> +               /*
> +                * If h->die() was implemented we assume recovery has been
> +                * attempted (probably regulator was shut down)
> +                * and we just enable IRQ and bail-out.
> +                */
> +               goto enable_out;
> +       }
> +       if (d->renable) {
> +               ret = d->renable(rid);
> +
> +               if (ret == REGULATOR_FAILED_RETRY) {
> +                       /* Driver could not get current status */
> +                       h->retry_cnt++;
> +                       if (!d->reread_ms)
> +                               goto reread;
> +
> +                       tmo = d->reread_ms;
> +                       goto reschedule;
> +               }
> +
> +               if (ret) {
> +                       /*
> +                        * IC status reading succeeded. update error info
> +                        * just in case the renable changed it.
> +                        */
> +                       for (i = 0; i < num_rdevs; i++) {
> +                               struct regulator_err_state *stat;
> +                               struct regulator_dev *rdev;
> +
> +                               stat = &rid->states[i];
> +                               rdev = stat->rdev;
> +                               rdev_clear_err(rdev, (~stat->errors) &
> +                                                     stat->possible_errs);
> +                       }
> +                       h->retry_cnt++;
> +                       /*
> +                        * The IC indicated problem is still ON - no point in
> +                        * re-enabling the IRQ. Retry later.
> +                        */
> +                       tmo = d->irq_off_ms;
> +                       goto reschedule;
> +               }
> +       }
> +
> +       /*
> +        * Either IC reported problem cleared or no status checker was provided.
> +        * If problems are gone - good. If not - then the IRQ will fire again
> +        * and we'll have new nice loop. In any case we should clear error flags

have a new

> +        * here and re-enable IRQs.
> +        */
> +       for (i = 0; i < num_rdevs; i++) {
> +               struct regulator_err_state *stat;
> +               struct regulator_dev *rdev;
> +
> +               stat = &rid->states[i];
> +               rdev = stat->rdev;
> +               rdev_clear_err(rdev, stat->possible_errs);
> +       }
> +
> +       /*
> +        * Things have been seemingly successful => zero retry-counter.
> +        */
> +       h->retry_cnt = 0;
> +
> +enable_out:
> +       enable_irq(h->irq);
> +
> +       return;
> +
> +reschedule:
> +       if (!d->high_prio)
> +               mod_delayed_work(system_wq, &h->isr_work,
> +                                msecs_to_jiffies(tmo));
> +       else
> +               mod_delayed_work(system_highpri_wq, &h->isr_work,
> +                                msecs_to_jiffies(tmo));
> +}
> +
> +static irqreturn_t regulator_notifier_isr(int irq, void *data)
> +{
> +       struct regulator_irq *h = data;
> +       struct regulator_irq_desc *d;
> +       struct regulator_irq_data *rid;
> +       unsigned long rdev_map = 0;
> +       int num_rdevs;
> +       int ret, i, j;
> +
> +       d = &h->desc;
> +       rid = &h->rdata;
> +       num_rdevs = rid->num_states;
> +
> +       if (d->fatal_cnt)
> +               h->retry_cnt++;
> +
> +       /*
> +        * we spare few cycles by not clearing statuses prior this call.

We spare a few

prior to this

> +        * The IC driver must initialize the status buffers for rdevs
> +        * which it indicates having active events via rdev_map.
> +        *
> +        * Maybe we should just to be on a safer side(?)
> +        */
> +       ret = d->map_event(irq, rid, &rdev_map);
> +
> +       /*
> +        * If status reading fails (which is unlikely) we don't ack/disable
> +        * IRQ but just increase fail count and retry when IRQ fires again.
> +        * If retry_count exceeds given safety limit we call IC specific die

exceeds the given

> +        * handler which can try disabling regulator(s).
> +        *
> +        * If no die handler is given we will just bug() as a last resort.
> +        *
> +        * We could try disabling all associated rdevs - but we might shoot
> +        * ourself in the head and leave problematic regulator enabled. So

ourselves
the problematic regulator

> +        * if IC has no die-handler populated we just assume the regulator
> +        * can't be disabled.
> +        */
> +       if (unlikely(ret == REGULATOR_FAILED_RETRY))
> +               goto fail_out;
> +
> +       h->retry_cnt = 0;
> +       /*
> +        * Let's not disable IRQ if there was no status bits for us. We'd

were

> +        * better leave spurious IRQ handling to genirq
> +        */
> +       if (ret || !rdev_map)
> +               return IRQ_NONE;
> +
> +       /*
> +        * Some events are bogus if regulator is disabled. Skip such events

if the regulator

> +        * if all relevant regulators are disabled
> +        */
> +       if (d->skip_off) {
> +               for_each_set_bit(i, &rdev_map, num_rdevs) {
> +                       struct regulator_dev *rdev;
> +                       const struct regulator_ops *ops;
> +
> +                       rdev = rid->states[i].rdev;
> +                       ops = rdev->desc->ops;
> +
> +                       /*
> +                        * If any of the flagged regulators is enabled we do
> +                        * handle this
> +                        */
> +                       if (ops->is_enabled(rdev))
> +                               break;
> +               }
> +               if (i == num_rdevs)
> +                       return IRQ_NONE;
> +       }
> +
> +       /* Disable IRQ if HW keeps line asserted */
> +       if (d->irq_off_ms)
> +               disable_irq_nosync(irq);
> +
> +       /*
> +        * IRQ seems to be for us. Let's fire correct notifiers / store error
> +        * flags
> +        */
> +       for_each_set_bit(i, &rdev_map, num_rdevs) {
> +               struct regulator_err_state *stat;

> +               int len;

Redundant, see below.

> +               struct regulator_dev *rdev;
> +
> +               stat = &rid->states[i];
> +               len = sizeof(stat->notifs);
> +
> +               rdev = stat->rdev;

> +               for_each_set_bit(j, &stat->notifs, 8 * len)

BITS_PER_TYPE(stat->notifs)

> +                       regulator_notifier_call_chain(rdev, BIT(j - 1), NULL);
> +
> +               rdev_flag_err(rdev, stat->errors);
> +       }
> +
> +       if (d->irq_off_ms) {
> +               if (!d->high_prio)
> +                       schedule_delayed_work(&h->isr_work,
> +                                             msecs_to_jiffies(d->irq_off_ms));
> +               else
> +                       mod_delayed_work(system_highpri_wq,
> +                                        &h->isr_work,
> +                                        msecs_to_jiffies(d->irq_off_ms));
> +       }
> +
> +       return IRQ_HANDLED;
> +
> +fail_out:
> +       if (d->fatal_cnt && h->retry_cnt > d->fatal_cnt) {
> +               if (d->die)
> +                       ret = d->die(rid);
> +
> +               /*
> +                * If die() failed or was not implemented just BUG() as last
> +                * attemt to save HW.
> +                */
> +               if (ret)
> +                       die_loudly("Regulator HW failure? - retry count exceeded");
> +       }
> +
> +       return IRQ_NONE;
> +}
> +
> +static int init_rdev_state(struct device *dev, struct regulator_irq *h,
> +                          struct regulator_dev **rdev, int common_err,
> +                          int *rdev_err, int rdev_amount)
> +{
> +       int i;
> +
> +       h->rdata.states = devm_kzalloc(dev, sizeof(*h->rdata.states) *
> +                                      rdev_amount, GFP_KERNEL);
> +       if (!h->rdata.states)
> +               return -ENOMEM;
> +
> +       h->rdata.num_states = rdev_amount;
> +       h->rdata.data = h->desc.data;
> +
> +       for (i = 0; i < rdev_amount; i++) {
> +               h->rdata.states[i].possible_errs = common_err;
> +               if (rdev_err)
> +                       h->rdata.states[i].possible_errs |= *rdev_err++;
> +               h->rdata.states[i].rdev = *rdev++;
> +       }
> +
> +       return 0;
> +}
> +
> +static void init_rdev_errors(struct regulator_irq *h)
> +{
> +       int i;
> +
> +       for (i = 0; i < h->rdata.num_states; i++) {
> +               if (h->rdata.states[i].possible_errs)
> +                       /* Can we trust writing this boolean is atomic? */

No. boolean is a compiler / platform specific and it may potentially
be written in a non-atomic way.

> +                       h->rdata.states[i].rdev->use_cached_err = true;
> +       }
> +}
> +
> +/**
> + * regulator_irq_helper - register IRQ based regulator event/error notifier
> + *
> + * @dev:               device providing the IRQs
> + * @d:                 IRQ helper descriptor.
> + * @irq:               IRQ used to inform events/errors to be notified.
> + * @irq_flags:         Extra IRQ flags to be OR's with the default IRQF_ONESHOT

OR'ed

> + *                     when requesting the (threaded) irq.
> + * @common_errs:       Errors which can be flagged by this IRQ for all rdevs.
> + *                     When IRQ is re-enabled these errors will be cleared
> + *                     from all associated regulators
> + * @per_rdev_errs:     Optional error flag array describing errors specific
> + *                     for only some of the regulators. These errors will be
> + *                     or'ed with common errors. If this is given the array
> + *                     should contain rdev_amount flags. Can be set to NULL
> + *                     if there is no regulator specific error flags for this
> + *                     IRQ.
> + * @rdev:              Array of regulators associated with this IRQ.
> + * @rdev_amount:       Amount of regulators associated wit this IRQ.
> + */
> +void *regulator_irq_helper(struct device *dev,
> +                          const struct regulator_irq_desc *d, int irq,
> +                          int irq_flags, int common_errs, int *per_rdev_errs,
> +                          struct regulator_dev **rdev, int rdev_amount)
> +{
> +       struct regulator_irq *h;
> +       int ret;
> +
> +       if (!rdev_amount || !d || !d->map_event || !d->name)
> +               return ERR_PTR(-EINVAL);
> +
> +       h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL);
> +       if (!h)
> +               return ERR_PTR(-ENOMEM);
> +
> +       h->irq = irq;
> +       h->desc = *d;
> +
> +       ret = init_rdev_state(dev, h, rdev, common_errs, per_rdev_errs,
> +                             rdev_amount);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       init_rdev_errors(h);
> +
> +       if (h->desc.irq_off_ms)
> +               INIT_DELAYED_WORK(&h->isr_work, regulator_notifier_isr_work);
> +
> +       ret = request_threaded_irq(h->irq, NULL, regulator_notifier_isr,
> +                                  IRQF_ONESHOT | irq_flags, h->desc.name, h);
> +       if (ret) {
> +               dev_err(dev, "Failed to request IRQ %d\n", irq);
> +
> +               return ERR_PTR(ret);
> +       }
> +
> +       return h;
> +}
> +EXPORT_SYMBOL_GPL(regulator_irq_helper);
> +
> +/**
> + * regulator_irq_helper_cancel - drop IRQ based regulator event/error notifier
> + *
> + * @handle:            Pointer to handle returned by a successful call to
> + *                     regulator_irq_helper(). Will be NULLed upon return.
> + *
> + * The associated IRQ is released and work is cancelled when the function
> + * returns.
> + */
> +void regulator_irq_helper_cancel(void **handle)
> +{
> +       if (handle && *handle) {
> +               struct regulator_irq *h = *handle;
> +
> +               free_irq(h->irq, h);
> +               if (h->desc.irq_off_ms)
> +                       cancel_delayed_work_sync(&h->isr_work);
> +
> +               h = NULL;
> +       }
> +}
> +EXPORT_SYMBOL_GPL(regulator_irq_helper_cancel);
> diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
> index d7c77ee370f3..03a8eee9fca9 100644
> --- a/include/linux/regulator/driver.h
> +++ b/include/linux/regulator/driver.h
> @@ -409,6 +409,128 @@ struct regulator_config {
>         struct gpio_desc *ena_gpiod;
>  };
>
> +/**
> + * struct regulator_err_state - regulator error/notification status
> + *
> + * @rdev:              Regulator which status the struct indicates.
> + * @notifs:            Events which have occurred on regulator.

on the regulator

> + * @errors:            Errors which are active on regulator.
> + * @possible_errs:     Errors which can be signaled (by given IRQ).
> + */
> +struct regulator_err_state {
> +       struct regulator_dev *rdev;
> +       unsigned long notifs;
> +       unsigned long errors;
> +       int possible_errs;
> +};
> +
> +/**
> + * struct regulator_irq_data - regulator error/notification status date
> + *
> + * @states:    Status structs for each of the associated regulators.
> + * @num_states:        Amount of associated regulators.
> + * @data:      Driver data pointer given at regulator_irq_desc.
> + * @opaque:    Value storage for IC driver. Core does not update this. ICs
> + *             may want to store status register value here at map_event and
> + *             compare contents at renable to see if new problems have been

re-enable / reenable

> + *             added to status. If that is the case it may be desirable to
> + *             return REGULATOR_ERROR_CLEARED and not REGULATOR_ERROR_ON to
> + *             allow IRQ fire again and to generate notifications also for
> + *             the new issues.
> + *
> + * This structure is passed to map_event and renable for reporting regulator

Ditto.

> + * status to core.
> + */
> +struct regulator_irq_data {
> +       struct regulator_err_state *states;
> +       int num_states;
> +       void *data;
> +       long opaque;
> +};
> +
> +/**
> + * struct regulator_irq_desc - notification sender for IRQ based events.
> + *
> + * @name:      The visible name for the IRQ
> + * @fatal_cnt: If this IRQ is used to signal HW damaging condition it may be
> + *             best to shut-down regulator(s) or reboot the SOC if error
> + *             handling is repeteadly failing. If fatal_cnt is given the IRQ

repeatedly

> + *             handling is aborted if it fails for fatal_cnt times and die()
> + *             callback (if populated) or BUG() is called to try to prevent
> + *             further damage.
> + * @reread_ms: The time which is waited before attempting to re-read status
> + *             at the worker if IC reading fails. Immediate re-read is done
> + *             if time is not specified.
> + * @irq_off_ms:        The time which IRQ is kept disabled before re-evaluating the
> + *             status for devices which keep IRQ disabled for duration of the
> + *             error. If this is not given the IRQ is left enabled and renable
> + *             is not called.
> + * @skip_off:  If set to true the IRQ handler will attempt to check if any of
> + *             the associated regulators are enabled prior to taking other
> + *             actions. If no regulators are enabled and this is set to true
> + *             a spurious IRQ is assumed and IRQ_NONE is returned.
> + * @high_prio: Boolean to indicate that high priority WQ should be used.
> + * @data:      Driver private data pointer which will be passed as such to
> + *             the renable, map_event and die callbacks in regulator_irq_data.
> + * @die:       Protection callback. If IC status reading or recovery actions
> + *             fail fatal_cnt times this callback or BUG() is called. This
> + *             callback should implement final protection attempt like

implement a final

> + *             disabling the regulator. If protection succeeded this may
> + *             return 0. If anything else is returned the core assumes final
> + *             protection failed and calls BUG() as a last resort.
> + * @map_event: Driver callback to map IRQ status into regulator devices with
> + *             events / errors. NOTE: callback MUST initialize both the
> + *             errors and notifs for all rdevs which it signals having
> + *             active events as core does not clean the map data.
> + *             REGULATOR_FAILED_RETRY can be returned to indicate that the
> + *             status reading from IC failed. If this is repeated for
> + *             fatal_cnt times the core will call die() callback or BUG()
> + *             as a last resort to protect the HW.
> + * @renable:   Optional callback to check status (if HW supports that) before
> + *             re-enabling IRQ. If implemented this should clear the error
> + *             flags so that errors fetched by regulator_get_error_flags()
> + *             are updated. If callback is not impelemted then errors are

implemented

> + *             assumed to be cleared and IRQ is re-enabled.
> + *             REGULATOR_FAILED_RETRY can be returned to
> + *             indicate that the status reading from IC failed. If this is
> + *             repeated for 'fatal_cnt' times the core will call die()
> + *             callback or BUG() as a last resort to protect the HW.
> + *             Returning zero indicates that the problem in HW has been solved
> + *             and IRQ will be re-enabled. Returning REGULATOR_ERROR_ON
> + *             indicates the error condition is still active and keeps IRQ
> + *             disabled. Please note that returning REGULATOR_ERROR_ON does
> + *             not retrigger evaluating what events are active or resending
> + *             notifications. If this is needed you probably want to return
> + *             zero and allow IRQ to retrigger causing events to be
> + *             re-evaluated and re-sent.
> + *
> + * This structure is used for registering regulator IRQ notification helper.
> + */
> +struct regulator_irq_desc {
> +       const char *name;
> +       int irq_flags;
> +       int fatal_cnt;
> +       int reread_ms;
> +       int irq_off_ms;
> +       bool skip_off;
> +       bool high_prio;
> +       void *data;
> +
> +       int (*die)(struct regulator_irq_data *rid);
> +       int (*map_event)(int irq, struct regulator_irq_data *rid,
> +                         unsigned long *dev_mask);
> +       int (*renable)(struct regulator_irq_data *rid);
> +};
> +
> +/*
> + * Return values for regulator IRQ helpers.
> + */
> +enum {
> +       REGULATOR_ERROR_CLEARED,
> +       REGULATOR_FAILED_RETRY,
> +       REGULATOR_ERROR_ON,
> +};
> +
>  /*
>   * struct coupling_desc
>   *
> @@ -473,6 +595,9 @@ struct regulator_dev {
>
>         /* time when this regulator was disabled last time */
>         unsigned long last_off_jiffy;
> +       int cached_err;
> +       bool use_cached_err;
> +       spinlock_t err_lock;
>  };
>
>  struct regulator_dev *
> @@ -487,6 +612,16 @@ void devm_regulator_unregister(struct device *dev, struct regulator_dev *rdev);
>
>  int regulator_notifier_call_chain(struct regulator_dev *rdev,
>                                   unsigned long event, void *data);
> +void *devm_regulator_irq_helper(struct device *dev,
> +                               const struct regulator_irq_desc *d, int irq,
> +                               int irq_flags, int common_errs,
> +                               int *per_rdev_errs, struct regulator_dev **rdev,
> +                               int rdev_amount);
> +void *regulator_irq_helper(struct device *dev,
> +                          const struct regulator_irq_desc *d, int irq,
> +                          int irq_flags, int common_errs, int *per_rdev_errs,
> +                          struct regulator_dev **rdev, int rdev_amount);
> +void regulator_irq_helper_cancel(void **handle);
>
>  void *rdev_get_drvdata(struct regulator_dev *rdev);
>  struct device *rdev_get_dev(struct regulator_dev *rdev);
> --
> 2.25.4
>
>
> --
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
>
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =]



-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ