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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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@opensource.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@opensource.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