Hi Thomas.
> >> @@ -1161,9 +1174,11 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
> >> }
> >>
> >> WREG_ECRT(0, ext_vga[0]);
> >> - /* Enable mga pixel clock */
> >> - misc = 0x2d;
> >>
> >> + misc = RREG8(MGA_MISC_IN);
> >> + misc |= MGAREG_MISC_IOADSEL |
> >> + MGAREG_MISC_RAMMAPEN |
> >> + MGAREG_MISC_HIGH_PG_SEL;
> >> WREG8(MGA_MISC_OUT, misc);
> >
> > I am left puzzeled why there is several writes to MGA_MISC_OUT.
> > The driver needs to read back and then write again.
> >
> > Would it be simpler to have one function that only took care of
> > misc register update?
>
> I'm not quite sure what you mean. MISC contains all kinds of unrelated
> state. In the final atomic code, different state is set in different
> places. It's simple to have a function read-modify-write the content of
> MISC for the bits that it cares about. With multiple functions, that
> leads to some duplicated reads and write, but the code as a whole is simple.
OK, when I looked at the code I initially got the impression that it was
a bit random. But then I also switched between patch and code etc.
The explanation above makes sense.
Sam
>
> Best regards
> Thomas
>
> >
> >>
> >> mga_crtc_do_set_base(mdev, fb, old_fb);
> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h
> >> b/drivers/gpu/drm/mgag200/mgag200_reg.h
> >> index c096a9d6bcbc1..89e12c55153cf 100644
> >> --- a/drivers/gpu/drm/mgag200/mgag200_reg.h
> >> +++ b/drivers/gpu/drm/mgag200/mgag200_reg.h
> >> @@ -16,10 +16,11 @@
> >> * MGA1064SG Mystique register file
> >> */
> >>
> >> -
> >> #ifndef _MGA_REG_H_
> >> #define _MGA_REG_H_
> >>
> >> +#include <linux/bits.h>
> >> +
> >> #define MGAREG_DWGCTL 0x1c00
> >> #define MGAREG_MACCESS 0x1c04
> >> /* the following is a mystique only register */
> >> @@ -227,6 +228,8 @@
> >> #define MGAREG_MISC_CLK_SEL_MGA_MSK (0x3 << 2)
> >> #define MGAREG_MISC_VIDEO_DIS (0x1 << 4)
> >> #define MGAREG_MISC_HIGH_PG_SEL (0x1 << 5)
> >> +#define MGAREG_MISC_HSYNCPOL BIT(6)
> >> +#define MGAREG_MISC_VSYNCPOL BIT(7)
> > I like BIT(), but mixing (1 << N) and BIT() does not look nice.
> > Oh, and do not get me started on GENMASK() :-)
> >
> > Sam
> >>
> >> /* MMIO VGA registers */
> >> #define MGAREG_SEQ_INDEX 0x1fc4
> >> --
> >> 2.26.0
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
