Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <80f5acfe-a32c-48cc-590a-ee02d2b494aa@linaro.org>
Date:   Fri, 12 May 2023 17:27:34 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH 07/10] irqchip/cs42l43: Add support for the cs42l43 IRQs
Content-Language: en-US
To:     Charles Keepax <ckeepax@opensource.cirrus.com>, broonie@kernel.org,
        lee@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        tglx@linutronix.de, maz@kernel.org, linus.walleij@linaro.org,
        vkoul@kernel.org
Cc:     lgirdwood@gmail.com, yung-chuan.liao@linux.intel.com,
        sanyog.r.kale@intel.com, pierre-louis.bossart@linux.intel.com,
        alsa-devel@alsa-project.org, patches@opensource.cirrus.com,
        devicetree@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230512122838.243002-1-ckeepax@opensource.cirrus.com>
 <20230512122838.243002-8-ckeepax@opensource.cirrus.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230512122838.243002-8-ckeepax@opensource.cirrus.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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