Re: [PATCH 1/5] fbdev: tdfxfb: Add helper to read config table from BIOS

[email protected]
Newsgroups org.freedesktop.lists.dri-devel
Message-ID <[email protected]>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch introduces `tdfxfb_get_bios_cfg` and `tdfx_pll_to_khz` as `static` functions but does not use them anywhere in the file. This triggers `-Wunused-function` compiler warnings, which break builds with `CONFIG_WERROR` enabled.
--

--- Patch [1]: [PATCH 1/5] fbdev: tdfxfb: Add helper to read config table from BIOS ---
commit d67a333d81585d56234b84350edd145b08f26be1
Author: Daniel Palmer <[email protected]>

fbdev: tdfxfb: Add helper to read config table from BIOS

This commit adds a helper to get the config table from the video BIOS in
preparation for manually booting cards.

> diff --git a/drivers/video/fbdev/tdfxfb.c b/drivers/video/fbdev/tdfxfb.c
> --- a/drivers/video/fbdev/tdfxfb.c
> +++ b/drivers/video/fbdev/tdfxfb.c
[ ... ]
> +/*
> + * Convert a pllctrl register value back to a frequency in kHz.
> + * Formula from 3dfx documentation.
> + */
> +static u32 tdfx_pll_to_khz(u32 pll)
> +{
> +	return (14318 * (((pll >> 8) & 0xff) + 2) /
> +		(((pll >> 2) & 0x3f) + 2)) >> (pll & 3);
> +}
> +
> +/* Layout of the "OEM config" table in voodoo 3 BIOS */
> +struct tdfx_bios_cfg {
[ ... ]
> +};
> +
> +#define TDFX_ROM_CFG_PTR	0x50
> +
> +static bool tdfxfb_get_bios_cfg(struct pci_dev *pdev,
> +				struct tdfx_bios_cfg *cfg)
> +{

[Severity: Low]
Does adding this static function and its helper tdfx_pll_to_khz without any
callers trigger a -Wunused-function compiler warning that breaks builds with
CONFIG_WERROR enabled? 

Could these functions be marked with __maybe_unused, or introduced in the
same commit as their callers to ensure the tree compiles cleanly at every
commit?

> +	u16 romcfg, oemcfg;
> +	void __iomem *rom;
> +	size_t romsize;
> +	u8 *image;
> +	u32 khz;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.