From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756625AbbICFJP (ORCPT ); Thu, 3 Sep 2015 01:09:15 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:40608 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755040AbbICFJN (ORCPT ); Thu, 3 Sep 2015 01:09:13 -0400 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 X-AuditID: cbfec7f5-f794b6d000001495-f1-55e7d5f594fc Content-transfer-encoding: 8BIT Subject: Re: [PATCH v4 01/16] drm: exynos/dp: fix code style To: Yakir Yang , Heiko Stuebner , Thierry Reding , Jingoo Han , Inki Dae , joe@perches.com, Kukjin Kim , Mark Yao References: <1441086371-24838-1-git-send-email-ykk@rock-chips.com> <1441086399-24889-1-git-send-email-ykk@rock-chips.com> <55E79297.3050203@samsung.com> <55E7D4E4.6070808@rock-chips.com> Cc: Russell King , djkurtz@chromium.com, dianders@chromium.com, seanpaul@chromium.com, ajaynumb@gmail.com, Andrzej Hajda , Kyungmin Park , David Airlie , Gustavo Padovan , Andy Yan , Kumar Gala , Ian Campbell , Rob Herring , Pawel Moll , Kishon Vijay Abraham I , architt@codeaurora.org, robherring2@gmail.com, dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org From: Krzysztof Kozlowski Message-id: <55E7D5EB.1060505@samsung.com> Date: Thu, 03 Sep 2015 14:08:59 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 In-reply-to: <55E7D4E4.6070808@rock-chips.com> X-Brightmail-Tracker: H4sIAAAAAAAAA02Sa0hTYRjHec/lPUdzdVpZL0YYi5Ru2g15Cbt8KDp9iYIG3cCWHdTmpmwa GVRrapmmjlzlZhe7iKVWNsPM8JJIpum0vAwrmyi2bHOWTGeJmnOEffs////v4fl/eFhSnEEH sDHKBEGllMVKoC/1fqqhY/1op026wfokGH96aqZxprmRwKWOeyTObu0GWJs2ROO79TPBm+8p EKe5swncMToMcbbjHo1HilMZPN1np7H5x2OAr1l1FH7kMjA4z9pPYfv3jTi7307itoGrELdo HQw29XfRuL3yFsQjvdMkzm2tJnCFvpbAuptPKFz2cxDi1Kp6BrvHxiD+8MDK4M8T87FZk8Ps XMGX3CkBfEryVcjbKu0E356VSfCTNgvFvzL2MPzjQhfkTUVXIP9yrJfmrRkNBF/28AKfmeyE vKuoi+QnjTUUn/WiCPAvu+6Q+8VHfMNPCrExpwVV6PbjvtH142YY/27dmYb7DkYDnIHpwIdF 3BaU49JCr16C2r4+m9G+rJgrAOjB5A3SE4i4hWg85yuVDliW5AJR/Ue5VwYjvV7hxV0A9Vn+ UB58EReOqkr1s6uLuTQCvbbs8ULVANWOTdGegeQMEP0utgAPBbnNqKzwIfQeW4MuNg8AzwWK W4XcnXs9tj93CDW1DM0iPlwIuv/oNqMDnPG/esa5esa5evmALAL+QmJkvPpElGJTiFqmUCcq o0Ii4xQm4H2R0QpQ8HZrHeBYIPET6fJsUjEtO61OUtQBxJKSxaLaCzOW6KQs6aygiotQJcYK 6jqwjKUkS0WGyuGDYi5KliDIBSFeUP1LCdYnQAMMFvmXoOTDAeS3Y7tDm3+FrTZMODdd37e2 UPv8QHlPY1jM5fO54Z07TEsoS6ZV17PNbpBNzVtVtuKF0g27+7o1TRXKlKBbbYzJMajNXxkg OPUOP1MF67A23phIZzsuHa0Ryk39E19GpysjHIcG5eQim3T5LkPCAiANLDl27pSEUkfLNq4h VWrZX0vw7lkeAwAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03.09.2015 14:04, Yakir Yang wrote: > Hi Krzysztof, > > 在 09/03/2015 08:21 AM, Krzysztof Kozlowski 写道: >> On 01.09.2015 14:46, Yakir Yang wrote: >>> After run "checkpatch.pl -f --subjective" command, I see there >>> are lots of alignment problem in exynos_dp driver, so let just >>> fix them. >> Hi, >> >> Warnings from checkpatch are not a reason for a commit. Reason for a >> commit could be for example an unreadable code, violation of >> coding-style leading to decrease in code maintainability or just >> improving the code readability so it will be easier to review and >> maintain it. >> >> You do not make commits because some tool tells you that. We do not >> listen to machines :) ... If that would be the case, the commit could be >> made automatically, without human interaction. Such automated commit >> could be even easily tested by the machine by comparing object files. >> >> Especially that you enabled "subjective" rule. This is not a valid >> motivation for a commit. >> >> Please rephrase this to sensible reason and convince that change is >> worth the effort. > > Oh, nice, thanks for your remind. I would rephrase the commit. > >>> - Take Romain suggest, rebase on linux-next branch >> That comment seems unrelated to the commit. Please remove it. > > Done, > >> >>> Signed-off-by: Yakir Yang >>> --- >>> Changes in v4: None >>> Changes in v3: None >>> Changes in v2: >>> - Take Joe Preches advise, improved commit message more readable, and >>> avoid using some uncommon style like bellow: >>> - retval = exynos_dp_read_bytes_from_i2c(... >>> ...) >>> + retval = >>> + exynos_dp_read_bytes_from_i2c(......); >>> >>> drivers/gpu/drm/exynos/exynos_dp_core.c | 226 >>> ++++++++++++++++---------------- >>> drivers/gpu/drm/exynos/exynos_dp_core.h | 54 ++++---- >>> drivers/gpu/drm/exynos/exynos_dp_reg.c | 106 +++++++-------- >>> 3 files changed, 188 insertions(+), 198 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c >>> b/drivers/gpu/drm/exynos/exynos_dp_core.c >>> index d66ade0..266f7f7 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c >>> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c >>> @@ -115,8 +115,8 @@ static int exynos_dp_read_edid(struct >>> exynos_dp_device *dp) >>> /* Read Extension Flag, Number of 128-byte EDID extension >>> blocks */ >>> retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR, >>> - EDID_EXTENSION_FLAG, >>> - &extend_block); >>> + EDID_EXTENSION_FLAG, >>> + &extend_block); >>> if (retval) >>> return retval; >>> @@ -124,10 +124,11 @@ static int exynos_dp_read_edid(struct >>> exynos_dp_device *dp) >>> dev_dbg(dp->dev, "EDID data includes a single extension!\n"); >>> /* Read EDID data */ >>> - retval = exynos_dp_read_bytes_from_i2c(dp, >>> I2C_EDID_DEVICE_ADDR, >>> - EDID_HEADER_PATTERN, >>> - EDID_BLOCK_LENGTH, >>> - &edid[EDID_HEADER_PATTERN]); >>> + retval = exynos_dp_read_bytes_from_i2c( >>> + dp, I2C_EDID_DEVICE_ADDR, >>> + EDID_HEADER_PATTERN, >>> + EDID_BLOCK_LENGTH, >>> + &edid[EDID_HEADER_PATTERN]); >>> if (retval != 0) { >>> dev_err(dp->dev, "EDID Read failed!\n"); >>> return -EIO; >>> @@ -139,11 +140,11 @@ static int exynos_dp_read_edid(struct >>> exynos_dp_device *dp) >>> } >>> /* Read additional EDID data */ >>> - retval = exynos_dp_read_bytes_from_i2c(dp, >>> - I2C_EDID_DEVICE_ADDR, >>> - EDID_BLOCK_LENGTH, >>> - EDID_BLOCK_LENGTH, >>> - &edid[EDID_BLOCK_LENGTH]); >>> + retval = exynos_dp_read_bytes_from_i2c( >>> + dp, I2C_EDID_DEVICE_ADDR, >>> + EDID_BLOCK_LENGTH, >>> + EDID_BLOCK_LENGTH, >>> + &edid[EDID_BLOCK_LENGTH]); >>> if (retval != 0) { >>> dev_err(dp->dev, "EDID Read failed!\n"); >>> return -EIO; >>> @@ -155,24 +156,22 @@ static int exynos_dp_read_edid(struct >>> exynos_dp_device *dp) >>> } >>> exynos_dp_read_byte_from_dpcd(dp, DP_TEST_REQUEST, >>> - &test_vector); >>> + &test_vector); >>> if (test_vector & DP_TEST_LINK_EDID_READ) { >>> - exynos_dp_write_byte_to_dpcd(dp, >>> - DP_TEST_EDID_CHECKSUM, >>> + exynos_dp_write_byte_to_dpcd( >>> + dp, DP_TEST_EDID_CHECKSUM, >>> edid[EDID_BLOCK_LENGTH + EDID_CHECKSUM]); >>> - exynos_dp_write_byte_to_dpcd(dp, >>> - DP_TEST_RESPONSE, >>> + exynos_dp_write_byte_to_dpcd( >>> + dp, DP_TEST_RESPONSE, >>> DP_TEST_EDID_CHECKSUM_WRITE); >> To me, missing argument after opening parenthesis, looks worse. I would >> prefer: >> >> exynos_dp_write_byte_to_dpcd(dp, >> >> Why you moved the 'dp' argument to new line? > > Hmm... Just like style tool indicate, no more warning after > that change. > > For now, I would like to follow the original style, just improved > some obvious style problem. :-) What was the checkpatch warning that said 'dp' has to move to new line? I tried this and I don't see it. Best regards, Krzysztof From mboxrd@z Thu Jan 1 00:00:00 1970 From: k.kozlowski@samsung.com (Krzysztof Kozlowski) Date: Thu, 03 Sep 2015 14:08:59 +0900 Subject: [PATCH v4 01/16] drm: exynos/dp: fix code style In-Reply-To: <55E7D4E4.6070808@rock-chips.com> References: <1441086371-24838-1-git-send-email-ykk@rock-chips.com> <1441086399-24889-1-git-send-email-ykk@rock-chips.com> <55E79297.3050203@samsung.com> <55E7D4E4.6070808@rock-chips.com> Message-ID: <55E7D5EB.1060505@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 03.09.2015 14:04, Yakir Yang wrote: > Hi Krzysztof, > > ? 09/03/2015 08:21 AM, Krzysztof Kozlowski ??: >> On 01.09.2015 14:46, Yakir Yang wrote: >>> After run "checkpatch.pl -f --subjective" command, I see there >>> are lots of alignment problem in exynos_dp driver, so let just >>> fix them. >> Hi, >> >> Warnings from checkpatch are not a reason for a commit. Reason for a >> commit could be for example an unreadable code, violation of >> coding-style leading to decrease in code maintainability or just >> improving the code readability so it will be easier to review and >> maintain it. >> >> You do not make commits because some tool tells you that. We do not >> listen to machines :) ... If that would be the case, the commit could be >> made automatically, without human interaction. Such automated commit >> could be even easily tested by the machine by comparing object files. >> >> Especially that you enabled "subjective" rule. This is not a valid >> motivation for a commit. >> >> Please rephrase this to sensible reason and convince that change is >> worth the effort. > > Oh, nice, thanks for your remind. I would rephrase the commit. > >>> - Take Romain suggest, rebase on linux-next branch >> That comment seems unrelated to the commit. Please remove it. > > Done, > >> >>> Signed-off-by: Yakir Yang >>> --- >>> Changes in v4: None >>> Changes in v3: None >>> Changes in v2: >>> - Take Joe Preches advise, improved commit message more readable, and >>> avoid using some uncommon style like bellow: >>> - retval = exynos_dp_read_bytes_from_i2c(... >>> ...) >>> + retval = >>> + exynos_dp_read_bytes_from_i2c(......); >>> >>> drivers/gpu/drm/exynos/exynos_dp_core.c | 226 >>> ++++++++++++++++---------------- >>> drivers/gpu/drm/exynos/exynos_dp_core.h | 54 ++++---- >>> drivers/gpu/drm/exynos/exynos_dp_reg.c | 106 +++++++-------- >>> 3 files changed, 188 insertions(+), 198 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c >>> b/drivers/gpu/drm/exynos/exynos_dp_core.c >>> index d66ade0..266f7f7 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c >>> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c >>> @@ -115,8 +115,8 @@ static int exynos_dp_read_edid(struct >>> exynos_dp_device *dp) >>> /* Read Extension Flag, Number of 128-byte EDID extension >>> blocks */ >>> retval = exynos_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR, >>> - EDID_EXTENSION_FLAG, >>> - &extend_block); >>> + EDID_EXTENSION_FLAG, >>> + &extend_block); >>> if (retval) >>> return retval; >>> @@ -124,10 +124,11 @@ static int exynos_dp_read_edid(struct >>> exynos_dp_device *dp) >>> dev_dbg(dp->dev, "EDID data includes a single extension!\n"); >>> /* Read EDID data */ >>> - retval = exynos_dp_read_bytes_from_i2c(dp, >>> I2C_EDID_DEVICE_ADDR, >>> - EDID_HEADER_PATTERN, >>> - EDID_BLOCK_LENGTH, >>> - &edid[EDID_HEADER_PATTERN]); >>> + retval = exynos_dp_read_bytes_from_i2c( >>> + dp, I2C_EDID_DEVICE_ADDR, >>> + EDID_HEADER_PATTERN, >>> + EDID_BLOCK_LENGTH, >>> + &edid[EDID_HEADER_PATTERN]); >>> if (retval != 0) { >>> dev_err(dp->dev, "EDID Read failed!\n"); >>> return -EIO; >>> @@ -139,11 +140,11 @@ static int exynos_dp_read_edid(struct >>> exynos_dp_device *dp) >>> } >>> /* Read additional EDID data */ >>> - retval = exynos_dp_read_bytes_from_i2c(dp, >>> - I2C_EDID_DEVICE_ADDR, >>> - EDID_BLOCK_LENGTH, >>> - EDID_BLOCK_LENGTH, >>> - &edid[EDID_BLOCK_LENGTH]); >>> + retval = exynos_dp_read_bytes_from_i2c( >>> + dp, I2C_EDID_DEVICE_ADDR, >>> + EDID_BLOCK_LENGTH, >>> + EDID_BLOCK_LENGTH, >>> + &edid[EDID_BLOCK_LENGTH]); >>> if (retval != 0) { >>> dev_err(dp->dev, "EDID Read failed!\n"); >>> return -EIO; >>> @@ -155,24 +156,22 @@ static int exynos_dp_read_edid(struct >>> exynos_dp_device *dp) >>> } >>> exynos_dp_read_byte_from_dpcd(dp, DP_TEST_REQUEST, >>> - &test_vector); >>> + &test_vector); >>> if (test_vector & DP_TEST_LINK_EDID_READ) { >>> - exynos_dp_write_byte_to_dpcd(dp, >>> - DP_TEST_EDID_CHECKSUM, >>> + exynos_dp_write_byte_to_dpcd( >>> + dp, DP_TEST_EDID_CHECKSUM, >>> edid[EDID_BLOCK_LENGTH + EDID_CHECKSUM]); >>> - exynos_dp_write_byte_to_dpcd(dp, >>> - DP_TEST_RESPONSE, >>> + exynos_dp_write_byte_to_dpcd( >>> + dp, DP_TEST_RESPONSE, >>> DP_TEST_EDID_CHECKSUM_WRITE); >> To me, missing argument after opening parenthesis, looks worse. I would >> prefer: >> >> exynos_dp_write_byte_to_dpcd(dp, >> >> Why you moved the 'dp' argument to new line? > > Hmm... Just like style tool indicate, no more warning after > that change. > > For now, I would like to follow the original style, just improved > some obvious style problem. :-) What was the checkpatch warning that said 'dp' has to move to new line? I tried this and I don't see it. Best regards, Krzysztof