[haiku-commits] Re: haiku: hrev43437 - src/add-ons/accelerants/radeon_hd
- From: Alexander von Gluck <kallisti5@xxxxxxxxxxx>
- To: <haiku-commits@xxxxxxxxxxxxx>
- Date: Fri, 09 Dec 2011 21:29:14 -0600
On Fri, 09 Dec 2011 22:55:30 +0100, Axel Dörfler wrote:
On 08.12.2011 21:04, kallisti5@xxxxxxxxxxx wrote:
hrev43450.. one style cleanup to rule them all.
+++ 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.
done in 6dae5a
+ gpio_info *info = (gpio_info*)cookie;
Mix of asterisk style.
done in 6142e9, entire accelerant cleaned up.
+ radeon_shared_info&sinfo = *gInfo->shared_info;
"sinfo" is not a good name.
done in 6142e9
+ // 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 {}.
done in fcaa2c
+ 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.
done in fcaa2c
+ *_clock = (scl != 0);
+ *_data = (sda != 0);
Extraneous ().
done in 6dae5a
+++ 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.
Was already fixed in a recent commit.
- struct _ATOM_GPIO_I2C_INFO *i2c_info
This does not follow our naming style.
fixed in 6904bb
+++ 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.
The reasoning behind this is that the union happens directly above the
function
that uses it, as these unions are one-time for AtomBIOS calls it made
sense
as it makes them easy to find. Can move though.
+ 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.
fixed in 6904bb
+ uint8 con_obj_id =
(B_LENDIAN_TO_HOST_INT16(path->usConnObjectId)
+ & OBJECT_ID_MASK)>> OBJECT_ID_SHIFT;
And another one.
fixed in 6904bb
+ uint16 igp_lane_info;
And another one.
fixed in 980490
+ if (0)
+ ERROR("%s: TODO: IGP chip connector
detection\n", __func__);
Why not a TODO comment??
Because I wanted to remember the code below wasn't called once IGP
stuff was done.
Fixed in 980490
+ uint8 grph_obj_type
And more.
fixed in 6904bb
+ uint16 encoder_obj
And more.
fixed in 6904bb
+ switch(connectorType) {
Not sure if Thunderbird messed this up again, or if there is a space
missing.
fixed in d4e792
+ } // END for each valid connector
+ } // end for each display path
We don't do that.
That was my comment... did you see how many levels deep that for loop
was? :)
+const int connector_convert_legacy[] = {
[...]
+const int connector_convert[] = {
Not style guide conforming -> kConnectorConvert[Legacy].
done in 980490
Other related posts:
