drm/exynos: gsc: Get device id from OF alias

Message ID 20180615122653eucas1p2254aad8e6497ab1bcf11eb5c7fa6b063~4VRMRscw30433304333eucas1p2B@eucas1p2.samsung.com
State New
Headers show
Series
  • drm/exynos: gsc: Get device id from OF alias
Related show

Commit Message

Marek Szyprowski June 15, 2018, 12:26 p.m.
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

Comments

Krzysztof Kozlowski June 20, 2018, 10:40 a.m. | #1
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
Andrzej Hajda June 20, 2018, 11:38 a.m. | #2
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
Krzysztof Kozlowski June 20, 2018, 11:53 a.m. | #3
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
Andrzej Hajda June 20, 2018, 12:44 p.m. | #4
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

Patch

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