| Message ID | 20240728130029.78279-1-wahrenst@gmx.net |
|---|---|
| State | New |
| Headers | show |
| Series | ARM: bcm2835: Implement initial S2Idle for Raspberry Pi | expand |
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 >>> >> >
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 >>>> >>> >>
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 --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);
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