diff mbox series

[V2,09/16] drm/vc4: v3d: simplify clock retrieval

Message ID 20240728130029.78279-1-wahrenst@gmx.net (mailing list archive)
State New, archived
Headers show
Series ARM: bcm2835: Implement initial S2Idle for Raspberry Pi | expand

Commit Message

Stefan Wahren July 28, 2024, 1 p.m. UTC
Common pattern of handling deferred probe can be simplified with
dev_err_probe() and devm_clk_get_optional(). This results in much
less code.

Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
---
 drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

--
2.34.1

Comments

Maíra Canal July 30, 2024, 11:23 a.m. UTC | #1
On 7/28/24 10:00, Stefan Wahren wrote:
> Common pattern of handling deferred probe can be simplified with
> dev_err_probe() and devm_clk_get_optional(). This results in much
> less code.
> 
> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
> ---
>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>   1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> index 1ede508a67d3..4bf3a8d24770 100644
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -441,20 +441,11 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
>   	vc4->v3d = v3d;
>   	v3d->vc4 = vc4;
> 
> -	v3d->clk = devm_clk_get(dev, NULL);
> +	v3d->clk = devm_clk_get_optional(dev, NULL);
>   	if (IS_ERR(v3d->clk)) {
>   		int ret = PTR_ERR(v3d->clk);
> 

Super nit: you could delete this line ^

Reviewed-by: Maíra Canal <mcanal@igalia.com>

Best Regards,
- Maíra

> -		if (ret == -ENOENT) {
> -			/* bcm2835 didn't have a clock reference in the DT. */
> -			ret = 0;
> -			v3d->clk = NULL;
> -		} else {
> -			if (ret != -EPROBE_DEFER)
> -				dev_err(dev, "Failed to get V3D clock: %d\n",
> -					ret);
> -			return ret;
> -		}
> +		return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
>   	}
> 
>   	ret = platform_get_irq(pdev, 0);
> --
> 2.34.1
>
Stefan Wahren July 31, 2024, 4:41 p.m. UTC | #2
Hi Maíra,

Am 30.07.24 um 13:23 schrieb Maíra Canal:
> On 7/28/24 10:00, Stefan Wahren wrote:
>> Common pattern of handling deferred probe can be simplified with
>> dev_err_probe() and devm_clk_get_optional(). This results in much
>> less code.
>>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> ---
>>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c
>> b/drivers/gpu/drm/vc4/vc4_v3d.c
>> index 1ede508a67d3..4bf3a8d24770 100644
>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
>> @@ -441,20 +441,11 @@ static int vc4_v3d_bind(struct device *dev,
>> struct device *master, void *data)
>>       vc4->v3d = v3d;
>>       v3d->vc4 = vc4;
>>
>> -    v3d->clk = devm_clk_get(dev, NULL);
>> +    v3d->clk = devm_clk_get_optional(dev, NULL);
>>       if (IS_ERR(v3d->clk)) {
>>           int ret = PTR_ERR(v3d->clk);
>>
>
> Super nit: you could delete this line ^
Can you please explain? ret is required for dev_err_probe or do you mean
the empty line after the declaration?
>
> Reviewed-by: Maíra Canal <mcanal@igalia.com>
>
> Best Regards,
> - Maíra
>
>> -        if (ret == -ENOENT) {
>> -            /* bcm2835 didn't have a clock reference in the DT. */
>> -            ret = 0;
>> -            v3d->clk = NULL;
>> -        } else {
>> -            if (ret != -EPROBE_DEFER)
>> -                dev_err(dev, "Failed to get V3D clock: %d\n",
>> -                    ret);
>> -            return ret;
>> -        }
>> +        return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
>>       }
>>
>>       ret = platform_get_irq(pdev, 0);
>> --
>> 2.34.1
>>
>
Maíra Canal Aug. 2, 2024, 12:56 p.m. UTC | #3
Hi Stefan,

On 7/31/24 13:41, Stefan Wahren wrote:
> Hi Maíra,
> 
> Am 30.07.24 um 13:23 schrieb Maíra Canal:
>> On 7/28/24 10:00, Stefan Wahren wrote:
>>> Common pattern of handling deferred probe can be simplified with
>>> dev_err_probe() and devm_clk_get_optional(). This results in much
>>> less code.
>>>
>>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>>> ---
>>>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c
>>> b/drivers/gpu/drm/vc4/vc4_v3d.c
>>> index 1ede508a67d3..4bf3a8d24770 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
>>> @@ -441,20 +441,11 @@ static int vc4_v3d_bind(struct device *dev,
>>> struct device *master, void *data)
>>>       vc4->v3d = v3d;
>>>       v3d->vc4 = vc4;
>>>
>>> -    v3d->clk = devm_clk_get(dev, NULL);
>>> +    v3d->clk = devm_clk_get_optional(dev, NULL);
>>>       if (IS_ERR(v3d->clk)) {
>>>           int ret = PTR_ERR(v3d->clk);
>>>
>>
>> Super nit: you could delete this line ^
> Can you please explain? ret is required for dev_err_probe or do you mean
> the empty line after the declaration?

Just deleting the empty line after the declaration. It is a super small
nit indeed.

Best Regards,
- Maíra

>>
>> Reviewed-by: Maíra Canal <mcanal@igalia.com>
>>
>> Best Regards,
>> - Maíra
>>
>>> -        if (ret == -ENOENT) {
>>> -            /* bcm2835 didn't have a clock reference in the DT. */
>>> -            ret = 0;
>>> -            v3d->clk = NULL;
>>> -        } else {
>>> -            if (ret != -EPROBE_DEFER)
>>> -                dev_err(dev, "Failed to get V3D clock: %d\n",
>>> -                    ret);
>>> -            return ret;
>>> -        }
>>> +        return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
>>>       }
>>>
>>>       ret = platform_get_irq(pdev, 0);
>>> -- 
>>> 2.34.1
>>>
>>
>
Stefan Wahren Aug. 2, 2024, 1 p.m. UTC | #4
Hi Maíra,

Am 02.08.24 um 14:56 schrieb Maíra Canal:
> Hi Stefan,
>
> On 7/31/24 13:41, Stefan Wahren wrote:
>> Hi Maíra,
>>
>> Am 30.07.24 um 13:23 schrieb Maíra Canal:
>>> On 7/28/24 10:00, Stefan Wahren wrote:
>>>> Common pattern of handling deferred probe can be simplified with
>>>> dev_err_probe() and devm_clk_get_optional(). This results in much
>>>> less code.
>>>>
>>>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>>>> ---
>>>>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>> b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>> index 1ede508a67d3..4bf3a8d24770 100644
>>>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>> @@ -441,20 +441,11 @@ static int vc4_v3d_bind(struct device *dev,
>>>> struct device *master, void *data)
>>>>       vc4->v3d = v3d;
>>>>       v3d->vc4 = vc4;
>>>>
>>>> -    v3d->clk = devm_clk_get(dev, NULL);
>>>> +    v3d->clk = devm_clk_get_optional(dev, NULL);
>>>>       if (IS_ERR(v3d->clk)) {
>>>>           int ret = PTR_ERR(v3d->clk);
>>>>
>>>
>>> Super nit: you could delete this line ^
>> Can you please explain? ret is required for dev_err_probe or do you mean
>> the empty line after the declaration?
>
> Just deleting the empty line after the declaration. It is a super small
> nit indeed.
AFAIK an empty line after a declaration is part of the coding style. Or
is different in drm?

Best regards
>
> Best Regards,
> - Maíra
>
>>>
>>> Reviewed-by: Maíra Canal <mcanal@igalia.com>
>>>
>>> Best Regards,
>>> - Maíra
>>>
>>>> -        if (ret == -ENOENT) {
>>>> -            /* bcm2835 didn't have a clock reference in the DT. */
>>>> -            ret = 0;
>>>> -            v3d->clk = NULL;
>>>> -        } else {
>>>> -            if (ret != -EPROBE_DEFER)
>>>> -                dev_err(dev, "Failed to get V3D clock: %d\n",
>>>> -                    ret);
>>>> -            return ret;
>>>> -        }
>>>> +        return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
>>>>       }
>>>>
>>>>       ret = platform_get_irq(pdev, 0);
>>>> --
>>>> 2.34.1
>>>>
>>>
>>
Maíra Canal Aug. 7, 2024, 2:31 p.m. UTC | #5
Hi Stefan,

On 8/2/24 10:00, Stefan Wahren wrote:
> Hi Maíra,
> 
> Am 02.08.24 um 14:56 schrieb Maíra Canal:
>> Hi Stefan,
>>
>> On 7/31/24 13:41, Stefan Wahren wrote:
>>> Hi Maíra,
>>>
>>> Am 30.07.24 um 13:23 schrieb Maíra Canal:
>>>> On 7/28/24 10:00, Stefan Wahren wrote:
>>>>> Common pattern of handling deferred probe can be simplified with
>>>>> dev_err_probe() and devm_clk_get_optional(). This results in much
>>>>> less code.
>>>>>
>>>>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>>>>> ---
>>>>>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>>>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>> b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>> index 1ede508a67d3..4bf3a8d24770 100644
>>>>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>> @@ -441,20 +441,11 @@ static int vc4_v3d_bind(struct device *dev,
>>>>> struct device *master, void *data)
>>>>>       vc4->v3d = v3d;
>>>>>       v3d->vc4 = vc4;
>>>>>
>>>>> -    v3d->clk = devm_clk_get(dev, NULL);
>>>>> +    v3d->clk = devm_clk_get_optional(dev, NULL);
>>>>>       if (IS_ERR(v3d->clk)) {
>>>>>           int ret = PTR_ERR(v3d->clk);
>>>>>
>>>>
>>>> Super nit: you could delete this line ^
>>> Can you please explain? ret is required for dev_err_probe or do you mean
>>> the empty line after the declaration?
>>
>> Just deleting the empty line after the declaration. It is a super small
>> nit indeed.
> AFAIK an empty line after a declaration is part of the coding style. Or
> is different in drm?

TBH I just checked the result of `git grep "dev_err_probe"` and I
noticed that most of the times, we don't add an empty line after the
declaration in this case or we don't even create a variable, something
like:

return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");

But it is a pretty small nit. Feel free to ignore it.

Also, let me know if you need me to apply any patches to drm-misc-next.

Best Regards,
- Maíra

> 
> Best regards
>>
>> Best Regards,
>> - Maíra
>>
>>>>
>>>> Reviewed-by: Maíra Canal <mcanal@igalia.com>
>>>>
>>>> Best Regards,
>>>> - Maíra
>>>>
>>>>> -        if (ret == -ENOENT) {
>>>>> -            /* bcm2835 didn't have a clock reference in the DT. */
>>>>> -            ret = 0;
>>>>> -            v3d->clk = NULL;
>>>>> -        } else {
>>>>> -            if (ret != -EPROBE_DEFER)
>>>>> -                dev_err(dev, "Failed to get V3D clock: %d\n",
>>>>> -                    ret);
>>>>> -            return ret;
>>>>> -        }
>>>>> +        return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
>>>>>       }
>>>>>
>>>>>       ret = platform_get_irq(pdev, 0);
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>>
>>>
>
Stefan Wahren Aug. 7, 2024, 2:49 p.m. UTC | #6
Hi Maíra,

Am 07.08.24 um 16:31 schrieb Maíra Canal:
> Hi Stefan,
>
> On 8/2/24 10:00, Stefan Wahren wrote:
>> Hi Maíra,
>>
>> Am 02.08.24 um 14:56 schrieb Maíra Canal:
>>> Hi Stefan,
>>>
>>> On 7/31/24 13:41, Stefan Wahren wrote:
>>>> Hi Maíra,
>>>>
>>>> Am 30.07.24 um 13:23 schrieb Maíra Canal:
>>>>> On 7/28/24 10:00, Stefan Wahren wrote:
>>>>>> Common pattern of handling deferred probe can be simplified with
>>>>>> dev_err_probe() and devm_clk_get_optional(). This results in much
>>>>>> less code.
>>>>>>
>>>>>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>>>>>> ---
>>>>>>   drivers/gpu/drm/vc4/vc4_v3d.c | 13 ++-----------
>>>>>>   1 file changed, 2 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>>> b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>>> index 1ede508a67d3..4bf3a8d24770 100644
>>>>>> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>>> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
>>>>>> @@ -441,20 +441,11 @@ static int vc4_v3d_bind(struct device *dev,
>>>>>> struct device *master, void *data)
>>>>>>       vc4->v3d = v3d;
>>>>>>       v3d->vc4 = vc4;
>>>>>>
>>>>>> -    v3d->clk = devm_clk_get(dev, NULL);
>>>>>> +    v3d->clk = devm_clk_get_optional(dev, NULL);
>>>>>>       if (IS_ERR(v3d->clk)) {
>>>>>>           int ret = PTR_ERR(v3d->clk);
>>>>>>
>>>>>
>>>>> Super nit: you could delete this line ^
>>>> Can you please explain? ret is required for dev_err_probe or do you
>>>> mean
>>>> the empty line after the declaration?
>>>
>>> Just deleting the empty line after the declaration. It is a super small
>>> nit indeed.
>> AFAIK an empty line after a declaration is part of the coding style. Or
>> is different in drm?
>
> TBH I just checked the result of `git grep "dev_err_probe"` and I
> noticed that most of the times, we don't add an empty line after the
> declaration in this case or we don't even create a variable, something
> like:
>
> return dev_err_probe(dev, PTR_ERR(v3d->clk), "Failed to get V3D clock\n");
i will go for the latter variant.

I will send a new version which also addresses your comments regarding
patch 7, so they can be applied at once.

But i still need to wait for some feedback for patch 14 before sending
v3, which is the most important part of the series. But I also hope that
some of the firmware/mailbox/pmdomain patches at the beginning are also
applied before.

>
> But it is a pretty small nit. Feel free to ignore it.
>
> Also, let me know if you need me to apply any patches to drm-misc-next.

Yes, this would be nice to apply the vc4/v3d stuff in the next version,
so the series becomes shorter and easier to handle.

Best regards
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 1ede508a67d3..4bf3a8d24770 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -441,20 +441,11 @@  static int vc4_v3d_bind(struct device *dev, struct device *master, void *data)
 	vc4->v3d = v3d;
 	v3d->vc4 = vc4;

-	v3d->clk = devm_clk_get(dev, NULL);
+	v3d->clk = devm_clk_get_optional(dev, NULL);
 	if (IS_ERR(v3d->clk)) {
 		int ret = PTR_ERR(v3d->clk);

-		if (ret == -ENOENT) {
-			/* bcm2835 didn't have a clock reference in the DT. */
-			ret = 0;
-			v3d->clk = NULL;
-		} else {
-			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "Failed to get V3D clock: %d\n",
-					ret);
-			return ret;
-		}
+		return dev_err_probe(dev, ret, "Failed to get V3D clock\n");
 	}

 	ret = platform_get_irq(pdev, 0);