linux-kernel - Re: [PATCH 07/10] irqchip/cs42l43: Add support for the cs42l43 IRQs

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <80f5acfe-a32c-48cc-590a-ee02d2b494aa@linaro.org>
Date:   Fri, 12 May 2023 17:27:34 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Charles Keepax <ckeepax@...nsource.cirrus.com>, broonie@...nel.org,
        lee@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        tglx@...utronix.de, maz@...nel.org, linus.walleij@...aro.org,
        vkoul@...nel.org
Cc:     lgirdwood@...il.com, yung-chuan.liao@...ux.intel.com,
        sanyog.r.kale@...el.com, pierre-louis.bossart@...ux.intel.com,
        alsa-devel@...a-project.org, patches@...nsource.cirrus.com,
        devicetree@...r.kernel.org, linux-gpio@...r.kernel.org,
        linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 07/10] irqchip/cs42l43: Add support for the cs42l43 IRQs

On 12/05/2023 14:28, Charles Keepax wrote:
> The CS42L43 is an audio CODEC with integrated MIPI SoundWire interface
> (Version 1.2.1 compliant), I2C, SPI, and I2S/TDM interfaces designed
> for portable applications. It provides a high dynamic range, stereo
> DAC for headphone output, two integrated Class D amplifiers for
> loudspeakers, and two ADCs for wired headset microphone input or
> stereo line input. PDM inputs are provided for digital microphones.
> 
> The IRQ chip provides IRQ functionality both to other parts of the
> cs42l43 device and to external devices that wish to use its IRQs.
> 
> Signed-off-by: Charles Keepax <ckeepax@...nsource.cirrus.com>

Thank you for your patch. There is something to discuss/improve.

> +
> +static struct platform_driver cs42l43_irq_driver = {
> +	.driver = {
> +		.name	= "cs42l43-irq",
> +	},
> +
> +	.probe		= cs42l43_irq_probe,
> +	.remove		= cs42l43_irq_remove,
> +};
> +module_platform_driver(cs42l43_irq_driver);
> +
> +MODULE_DESCRIPTION("CS42L43 IRQ Driver");
> +MODULE_AUTHOR("Charles Keepax <ckeepax@...nsource.cirrus.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:cs42l43-irq");

You miss the ID table. Don't add aliases for missing ID entries. They do
not scale and it is not their purpose.

> diff --git a/include/linux/irqchip/cs42l43.h b/include/linux/irqchip/cs42l43.h
> new file mode 100644
> index 0000000000000..99ce0dbc96a77
> --- /dev/null
> +++ b/include/linux/irqchip/cs42l43.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * CS42L43 IRQ driver external data
> + *
> + * Copyright (C) 2022-2023 Cirrus Logic, Inc. and
> + *                         Cirrus Logic International Semiconductor Ltd.
> + */
> +
> +#ifndef CS42L43_IRQ_EXT_H
> +#define CS42L43_IRQ_EXT_H
> +
> +enum cs42l43_irq_numbers {
> +	CS42L43_PLL_LOST_LOCK,
> +	CS42L43_PLL_READY,
> +

Are these really used by other subsystems? Your IRQ handling should be
anyway next to the driver.

Best regards,
Krzysztof