Re: [PATCH] video: fbdev: matroxfb: remove dead code and set but not used variable

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

On Thu, Mar 19, 2026 at 01:44:28PM +0100, Helge Deller wrote:
> On 3/19/26 10:08, Andy Shevchenko wrote:
> > On Thu, Mar 19, 2026 at 09:35:33AM +0100, Helge Deller wrote:
> > > On 3/19/26 09:21, Andy Shevchenko wrote:
> > > > On Thu, Mar 19, 2026 at 04:06:44PM +0800, Jason Yan wrote:
> > > > > 在 2026/3/19 15:38, Andy Shevchenko 写道:
> > > > > > On Thu, Mar 19, 2026 at 10:22:08AM +0800, Jason Yan wrote:

...

> > > > > > Helge, can we revert this change and start over, please? (I can also send
> > > > > > revert if you think it's a better way)
> > > 
> > > Andy, all points you make against removing relevant code is absolutely right.
> > > 
> > > But for this specific commit 7b987887f97b ("video: fbdev: matroxfb: remove dead code and
> > > set but not used variable") I have to agree with Jason, that the patch is ok.
> > > I don't see any build errors either.
> > 
> > Just run on today's Linux Next (since the driver has not been changed in that
> > sense for a few years, the very same issue is present for a long time):
> > 
> > drivers/video/fbdev/matrox/g450_pll.c:412:18: error: variable 'mnp' set but not used [-Werror,-Wunused-but-set-variable]
> >    412 |                                 unsigned int mnp;
> >        |                                              ^
> > 1 error generated.
> 
> Ok.
> 
> > > Are we mixing up things here maybe?
> > 
> > ...
> > 
> > FWIW, I dove to the history of the driver.
> > 
> > So, the code, that was guarded by #if 0 was introduced in the original commit
> > 213d22146d1f ("[PATCH] (1/3) matroxfb for 2.5.3"). And then guarded in the
> > commit 705e41f82988 ("matroxfb DVI updates: Handle DVI output on G450/G550.
> > Powerdown unused portions of G450/G550 DAC. Split G450/G550 DAC from older
> > DAC1064 handling. Modify PLL setting when both CRTCs use same pixel clocks.").
> > 
> > Even without that guard the modern compilers may see that the pixel_vco wasn't
> > ever used and seems a leftover after some debug or review made 25 years ago.
> > 
> > The g450_mnp2vco() doesn't have any IO and as Jason said doesn't seem to have
> > any side effects either than some unneeded CPU processing during runtime. I
> > agree that's unlikely that timeout (or heating up the CPU) has any effect on
> > the HW (GPU/display) functionality.
> > 
> > So, since the commit 7b987887f97b ("video: fbdev: matroxfb: remove dead code
> > and set but not used variable") the 'mnp' became unused, but eliminating that
> > code might have side-effects. The question here is what should we do with mnp?
> > The easiest way out is just mark it with __maybe_unused which will shut the
> > compiler up and won't change any possible IO flow.
> 
> Yes, I think that's what I'd prefer.
> Do you mind sending a patch?

I have just sent 20260320143646.3218199-1-andriy.shevchenko@linux.intel.com.

-- 
With Best Regards,
Andy Shevchenko