From mboxrd@z Thu Jan  1 00:00:00 1970
From: Anatolij Gustschin <agust@denx.de>
Date: Sat, 26 May 2018 00:24:08 +0200
Subject: [U-Boot] [PATCH v2 7/7] video_display: Add Xilinx LogiCore DP TX
In-Reply-To: <20180523121047.28330-7-mario.six@gdsys.cc>
References: <20180523121047.28330-1-mario.six@gdsys.cc>
 <20180523121047.28330-7-mario.six@gdsys.cc>
Message-ID: <20180526002408.5f4b78db@crub>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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

On Wed, 23 May 2018 14:10:47 +0200
Mario Six mario.six at 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.

...
> 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
	 */

...
> +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 handling

please no C++ comments.

...
> +bool is_connected(struct udevice *dev)
> +{

shouldn't it be static?

...
> +
> +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.

...
> +int logicore_dp_tx_enable(struct udevice *dev, int panel_bpp,
> +			  const struct display_timing *timing)
> +{

should be static?

...
> +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.

...
> 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.

...
> +/*
> + * PHY_STATUS_ALL_LANES_READY_MASK seems zo be missing lanes 0 and 1 in

s/zo/to  ?

Thanks,

--
Anatolij