diff mbox

omapfb: dss: Do not duplicate features data

Message ID 20171129123308.GA26578@lenoch (mailing list archive)
State New, archived
Headers show

Commit Message

Ladislav Michl Nov. 29, 2017, 12:33 p.m. UTC
As features data are read only, there is no need to allocate their
copy on the heap.

Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
Suggested-by: Markus Elfring <elfring@users.sourceforge.net>
---
 Note: This patch is not runtime tested. If you have hardware and can test
       it, please do so and eventually add you Tested-by tag. Thank you.
 Note2: Marcus, I hope it is okay to add your Suggested-by tag. Please
        let me know, if I'm wrong.

 drivers/video/fbdev/omap2/omapfb/dss/dispc.c    | 39 ++++++---------------
 drivers/video/fbdev/omap2/omapfb/dss/dss.c      | 45 +++++++------------------
 drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c | 31 ++++-------------
 3 files changed, 29 insertions(+), 86 deletions(-)

Comments

SF Markus Elfring Nov. 29, 2017, 2:38 p.m. UTC | #1
> As features data are read only, there is no need to allocate their
> copy on the heap.>  Note2: Marcus, I hope it is okay to add your Suggested-by tag.

Maybe.


>         Please let me know, if I'm wrong.

I am unsure about the view on how much I suggested such a source code adjustment.

A development discussion for my update suggestion “omapfb/dss: Delete an error message
for a failed memory allocation in three functions” triggered the contribution of your idea.

https://lkml.org/lkml/2017/11/27/919


Did you omit the general Linux kernel mailing list from your proposal intentionally?

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
Adam Ford Nov. 29, 2017, 3:12 p.m. UTC | #2
On Wed, Nov 29, 2017 at 6:33 AM, Ladislav Michl <ladis@linux-mips.org> wrote:
> As features data are read only, there is no need to allocate their
> copy on the heap.
>
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> Suggested-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  Note: This patch is not runtime tested. If you have hardware and can test
>        it, please do so and eventually add you Tested-by tag. Thank you.

Unfortunately, this fails to load properly on a Logic PD Torpedo kit
(omap3630/DM3730).

[    0.975097] omapdss_dss 48050000.dss: master bind failed: -517
[    3.821807] omapdss_dispc 48050400.dispc: Failed to allocate DISPC Features
[    3.829345] omapdss_dss 48050000.dss: failed to bind 48050400.dispc
(ops dispc_component_ops): -12
[    3.842254] omapdss_dss 48050000.dss: master bind failed: -12
[    3.848724] omapdss_dispc: probe of 48050400.dispc failed with error -12

I also get
[   12.258056] panel-dpi display: failed to find video source





>  Note2: Marcus, I hope it is okay to add your Suggested-by tag. Please
>         let me know, if I'm wrong.
>
>  drivers/video/fbdev/omap2/omapfb/dss/dispc.c    | 39 ++++++---------------
>  drivers/video/fbdev/omap2/omapfb/dss/dss.c      | 45 +++++++------------------
>  drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c | 31 ++++-------------
>  3 files changed, 29 insertions(+), 86 deletions(-)
>
> 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) {
> --
> 2.15.0
>
> --
> 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
--
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
Ladislav Michl Nov. 29, 2017, 4:08 p.m. UTC | #3
Hi Adam,

On Wed, Nov 29, 2017 at 09:12:34AM -0600, Adam Ford wrote:
> On Wed, Nov 29, 2017 at 6:33 AM, Ladislav Michl <ladis@linux-mips.org> wrote:
> > As features data are read only, there is no need to allocate their
> > copy on the heap.
> >
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > Suggested-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  Note: This patch is not runtime tested. If you have hardware and can test
> >        it, please do so and eventually add you Tested-by tag. Thank you.
> 
> Unfortunately, this fails to load properly on a Logic PD Torpedo kit
> (omap3630/DM3730).

Thank you for testing.

> [    0.975097] omapdss_dss 48050000.dss: master bind failed: -517

This is -EPROBE_DEFER, you'll probably get that also without patch.

> [    3.821807] omapdss_dispc 48050400.dispc: Failed to allocate DISPC Features

I wonder how could you get this as this is the message patch removes...

> [    3.829345] omapdss_dss 48050000.dss: failed to bind 48050400.dispc
> (ops dispc_component_ops): -12
> [    3.842254] omapdss_dss 48050000.dss: master bind failed: -12
> [    3.848724] omapdss_dispc: probe of 48050400.dispc failed with error -12
> 
> I also get
> [   12.258056] panel-dpi display: failed to find video source

These are only consequences of above error.

> >  Note2: Marcus, I hope it is okay to add your Suggested-by tag. Please
> >         let me know, if I'm wrong.
> >
> >  drivers/video/fbdev/omap2/omapfb/dss/dispc.c    | 39 ++++++---------------
> >  drivers/video/fbdev/omap2/omapfb/dss/dss.c      | 45 +++++++------------------
> >  drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c | 31 ++++-------------
> >  3 files changed, 29 insertions(+), 86 deletions(-)
> >
> > 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");

Here  -------------------------------------^

> > -               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) {
> > --
> > 2.15.0
> >
> > --
> > 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
--
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
Ladislav Michl Nov. 29, 2017, 4:18 p.m. UTC | #4
On Wed, Nov 29, 2017 at 03:38:46PM +0100, SF Markus Elfring wrote:
> > As features data are read only, there is no need to allocate their
> > copy on the heap.
> …
> >  Note2: Marcus, I hope it is okay to add your Suggested-by tag.
> 
> Maybe.
> 
> >         Please let me know, if I'm wrong.
> 
> I am unsure about the view on how much I suggested such a source code adjustment.

Well, it is up to you completely. I didn't find any more relevant tag, which
neccessarily doesn't mean it is appropriate.

> A development discussion for my update suggestion “omapfb/dss: Delete an error message
> for a failed memory allocation in three functions” triggered the contribution of your idea.

I would never look at that code without looking to said patch.

> https://lkml.org/lkml/2017/11/27/919
> 
> Did you omit the general Linux kernel mailing list from your proposal intentionally?

I've never sent single mail to the general Linux kernel mailing list other than
it already was in Cc. It was write-only list decade ago and now it is not any
better :)

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
Adam Ford Nov. 29, 2017, 4:36 p.m. UTC | #5
On Wed, Nov 29, 2017 at 10:08 AM, Ladislav Michl <ladis@linux-mips.org> wrote:
> Hi Adam,
>
> On Wed, Nov 29, 2017 at 09:12:34AM -0600, Adam Ford wrote:
>> On Wed, Nov 29, 2017 at 6:33 AM, Ladislav Michl <ladis@linux-mips.org> wrote:
>> > As features data are read only, there is no need to allocate their
>> > copy on the heap.
>> >
>> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
>> > Suggested-by: Markus Elfring <elfring@users.sourceforge.net>
>> > ---
>> >  Note: This patch is not runtime tested. If you have hardware and can test
>> >        it, please do so and eventually add you Tested-by tag. Thank you.
>>
>> Unfortunately, this fails to load properly on a Logic PD Torpedo kit
>> (omap3630/DM3730).
>
> Thank you for testing.
>
>> [    0.975097] omapdss_dss 48050000.dss: master bind failed: -517
>
> This is -EPROBE_DEFER, you'll probably get that also without patch.
>
>> [    3.821807] omapdss_dispc 48050400.dispc: Failed to allocate DISPC Features
>
> I wonder how could you get this as this is the message patch removes...
>
>> [    3.829345] omapdss_dss 48050000.dss: failed to bind 48050400.dispc
>> (ops dispc_component_ops): -12
>> [    3.842254] omapdss_dss 48050000.dss: master bind failed: -12
>> [    3.848724] omapdss_dispc: probe of 48050400.dispc failed with error -12
>>
>> I also get
>> [   12.258056] panel-dpi display: failed to find video source
>
> These are only consequences of above error.
>
>> >  Note2: Marcus, I hope it is okay to add your Suggested-by tag. Please
>> >         let me know, if I'm wrong.
>> >
>> >  drivers/video/fbdev/omap2/omapfb/dss/dispc.c    | 39 ++++++---------------
>> >  drivers/video/fbdev/omap2/omapfb/dss/dss.c      | 45 +++++++------------------
>> >  drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c | 31 ++++-------------
>> >  3 files changed, 29 insertions(+), 86 deletions(-)
>> >
>> > 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");
>
> Here  -------------------------------------^
>

I copy-pasted the code from the URL. I assumed they were the same.  I
used it since copy-pasting tabs and spacing gets messaged up from my
e-mail.  Sorry about that.

I tried to apply the patch in the e-mail to 4.14.2 and it failed. (it
could also be the fact that my e-mail program sucks and it's messing
up characters)

Do you have a version or trunk you want me to use as the basis?

thanks

adam

>> > -               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) {
>> > --
>> > 2.15.0
>> >
>> > --
>> > 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
--
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
Adam Ford Nov. 29, 2017, 4:45 p.m. UTC | #6
On Wed, Nov 29, 2017 at 10:36 AM, Adam Ford <aford173@gmail.com> wrote:
> On Wed, Nov 29, 2017 at 10:08 AM, Ladislav Michl <ladis@linux-mips.org> wrote:
>> Hi Adam,
>>
>> On Wed, Nov 29, 2017 at 09:12:34AM -0600, Adam Ford wrote:
>>> On Wed, Nov 29, 2017 at 6:33 AM, Ladislav Michl <ladis@linux-mips.org> wrote:
>>> > As features data are read only, there is no need to allocate their
>>> > copy on the heap.
>>> >
>>> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
>>> > Suggested-by: Markus Elfring <elfring@users.sourceforge.net>
>>> > ---
>>> >  Note: This patch is not runtime tested. If you have hardware and can test
>>> >        it, please do so and eventually add you Tested-by tag. Thank you.
>>>
>>> Unfortunately, this fails to load properly on a Logic PD Torpedo kit
>>> (omap3630/DM3730).
>>
>> Thank you for testing.
>>
>>> [    0.975097] omapdss_dss 48050000.dss: master bind failed: -517
>>
>> This is -EPROBE_DEFER, you'll probably get that also without patch.
>>
>>> [    3.821807] omapdss_dispc 48050400.dispc: Failed to allocate DISPC Features
>>
>> I wonder how could you get this as this is the message patch removes...
>>
>>> [    3.829345] omapdss_dss 48050000.dss: failed to bind 48050400.dispc
>>> (ops dispc_component_ops): -12
>>> [    3.842254] omapdss_dss 48050000.dss: master bind failed: -12
>>> [    3.848724] omapdss_dispc: probe of 48050400.dispc failed with error -12
>>>
>>> I also get
>>> [   12.258056] panel-dpi display: failed to find video source
>>
>> These are only consequences of above error.
>>
>>> >  Note2: Marcus, I hope it is okay to add your Suggested-by tag. Please
>>> >         let me know, if I'm wrong.
>>> >
>>> >  drivers/video/fbdev/omap2/omapfb/dss/dispc.c    | 39 ++++++---------------
>>> >  drivers/video/fbdev/omap2/omapfb/dss/dss.c      | 45 +++++++------------------
>>> >  drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c | 31 ++++-------------
>>> >  3 files changed, 29 insertions(+), 86 deletions(-)
>>> >
>>> > 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");
>>
>> Here  -------------------------------------^
>>
>
> I copy-pasted the code from the URL. I assumed they were the same.  I
> used it since copy-pasting tabs and spacing gets messaged up from my
> e-mail.  Sorry about that.
>
> I tried to apply the patch in the e-mail to 4.14.2 and it failed. (it
> could also be the fact that my e-mail program sucks and it's messing
> up characters)
>
> Do you have a version or trunk you want me to use as the basis?
>

If it's correct and the patch is this one:
https://patchwork.kernel.org/patch/10082025/

Then you can mark me as:

Tested-by: Adam Ford <aford173@gmail.com> #omap3630


> thanks
>
> adam
>
>>> > -               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) {
>>> > --
>>> > 2.15.0
>>> >
>>> > --
>>> > 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
--
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
Ladislav Michl Nov. 29, 2017, 5:03 p.m. UTC | #7
On Wed, Nov 29, 2017 at 10:45:52AM -0600, Adam Ford wrote:
[snip]
> If it's correct and the patch is this one:
> https://patchwork.kernel.org/patch/10082025/

Yes.

> Then you can mark me as:
> 
> Tested-by: Adam Ford <aford173@gmail.com> #omap3630

Thank you for testing. I'll wait a bit more for eventual objections
and Marcus' decision about his credit before sending v2 :)

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
SF Markus Elfring Nov. 29, 2017, 5:30 p.m. UTC | #8
>>>  Note2: Marcus, I hope it is okay to add your Suggested-by tag.
>>
>> Maybe.
>>
>>>         Please let me know, if I'm wrong.
>>
>> I am unsure about the view on how much I suggested such a source code adjustment.
> 
> Well, it is up to you completely.

I would prefer then to omit this tag in case you are going to publish
a subsequent version for your current update suggestion.


> I didn't find any more relevant tag,
> which neccessarily doesn't mean it is appropriate.

Would you like to add your “own tag”?


>> A development discussion for my update suggestion “omapfb/dss: Delete an error message
>> for a failed memory allocation in three functions” triggered the contribution of your idea.
> 
> I would never look at that code without looking to said patch.
> 
>> https://lkml.org/lkml/2017/11/27/919

* Did you become inspired by this discussion anyhow?

* Will a link become useful for your commit message?

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
Bartlomiej Zolnierkiewicz Jan. 4, 2018, 2:10 p.m. UTC | #9
On Wednesday, November 29, 2017 06:03:14 PM Ladislav Michl wrote:
> On Wed, Nov 29, 2017 at 10:45:52AM -0600, Adam Ford wrote:
> [snip]
> > If it's correct and the patch is this one:
> > https://patchwork.kernel.org/patch/10082025/
> 
> Yes.
> 
> > Then you can mark me as:
> > 
> > Tested-by: Adam Ford <aford173@gmail.com> #omap3630
> 
> Thank you for testing. I'll wait a bit more for eventual objections
> and Marcus' decision about his credit before sending v2 :)

After adding Adam's Tested-by tag and fixing minor CodingStyle
errors reported by checkpatch.pl:

ERROR: "foo* bar" should be "foo *bar"
#37: FILE: drivers/video/fbdev/omap2/omapfb/dss/dispc.c:3979:
+static const struct dispc_features* dispc_get_features(void)

ERROR: "foo* bar" should be "foo *bar"
#114: FILE: drivers/video/fbdev/omap2/omapfb/dss/dss.c:890:
+static const struct dss_features* dss_get_features(void)

ERROR: "foo* bar" should be "foo *bar"
#199: FILE: drivers/video/fbdev/omap2/omapfb/dss/hdmi_phy.c:192:
+static const struct hdmi_phy_features* hdmi_phy_get_features(void)

I've queued your patch for 4.16, thanks.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

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

Patch

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) {