[U-Boot] [PATCH v2 7/7] video_display: Add Xilinx LogiCore DP TX

[Mario Six]

Hi Anatolij,

On Sat, May 26, 2018 at 12:24 AM, Anatolij Gustschin <agust at 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 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.
>

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 handling
>
> please 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