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
