linux-kernel - RE: [PATCH] mfd: da9062: add support for interrupt polarity defined in device tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <AM6PR10MB226310AB7693DEAA44EFA90D80040@AM6PR10MB2263.EURPRD10.PROD.OUTLOOK.COM>
Date:   Thu, 30 Jan 2020 09:57:39 +0000
From:   Adam Thomson <Adam.Thomson.Opensource@...semi.com>
To:     Shreyas Joshi <Shreyas.Joshi@...mp.com>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        Support Opensource <Support.Opensource@...semi.com>,
        "shreyasjoshi15@...il.com" <shreyasjoshi15@...il.com>,
        Adam Thomson <Adam.Thomson.Opensource@...semi.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] mfd: da9062: add support for interrupt polarity defined
 in device tree

On 29 January 2020 23:20, Shreyas Joshi wrote:

> The da9062 interrupt handler cannot necessarily be low active.
> Add a helper function to read the polarity defined in the device tree.
> Based on the user defined polarity in device tree, the interrupt is
> set active.

When sending an updated patch you normally add a version identifier to the patch
title/subject, for example '[PATCH v2]'. 

> 
> Signed-off-by: shreyas <shreyas.joshi@...mp.com>

Would suggest using your full name above in the 'Signed-off-by'.

> ---
And under here you would normally add a description of changes in each version
of a patch. Don't suppose it matters too much this time, but just for future
reference. See 'Documentation/process/submitting-patches.rst' for further
information.

>  drivers/mfd/da9062-core.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
> index 419c73533401..62ba10aeb440 100644
> --- a/drivers/mfd/da9062-core.c
> +++ b/drivers/mfd/da9062-core.c
> @@ -369,6 +369,22 @@ static int da9062_get_device_type(struct da9062 *chip)
>  	return ret;
>  }
> 
> +static inline u32 irqd_get_polarity(struct irq_data *d, struct device *dev)
> +{
> +	u32 trigger_type = irqd_get_trigger_type(d);
> +
> +	switch (trigger_type) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +	case IRQ_TYPE_LEVEL_LOW:
> +	case IRQ_TYPE_NONE:
> +		return trigger_type;
> +	default:
> +		dev_warn(dev, "Invalid IRQ type: %d defaulting to
> IRQ_TYPE_NONE\n",
> +				trigger_type);
> +		return IRQ_TYPE_NONE;
> +	}
> +}
> +
>  static const struct regmap_range da9061_aa_readable_ranges[] = {
>  	regmap_reg_range(DA9062AA_PAGE_CON, DA9062AA_STATUS_B),
>  	regmap_reg_range(DA9062AA_STATUS_D, DA9062AA_EVENT_C),
> @@ -592,6 +608,7 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
>  	struct da9062 *chip;
>  	const struct of_device_id *match;
>  	unsigned int irq_base;
> +	struct irq_data *irq_data;
>  	const struct mfd_cell *cell;
>  	const struct regmap_irq_chip *irq_chip;
>  	const struct regmap_config *config;
> @@ -654,9 +671,16 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
>  	if (ret)
>  		return ret;
> 
> +	irq_data = irq_get_irq_data(i2c->irq);
> +	if (!irq_data) {
> +		dev_err(chip->dev, "Invalid IRQ: %d\n", i2c->irq);
> +		return -EINVAL;
> +	}
> +
>  	ret = regmap_add_irq_chip(chip->regmap, i2c->irq,
> -			IRQF_TRIGGER_LOW | IRQF_ONESHOT | IRQF_SHARED,
> -			-1, irq_chip,
> +			irqd_get_polarity(irq_data, chip->dev)
> +			| IRQF_ONESHOT | IRQF_SHARED | IRQF_ONESHOT
> +			| IRQF_SHARED, -1, irq_chip,
>  			&chip->regmap_irq);

I think you miss my point here. The HW has the option to be either ACTIVE_LOW
(default) or ACTIVE_HIGH. If DT is specifiying for example ACTIVE_HIGH but the
HW is configured for ACTIVE_LOW then you would likely see an IRQ storm.

As mentioned before, I think you should update the setting in CONFIG_A register,
regarding the nIRQ pin polarity, based on the DT selection you've read. 

>  	if (ret) {
>  		dev_err(chip->dev, "Failed to request IRQ %d: %d\n",
> --
> 2.20.1