lkml.org 
[lkml]   [2022]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v4 1/1] soc: fujitsu: Add A64FX diagnostic interrupt driver
Date
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
\
 
 \ /
  Last update: 2022-05-17 11:55    [W:0.066 / U:0.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site