Re: [PATCH v4 1/1] soc: fujitsu: Add A64FX diagnostic interrupt driver

[hasegawa-hitomi@xxxxxxxxxxx]

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