Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()
Hi
Am 07.10.22 um 09:29 schrieb Ville Syrjälä:
On Fri, Oct 07, 2022 at 09:17:50AM +0200, Javier Martinez Canillas wrote:
On 10/7/22 09:07, Ville Syrjälä wrote:
On Thu, Oct 06, 2022 at 10:28:12PM +0200, Javier Martinez Canillas wrote:
On 10/5/22 13:40, Thomas Zimmermann wrote:
Rename the atomic helper function drm_atomic_helper_check_crtc_state()
to drm_atomic_helper_check_crtc_primary_plane() and only check for an
attached primary plane. Adapt callers.
Instead of having one big function to check for various CRTC state
conditions, we rather want smaller functions that drivers can pick
individually.
Signed-off-by: Thomas Zimmermann
---
Reviewed-by: Javier Martinez Canillas
[...]
+ drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
+ if (plane->type == DRM_PLANE_TYPE_PRIMARY)
+ return 0;
}
I believe the code convention is to drop the curly braces when you
have a single statement inside the a loop ?
This has two.
No, it has only one that is the if statement. So according to the Linux
kernel coding style AFAIU it should be written as:
drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask)
if (plane->type == DRM_PLANE_TYPE_PRIMARY)
return 0;
That is exactly what it says not to do.
Hey, no need to be so upfront about it. Without the outer braces, I'd
find it hard to parse anyway.
Best regard
Thomas
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()
On 10/7/22 09:29, Ville Syrjälä wrote: [...] >> drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) >> if (plane->type == DRM_PLANE_TYPE_PRIMARY) >> return 0; > > That is exactly what it says not to do. > Ah, good to know. I re-read the kernel coding style and see now that it only applies to single simple statements. Better then, since not using braces even for single statements is something that's error prone IMO, as mentioned before. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()
On Fri, Oct 07, 2022 at 09:17:50AM +0200, Javier Martinez Canillas wrote:
> On 10/7/22 09:07, Ville Syrjälä wrote:
> > On Thu, Oct 06, 2022 at 10:28:12PM +0200, Javier Martinez Canillas wrote:
> >> On 10/5/22 13:40, Thomas Zimmermann wrote:
> >>> Rename the atomic helper function drm_atomic_helper_check_crtc_state()
> >>> to drm_atomic_helper_check_crtc_primary_plane() and only check for an
> >>> attached primary plane. Adapt callers.
> >>>
> >>> Instead of having one big function to check for various CRTC state
> >>> conditions, we rather want smaller functions that drivers can pick
> >>> individually.
> >>>
> >>> Signed-off-by: Thomas Zimmermann
> >>> ---
> >>
> >> Reviewed-by: Javier Martinez Canillas
> >>
> >> [...]
> >>
> >>> + drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> >>> + if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> >>> + return 0;
> >>> }
> >>
> >> I believe the code convention is to drop the curly braces when you
> >> have a single statement inside the a loop ?
> >
> > This has two.
> >
>
> No, it has only one that is the if statement. So according to the Linux
> kernel coding style AFAIU it should be written as:
>
> drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask)
> if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> return 0;
That is exactly what it says not to do.
--
Ville Syrjälä
Intel
Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()
On 10/7/22 09:07, Ville Syrjälä wrote:
> On Thu, Oct 06, 2022 at 10:28:12PM +0200, Javier Martinez Canillas wrote:
>> On 10/5/22 13:40, Thomas Zimmermann wrote:
>>> Rename the atomic helper function drm_atomic_helper_check_crtc_state()
>>> to drm_atomic_helper_check_crtc_primary_plane() and only check for an
>>> attached primary plane. Adapt callers.
>>>
>>> Instead of having one big function to check for various CRTC state
>>> conditions, we rather want smaller functions that drivers can pick
>>> individually.
>>>
>>> Signed-off-by: Thomas Zimmermann
>>> ---
>>
>> Reviewed-by: Javier Martinez Canillas
>>
>> [...]
>>
>>> + drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
>>> + if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>>> + return 0;
>>> }
>>
>> I believe the code convention is to drop the curly braces when you
>> have a single statement inside the a loop ?
>
> This has two.
>
No, it has only one that is the if statement. So according to the Linux
kernel coding style AFAIU it should be written as:
drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask)
if (plane->type == DRM_PLANE_TYPE_PRIMARY)
return 0;
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()
On Thu, Oct 06, 2022 at 10:28:12PM +0200, Javier Martinez Canillas wrote:
> On 10/5/22 13:40, Thomas Zimmermann wrote:
> > Rename the atomic helper function drm_atomic_helper_check_crtc_state()
> > to drm_atomic_helper_check_crtc_primary_plane() and only check for an
> > attached primary plane. Adapt callers.
> >
> > Instead of having one big function to check for various CRTC state
> > conditions, we rather want smaller functions that drivers can pick
> > individually.
> >
> > Signed-off-by: Thomas Zimmermann
> > ---
>
> Reviewed-by: Javier Martinez Canillas
>
> [...]
>
> > + drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> > + if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > + return 0;
> > }
>
> I believe the code convention is to drop the curly braces when you
> have a single statement inside the a loop ?
This has two.
>
> Feel free to ignore it though. I particularly don't agree with that
> convention anyways, because I think that makes the code more error
> prone. But still thought that was worth to point that out.
>
> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
--
Ville Syrjälä
Intel
Re: [PATCH v4 2/2] drm/atomic-helper: Replace drm_atomic_helper_check_crtc_state()
On 10/5/22 13:40, Thomas Zimmermann wrote:
> Rename the atomic helper function drm_atomic_helper_check_crtc_state()
> to drm_atomic_helper_check_crtc_primary_plane() and only check for an
> attached primary plane. Adapt callers.
>
> Instead of having one big function to check for various CRTC state
> conditions, we rather want smaller functions that drivers can pick
> individually.
>
> Signed-off-by: Thomas Zimmermann
> ---
Reviewed-by: Javier Martinez Canillas
[...]
> + drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) {
> + if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> + return 0;
> }
I believe the code convention is to drop the curly braces when you
have a single statement inside the a loop ?
Feel free to ignore it though. I particularly don't agree with that
convention anyways, because I think that makes the code more error
prone. But still thought that was worth to point that out.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
