Re: [PATCH] ipmi:si: Move SI type information into an info structure

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Apr 16, 2025 at 07:09:12AM -0500, Corey Minyard wrote:
> Andy reported:
> 
> Debian clang version 19.1.7 is not happy when compiled with
> `make W=1` (note, CONFIG_WERROR=y is the default):
> 
> ipmi_si_platform.c:268:15: error: cast to smaller integer type 'enum si_type'
> +from 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast]
>   268 |         io.si_type      = (enum
> +si_type)device_get_match_data(&pdev->dev);
> 
> The IPMI SI type is an enum that was cast into a pointer that was
> then cast into an enum again.  That's not the greatest style, so
> instead create an info structure to hold the data and use that.

I'm going to test this today or latest tomorrow, below some comments.

...

>  	u8 slave_addr;
> -	enum si_type si_type;
> +	struct ipmi_match_info si_info;

	const struct ipmi_match_info *si_info;

?

I understand that right now there is no benefit, but my suggestion is scalable
and prevents from sudden values rewrites. Also that's how other drivers do, but
of course not all of them.

>  	struct device *dev;

...

> -	else if ((new_smi->io.si_type != SI_BT) && (!new_smi->io.irq))
> +	else if ((new_smi->io.si_info.type != SI_BT) && (!new_smi->io.irq))

While at it, drop unneeded parentheses (at least in the second part).

>  		enable = 1;

...

>  static int ipmi_pci_probe_regspacing(struct si_sm_io *io)
>  {
> -	if (io->si_type == SI_KCS) {
> +	if (io->si_info.type == SI_KCS) {
>  		unsigned char	status;
>  		int		regspacing;

It looks like the above conditional is for the whole function, if this is
the case, I would go with an additional patch to drop the indentation level.

	unsigned char	status;
	int		regspacing;
	...
	if (io->si_info.type != SI_KCS)
		return ...

...

>  #ifdef CONFIG_OF

I would clean up this ugly ifdeffery as well, but it can be done after this
patch.

> +static struct ipmi_match_info kcs_info = { .type = SI_KCS };
> +static struct ipmi_match_info smic_info = { .type = SI_SMIC };
> +static struct ipmi_match_info bt_info = { .type = SI_BT };
> +
>  static const struct of_device_id of_ipmi_match[] = {
> -	{ .type = "ipmi", .compatible = "ipmi-kcs",
> -	  .data = (void *)(unsigned long) SI_KCS },
> -	{ .type = "ipmi", .compatible = "ipmi-smic",
> -	  .data = (void *)(unsigned long) SI_SMIC },
> -	{ .type = "ipmi", .compatible = "ipmi-bt",
> -	  .data = (void *)(unsigned long) SI_BT },
> +	{ .type = "ipmi", .compatible = "ipmi-kcs", .data = &kcs_info },
> +	{ .type = "ipmi", .compatible = "ipmi-smic", .data = &smic_info },
> +	{ .type = "ipmi", .compatible = "ipmi-bt", .data = &bt_info },
>  	{},

...and this line should not have a trailing comma.

>  };
>  MODULE_DEVICE_TABLE(of, of_ipmi_match);

...

> +	io.si_info	= *((struct ipmi_match_info *)
> +			    device_get_match_data(&pdev->dev));

This ugly casting won't be needed if you use const pointer as suggested above.

	io.si_info	= *((struct ipmi_match_info *)
			    device_get_match_data(&pdev->dev));

...

>  	switch (tmp) {
>  	case 1:
> -		io.si_type = SI_KCS;
> +		io.si_info.type = SI_KCS;
>  		break;
>  	case 2:
> -		io.si_type = SI_SMIC;
> +		io.si_info.type = SI_SMIC;
>  		break;
>  	case 3:
> -		io.si_type = SI_BT;
> +		io.si_info.type = SI_BT;
>  		break;
>  	case 4: /* SSIF, just ignore */
>  		return -ENODEV;

I haven't looked where tmp comes from, but maybe that's a candidate to be in
the info structure to begin with?

-- 
With Best Regards,
Andy Shevchenko