Re: [PATCH v3 09/21] mfd: axp20x: Allow AXP chips without interrupt lines

From: Samuel Holland <samuel@sholland.org>
To: Andre Przywara <andre.przywara@arm.com>,
	Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
	Jernej Skrabec <jernej.skrabec@siol.net>
Cc: "Icenowy Zheng" <icenowy@aosc.io>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Rob Herring" <robh@kernel.org>,
	"Clément Péron" <peron.clem@gmail.com>,
	"Shuosheng Huang" <huangshuosheng@allwinnertech.com>,
	"Yangtao Li" <tiny.windzz@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-sunxi@googlegroups.com,
	"Lee Jones" <lee.jones@linaro.org>
Subject: Re: [PATCH v3 09/21] mfd: axp20x: Allow AXP chips without interrupt lines
Date: Sun, 17 Jan 2021 21:37:22 -0600	[thread overview]
Message-ID: <b3d1cc46-3311-ba1a-f71a-37096ed1bfa8@sholland.org> (raw)
In-Reply-To: <20210118020848.11721-10-andre.przywara@arm.com>

On 1/17/21 8:08 PM, Andre Przywara wrote:
> Currently the AXP chip requires to have its IRQ line connected to some
> interrupt controller, and will fail probing when this is not the case.
> 
> On a new Allwinner SoC (H616) there is no NMI pin anymore, so the
> interrupt functionality of the AXP chip is simply not available.
> 
> Check whether the DT describes the AXP chip as an interrupt controller
> before trying to register the irqchip, to avoid probe failures on
> setups without an interrupt.

The AXP305 has an IRQ pin. It is still an interrupt controller, even if
its output is not connected anywhere. And even though the NMI pin on the
H616 is gone, the PMIC IRQ line could be connected to a GPIO. So it is
not appropriate to remove "interrupt-controller".

Per the binding, both "interrupts" and "interrupt-controller" are
required properties. It would make more sense to make "interrupts"
optional. Either way, you need to update the binding.

Though I'm concerned about how this may affect drivers for regmap cells
which use interrupts (such as axp20x-pek). If the irqchip is not
registered, requesting those interrupts will fail. While I don't
currently know of any boards that have the AXP305 power key wired up, it
prevents us from modelling the hardware correctly and supporting that
configuration.

Cheers,
Samuel

> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/mfd/axp20x.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index aa59496e4376..a52595c49d40 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -959,12 +959,17 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
>  				     AXP806_REG_ADDR_EXT_ADDR_SLAVE_MODE);
>  	}
>  
> -	ret = regmap_add_irq_chip(axp20x->regmap, axp20x->irq,
> -			  IRQF_ONESHOT | IRQF_SHARED | axp20x->irq_flags,
> -			   -1, axp20x->regmap_irq_chip, &axp20x->regmap_irqc);
> -	if (ret) {
> -		dev_err(axp20x->dev, "failed to add irq chip: %d\n", ret);
> -		return ret;
> +	if (!axp20x->dev->of_node ||
> +	    of_property_read_bool(axp20x->dev->of_node, "interrupt-controller")) {
> +		ret = regmap_add_irq_chip(axp20x->regmap, axp20x->irq,
> +				IRQF_ONESHOT | IRQF_SHARED | axp20x->irq_flags,
> +				-1, axp20x->regmap_irq_chip,
> +				&axp20x->regmap_irqc);
> +		if (ret) {
> +			dev_err(axp20x->dev, "failed to add irq chip: %d\n",
> +				ret);
> +			return ret;
> +		}
>  	}
>  
>  	ret = mfd_add_devices(axp20x->dev, -1, axp20x->cells,
> 