Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <5d71a9ef.1c69fb81.c5fbe.e477@mx.google.com>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20190829181203.2660-8-ilina@codeaurora.org>
References: <20190829181203.2660-1-ilina@codeaurora.org> <20190829181203.2660-8-ilina@codeaurora.org>
To:     Lina Iyer <ilina@codeaurora.org>, evgreen@chromium.org,
        linus.walleij@linaro.org, marc.zyngier@arm.com
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        bjorn.andersson@linaro.org, mkshah@codeaurora.org,
        linux-gpio@vger.kernel.org, rnayak@codeaurora.org
Subject: Re: [PATCH RFC 07/14] genirq: Introduce irq_chip_get/set_parent_state calls
User-Agent: alot/0.8.1
Date:   Thu, 05 Sep 2019 17:35:58 -0700
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Lina Iyer (2019-08-29 11:11:56)
> From: Maulik Shah <mkshah@codeaurora.org>
>=20
> On certain QTI chipsets some GPIOs are direct-connect interrupts
> to the GIC.
>=20
> Even when GPIOs are not used for interrupt generation and interrupt
> line is disabled, it does not prevent interrupt to get pending at
> GIC_ISPEND. When drivers call enable_irq unwanted interrupt occures.

Inidicate functions with parenthesis like enable_irq().

>=20
> Introduce irq_chip_get/set_parent_state calls to clear pending irq
> which can get called within irq_enable of child irq chip to clear
> any pending irq before enabling.

This sentence is hard to read.

>=20
> index b76703b2c0af..6bb5b22bb0a7 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -1297,6 +1297,50 @@ EXPORT_SYMBOL_GPL(handle_fasteoi_mask_irq);
> =20
>  #endif /* CONFIG_IRQ_FASTEOI_HIERARCHY_HANDLERS */
> =20
> +/**
> + *     irq_chip_set_parent_state - set the state of a parent interrupt.
> + *     @data: Pointer to interrupt specific data
> + *     @which: State to be restored (one of IRQCHIP_STATE_*)
> + *     @val: Value corresponding to @which
> + *
> + */
> +int irq_chip_set_parent_state(struct irq_data *data,
> +                             enum irqchip_irq_state which,
> +                             bool val)
> +{
> +       data =3D data->parent_data;
> +       if (!data)
> +               return 0;
> +
> +       if (data->chip->irq_set_irqchip_state)
> +               return data->chip->irq_set_irqchip_state(data, which, val=
);
> +
> +       return 0;

How about=20

	if (!data || !data->chip->irq_set_irqchip_state)
		return 0;
=09
	return data->chip->irq_set_irqchip_state(...)

> +}
> +EXPORT_SYMBOL(irq_chip_set_parent_state);
> +
> +/**
> + *     irq_chip_get_parent_state - get the state of a parent interrupt.

Why is this indented so much?

> + *     @data: Pointer to interrupt specific data
> + *     @which: one of IRQCHIP_STATE_* the caller wants to know
> + *     @state: a pointer to a boolean where the state is to be stored
> + *

Document return value?

> + */
> +int irq_chip_get_parent_state(struct irq_data *data,
> +                             enum irqchip_irq_state which,
> +                             bool *state)
> +{
> +       data =3D data->parent_data;
> +       if (!data)
> +               return 0;
> +
> +       if (data->chip->irq_get_irqchip_state)
> +               return data->chip->irq_get_irqchip_state(data, which, sta=
te);
> +

Same comment here about collapsing logic.

> +       return 0;
> +}
> +EXPORT_SYMBOL(irq_chip_get_parent_state);

Please make these symbols _GPL.

> +
>  /**
>   * irq_chip_enable_parent - Enable the parent interrupt (defaults to unm=
ask if
>   * NULL)