On Mon, Mar 03, 2025 at 08:31:32PM +0100, Thomas Gleixner wrote:
> On Mon, Mar 03 2025 at 19:16, Inochi Amaoto wrote:
> > Add support for Sophgo SG2044 MSI interrupt controller.
>
> This patch fails to apply on top of:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/drivers
>
> Please always ensure that your patches apply against the tree/branch
> into which they are supposed to be merged. Grabbing random patches from
> the mailing list as base is not sufficient. It's clearly documented
> against what you should work.
>
Thanks for pointing that, I will check the tip tree and see what
is conflicted. I forgot there will be something changed when
merging patch.
> > +struct sg2042_msi_of_data {
>
> There is nothing specific to OF in this data structure. This structure
> contains the chip and the MSI parent ops of each variant. So something
> like sg204x_chip_info is way more descriptive.
>
Yeah, chip_info it more clear than of_data. I have forgotten this
driver is not just for dtb but also UEFI fdt.
> > + const struct irq_chip *irqchip;
> > + const struct msi_parent_ops *parent_ops;
> > +};
> > +
> > struct sg2042_msi_chipdata {
>
> and rename that one to sg204x_... as it is not longer sg2042 specific.
>
This is OK for me.
> > void __iomem *reg_clr; // clear reg, see TRM, 10.1.33, GP_INTR0_CLR
> >
> > @@ -29,8 +34,10 @@ struct sg2042_msi_chipdata {
> > u32 irq_first; // The vector number that MSIs starts
> > u32 num_irqs; // The number of vectors for MSIs
> >
> > - DECLARE_BITMAP(msi_map, SG2042_MAX_MSI_VECTOR);
> > + unsigned long *msi_map;
> > struct mutex msi_map_lock; // lock for msi_map
> > +
> > + const struct sg2042_msi_of_data *data;
>
> Please keep the tabular formatting of this struct. See:
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#coding-style-notes
>
> > };
> >
> > static int sg2042_msi_allocate_hwirq(struct sg2042_msi_chipdata *data, int num_req)
> > @@ -81,6 +88,37 @@ static const struct irq_chip sg2042_msi_middle_irq_chip = {
> > .irq_compose_msi_msg = sg2042_msi_irq_compose_msi_msg,
> > };
> >
> > +static void sg2044_msi_irq_ack(struct irq_data *d)
> > +{
> > + struct sg2042_msi_chipdata *data = irq_data_get_irq_chip_data(d);
> > +
> > + writel(0, (unsigned int *)data->reg_clr + d->hwirq);
> > +
>
> Pointless newline
>
> > + irq_chip_ack_parent(d);
> > +}
> > +
> > +static void sg2044_msi_irq_compose_msi_msg(struct irq_data *d,
> > + struct msi_msg *msg)
>
> No line break required. Please use up to 100 characters.
>
> > static int sg2042_msi_parent_domain_alloc(struct irq_domain *domain,
> > unsigned int virq, int hwirq)
> > {
> > @@ -119,7 +157,7 @@ static int sg2042_msi_middle_domain_alloc(struct irq_domain *domain,
> > goto err_hwirq;
> >
> > irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> > - &sg2042_msi_middle_irq_chip, data);
> > + data->data->irqchip, data);
>
> The conversion of the existing code to this should be a preparatory patch
> for ease of review and the support for the new chip built on top.
>
> Also please come up with a sensible name for this new 'data' pointer.
>
> data->data->
>
> is horribly unintuitive. It's not the same data type.
>
> data->chip_info
>
> or such makes it clear what this is about.
>
Good, I will take care of that.
Regards,
Inochi
