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
