Re: [PATCH v6 3/8] regulator: IRQ based event/error notification helpers

[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@xxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>

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