Re: [PATCH 2/2] irqchip/sg2042-msi: Add the Sophgo SG2044 MSI interrupt controller

[Thomas Gleixner <tglx@linutronix.de>]

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.
     
> +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.

> +	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.

>  	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.

Thanks,

        tglx