Messages in this thread | | | From | "" <> | Subject | Re: [PATCH v4 1/1] soc: fujitsu: Add A64FX diagnostic interrupt driver | Date | Tue, 17 May 2022 09:53:57 +0000 |
| |
Hi Jiri,
> I'm not sure why you cc linux-serial, but anyway, comments below :).
I used sysrq until the last version, so I still included kernel-serial in the destination. I am not planning to use sysrq now, so I will remove it from the destination from the next version. Thank you for your comment.
> > +struct a64fx_diag_priv { > > + int irq; > > + void __iomem *mmsc_reg_base; > > + bool has_nmi; > > There are unnecessary holes in the struct. If you reorder it, you drop some > alignment. Like: pointer, int, bool.
> > + u32 mmsc; > > + void __iomem *diag_status_reg_addr; > > I'm not sure what soc/ maintainers prefer, but inverted xmas tree would look/read > better.
> > + priv = devm_kzalloc(dev, sizeof(struct a64fx_diag_priv), > > +GFP_KERNEL); > > Don't we prefer sizeof(*priv)?
> > + ret = request_irq(priv->irq, &a64fx_diag_handler_irq, > > + irq_flags, "a64fx_diag_irq", NULL); > > + if (ret) { > > + dev_err(dev, "cannot register IRQ %d\n", ret); > > No a64fx_diag_interrupt_disable()?
> > + priv->has_nmi = false; > > No need to set zeroed priv member to zero.
I understand. I will fix it as per your comment. Thank you.
> > + enable_nmi(priv->irq); > > Provided the above, I don't immediatelly see, what's the purpose of > IRQF_NO_AUTOEN then?
It seems that request_nmi() requires IRQF_NO_AUTOEN.
> > +static int __exit a64fx_diag_remove(struct platform_device *pdev) > > Is __exit appropriate here at all -- I doubt that.
I will remove __exit as it seems unnecessary as you suggested.
Also, I will correct BMC_DIAG_INTERRUPT_STATUS_OFFSET and BMC_DIAG_INTERRUPT_ENABLE_OFFSET.
Thank you. Hitomi Hasegawa
| |