Re: [PATCH 9/9] regulator: da9121: add interrupt support

From: Mark Brown <broonie@kernel.org>
To: Adam Ward <adam.ward@diasemi.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Vincent Whitchurch <vincent.whitchurch@axis.com>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 9/9] regulator: da9121: add interrupt support
Date: Fri, 20 Nov 2020 13:45:58 +0000	[thread overview]
Message-ID: <20201120134558.GE6751@sirena.org.uk> (raw)
In-Reply-To: <ef98a01f6281ae6c925f283b51804f7f5194d230.1605868780.git.Adam.Ward.opensource@diasemi.com>

[-- Attachment #1: Type: text/plain, Size: 3084 bytes --]

On Fri, Nov 20, 2020 at 12:14:59PM +0000, Adam Ward wrote:
> Adds interrupt handler for variants, and notifications for events; over
> temperature/voltage/current.
> Also handling of persistent events and respective timing configuration.

Again the "also" suggests that this should be multiple changes.

> +	struct da9121 *chip = container_of(work, struct da9121, work.work);
> +	enum { R0 = 0, R1, R2, REG_MAX_NUM };

This enum appears to map the numbers 0, 1 and 2 onto the constants 0, 1
and 2?  It also seems to be repeated in several functions?

> +	int status[REG_MAX_NUM] = {0};
> +	int clear[REG_MAX_NUM] = {0};
> +	unsigned long delay;
> +	int i;
> +	int ret;

> +	/* If persistent-notification, status will be true
> +	 * If not persistent-notification any longer, status will be false
> +	 */
> +	ret = regmap_bulk_read(chip->regmap, DA9121_REG_SYS_STATUS_0,
> +			(void *)status, (size_t)REG_MAX_NUM);

If these casts are needed something is very wrong with what you're
doing.

> +static inline int da9121_handle_notifier(
> +		struct da9121 *chip, struct regulator_dev *rdev,
> +		unsigned int event_bank, unsigned int event, unsigned int ebit)

Why is this flagged as inline?

> +	if (event & ebit) {
> +		switch (event_bank) {
> +		case DA9121_REG_SYS_EVENT_0:
> +			switch (event & ebit) {
> +			case DA9121_MASK_SYS_EVENT_0_E_TEMP_CRIT:

The structure of this code seems extremely confusing and hard to follow.
I really don't understand what purpose this function serves at all, it
appears to be factoring out the check to see if the bit is set and then
wrapping that in three layers of unpacking to work out setting the bit
in persistent and which notification to flag.  I don't understand why
the interrupt handler doesn't just directly do these things, this just
seems to be adding a lot of redundant code.

> +			case DA9xxx_MASK_SYS_EVENT_1_E_OC2:
> +				chip->persistent[R1] |= DA9xxx_MASK_SYS_EVENT_1_E_OC2;
> +				notification |= REGULATOR_EVENT_OVER_CURRENT;
> +				break;

> +		regulator_notifier_call_chain(rdev, notification, NULL);

The expectation is that one notification will be delivered per event,
which fortunately as far as I can see is pretty much what happens.

> +	/* 0 SYSTEM_GOOD */
> +	if (!(mask[R0] & DA9xxx_MASK_SYS_MASK_0_M_SG) &&
> +	    (event[R0] & DA9xxx_MASK_SYS_EVENT_0_E_SG)) {
> +		dev_warn(chip->dev, "Handled E_SG\n");
> +		handled[R0] |= DA9xxx_MASK_SYS_EVENT_0_E_SG;
> +		ret = IRQ_HANDLED;
> +	}

If the interrupt is saying that the system is good why are we logging
that as a warning?

> +static int da9121_i2c_remove(struct i2c_client *i2c)
> +{
> +	struct da9121 *chip = i2c_get_clientdata(i2c);
> +	int ret = 0;
> +
> +	ret = da9121_set_irq_masks(chip, true);
> +	if (ret != 0) {
> +		dev_err(chip->dev, "Failed to set IRQ masks: %d\n", ret);
> +		goto error;
> +	}

It would simplify the rest of the code to just unconditionally mask all
interrupts here.

> +
> +	cancel_delayed_work(&chip->work);

This doesn't synchronize with the work being cancelled, the driver may
be unregistered while the work is still running.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]