Re: [PATCH v4 4/4] iio: accel: adxl367: add support for INT2 interrupt pin

[Jonathan Cameron]

> The ADXL367 provides two independent interrupt output pins, INT1 and
> INT2, each with its own event mapping register (INTMAP1_LOWER at 0x2A
> and INTMAP2_LOWER at 0x2B) sharing an identical bit layout. Until now
> the driver hardcoded INT1 for all interrupt mappings, so a board that
> routes only INT2 to the host could never receive activity, inactivity
> or FIFO watermark interrupts.
> 
> Determine the connected pin from the interrupt-names device tree
> property using fwnode_irq_get_byname(), and route the interrupt
> mappings to the matching register. Use the interrupt number returned by
> the lookup for devm_request_threaded_irq() so the requested line always
> matches the routed INTMAP register, regardless of the order in which the
> interrupts are listed. When no interrupt-names are present, default to
> INT1 and the bus-provided interrupt to preserve the existing behaviour.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
>

One follow on thing to look at via sashiko.

Otherwise, this looks good to me, but given the active discussion on previous
version I'm not going to pick this up until plenty of time has passed
or tags have been given.

> diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
> index b4e8dc6d6a5c..9b47e66c49ea 100644
> --- a/drivers/iio/accel/adxl367.c
> +++ b/drivers/iio/accel/adxl367.c

> @@ -1430,6 +1434,36 @@ static int adxl367_setup(struct adxl367_state *st)
>  	return adxl367_set_measure_en(st, true);
>  }
>  
> +static int adxl367_set_int_map_reg(struct adxl367_state *st, int irq)
> +{
> +	int ret;
> +
> +	/*
> +	 * Route the mappings to whichever of INT1/INT2 is named in the
> +	 * interrupt-names property. -EINVAL (property absent) and -ENODATA
> +	 * (name not listed) are not errors; anything else is.
> +	 */
> +	ret = fwnode_irq_get_byname(dev_fwnode(st->dev), "INT1");
> +	if (ret > 0) {
> +		st->int_map_reg = ADXL367_REG_INT1_MAP;
> +		return ret;
> +	}
> +	if (ret != -ENODATA && ret != -EINVAL)
> +		return ret;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(st->dev), "INT2");
> +	if (ret > 0) {
> +		st->int_map_reg = ADXL367_REG_INT2_MAP;
> +		return ret;
> +	}
> +	if (ret != -ENODATA && ret != -EINVAL)
> +		return ret;
> +
> +	/* No INT1/INT2 named: default to INT1 and the bus interrupt. */
> +	st->int_map_reg = ADXL367_REG_INT1_MAP;
> +	return irq;

I'd like to explore the helper idea for this, but it can be a separate
series when someone has the time.

> +}
> +
>  int adxl367_probe(struct device *dev, const struct adxl367_ops *ops,
>  		  void *context, struct regmap *regmap, int irq)
>  {
> @@ -1486,6 +1520,11 @@ int adxl367_probe(struct device *dev, const struct adxl367_ops *ops,
>  	if (ret)
>  		return ret;
>  

[Severity: Medium]
Since fwnode_irq_get_byname() inside adxl367_set_int_map_reg() can return
-EPROBE_DEFER if the interrupt controller is not yet ready, will placing this
lookup after adxl367_setup() cause repeated boot delays and leave the device
powered up on deferral?

Looking at adxl367_probe(), it resets the hardware, sleeps for 15ms, and calls
adxl367_setup() which powers up the device into measurement mode.

If adxl367_set_int_map_reg() then returns -EPROBE_DEFER, the probe aborts
without an error cleanup path to power down the device.

Could this software resource acquisition be performed before the hardware
interaction to avoid unnecessary boot stalls and power leaks during probe
deferral?

-

This obviously isn't new, but it might make sense to turn the device
off at least.

Can be a follow up patch.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>