Re: agp_nvidia port for review

[Andrius V <vezhlys%gmail.com@localhost>]

On Tue, Jun 9, 2026 at 9:47 AM Valery Ushakov <uwe%stderr.spb.ru@localhost> wrote:
>
> On Tue, Jun 09, 2026 at 01:29:10 +0300, Andrius V wrote:
>
> > > > > Why the intermediate val, and the cast?  Why not just:
> > > > >
> > > > >         switch (pci_conf_read(sc->as_pc, sc->as_tag, AGP_NVIDIA_0_APSIZE) &
> > > > >             0xff) {
> > > > >
> > > >
> > > > I think I felt that it was a very "bulky" statement for a switch key.
> > >
> > > I'd agree.  A better variable name than "val" would also help.  For a
> > > drive-by reader it's also not so clear what does 0xff do.  Is it an
> > > expected part of obtaining the `apsize` value, or does does it select
> > > a subfield, or something...  In the one-liner - "APSIZE" is hidden in
> > > the middle of a long line, closer to the rigtht marging, and further
> > > obscured by the all-caps prefix before it and the bitmask after it.
> > > `switch (apsize)` OTOH, is easier to read.
> >
> > Renamed variable to reg and used pcireg_t type to make consistent with
> > agp_nvidia_set_aperture() variable name.
> > The mask was supposed to be 0x0f to take the lower four bits of the
> > aperture size field. Fixed that...
>
> Thats's a _horrible_ name :)
>
> Looking at agp_nvidia_set_aperture I guess the right name for this
> variable is "key", if only for consistency with the inverse function.
>
> prcireg_t is what you "physically" read (or write, in the setter), but
> the "key" you are extracting from it and looking at here is something
> else.
>
> -uwe

Yeah, you are right! But "key" is likely also bad name :). Will check
how to change later today.

Regards,
Andrius V