Hi Anatolij,
On Sat, May 26, 2018 at 12:24 AM, Anatolij Gustschin agust@denx.de wrote:
Hi Mario,
Please test the patch with ./scripts/checkpatch.pl, there are warnings/ errors reported that need fixing: ... total: 1 errors, 31 warnings, 26 checks, 2746 lines checked
Right, sorry, will be fixed in v3.
On Wed, 23 May 2018 14:10:47 +0200 Mario Six mario.six@gdsys.cc wrote: ...
diff --git a/drivers/video/Makefile b/drivers/video/Makefile index cf7ad281c3b..fa4ac715fcf 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_IVYBRIDGE_IGD) += ivybridge_igd.o
obj-$(CONFIG_ATI_RADEON_FB) += ati_radeon_fb.o videomodes.o obj-$(CONFIG_ATMEL_HLCD) += atmel_hlcdfb.o +obj-$(CONFIG_LOGICORE_DP_TX) += logicore_dp_tx.o
Please add new driver entry sorted alphabetically. I know there are already not sorted entries in the list here, but we shouldn't add more.
OK, I'll add a patch that sorts the entries, and add the new entry with respect to the new ordering.
...
diff --git a/drivers/video/logicore_dp_dpcd.h b/drivers/video/logicore_dp_dpcd.h new file mode 100644 index 00000000000..68582945514 --- /dev/null +++ b/drivers/video/logicore_dp_dpcd.h
...
+struct main_stream_attributes {
/* Pixel clock of the stream (in Hz). */u32 pixel_clock_hz;/* Miscellaneous stream attributes 0 as specified by the DisplayPort* 1.2 specification.*/Please fix multi-line comment style throughout this patch, we use this style: /* * multi-line * comment */
Will be fixed in v3.
...
+static u32 get_reg(struct udevice *dev, u32 reg) +{
struct dp_tx *dp_tx = dev_get_priv(dev);u32 value = 0;int res;// TODO error handlingplease no C++ comments.
Overlooked that one. Will be fixed in v3.
...
+bool is_connected(struct udevice *dev) +{
shouldn't it be static?
Yes, good idea, I'll declare it static for v3.
...
+bool is_link_rate_valid(struct udevice *dev, u8 link_rate) +{
...
+bool is_lane_count_valid(struct udevice *dev, u8 lane_count) +{
Are these functions supposed to be used externally? If not, please add static. Otherwise move them to /* external API */ section.
No, the U-Boot driver doesn't use them outside the file. I'll mark them static in v3.
...
+int logicore_dp_tx_enable(struct udevice *dev, int panel_bpp,
const struct display_timing *timing)+{
should be static?
Dito.
...
+int logicore_dp_tx_probe(struct udevice *dev) +{
should be static?
...
+struct dm_display_ops logicore_dp_tx_ops = {
.enable = logicore_dp_tx_enable,+};
should be static, too.
Dito.
...
diff --git a/drivers/video/logicore_dp_tx_regif.h b/drivers/video/logicore_dp_tx_regif.h new file mode 100644 index 00000000000..c4105605c9b --- /dev/null +++ b/drivers/video/logicore_dp_tx_regif.h
...
/* core ID */REG_VERSION = 0x0F8,REG_CORE_ID = 0x0FC,here a space and tabs follow '=', please use tabs only.
...
REG_USER_PIXEL_WIDTH = 0x1B8,REG_USER_DATA_COUNT_PER_LANE =0x1BC,spaces around '=', checkpatch will report style issues like this.
I'll fix the whitespace issues in v3.
...
+/*
- PHY_STATUS_ALL_LANES_READY_MASK seems zo be missing lanes 0 and 1 in
s/zo/to ?
Yeah, that's a typo. Will fix in v3.
Thanks,
Thanks for reviewing, and best regards, Mario