[PATCH 0/5] GPU-DRM-STI: Fine-tuning for some function implementations

[PATCH 0/5] GPU-DRM-STI: Fine-tuning for some function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 15:45:45 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Reduce function calls for sequence output at five places
  Replace 17 seq_puts() calls by seq_putc()
  Fix a typo in a comment line
  Fix typos in a comment line
  Adjust two checks for null pointers in sti_hqvdp_probe()

 drivers/gpu/drm/sti/sti_cursor.c |  5 ++---
 drivers/gpu/drm/sti/sti_dvo.c    |  3 +--
 drivers/gpu/drm/sti/sti_gdp.c    |  3 +--
 drivers/gpu/drm/sti/sti_hda.c    |  9 +++------
 drivers/gpu/drm/sti/sti_hdmi.c   | 23 ++++++++++-------------
 drivers/gpu/drm/sti/sti_hqvdp.c  |  7 +++----
 drivers/gpu/drm/sti/sti_mixer.c  |  3 +--
 drivers/gpu/drm/sti/sti_tvout.c  |  7 +++----
 drivers/gpu/drm/sti/sti_vid.c    |  5 ++---
 9 files changed, 26 insertions(+), 39 deletions(-)

-- 
2.12.2

[PATCH 1/5] drm/sti: Reduce function calls for sequence output at five places

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 14:54:52 +0200

Some data were put into a sequence by separate function calls.
Print the same data by five single function calls instead.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/sti/sti_gdp.c  | 3 +--
 drivers/gpu/drm/sti/sti_hda.c  | 6 ++----
 drivers/gpu/drm/sti/sti_hdmi.c | 6 ++----
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
index 88f16cdf6a4b..3caced5f2e86 100644
--- a/drivers/gpu/drm/sti/sti_gdp.c
+++ b/drivers/gpu/drm/sti/sti_gdp.c
@@ -266,8 +266,7 @@ static void gdp_node_dump_node(struct seq_file *s, struct sti_gdp_node *node)
 	seq_printf(s, "\n\tKEY2 0x%08X", node->gam_gdp_key2);
 	seq_printf(s, "\n\tPPT  0x%08X", node->gam_gdp_ppt);
 	gdp_dbg_ppt(s, node->gam_gdp_ppt);
-	seq_printf(s, "\n\tCML  0x%08X", node->gam_gdp_cml);
-	seq_puts(s, "\n");
+	seq_printf(s, "\n\tCML  0x%08X\n", node->gam_gdp_cml);
 }
 
 static int gdp_node_dbg_show(struct seq_file *s, void *arg)
diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index 0c0a75bc8bc3..e3475a17eaeb 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -320,8 +320,7 @@ static void hda_dbg_awg_microcode(struct seq_file *s, void __iomem *reg)
 {
 	unsigned int i;
 
-	seq_puts(s, "\n\n");
-	seq_puts(s, "  HDA AWG microcode:");
+	seq_puts(s, "\n\n  HDA AWG microcode:");
 	for (i = 0; i < AWG_MAX_INST; i++) {
 		if (i % 8 == 0)
 			seq_printf(s, "\n  %04X:", i);
@@ -333,8 +332,7 @@ static void hda_dbg_video_dacs_ctrl(struct seq_file *s, void __iomem *reg)
 {
 	u32 val = readl(reg);
 
-	seq_puts(s, "\n");
-	seq_printf(s, "\n  %-25s 0x%08X", "VIDEO_DACS_CONTROL", val);
+	seq_printf(s, "\n\n  %-25s 0x%08X", "VIDEO_DACS_CONTROL", val);
 	seq_puts(s, "\tHD DACs ");
 	seq_puts(s, val & DAC_CFG_HD_HZUVW_OFF_MASK ? "disabled" : "enabled");
 }
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 243905b6ae59..52cdff651c0d 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -692,8 +692,7 @@ static int hdmi_dbg_show(struct seq_file *s, void *data)
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD4, HDMI_IFRAME_SLOT_AVI);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD5, HDMI_IFRAME_SLOT_AVI);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD6, HDMI_IFRAME_SLOT_AVI);
-	seq_puts(s, "\n");
-	seq_printf(s, "\n AUDIO Infoframe (Data Island slot N=%d):",
+	seq_printf(s, "\n\n AUDIO Infoframe (Data Island slot N=%d):",
 		   HDMI_IFRAME_SLOT_AUDIO);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_HEAD_WORD, HDMI_IFRAME_SLOT_AUDIO);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD0, HDMI_IFRAME_SLOT_AUDIO);
@@ -703,8 +702,7 @@ static int hdmi_dbg_show(struct seq_file *s, void *data)
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD4, HDMI_IFRAME_SLOT_AUDIO);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD5, HDMI_IFRAME_SLOT_AUDIO);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD6, HDMI_IFRAME_SLOT_AUDIO);
-	seq_puts(s, "\n");
-	seq_printf(s, "\n VENDOR SPECIFIC Infoframe (Data Island slot N=%d):",
+	seq_printf(s, "\n\n VENDOR SPECIFIC Infoframe (Data Island slot N=%d):",
 		   HDMI_IFRAME_SLOT_VENDOR);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_HEAD_WORD, HDMI_IFRAME_SLOT_VENDOR);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD0, HDMI_IFRAME_SLOT_VENDOR);
-- 
2.12.2

[PATCH 2/5] drm/sti: Replace 17 seq_puts() calls by seq_putc()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 15:00:46 +0200

Single characters should be put into a sequence at several places.
Thus use the corresponding function "seq_putc".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/sti/sti_cursor.c |  3 +--
 drivers/gpu/drm/sti/sti_dvo.c    |  3 +--
 drivers/gpu/drm/sti/sti_hda.c    |  3 +--
 drivers/gpu/drm/sti/sti_hdmi.c   | 17 ++++++++---------
 drivers/gpu/drm/sti/sti_hqvdp.c  |  3 +--
 drivers/gpu/drm/sti/sti_mixer.c  |  3 +--
 drivers/gpu/drm/sti/sti_tvout.c  |  5 ++---
 drivers/gpu/drm/sti/sti_vid.c    |  5 ++---
 8 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c
index cca75bddb9ad..cd35b9d9de26 100644
--- a/drivers/gpu/drm/sti/sti_cursor.c
+++ b/drivers/gpu/drm/sti/sti_cursor.c
@@ -121,8 +121,7 @@ static int cursor_dbg_show(struct seq_file *s, void *data)
 	cursor_dbg_cml(s, cursor, readl(cursor->regs + CUR_CML));
 	DBGFS_DUMP(CUR_AWS);
 	DBGFS_DUMP(CUR_AWE);
-	seq_puts(s, "\n");
-
+	seq_putc(s, '\n');
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
index bb23318a44b7..24ebc6b2f34d 100644
--- a/drivers/gpu/drm/sti/sti_dvo.c
+++ b/drivers/gpu/drm/sti/sti_dvo.c
@@ -186,8 +186,7 @@ static int dvo_dbg_show(struct seq_file *s, void *data)
 	DBGFS_DUMP(DVO_LUT_PROG_MID);
 	DBGFS_DUMP(DVO_LUT_PROG_HIGH);
 	dvo_dbg_awg_microcode(s, dvo->regs + DVO_DIGSYNC_INSTR_I);
-	seq_puts(s, "\n");
-
+	seq_putc(s, '\n');
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index e3475a17eaeb..d6ed909d9d75 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -354,8 +354,7 @@ static int hda_dbg_show(struct seq_file *s, void *data)
 	hda_dbg_awg_microcode(s, hda->regs + HDA_SYNC_AWGI);
 	if (hda->video_dacs_ctrl)
 		hda_dbg_video_dacs_ctrl(s, hda->video_dacs_ctrl);
-	seq_puts(s, "\n");
-
+	seq_putc(s, '\n');
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 52cdff651c0d..a59c95a8081b 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -592,7 +592,7 @@ static void hdmi_dbg_cfg(struct seq_file *s, int val)
 {
 	int tmp;
 
-	seq_puts(s, "\t");
+	seq_putc(s, '\t');
 	tmp = val & HDMI_CFG_HDMI_NOT_DVI;
 	DBGFS_PRINT_STR("mode:", tmp ? "HDMI" : "DVI");
 	seq_puts(s, "\t\t\t\t\t");
@@ -616,7 +616,7 @@ static void hdmi_dbg_sta(struct seq_file *s, int val)
 {
 	int tmp;
 
-	seq_puts(s, "\t");
+	seq_putc(s, '\t');
 	tmp = (val & HDMI_STA_DLL_LCK);
 	DBGFS_PRINT_STR("pll:", tmp ? "locked" : "not locked");
 	seq_puts(s, "\t\t\t\t\t");
@@ -632,7 +632,7 @@ static void hdmi_dbg_sw_di_cfg(struct seq_file *s, int val)
 			       "once every field",
 			       "once every frame"};
 
-	seq_puts(s, "\t");
+	seq_putc(s, '\t');
 	tmp = (val & HDMI_IFRAME_CFG_DI_N(HDMI_IFRAME_MASK, 1));
 	DBGFS_PRINT_STR("Data island 1:", en_di[tmp]);
 	seq_puts(s, "\t\t\t\t\t");
@@ -664,16 +664,16 @@ static int hdmi_dbg_show(struct seq_file *s, void *data)
 	DBGFS_DUMP("\n", HDMI_STA);
 	hdmi_dbg_sta(s, hdmi_read(hdmi, HDMI_STA));
 	DBGFS_DUMP("", HDMI_ACTIVE_VID_XMIN);
-	seq_puts(s, "\t");
+	seq_putc(s, '\t');
 	DBGFS_PRINT_INT("Xmin:", hdmi_read(hdmi, HDMI_ACTIVE_VID_XMIN));
 	DBGFS_DUMP("", HDMI_ACTIVE_VID_XMAX);
-	seq_puts(s, "\t");
+	seq_putc(s, '\t');
 	DBGFS_PRINT_INT("Xmax:", hdmi_read(hdmi, HDMI_ACTIVE_VID_XMAX));
 	DBGFS_DUMP("", HDMI_ACTIVE_VID_YMIN);
-	seq_puts(s, "\t");
+	seq_putc(s, '\t');
 	DBGFS_PRINT_INT("Ymin:", hdmi_read(hdmi, HDMI_ACTIVE_VID_YMIN));
 	DBGFS_DUMP("", HDMI_ACTIVE_VID_YMAX);
-	seq_puts(s, "\t");
+	seq_putc(s, '\t');
 	DBGFS_PRINT_INT("Ymax:", hdmi_read(hdmi, HDMI_ACTIVE_VID_YMAX));
 	DBGFS_DUMP("", HDMI_SW_DI_CFG);
 	hdmi_dbg_sw_di_cfg(s, hdmi_read(hdmi, HDMI_SW_DI_CFG));
@@ -712,8 +712,7 @@ static int hdmi_dbg_show(struct seq_file *s, void *data)
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD4, HDMI_IFRAME_SLOT_VENDOR);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD5, HDMI_IFRAME_SLOT_VENDOR);
 	DBGFS_DUMP_DI(HDMI_SW_DI_N_PKT_WORD6, HDMI_IFRAME_SLOT_VENDOR);
-	seq_puts(s, "\n");
-
+	seq_putc(s, '\n');
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/sti/sti_hqvdp.c b/drivers/gpu/drm/sti/sti_hqvdp.c
index 66f843148ef7..372ea294da80 100644
--- a/drivers/gpu/drm/sti/sti_hqvdp.c
+++ b/drivers/gpu/drm/sti/sti_hqvdp.c
@@ -625,8 +625,7 @@ static int hqvdp_dbg_show(struct seq_file *s, void *data)
 		hqvdp_dbg_dump_cmd(s, (struct sti_hqvdp_cmd *)virt);
 	}
 
-	seq_puts(s, "\n");
-
+	seq_putc(s, '\n');
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/sti/sti_mixer.c b/drivers/gpu/drm/sti/sti_mixer.c
index 4ddc58f7fe2e..2bd1d46fe1cd 100644
--- a/drivers/gpu/drm/sti/sti_mixer.c
+++ b/drivers/gpu/drm/sti/sti_mixer.c
@@ -162,8 +162,7 @@ static int mixer_dbg_show(struct seq_file *s, void *arg)
 	DBGFS_DUMP(GAM_MIXER_MBP);
 	DBGFS_DUMP(GAM_MIXER_MX0);
 	mixer_dbg_mxn(s, mixer->regs + GAM_MIXER_MX0);
-	seq_puts(s, "\n");
-
+	seq_putc(s, '\n');
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c
index 8b8ea717c121..63b982048743 100644
--- a/drivers/gpu/drm/sti/sti_tvout.c
+++ b/drivers/gpu/drm/sti/sti_tvout.c
@@ -459,7 +459,7 @@ static void tvout_dbg_vip(struct seq_file *s, int val)
 				   "Aux (color matrix by-passed)",
 				   "", "", "", "", "", "Force value"};
 
-	seq_puts(s, "\t");
+	seq_putc(s, '\t');
 	mask = TVO_VIP_REORDER_MASK << TVO_VIP_REORDER_R_SHIFT;
 	r = (val & mask) >> TVO_VIP_REORDER_R_SHIFT;
 	mask = TVO_VIP_REORDER_MASK << TVO_VIP_REORDER_G_SHIFT;
@@ -558,8 +558,7 @@ static int tvout_dbg_show(struct seq_file *s, void *data)
 	DBGFS_DUMP(TVO_CSC_AUX_M6);
 	DBGFS_DUMP(TVO_CSC_AUX_M7);
 	DBGFS_DUMP(TVO_AUX_IN_VID_FORMAT);
-	seq_puts(s, "\n");
-
+	seq_putc(s, '\n');
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/sti/sti_vid.c b/drivers/gpu/drm/sti/sti_vid.c
index 2ad59892b57e..577a3341d3c1 100644
--- a/drivers/gpu/drm/sti/sti_vid.c
+++ b/drivers/gpu/drm/sti/sti_vid.c
@@ -61,7 +61,7 @@
 static void vid_dbg_ctl(struct seq_file *s, int val)
 {
 	val = val >> 30;
-	seq_puts(s, "\t");
+	seq_putc(s, '\t');
 
 	if (!(val & 1))
 		seq_puts(s, "NOT ");
@@ -114,8 +114,7 @@ static int vid_dbg_show(struct seq_file *s, void *arg)
 	DBGFS_DUMP(VID_BC);
 	DBGFS_DUMP(VID_TINT);
 	DBGFS_DUMP(VID_CSAT);
-	seq_puts(s, "\n");
-
+	seq_putc(s, '\n');
 	return 0;
 }
 
-- 
2.12.2

[PATCH 3/5] drm/sti: Fix a typo in a comment line

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 15:30:44 +0200

Add a missing character in this description for a data structure.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/sti/sti_cursor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/sti_cursor.c b/drivers/gpu/drm/sti/sti_cursor.c
index cd35b9d9de26..5b3a41f74f21 100644
--- a/drivers/gpu/drm/sti/sti_cursor.c
+++ b/drivers/gpu/drm/sti/sti_cursor.c
@@ -33,7 +33,7 @@
 #define STI_CURS_MAX_SIZE   128
 
 /*
- * pixmap dma buffer stucture
+ * pixmap dma buffer structure
  *
  * @paddr:  physical address
  * @size:   buffer size
-- 
2.12.2

[PATCH 4/5] drm/sti: Fix typos in a comment line

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 15:32:08 +0200

Adjust this description for a function call.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/sti/sti_tvout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/sti_tvout.c b/drivers/gpu/drm/sti/sti_tvout.c
index 63b982048743..8959fcc743a8 100644
--- a/drivers/gpu/drm/sti/sti_tvout.c
+++ b/drivers/gpu/drm/sti/sti_tvout.c
@@ -846,7 +846,7 @@ static int sti_tvout_probe(struct platform_device *pdev)
 
 	tvout->dev = dev;
 
-	/* get Memory ressources */
+	/* get memory resources */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tvout-reg");
 	if (!res) {
 		DRM_ERROR("Invalid glue resource\n");
-- 
2.12.2

[PATCH 5/5] drm/sti: Adjust two checks for null pointers in sti_hqvdp_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 May 2017 15:33:19 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The script “checkpatch.pl” pointed information out like the following.

Comparison to NULL could be written !…

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/gpu/drm/sti/sti_hqvdp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_hqvdp.c b/drivers/gpu/drm/sti/sti_hqvdp.c
index 372ea294da80..a1c161f77804 100644
--- a/drivers/gpu/drm/sti/sti_hqvdp.c
+++ b/drivers/gpu/drm/sti/sti_hqvdp.c
@@ -1356,12 +1356,12 @@ static int sti_hqvdp_probe(struct platform_device *pdev)
 
 	/* Get Memory resources */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (res == NULL) {
+	if (!res) {
 		DRM_ERROR("Get memory resource failed\n");
 		return -ENXIO;
 	}
 	hqvdp->regs = devm_ioremap(dev, res->start, resource_size(res));
-	if (hqvdp->regs == NULL) {
+	if (!hqvdp->regs) {
 		DRM_ERROR("Register mapping failed\n");
 		return -ENXIO;
 	}
-- 
2.12.2

Re: [PATCH 0/5] GPU-DRM-STI: Fine-tuning for some function implementations

[Sean Paul]

On Fri, May 05, 2017 at 03:50:49PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 5 May 2017 15:45:45 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.
> 

Hi Markus,
It seems like you're back to submitting cocci patches again :)

We left off in September asking you to contact the list before sending these
types of patches, and to ensure they were properly tested [1]. I don't see any
evidence that either of these things have been done.

I don't want to waste your time by ignoring your patches, so please ensure that
your patches provide value and that they are tested.

Sean


[1]- http://www.spinics.net/lists/linux-kernel-janitors/msg28134.html

> Markus Elfring (5):
>   Reduce function calls for sequence output at five places
>   Replace 17 seq_puts() calls by seq_putc()
>   Fix a typo in a comment line
>   Fix typos in a comment line
>   Adjust two checks for null pointers in sti_hqvdp_probe()
> 
>  drivers/gpu/drm/sti/sti_cursor.c |  5 ++---
>  drivers/gpu/drm/sti/sti_dvo.c    |  3 +--
>  drivers/gpu/drm/sti/sti_gdp.c    |  3 +--
>  drivers/gpu/drm/sti/sti_hda.c    |  9 +++------
>  drivers/gpu/drm/sti/sti_hdmi.c   | 23 ++++++++++-------------
>  drivers/gpu/drm/sti/sti_hqvdp.c  |  7 +++----
>  drivers/gpu/drm/sti/sti_mixer.c  |  3 +--
>  drivers/gpu/drm/sti/sti_tvout.c  |  7 +++----
>  drivers/gpu/drm/sti/sti_vid.c    |  5 ++---
>  9 files changed, 26 insertions(+), 39 deletions(-)
> 
> -- 
> 2.12.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: GPU-DRM-STI: Fine-tuning for some function implementations

[SF Markus Elfring]

> It seems like you're back to submitting cocci patches again :)

My contribution activities are varying also for Linux software over time.   ;-)

The corresponding source code search patterns get different popularity.


> I don't want to waste your time by ignoring your patches, so please ensure that
> your patches provide value and that they are tested.

Which benchmarks and system tests would you find representative for this patch series?

How do you think generally about the proposed change possibilities?

Regards,
Markus

Re: GPU-DRM-STI: Fine-tuning for some function implementations

[Sean Paul]

On Fri, May 05, 2017 at 05:04:40PM +0200, SF Markus Elfring wrote:
> > It seems like you're back to submitting cocci patches again :)
> 
> My contribution activities are varying also for Linux software over time.   ;-)
> 
> The corresponding source code search patterns get different popularity.
> 
> 
> > I don't want to waste your time by ignoring your patches, so please ensure that
> > your patches provide value and that they are tested.
> 
> Which benchmarks and system tests would you find representative for this patch series?
> 

Given your history of submitting changes which break working code, I want
assurance that you've actually run the code and verified that it does what you
want it to do.

> How do you think generally about the proposed change possibilities?

Generally speaking, I don't care about checkpatch/cocci changes that aren't
tested. They clutter the log and don't provide enough value to justify the risk
of breaking stuff. 

IMO, the only time this would be acceptable is if a new contributor wanted to
wet their feet with a couple cleanup patches before diving in to actual
functional changes. In that case, I wouldn't mind dealing with breakage since
we'll benefit from their contributions in the future. With your changes, we
don't have this upside.

Sean


> 
> Regards,
> Markus

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: GPU-DRM-STI: Fine-tuning for some function implementations

[SF Markus Elfring]

> Generally speaking, I don't care about checkpatch/cocci changes that aren't tested.

I find this view interesting only to some degree.

1. I suggest to combine a few functions into fewer ones.
   * Do you spot any programming mistakes in these concrete cases?
   * Can such code reduction result into desired effects?

2. I propose to use the function “seq_putc” at more source code places.
   * Do you really find any previous system test approaches insufficient around
     such a Linux feature?
   * Does the programming interface “seq_puts” provide any properties
     that you prefer over the other one for the sequence output of single characters?
     http://elixir.free-electrons.com/linux/v4.11/source/fs/seq_file.c#L664


> With your changes, we don't have this upside.

How do you think about to pick spelling corrections up for two comment lines?

Regards,
Markus

Re: GPU-DRM-STI: Fine-tuning for some function implementations

[Sean Paul]

On Sat, May 06, 2017 at 03:54:51PM +0200, SF Markus Elfring wrote:
> > Generally speaking, I don't care about checkpatch/cocci changes that aren't tested.
> 
> I find this view interesting only to some degree.

We're bordering on becoming unproductive here, but I'll try once more.

> 
> 1. I suggest to combine a few functions into fewer ones.
>    * Do you spot any programming mistakes in these concrete cases?

Not in the patches I skimmed. However, your history of breaking code tells me that
there have been mistakes missed in the past. As such, I'm not willing to take
untested code from you that does not change functionality at the risk of
breaking something that is currently working. This is non-negotiable.

>    * Can such code reduction result into desired effects?
> 
> 2. I propose to use the function “seq_putc” at more source code places.
>    * Do you really find any previous system test approaches insufficient around
>      such a Linux feature?
>    * Does the programming interface “seq_puts” provide any properties
>      that you prefer over the other one for the sequence output of single characters?
>      http://elixir.free-electrons.com/linux/v4.11/source/fs/seq_file.c#L664
> 

As I said before, if you test it, I'll consider it. If you are unwilling to test
your changes, I'm unwilling to apply them. I'm not interested in double checking
all of your work, and fixing your bugs for no functional benefit.

> 
> > With your changes, we don't have this upside.
> 
> How do you think about to pick spelling corrections up for two comment lines?

Well, it wouldn't break anything, so that's positive. As I said in my last
email, these types of changes are perfect for new contributors to get started
with kernel development. I find less value in these patches if they're from
someone seemingly trying to rack up patch count.

Sean


> 
> Regards,
> Markus

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: GPU-DRM-STI: Fine-tuning for some function implementations

[SF Markus Elfring]

>> 1. I suggest to combine a few functions into fewer ones.
>>    * Do you spot any programming mistakes in these concrete cases?
> 
> Not in the patches I skimmed.

Thanks for such feedback.


> However, your history of breaking code tells me that there have been mistakes
> missed in the past.

I admit that I had my own share of software development hiccups. I would also
like to reduce them. But a probability remains that I will stumble on
various glitches as usual.


> As such, I'm not willing to take untested code from you that does not change
> functionality at the risk of breaking something that is currently working.

I imagine that the shown software refactoring will improve the affected
sequence outputs in useful ways, won't it?


> This is non-negotiable.

It seems that we have got different views around the ways to get to acceptable
final system test results.


> As I said before, if you test it, I'll consider it.

I got a few doubts for this information. If you find my software development
reputation so questionable, I assume that you would not trust any tests
that I would try out on my own.


> If you are unwilling to test your changes, I'm unwilling to apply them.

I guess that the desired willingness will depend on a test environment
which will be trusted by all involved parties. Other incentives might
also matter.


> I'm not interested in double checking all of your work, and fixing your bugs
> for no functional benefit.

Do you care for improvements in the implementation of logging functions?


> I find less value in these patches if they're from someone seemingly
> trying to rack up patch count.

I am picking special source code search patterns up.
The evolving development tools can point then hundreds of source files
out which contain similar update candidates.
I found also a few spelling weaknesses while I was looking around
in affected source code. These tools can also increase the awareness
for such change possibilities, can't they?

Regards,
Markus

Re: GPU-DRM-STI: Fine-tuning for some function implementations

[Benjamin Gaignard]

2017-05-06 19:00 GMT+02:00 SF Markus Elfring <elfring@users.sourceforge.net>:
>>> 1. I suggest to combine a few functions into fewer ones.
>>>    * Do you spot any programming mistakes in these concrete cases?
>>
>> Not in the patches I skimmed.
>
> Thanks for such feedback.
>
>
>> However, your history of breaking code tells me that there have been mistakes
>> missed in the past.
>
> I admit that I had my own share of software development hiccups. I would also
> like to reduce them. But a probability remains that I will stumble on
> various glitches as usual.
>
>
>> As such, I'm not willing to take untested code from you that does not change
>> functionality at the risk of breaking something that is currently working.
>
> I imagine that the shown software refactoring will improve the affected
> sequence outputs in useful ways, won't it?
>
>
>> This is non-negotiable.
>
> It seems that we have got different views around the ways to get to acceptable
> final system test results.
>
>
>> As I said before, if you test it, I'll consider it.

As sti driver maintainer I will test those patches.
If their are ok and get some other reviewed/ack I will use them
for myself training on how push patches in drm-misc.

Benjamin

>
> I got a few doubts for this information. If you find my software development
> reputation so questionable, I assume that you would not trust any tests
> that I would try out on my own.
>
>
>> If you are unwilling to test your changes, I'm unwilling to apply them.
>
> I guess that the desired willingness will depend on a test environment
> which will be trusted by all involved parties. Other incentives might
> also matter.
>
>
>> I'm not interested in double checking all of your work, and fixing your bugs
>> for no functional benefit.
>
> Do you care for improvements in the implementation of logging functions?
>
>
>> I find less value in these patches if they're from someone seemingly
>> trying to rack up patch count.
>
> I am picking special source code search patterns up.
> The evolving development tools can point then hundreds of source files
> out which contain similar update candidates.
> I found also a few spelling weaknesses while I was looking around
> in affected source code. These tools can also increase the awareness
> for such change possibilities, can't they?
>
> Regards,
> Markus

Re: GPU-DRM-STI: Fine-tuning for some function implementations

[Benjamin Gaignard]

2017-05-09 10:03 GMT+02:00 Benjamin Gaignard <benjamin.gaignard@linaro.org>:
> 2017-05-06 19:00 GMT+02:00 SF Markus Elfring <elfring@users.sourceforge.net>:
>>>> 1. I suggest to combine a few functions into fewer ones.
>>>>    * Do you spot any programming mistakes in these concrete cases?
>>>
>>> Not in the patches I skimmed.
>>
>> Thanks for such feedback.
>>
>>
>>> However, your history of breaking code tells me that there have been mistakes
>>> missed in the past.
>>
>> I admit that I had my own share of software development hiccups. I would also
>> like to reduce them. But a probability remains that I will stumble on
>> various glitches as usual.
>>
>>
>>> As such, I'm not willing to take untested code from you that does not change
>>> functionality at the risk of breaking something that is currently working.
>>
>> I imagine that the shown software refactoring will improve the affected
>> sequence outputs in useful ways, won't it?
>>
>>
>>> This is non-negotiable.
>>
>> It seems that we have got different views around the ways to get to acceptable
>> final system test results.
>>
>>
>>> As I said before, if you test it, I'll consider it.
>
> As sti driver maintainer I will test those patches.
> If their are ok and get some other reviewed/ack I will use them
> for myself training on how push patches in drm-misc.
>
> Benjamin

After testing:
Acked-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

>
>>
>> I got a few doubts for this information. If you find my software development
>> reputation so questionable, I assume that you would not trust any tests
>> that I would try out on my own.
>>
>>
>>> If you are unwilling to test your changes, I'm unwilling to apply them.
>>
>> I guess that the desired willingness will depend on a test environment
>> which will be trusted by all involved parties. Other incentives might
>> also matter.
>>
>>
>>> I'm not interested in double checking all of your work, and fixing your bugs
>>> for no functional benefit.
>>
>> Do you care for improvements in the implementation of logging functions?
>>
>>
>>> I find less value in these patches if they're from someone seemingly
>>> trying to rack up patch count.
>>
>> I am picking special source code search patterns up.
>> The evolving development tools can point then hundreds of source files
>> out which contain similar update candidates.
>> I found also a few spelling weaknesses while I was looking around
>> in affected source code. These tools can also increase the awareness
>> for such change possibilities, can't they?
>>
>> Regards,
>> Markus