| Message ID | 20180615122653eucas1p2254aad8e6497ab1bcf11eb5c7fa6b063~4VRMRscw30433304333eucas1p2B@eucas1p2.samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/exynos: gsc: Get device id from OF alias | expand |
On 15 June 2018 at 14:26, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Platform devices instantiated from device-tree always have pdev->id set to > -1, so use of_get_alias_id() helper to retrieve proper device id. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_drm_gsc.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c > index e99dd1e4ba65..a63287597985 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c > @@ -1196,7 +1196,12 @@ static int gsc_probe(struct platform_device *pdev) > struct exynos_drm_ipp_formats *formats; > struct gsc_context *ctx; > struct resource *res; > - int ret, i; > + int ret, i, id; > + > + ret = of_alias_get_id(pdev->dev.of_node, "gsc"); > + if (ret < 0) > + return ret; > + id = ret; > > ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); > if (!ctx) > @@ -1254,7 +1259,7 @@ static int gsc_probe(struct platform_device *pdev) > } > > /* context initailization */ > - ctx->id = pdev->id; > + ctx->id = id; Why do you need ctx->id at all? I see it is used only in dev_dbg and dev_err messages but these should be easily identifiable by device name+address. Maybe get rid of ctx->id entirely? Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Krzysztof, On 20.06.2018 12:40, Krzysztof Kozlowski wrote: > On 15 June 2018 at 14:26, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >> Platform devices instantiated from device-tree always have pdev->id set to >> -1, so use of_get_alias_id() helper to retrieve proper device id. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_drm_gsc.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c >> index e99dd1e4ba65..a63287597985 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c >> @@ -1196,7 +1196,12 @@ static int gsc_probe(struct platform_device *pdev) >> struct exynos_drm_ipp_formats *formats; >> struct gsc_context *ctx; >> struct resource *res; >> - int ret, i; >> + int ret, i, id; >> + >> + ret = of_alias_get_id(pdev->dev.of_node, "gsc"); >> + if (ret < 0) >> + return ret; >> + id = ret; >> >> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >> if (!ctx) >> @@ -1254,7 +1259,7 @@ static int gsc_probe(struct platform_device *pdev) >> } >> >> /* context initailization */ >> - ctx->id = pdev->id; >> + ctx->id = id; > Why do you need ctx->id at all? I see it is used only in dev_dbg and > dev_err messages but these should be easily identifiable by device > name+address. Maybe get rid of ctx->id entirely? I am working on patches adding framebuffer display pre-processing on-the-fly, they requires gscaler id to program sysreg registers. I hope to post it in near future. Regards Andrzej > > Best regards, > Krzysztof > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20 June 2018 at 13:38, Andrzej Hajda <a.hajda@samsung.com> wrote: > Hi Krzysztof, > > On 20.06.2018 12:40, Krzysztof Kozlowski wrote: >> On 15 June 2018 at 14:26, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >>> Platform devices instantiated from device-tree always have pdev->id set to >>> -1, so use of_get_alias_id() helper to retrieve proper device id. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> --- >>> drivers/gpu/drm/exynos/exynos_drm_gsc.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c >>> index e99dd1e4ba65..a63287597985 100644 >>> --- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c >>> @@ -1196,7 +1196,12 @@ static int gsc_probe(struct platform_device *pdev) >>> struct exynos_drm_ipp_formats *formats; >>> struct gsc_context *ctx; >>> struct resource *res; >>> - int ret, i; >>> + int ret, i, id; >>> + >>> + ret = of_alias_get_id(pdev->dev.of_node, "gsc"); >>> + if (ret < 0) >>> + return ret; >>> + id = ret; >>> >>> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >>> if (!ctx) >>> @@ -1254,7 +1259,7 @@ static int gsc_probe(struct platform_device *pdev) >>> } >>> >>> /* context initailization */ >>> - ctx->id = pdev->id; >>> + ctx->id = id; >> Why do you need ctx->id at all? I see it is used only in dev_dbg and >> dev_err messages but these should be easily identifiable by device >> name+address. Maybe get rid of ctx->id entirely? > > I am working on patches adding framebuffer display pre-processing > on-the-fly, they requires gscaler id to program sysreg registers. I hope > to post it in near future. OK, makes sense. Then I have a dependent comment - if alias id is used by driver then probably it should be validated to prevent errors like out-of-bounds access. DTB theoretically might come from out-of-tree or from older version. Something like this was reported for our pinctrl driver: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=93b0beae721b3344923b4b8317e9d83b542f4ca6 Probably the validation should come with your code, Andrzej. Not here. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20.06.2018 13:53, Krzysztof Kozlowski wrote: > On 20 June 2018 at 13:38, Andrzej Hajda <a.hajda@samsung.com> wrote: >> Hi Krzysztof, >> >> On 20.06.2018 12:40, Krzysztof Kozlowski wrote: >>> On 15 June 2018 at 14:26, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >>>> Platform devices instantiated from device-tree always have pdev->id set to >>>> -1, so use of_get_alias_id() helper to retrieve proper device id. >>>> >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>> --- >>>> drivers/gpu/drm/exynos/exynos_drm_gsc.c | 9 +++++++-- >>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c >>>> index e99dd1e4ba65..a63287597985 100644 >>>> --- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c >>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c >>>> @@ -1196,7 +1196,12 @@ static int gsc_probe(struct platform_device *pdev) >>>> struct exynos_drm_ipp_formats *formats; >>>> struct gsc_context *ctx; >>>> struct resource *res; >>>> - int ret, i; >>>> + int ret, i, id; >>>> + >>>> + ret = of_alias_get_id(pdev->dev.of_node, "gsc"); >>>> + if (ret < 0) >>>> + return ret; >>>> + id = ret; >>>> >>>> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >>>> if (!ctx) >>>> @@ -1254,7 +1259,7 @@ static int gsc_probe(struct platform_device *pdev) >>>> } >>>> >>>> /* context initailization */ >>>> - ctx->id = pdev->id; >>>> + ctx->id = id; >>> Why do you need ctx->id at all? I see it is used only in dev_dbg and >>> dev_err messages but these should be easily identifiable by device >>> name+address. Maybe get rid of ctx->id entirely? >> I am working on patches adding framebuffer display pre-processing >> on-the-fly, they requires gscaler id to program sysreg registers. I hope >> to post it in near future. > OK, makes sense. Then I have a dependent comment - if alias id is used > by driver then probably it should be validated to prevent errors like > out-of-bounds access. DTB theoretically might come from out-of-tree or > from older version. Something like this was reported for our pinctrl > driver: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=93b0beae721b3344923b4b8317e9d83b542f4ca6 > Probably the validation should come with your code, Andrzej. Not here. OK, I will add it in my patchset. Regards Andrzej > > Best regards, > Krzysztof > > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" 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/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c index e99dd1e4ba65..a63287597985 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c @@ -1196,7 +1196,12 @@ static int gsc_probe(struct platform_device *pdev) struct exynos_drm_ipp_formats *formats; struct gsc_context *ctx; struct resource *res; - int ret, i; + int ret, i, id; + + ret = of_alias_get_id(pdev->dev.of_node, "gsc"); + if (ret < 0) + return ret; + id = ret; ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); if (!ctx) @@ -1254,7 +1259,7 @@ static int gsc_probe(struct platform_device *pdev) } /* context initailization */ - ctx->id = pdev->id; + ctx->id = id; platform_set_drvdata(pdev, ctx);
Platform devices instantiated from device-tree always have pdev->id set to -1, so use of_get_alias_id() helper to retrieve proper device id. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/gpu/drm/exynos/exynos_drm_gsc.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) -- 2.17.1 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html