On Thu, Mar 11, 2021 at 12:22:36PM +0200, Matti Vaittinen wrote:
> @@ -0,0 +1,423 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 ROHM Semiconductors
Please make the entire comment a C++ one so things look more consistent.
> +static void regulator_notifier_isr_work(struct work_struct *work)
> +{
> + if (d->fatal_cnt && h->retry_cnt > d->fatal_cnt) {
> + if (d->die)
> + ret = d->die(rid);
> + else
> + BUG();
> +
> + /*
> + * If the 'last resort' IC recovery failed we will have
> + * nothing else left to do...
> + */
> + BUG_ON(ret);
This isn't good... we should be trying to provide more system level
handling of this, if nothing else it's quite possibly not a software bug
here but rather a hardware failure. An explicit message about what
happened would be more likely to be understood as a hardware failure,
and something which allows handling such as initiating a system shutdown
would be good as well - I'm not sure if there's any existing mechanism
to plumb userspace into, or perhaps some sort of policy configurable via
sysfs. That could be built on later though, I think the main thing here
is that the logging should be clearer and distinguishable from a random
software fault which is what BUG_ON() looks like. The backtrace and
whatnot that BUG_ON() provides aren't useful here and the message isn't
going to be very distinctive, some custom prints will attract more
attention.
> + /* 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
Missing blank lines in the file.
> + * This structure is passed to map_event and renable for reporting reulator
regulator.
Attachment:
signature.asc
Description: PGP signature
