| Message ID | cover.1617789229.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| Headers | show |
| Series | Extend regulator notification support | expand |
On Wed, Apr 7, 2021 at 1:04 PM Matti Vaittinen <matti.vaittinen@fi.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@gmail.com> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.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@fi.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 =]
Hello Andy, All. On Wed, 2021-04-07 at 16:21 +0300, Andy Shevchenko wrote: > On Wed, Apr 7, 2021 at 1:04 PM Matti Vaittinen > <matti.vaittinen@fi.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@gmail.com> > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > > > 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. No wonder if this looks hairy. I think I have got this plain wrong. The rdev_get_cached_err_flags() is not updating the flags. Looks like just plain mistake from my side. I think I've mixed the returning flags via parameter and return value. This must be fixed. Well spotted. > + */ > > +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. We have an array of pointers. And we give a pointer to the first pointer. Maybe it's the lack of coffee but I don't see why a single pointer would be correct? rdev structures are not in contagious memory, pointers to rdevs are. So we need address of first pointer, right? +#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? Thanks. I think this specific header may be a leftover from first draft where I thought I'll use named IRQs. The header was for of_irq_get_byname(). That ended up as a mess for everything else but platform devices :) I'll check the headers, thanks. > > +#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 I don't see this being used in regulator subsystem - and to tell the truth, I don't really see the value. > > +#include <linux/regulator/driver.h> ... > > + > > +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(...); > > ? I think this should simply be: if (!d->die) die_loudly("Regulator HW failure? - no IC recovery"); ret = d->die(rdev); if (ret) die_loudly(...); ... > > +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. Hmm.. I don't think this really is a problem here. We only use the use_cached_err for true/false evaluation - and if error getting api is called after the boolean is changed - then cached error is used, if before, then it is not used. Even if the value of the boolean was read in the middle of writing it, it will still evaluate either true or false - there is no 'maybe' state :) My point, I guess we can do the change without locking here. Please correct me if I am wrong. I'll just drop this comment. > > 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. the "renable" is referring to the callback function pointer which is named "renable". Best Regards -- Matti Vaittinen
Hello Andy, On Wed, 2021-04-07 at 16:21 +0300, Andy Shevchenko wrote: > On Wed, Apr 7, 2021 at 1:04 PM Matti Vaittinen > <matti.vaittinen@fi.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@gmail.com> I (eventually) disagreed with couple of points here and haven't changed those. Please see list below. I still do think you did a really good job reviewing this - and you should get the recognition from that work. Thus I'd nevertheless would like to add your Reviewed-by to the next version. Please let me know if you think it's ok. (I have the v7 ready but I'll wait until the next Monday before sending it to see if this brings more discussion). > > +/** > > + * 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 > > + * 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. > > 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. I added the return-value documentation as you suggested. For parameter descriptions I still think the best and clearest place is in parameter description. > > > + */ > > +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. > This I replied to earlier - I did change the parameter documentation a bit to explain this: "@rdev: Array of pointers to regulators associated with this IRQ" > > +#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 I haven't seen this practice in other parts of regulator subsystem (and I personally fail to see the value). > > +/** > > + * 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. 'renable' refers to the callback name. I tried to clarify that in comments though. "compare contents at 'renable' callback to see..." and "This structure is passed to 'map_event' and 'renable' callbacks for..." Best Regards Matti Vaittinen
On Thu, Apr 8, 2021 at 11:21 AM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > > Hello Andy, > > On Wed, 2021-04-07 at 16:21 +0300, Andy Shevchenko wrote: > > On Wed, Apr 7, 2021 at 1:04 PM Matti Vaittinen > > <matti.vaittinen@fi.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@gmail.com> > > I (eventually) disagreed with couple of points here and haven't changed > those. Please see list below. > > I still do think you did a really good job reviewing this - and you > should get the recognition from that work. Thus I'd nevertheless would > like to add your Reviewed-by to the next version. Please let me know if > you think it's ok. (I have the v7 ready but I'll wait until the next > Monday before sending it to see if this brings more discussion). Looks OK to me. Just rename die_loudly() to rdev_die_loudly() and in any way of conditionals with that, please mark it with __noreturn attribute, so if (bla bla bla) rdev_die_loudly(); will be an understandable trap.
On Thu, 2021-04-08 at 12:58 +0300, Andy Shevchenko wrote: > On Thu, Apr 8, 2021 at 11:21 AM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > Hello Andy, > > > > On Wed, 2021-04-07 at 16:21 +0300, Andy Shevchenko wrote: > > > On Wed, Apr 7, 2021 at 1:04 PM Matti Vaittinen > > > <matti.vaittinen@fi.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@gmail.com> > > > > I (eventually) disagreed with couple of points here and haven't > > changed > > those. Please see list below. > > > > I still do think you did a really good job reviewing this - and you > > should get the recognition from that work. Thus I'd nevertheless > > would > > like to add your Reviewed-by to the next version. Please let me > > know if > > you think it's ok. (I have the v7 ready but I'll wait until the > > next > > Monday before sending it to see if this brings more discussion). > > Looks OK to me. > Just rename die_loudly() to rdev_die_loudly() I did that. Thanks. > and in any way of > conditionals with that, please mark it with __noreturn attribute, so > if (bla bla bla) > rdev_die_loudly(); > > will be an understandable trap. Valid point. Will do, thanks again. Best Regards Matti Vaittinen
On Wed, 07 Apr 2021 13:04:51 +0300, Matti Vaittinen wrote: > BD9576MUF provides over-current protection and detection. Current is > measured as voltage loss over external FET. Allow specifying FET's on > resistance so current monitoring limits can be converted to voltages. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > v5 onwards: > - No changes > v4: > - Fixed the description indentiation > --- > .../bindings/regulator/rohm,bd9576-regulator.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
Extend regulator notification support This series extends the regulator notification and error flag support. Initial discussion on the topic can be found here: https://lore.kernel.org/lkml/6046836e22b8252983f08d5621c35ececb97820d.camel@fi.rohmeurope.com/ This series is built on top of the BD9576MUF support patch series v9 which is currently in MFD tree at immutable branch ib-mfd-watchdog-5.13 https://lore.kernel.org/lkml/cover.1615219345.git.matti.vaittinen@fi.rohmeurope.com/ (The series should apply without those patches but there is compile time dependency to definitions brought in at the last patch of the BD9576 series. This should be Ok though as there is a Kconfig dependency in BD9576 regulator driver) In a nutshell - the series adds: 1. WARNING level events/error flags. (Patch 2) Current regulator 'ERROR' event notifications for over/under voltage, over current and over temperature are used to indicate condition where monitored entity is so badly "off" that it actually indicates a hardware error which can not be recovered. The most typical hanling for that is believed to be a (graceful) system-shutdown. Here we add set of 'WARNING' level flags to allow sending notifications to consumers before things are 'that badly off' so that consumer drivers can implement recovery-actions. 2. Device-tree properties for specifying limit values. (Patches 1, 4) Add limits for above mentioned 'ERROR' and 'WARNING' levels (which send notifications to consumers) and also for a 'PROTECTION' level (which will be used to immediately shut-down the regulator(s) W/O informing consumer drivers. Typically implemented by hardware). Property parsing is implemented in regulator core which then calls callback operations for limit setting from the IC drivers. A warning is emitted if protection is requested by device tree but the underlying IC does not support configuring requested protection. 3. Helpers which can be registered by IC. (Patch 3) Target is to avoid implementing IRQ handling and IRQ storm protection in each IC driver. (Many of the ICs implementin these IRQs do not allow masking or acking the IRQ but keep the IRQ asserted for the whole duration of problem keeping the processor in IRQ handling loop). The helper was attempted to be done so it could be used to implement roughly same logic as is used in qcom-labibb regulator. This means amongst other things a safety shut-down if IC registers are not readable. Using these shut-down retry counters are optional. The idea is that the helper could be also used by simpler ICs which do not provide status register(s) which can be used to check if error is still active. ICs which do not have such status register can simply omit the 'renable' callback (and retry-counts etc) - and helper assumes the situation is Ok and re-enables IRQ after given time period. If problem persists the handler is ran again and another notification is sent - but at least the delay allows processor to avoid IRQ loop. Patch 6 takes this notification support in use at BD9576MUF. Patch 7 is related to MFD change which is not really related to the RFC here. It was added to this series in order to avoid potential conflicts. Patch 8 adds a maintainers entry. Changelog v6: Add MAINTAINERS entry Changes to IRQ notifiers - move devm functions to drivers/regulator/devres.c - drop irq validity check - use devm_add_action_or_reset() - fix styling issues - fix kerneldocs Changelog v5: - Fix the badly formatted pr_emerg() call. Changelog v4: - rebased on v5.12-rc6 - dropped RFC - fix external FET DT-binding. - improve prints for cases when expecting HW failure. - styling and typos Changelog v3: Regulator core: - Fix dangling pointer access at regulator_irq_helper() stpmic1_regulator: - fix function prototype (compile error) bd9576-regulator: - Update over current limits to what was given in new data-sheet (REV00K) - Allow over-current monitoring without external FET. Set limits to values given in data-sheet (REV00K). Changelog v2: Generic: - rebase on v5.12-rc2 + BD9576 series - Split devm variant of delayed wq to own series Regulator framework: - Provide non devm variant of IRQ notification helpers - shorten dt-property names as suggested by Rob - unconditionally call map_event in IRQ handling and require it to be populated BD9576 regulators: - change the FET resistance property to micro-ohms - fix voltage computation in OC limit setting -- Matti Vaittinen (8): dt_bindings: Add protection limit properties regulator: add warning flags regulator: IRQ based event/error notification helpers regulator: add property parsing and callbacks to set protection limits dt-bindings: regulator: bd9576 add FET ON-resistance for OCW regulator: bd9576: Support error reporting regulator: bd9576: Fix the driver name in id table MAINTAINERS: Add reviewer for regulator irq_helpers .../bindings/regulator/regulator.yaml | 82 ++ .../regulator/rohm,bd9576-regulator.yaml | 6 + MAINTAINERS | 4 + drivers/regulator/Makefile | 2 +- drivers/regulator/bd9576-regulator.c | 1060 +++++++++++++++-- drivers/regulator/core.c | 146 ++- drivers/regulator/devres.c | 49 + drivers/regulator/irq_helpers.c | 396 ++++++ drivers/regulator/of_regulator.c | 58 + drivers/regulator/qcom-labibb-regulator.c | 10 +- drivers/regulator/qcom_spmi-regulator.c | 6 +- drivers/regulator/stpmic1_regulator.c | 20 +- include/linux/regulator/consumer.h | 14 + include/linux/regulator/driver.h | 176 ++- include/linux/regulator/machine.h | 26 + 15 files changed, 1913 insertions(+), 142 deletions(-) create mode 100644 drivers/regulator/irq_helpers.c base-commit: e49d033bddf5b565044e2abe4241353959bc9120