Hi Thomas,
On Mon, Feb 3, 2025 at 7:20 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
> On Mon, Feb 03 2025 at 14:19, Anup Patel wrote:
> > +
> > +struct rpmi_sysmsi_priv {
> > + struct device *dev;
> > + struct mbox_client client;
> > + struct mbox_chan *chan;
> > + u32 nr_irqs;
> > + u32 gsi_base;
> > +};
>
> AS requested before please use tabular layout for structs:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#struct-declarations-and-initializers
>
> > +static int rpmi_sysmsi_set_msi_state(struct rpmi_sysmsi_priv *priv,
> > + u32 sys_msi_index, u32 sys_msi_state)
> > +{
> > + struct rpmi_sysmsi_set_msi_state_tx tx;
> > + struct rpmi_sysmsi_set_msi_state_rx rx;
> > + struct rpmi_mbox_message msg;
> > + int ret;
> > +
> > + tx.sys_msi_index = sys_msi_index;
> > + tx.sys_msi_state = sys_msi_state;
> > + rpmi_mbox_init_send_with_response(&msg, RPMI_SYSMSI_SRV_SET_MSI_STATE,
> > + &tx, sizeof(tx), &rx, sizeof(rx));
> > + ret = rpmi_mbox_send_message(priv->chan, &msg);
> > + if (ret)
> > + return ret;
> > + if (rx.status)
> > + return rpmi_to_linux_error(rx.status);
> > +
> > + return 0;
> > +}
> > +
> > +#define rpmi_sysmsi_mask(__priv, __msi_index) \
> > + rpmi_sysmsi_set_msi_state(__priv, __msi_index, 0)
> > +#define rpmi_sysmsi_unmask(__priv, __msi_index) \
> > + rpmi_sysmsi_set_msi_state(__priv, __msi_index, RPMI_SYSMSI_MSI_STATE_ENABLE)
>
> These macros are not really providing any value.
>
> > +static void rpmi_sysmsi_irq_mask(struct irq_data *d)
> > +{
> > + struct rpmi_sysmsi_priv *priv = irq_data_get_irq_chip_data(d);
> > + int ret;
> > +
> > + ret = rpmi_sysmsi_mask(priv, d->hwirq);
> > + if (ret)
> > + dev_warn(priv->dev, "Failed to mask hwirq %d (error %d)\n",
> > + (u32)d->hwirq, ret);
>
> if (ret) {
> ....
> }
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#bracket-rules
>
> > + irq_chip_mask_parent(d);
> > +}
>
> Other than those nits, this looks reasonable.
I will address all above comments in the next revision.
Thanks,
Anup
