[haiku-commits] Re: haiku: hrev43437 - src/add-ons/accelerants/radeon_hd
- From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Fri, 09 Dec 2011 22:55:30 +0100
Hey Alex,
just the usual coding style comments...
On 08.12.2011 21:04, kallisti5@xxxxxxxxxxx wrote:
+++ b/src/add-ons/accelerants/radeon_hd/connector.cpp
[...]
+#include "accelerant_protos.h"
+#include "accelerant.h"
+#include "bios.h"
+#include "connector.h"
+#include "gpu.h"
+#include "utility.h"
+
+#include<Debug.h>
Our headers should have a different order: "connector.h" is the header
that matches the source file. This should come first in order to make
sure it's self contained.
Then the headers in blocks of most general to most specific, ie.
<Debug.h> would make the first block, then your other local headers.
+ gpio_info *info = (gpio_info*)cookie;
Mix of asterisk style.
+ radeon_shared_info&sinfo = *gInfo->shared_info;
"sinfo" is not a good name.
+ // mask GPIO pins for software use
+ buffer = Read32(OUT, info->mask_scl_reg);
+ if (lock == true) {
+ buffer |= info->mask_scl_mask;
+ } else {
+ buffer&= ~info->mask_scl_mask;
+ }
Extraneous {}.
+ uint32 scl = Read32(OUT, info->y_scl_reg)
+ & info->y_scl_mask;
+ uint32 sda = Read32(OUT, info->y_sda_reg)
+ & info->y_sda_mask;
These definitely fit on a single line -- why break them?
There are lots of those in the diff.
+ *_clock = (scl != 0);
+ *_data = (sda != 0);
Extraneous ().
+++ b/src/add-ons/accelerants/radeon_hd/connector.h
@@ -0,0 +1,15 @@
+/*
+ * Copyright 2006-2011, Haiku, Inc. All Rights Reserved.
+ * Distributed under the terms of the MIT License.
+ *
+ * Authors:
+ * Alexander von Gluck, kallisti5@xxxxxxxxxxx
+ */
+#ifndef RADEON_HD_CONNECTOR_H
+#define RADEON_HD_CONNECTOR_H
+
+status_t gpio_probe();
+status_t connector_attach_gpio(uint32 id, uint8 hw_line);
+bool connector_read_edid(uint32 connector, edid1_info *edid);
+
+#endif
Two blank lines for each. The header is not self contained.
- struct _ATOM_GPIO_I2C_INFO *i2c_info
This does not follow our naming style.
+++ b/src/add-ons/accelerants/radeon_hd/connector.cpp
@@ -355,3 +355,399 @@ gpio_probe()
return B_OK;
}
+
+
+union atom_supported_devices {
+ struct _ATOM_SUPPORTED_DEVICES_INFO info;
+ struct _ATOM_SUPPORTED_DEVICES_INFO_2 info_2;
+ struct _ATOM_SUPPORTED_DEVICES_INFO_2d1 info_2d1;
+};
+
+
+status_t
+connector_probe_legacy()
Don't mix structure/union definitions with functions. They should be at
the beginning of the file.
+ obj_header = (ATOM_OBJECT_HEADER *)(gAtomContext->bios + tableOffset);
+ path_obj = (ATOM_DISPLAY_OBJECT_PATH_TABLE *)
+ (gAtomContext->bios + tableOffset
+ +
B_LENDIAN_TO_HOST_INT16(obj_header->usDisplayPathTableOffset));
+ con_obj = (ATOM_CONNECTOR_OBJECT_TABLE *)
+ (gAtomContext->bios + tableOffset
+ +
B_LENDIAN_TO_HOST_INT16(obj_header->usConnectorObjectTableOffset));
+ enc_obj = (ATOM_ENCODER_OBJECT_TABLE *)
+ (gAtomContext->bios + tableOffset
+ +
B_LENDIAN_TO_HOST_INT16(obj_header->usEncoderObjectTableOffset));
+ router_obj = (ATOM_OBJECT_TABLE *)
+ (gAtomContext->bios + tableOffset
+ +
B_LENDIAN_TO_HOST_INT16(obj_header->usRouterObjectTableOffset));
Bad names and not style conforming, all of them.
+ uint8 con_obj_id =
(B_LENDIAN_TO_HOST_INT16(path->usConnObjectId)
+ & OBJECT_ID_MASK)>> OBJECT_ID_SHIFT;
And another one.
+ uint16 igp_lane_info;
And another one.
+ if (0)
+ ERROR("%s: TODO: IGP chip connector
detection\n", __func__);
Why not a TODO comment??
+ uint8 grph_obj_type
And more.
+ uint16 encoder_obj
And more.
+ switch(connectorType) {
Not sure if Thunderbird messed this up again, or if there is a space
missing.
+ } // END for each valid connector
+ } // end for each display path
We don't do that.
+const int connector_convert_legacy[] = {
[...]
+const int connector_convert[] = {
Not style guide conforming -> kConnectorConvert[Legacy].
Bye,
Axel.
Other related posts:
