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@xxxxxxxxxxxxxxxxxxxxx > >> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
