Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Asmaa Mnebhi <asmaa@nvidia.com>
To:     Andrew Lunn <andrew@lunn.ch>
CC:     "andy.shevchenko@gmail.com" <andy.shevchenko@gmail.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "bgolaszewski@baylibre.com" <bgolaszewski@baylibre.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "rjw@rjwysocki.net" <rjw@rjwysocki.net>,
        David Thompson <davthompson@nvidia.com>
Subject: RE: [PATCH v1 1/2] gpio: mlxbf2: Introduce IRQ support
Thread-Topic: [PATCH v1 1/2] gpio: mlxbf2: Introduce IRQ support
Thread-Index: AQHXqoEVVyA+enuCGUKWJgFkwmB1oKumsuKAgAABitA=
Date:   Thu, 16 Sep 2021 15:48:51 +0000
Message-ID: <CH2PR12MB3895D5E16EAA1D3E5796C177D7DC9@CH2PR12MB3895.namprd12.prod.outlook.com>
References: <20210915222847.10239-1-asmaa@nvidia.com>
 <20210915222847.10239-2-asmaa@nvidia.com> <YUNPO9/YacBNr/yQ@lunn.ch>
In-Reply-To: <YUNPO9/YacBNr/yQ@lunn.ch>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ifJjatJ7eB2P/ravyLyPbh9rhKYy61WIklblQ/gcI22N7wHIk+IYHVUA5DMK?=
 =?us-ascii?Q?hoeDWBtUDh16xsUAtKKp93EHpVkm7i6BkzlXcXPxX3X98oynjiy8UBSMIw0C?=
 =?us-ascii?Q?l2yfbYDk/6FyzhsVE6H1SSSPdR4jeTLEh+gRiZ7oXtu1IxRzvMqIxFBGLwed?=
 =?us-ascii?Q?7+UhhqcBI3YStpKEq+DudAN7rD9rwEknx76wXInxiVJ3c2QcosQoDG8V1M5f?=
 =?us-ascii?Q?RwE54P0xNinZQipKjd0l0OX3VFfqA0JgZYhAPb9lgKjX804tGzLKx5RywOvn?=
 =?us-ascii?Q?7f7UcphTA0fQD7BVYG6jPZjrhiH37iTPGCmF84jLD9GJlasJrMS+P2zfJg5y?=
 =?us-ascii?Q?Uu17aFGMR7oWzVgqet2jvivQ15RYBauoHpW3wA6hDknuIqLXx2yhDyTtX/jt?=
 =?us-ascii?Q?P4f9R0rd9iMik/20Ja24nJdDobUT4bphNGMqyIlEWYWpEjGntX2gRMoGjV9c?=
 =?us-ascii?Q?pQQYDfYH4A69fZL44+cy2mVl5wB/ZnzGL+O7HLeNm31I0S11ywJwaMAbjZ1x?=
 =?us-ascii?Q?5uC8LLxPIiyg0phQgWPr0oQNgd4XyDTxDRp675mEjyLA3iLW9JAMd8ezWoij?=
 =?us-ascii?Q?+DSKwi3gbHtSffW6NaenfHkNbKMNt9Yh9IyiZ4DOhE2jKrj0QDELI2pkKIrx?=
 =?us-ascii?Q?WZBo1ez15upF9gFaypnkb8Sss2gvz6lV0MKDLjIuZzFAdl1tWkZaYTK2/XL6?=
 =?us-ascii?Q?kA5cuSyIz98Iw+nKOG/qiVlPMscEP3pMcWSHtfHkUmqw3NCXTt/PCDOsCSA5?=
 =?us-ascii?Q?AUrq0S1tGs86vgXvOc9Mfd0o81k3U6pZtXn5oRyYIFVwUTYZW4lQLBHLcL29?=
 =?us-ascii?Q?EBEtAMMV6B2c8AdeufiHQNaOXMF9Qf4CcKfWfbtSF85jjyC2m5rxrfvB785L?=
 =?us-ascii?Q?Y6JdRebyArELlGT6b6GCULBwiuGFJX4IcdPAaj45fZOfFU7GxSQO0fK/ztMK?=
 =?us-ascii?Q?xMB3sWMO+x8ckNf668yRs1Yvd1m6pQVgvU+ghjJm8t8snQaeOCdyr2dvIhTT?=
 =?us-ascii?Q?rtWkOUR/h14h0xqymBH4UL0QazF2yZHrBN5UiABYZ0+aK6WmojsDdMgjl6nA?=
 =?us-ascii?Q?kAQ/UKokAFmQslEtrnzkj3/wj6tsp4syLcdOpQT5i4BvecR2srQdWDD9HeWd?=
 =?us-ascii?Q?6QNnqoNxNf+fN9Inklb/id6u70XbuJzp7L/ssEZjj1KwuwqVOKJoeIKMr04m?=
 =?us-ascii?Q?NLMQIjwclKHKsjS9jLr8YCj4l24LgC3MWI+XH40MacDoWhuL2Yd9GgwVWJuB?=
 =?us-ascii?Q?dPYMHwndTvrTJVqeV1UFCYcU7ZZLTBlB2x2Nr3e7LKkkcseu1VD72XNBuB+t?=
 =?us-ascii?Q?xDaskvc3UkdWxi/z43hCvZ/8?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CH2PR12MB3895.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bc52171c-c330-4aa9-7d8c-08d979297b86
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Sep 2021 15:48:52.1108
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: +Q5t27dCDTPBbNtSiFPb3I2hiAdUU5MAFQbZDWzH4DmwizZnROjuTzjFte9yyXl9a6NwHG2AucOEVQG6+vScEg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4891
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> +	/* Enable PHY interrupt by setting the priority level */

This should be an abstract driver for a collection of GPIO lines.
Yes, one of these GPIOs is used for the PHY, but the GPIO driver does not c=
are. So please remove this comment.
Asmaa>> Done

> +	val =3D readl(gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);
> +	val |=3D BIT(offset);
> +	writel(val, gs->gpio_io + YU_GPIO_CAUSE_OR_EVTEN0);

What exactly does this do? It appears to clear the interrupt, if i understa=
nd mlxbf2_gpio_irq_handler(). I don't know the GPIO framework well enough t=
o know if this is correct. It does mean if the interrupt signal is active b=
ut masked, and you enable it, you appear to loose the interrupt? Maybe you =
want the interrupt to fire as soon as it is enabled?

Asmaa>>
YU_GPIO_CAUSE_OR_CLRCAUSE - Makes sure the interrupt is initially cleared. =
Otherwise, we will not receive further interrupts.
YU_GPIO_CAUSE_OR_EVTEN0 - All interrupts are disabled by default. This regi=
ster is what actually unmasks/enables the specific interrupt to start "firi=
ng".

> +static void mlxbf2_gpio_irq_mask(struct irq_data *irqd) {
> +	mlxbf2_gpio_irq_disable(irqd);
> +}
> +
> +static void mlxbf2_gpio_irq_unmask(struct irq_data *irqd) {
> +	mlxbf2_gpio_irq_enable(irqd);
> +}

Do these two functions have any value?

Asmaa>>
This code is actually not being called. enable/disable is what's being call=
ed. So I will remove it.

> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_BOTH:
> +	case IRQ_TYPE_LEVEL_MASK:
> +		fall =3D true;
> +		rise =3D true;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		rise =3D true;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +	case IRQ_TYPE_LEVEL_LOW:
> +		fall =3D true;
> +		break;

This looks wrong. You cannot map a level interrupt into an edge. It looks l=
ike your hardware only supports edges. If asked to do level, return -EINVAL=
.

Asmaa>> done

> +
> +	/* The INT_N interrupt level is active low.
> +	 * So enable cause fall bit to detect when GPIO
> +	 * state goes low.
> +	 */

I don't understand this comment.

Asmaa>> removed.=20