| Message ID | 20171128102327.GA30267@lenoch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>> How will this aspect evolve further? > > I do not follow. Interesting … > This is OMAP framebuffer driver, so in this case, there is zero variation. How much are you interested to compare differences in build results also for this software module because of varying parameters? Which parameters were applied for your size comparisons so far? > Could you, please, review following patch I assume that other OMAP developers are in a better position to decide about the deletion of extra memory allocations (instead of keeping questionable error messages). > and verify is it satisfies your automated static code analysis test? I am not going to “verify” your update suggestion by my evolving approaches around the semantic patch language (Coccinelle software) at the moment. But I thank you for this contribution. How will further feedback evolve for such an idea? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 28, 2017 at 11:50:14AM +0100, SF Markus Elfring wrote: > >> How will this aspect evolve further? > > > > I do not follow. > > Interesting … > > > This is OMAP framebuffer driver, so in this case, there is zero variation. > > How much are you interested to compare differences in build results > also for this software module because of varying parameters? > > Which parameters were applied for your size comparisons so far? It was just omap2plus_defconfig build using gcc-7.2.0 > > Could you, please, review following patch > > I assume that other OMAP developers are in a better position to decide > about the deletion of extra memory allocations (instead of keeping > questionable error messages). > > > and verify is it satisfies your automated static code analysis test? > > I am not going to “verify” your update suggestion by my evolving approaches > around the semantic patch language (Coccinelle software) at the moment. As you are sending patches as Markus Elfring I would expect you take Coccinelle's suggestion into account and actually try to understand code before sending patch. That suggestion may lead to actual bug in code which your patch just leaves unnoticed as it is not apparent from the patch itself (no, not talking about this very patch it all started with) That said, I'm considering Markus Elfring being a human. If you do not like reactions to your patches or are interested only in improving tool that generates them, it would be better to just setup a "tip bot for Markus Elfring" and let it send patches automatically. This way noone is going to waste time on them as it would be clear those are purely machine only generated and there's no point to reply. The way you are sending patches makes impression (at least to me), that you spent some time on fixing issue Coccinelle found and not just shut the warning up. > But I thank you for this contribution. > How will further feedback evolve for such an idea? And the idea is? > Regards, > Markus Thank you, ladis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> I am not going to “verify” your update suggestion by my evolving approaches >> around the semantic patch language (Coccinelle software) at the moment. > > As you are sending patches as Markus Elfring I am contributing also some update suggestions. > I would expect you take Coccinelle's suggestion into account The proposed change is based on a semantic patch script which I developed with the support of other well-known Linux contributors. > and actually try to understand code before sending patch. I concentrated my understanding on the concrete transformation pattern in this use case. > That suggestion may lead to actual bug in code which your patch just leaves > unnoticed as it is not apparent from the patch itself There can be other change possibilities left over as usual. > (no, not talking about this very patch it all started with) Thanks for your distinction. > That said, I'm considering Markus Elfring being a human. Thanks for this view. > If you do not like reactions to your patches I am looking for constructive responses. - Disagreements can trigger special communication challenges. > or are interested only in improving tool that generates them, How do you think about to look at any more background information? https://github.com/coccinelle/coccinelle/issues https://systeme.lip6.fr/pipermail/cocci/ > it would be better to just setup a "tip bot for Markus > Elfring" and let it send patches automatically. There is already an other automatic source code analysis system active. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/scripts/coccinelle > The way you are sending patches makes impression (at least to me), > that you spent some time on fixing issue Coccinelle found Yes. - This view is appropriate. > and not just shut the warning up. Additional improvement possibilities can be taken into account after corresponding software development discussions, can't they? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-11-28 at 11:23 +0100, Ladislav Michl wrote: > I do not follow. This is OMAP framebuffer driver, so in this case, there > is zero variation. Could you, please, review following patch and verify > is it satisfies your automated static code analysis test? Obviously a better patch. I suggest submitting it properly and letting the 0-day build bot have a go at it. cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 28, 2017 at 01:13:51PM +0100, SF Markus Elfring wrote: > Additional improvement possibilities can be taken into account > after corresponding software development discussions, can't they? Sure, but that is in contrary to all you replies. I guess you are familiar with Documentation/process/submitting-patches.rst chapter 8. No matter that patch was generated or suggested by a tool, you sent it and normal review procedure follows. And here you ignored _all_ suggestions and concentrate solely on improving Coccinelle scripts. On kernel related lists suggestions to patch itself are discussed. Whenever you take them into account while developing Coccinelle is up to you (on the Cocci list). Best regards, ladis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Additional improvement possibilities can be taken into account >> after corresponding software development discussions, can't they? > > Sure, but that is in contrary to all you replies. Where do you see a contradiction in this case? > I guess you are familiar with Documentation/process/submitting-patches.rst chapter 8. I hope so in principle. > No matter that patch was generated or suggested by a tool, you sent > it and normal review procedure follows. This is generally fine. > And here you ignored _all_ suggestions I did not integrate a few of them for my commit message so far because it seems that there are open issues for further clarification. Do you want that I send a second approach for this software module before your own evolving update suggestion? > and concentrate solely on improving Coccinelle scripts. I hope not. > On kernel related lists suggestions to patch itself are discussed. This is usual. > Whenever you take them into account while developing Coccinelle > is up to you (on the Cocci list). This is also happening, isn't it? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c index 7a75dfda9845..6be13a106ece 100644 --- a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c +++ b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c @@ -3976,52 +3976,33 @@ static const struct dispc_features omap54xx_dispc_feats = { .has_writeback = true, }; -static int dispc_init_features(struct platform_device *pdev) +static const struct dispc_features* dispc_get_features(void) { - const struct dispc_features *src; - struct dispc_features *dst; - - dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL); - if (!dst) { - dev_err(&pdev->dev, "Failed to allocate DISPC Features\n"); - return -ENOMEM; - } - switch (omapdss_get_version()) { case OMAPDSS_VER_OMAP24xx: - src = &omap24xx_dispc_feats; - break; + return &omap24xx_dispc_feats; case OMAPDSS_VER_OMAP34xx_ES1: - src = &omap34xx_rev1_0_dispc_feats; - break; + return &omap34xx_rev1_0_dispc_feats; case OMAPDSS_VER_OMAP34xx_ES3: case OMAPDSS_VER_OMAP3630: case OMAPDSS_VER_AM35xx: case OMAPDSS_VER_AM43xx: - src = &omap34xx_rev3_0_dispc_feats; - break; + return &omap34xx_rev3_0_dispc_feats; case OMAPDSS_VER_OMAP4430_ES1: case OMAPDSS_VER_OMAP4430_ES2: case OMAPDSS_VER_OMAP4: - src = &omap44xx_dispc_feats; - break; + return &omap44xx_dispc_feats; case OMAPDSS_VER_OMAP5: case OMAPDSS_VER_DRA7xx: - src = &omap54xx_dispc_feats; - break; + return &omap54xx_dispc_feats; default: - return -ENODEV; + return NULL; } - - memcpy(dst, src, sizeof(*dst)); - dispc.feat = dst; - - return 0; } static irqreturn_t dispc_irq_handler(int irq, void *arg) @@ -4078,9 +4059,9 @@ static int dispc_bind(struct device *dev, struct device *master, void *data) spin_lock_init(&dispc.control_lock); - r = dispc_init_features(dispc.pdev); - if (r) - return r; + dispc.feat = dispc_get_features(); + if (!dispc.feat) + return -ENODEV; dispc_mem = platform_get_resource(dispc.pdev, IORESOURCE_MEM, 0); if (!dispc_mem) { diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss.c b/drivers/video/fbdev/omap2/omapfb/dss/dss.c index 48c6500c24e1..9a255ffe77c5 100644 --- a/drivers/video/fbdev/omap2/omapfb/dss/dss.c +++ b/drivers/video/fbdev/omap2/omapfb/dss/dss.c @@ -887,58 +887,37 @@ static const struct dss_features dra7xx_dss_feats = { .num_ports = ARRAY_SIZE(dra7xx_ports), }; -static int dss_init_features(struct platform_device *pdev) +static const struct dss_features* dss_get_features(void) { - const struct dss_features *src; - struct dss_features *dst; - - dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL); - if (!dst) { - dev_err(&pdev->dev, "Failed to allocate local DSS Features\n"); - return -ENOMEM; - } - switch (omapdss_get_version()) { case OMAPDSS_VER_OMAP24xx: - src = &omap24xx_dss_feats; - break; + return &omap24xx_dss_feats; case OMAPDSS_VER_OMAP34xx_ES1: case OMAPDSS_VER_OMAP34xx_ES3: case OMAPDSS_VER_AM35xx: - src = &omap34xx_dss_feats; - break; + return &omap34xx_dss_feats; case OMAPDSS_VER_OMAP3630: - src = &omap3630_dss_feats; - break; + return &omap3630_dss_feats; case OMAPDSS_VER_OMAP4430_ES1: case OMAPDSS_VER_OMAP4430_ES2: case OMAPDSS_VER_OMAP4: - src = &omap44xx_dss_feats; - break; + return &omap44xx_dss_feats; case OMAPDSS_VER_OMAP5: - src = &omap54xx_dss_feats; - break; + return &omap54xx_dss_feats; case OMAPDSS_VER_AM43xx: - src = &am43xx_dss_feats; - break; + return &am43xx_dss_feats; case OMAPDSS_VER_DRA7xx: - src = &dra7xx_dss_feats; - break; + return &dra7xx_dss_feats; default: - return -ENODEV; + return NULL; } - - memcpy(dst, src, sizeof(*dst)); - dss.feat = dst; - - return 0; } static void dss_uninit_ports(struct platform_device *pdev); @@ -1104,9 +1083,9 @@ static int dss_bind(struct device *dev) dss.pdev = pdev; - r = dss_init_features(dss.pdev); - if (r) - return r; + dss.feat = dss_get_features(); + if (!dss.feat) + return -ENODEV; dss_mem = platform_get_resource(dss.pdev, IORESOURCE_MEM, 0); if (!dss_mem) { diff --git a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c index 9a13c35fd6d8..07d46e14cea4 100644 --- a/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c +++ b/drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c @@ -189,47 +189,30 @@ static const struct hdmi_phy_features omap54xx_phy_feats = { .max_phy = 186000000, }; -static int hdmi_phy_init_features(struct platform_device *pdev) +static const struct hdmi_phy_features* hdmi_phy_get_features(void) { - struct hdmi_phy_features *dst; - const struct hdmi_phy_features *src; - - dst = devm_kzalloc(&pdev->dev, sizeof(*dst), GFP_KERNEL); - if (!dst) { - dev_err(&pdev->dev, "Failed to allocate HDMI PHY Features\n"); - return -ENOMEM; - } - switch (omapdss_get_version()) { case OMAPDSS_VER_OMAP4430_ES1: case OMAPDSS_VER_OMAP4430_ES2: case OMAPDSS_VER_OMAP4: - src = &omap44xx_phy_feats; - break; + return &omap44xx_phy_feats; case OMAPDSS_VER_OMAP5: case OMAPDSS_VER_DRA7xx: - src = &omap54xx_phy_feats; - break; + return &omap54xx_phy_feats; default: - return -ENODEV; + return NULL; } - - memcpy(dst, src, sizeof(*dst)); - phy_feat = dst; - - return 0; } int hdmi_phy_init(struct platform_device *pdev, struct hdmi_phy_data *phy) { - int r; struct resource *res; - r = hdmi_phy_init_features(pdev); - if (r) - return r; + phy_feat = hdmi_phy_get_features(); + if (!phy_feat) + return -ENODEV; res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy"); if (!res) {