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