Re: [PATCH 11/21] irqchip: Add irqchip for ADI ADSP-SC5xx platform

[Thomas Gleixner]

On Thu, Sep 12 2024 at 19:24, Arturs Artamonovs via wrote:
> From: Arturs Artamonovs <arturs.artamonovs@xxxxxxxxxx>
>
> Support seting extra indepdendent interrupt on pin activity.

So the subject says it adds a new interrupt chip. Now the changelog
mumbles about support of something extra.

Please describe your changes properly and explain what this is
about. Also spell check your change log.

> +struct adsp_pint {
> +	struct irq_chip chip;
> +	void __iomem *regs;
> +	struct irq_domain *domain;
> +	unsigned int irq;

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers

And please read and follow the rest of that document too.

> + * This relies on the default configuration of the hardware, which we do not
> + * expose an interface to change.
> + */
> +int adsp_attach_pint_to_gpio(struct adsp_gpio_port *port)

Where is this function declared and where is it used?

> +static void adsp_pint_dispatch_irq(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct adsp_pint *pint = to_adsp_pint(chip);
> +	unsigned int type = irqd_get_trigger_type(&desc->irq_data);
> +	u32 pos = BIT(desc->irq_data.hwirq);
> +
> +	/* for both edge interrupt, toggle invert bit to catch next edge */
> +	if (type == IRQ_TYPE_EDGE_BOTH) {
> +		u32 invert = readl(pint->regs + ADSP_PINT_REG_INVERT_SET) & pos;
> +
> +		if (invert)
> +			writel(pos, pint->regs + ADSP_PINT_REG_INVERT_CLEAR);
> +		else
> +			writel(pos, pint->regs + ADSP_PINT_REG_INVERT_SET);

What protects pint->regs against concurrent modifications?

> +static void adsp_pint_irq_mask(struct irq_data *d)
> +{
> +	struct adsp_pint *pint = irq_data_get_irq_chip_data(d);
> +
> +	writel(BIT(d->hwirq), pint->regs + ADSP_PINT_REG_MASK_CLEAR);

Same question.

> +static int adsp_pint_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct adsp_pint *pint;
> +
> +	pint = devm_kzalloc(dev, sizeof(*pint), GFP_KERNEL);
> +	if (!pint)
> +		return -ENOMEM;
> +
> +	pint->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pint->regs))
> +		return PTR_ERR(pint->regs);
> +
> +	pint->chip.name = "adsp-pint";
> +	pint->chip.irq_ack = adsp_pint_irq_ack;
> +	pint->chip.irq_mask = adsp_pint_irq_mask;
> +	pint->chip.irq_unmask = adsp_pint_irq_unmask;
> +	pint->chip.irq_set_type = adsp_pint_irq_set_type;
> +	// @todo potentially only SEC supports wake options, not gic
> +
> +	// @todo determine if we actually need a raw spinlock

This should have been determined before posting, no?

> +	pint->domain = irq_domain_add_linear(np, ADSP_PINT_IRQS,
> +		&adsp_irq_domain_ops, pint);

devm_irq_domain_instantiate()

> +	if (!pint->domain) {
> +		dev_err(dev, "Could not create irq domain\n");
> +		return -EINVAL;
> +	}
> +
> +	pint->irq = platform_get_irq(pdev, 0);
> +	if (!pint->irq) {
> +		dev_err(dev, "Could not find parent interrupt for port\n");
> +		return -EINVAL;

Then this would not leak the interrupt domain. Also why is this not
checked _before_ instantiating the domain?

> +static int __init adsp_pint_init(void)
> +{
> +	return platform_driver_register(&adsp_pint_driver);
> +}
> +

Pointless new line

> +arch_initcall(adsp_pint_init);
> +
> +MODULE_DESCRIPTION("Analog Devices IRQChip driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Greg Malysa <greg.malysa@xxxxxxxxxxx>");
> \ No newline at end of file

This message has a meaning, no?

Thanks,

        tglx