[haiku-commits] Re: r42828 - haiku/trunk/src/add-ons/accelerants/radeon_hd
- From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Wed, 12 Oct 2011 20:00:28 +0200
Hi Alex,
just a look over your shoulder, after some time again.
On 12.10.2011 19:24, kallisti5@xxxxxxxxxxx wrote:
+++ haiku/trunk/src/add-ons/accelerants/radeon_hd/encoder.cpp 2011-10-12
17:24:15 UTC (rev 42828)
@@ -37,7 +37,7 @@
void
-encoder_assign_crtc(uint8 id)
+encoder_assign_crtc(uint8 crtc_id)
That would be crtcID.
int index = GetIndexIntoMasterTable(COMMAND, SelectCRTC_Source);
What exactly is SelectCRTC_Source, and where does it come from?
If it's a constant it should be named like a constant, if it's a define,
it should be named like a define.
+ uint16 connector_index = gDisplay[crtc_id]->connector_index;
uint16 encoder_id = gConnector[connector_index]->encoder.object_id;
That would be connectorIndex, and encoderID.
switch (frev) {
@@ -58,7 +58,7 @@
switch (crev) {
'frev' and 'crev' sound like bad naming to me, at least I would have no
idea what they are for.
void
+encoder_crtc_scratch(uint8 crtc_id)
+{
+ TRACE("%s\n", __func__);
+
+ uint32 connector_index = gDisplay[crtc_id]->connector_index;
+ uint32 encoder_flags = gConnector[connector_index]->encoder.flags;
> +
> + // TODO : r500
> + uint32 bios_3_scratch = Read32(OUT, R600_BIOS_3_SCRATCH);
Same issues here. Besides, R600_BIOS_3_SCRATCH doesn't seem to be a
scratch register at all, if it has a functionality - how about a better
name?
+ if (encoder_flags& ATOM_DEVICE_TV1_SUPPORT) {
+ bios_3_scratch&= ~ATOM_S3_TV1_CRTC_ACTIVE;
+ bios_3_scratch |= (crtc_id<< 18);
Coding style - what the hell happened to the spacing? The above should
look like this (the rest of the lines have the same issues):
if ((encoderFlags & ATOM_DEVICE_TV1_SUPPORT) != 0) {
bios3Scratch &= ~ATOM_S3_TV1_CRTC_ACTIVE;
bios3Scratch |= (crtcID << 18);
+ if (encoder_flags& ATOM_DEVICE_LCD_SUPPORT)
> + if (encoder_flags& ATOM_DEVICE_LCD_SUPPORT) {
> + if (encoder_flags& ATOM_DEVICE_LCD_SUPPORT) {
How can this happen so often? Same for the missing ((a & b) != 0).
+++ haiku/trunk/src/add-ons/accelerants/radeon_hd/pll.cpp 2011-10-12
17:24:15 UTC (rev 42828)
@@ -267,12 +267,14 @@
{
uint32 connector_index = gDisplay[crtc_id]->connector_index;
pll_info *pll =&gConnector[connector_index]->encoder.pll;
Missing space.
+
pll->pixel_clock = pixelClock;
pll->id = pll_id;
pll_id -> pllID.
+ // TODO : BPC == Digital Depth, EDID 1.4+ on digital displays
BTW unless you're French, there is no space before the colon :-)
+ switch (bpc) {
'bpc' is a bad name, too.
I hope you'll manage not to regress into those bad habits when no one
points a finger at you constantly in the future, some day. Please?
Bye,
Axel.
Other related posts:
