media: uvcvideo: Enable keep-sorted

[PATCH 0/2] media: uvcvideo: Enable keep-sorted

Posted by Ricardo Ribalda 9 months, 2 weeks ago

When committers contribute quirks to the uvc driver, they usually add
them out of order.

We can automatically validate that their follow our guidelines with the
use of keep-sorted.

This patchset adds support for keep-sorted in the uvc driver. The two
patches can be squashed if needed.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Ricardo Ribalda (2):
      media: uvcvideo: Rewrite uvc_ids for keep-sorted.
      media: uvcvideo: Add keep-sorted statement and sort uvc_ids

 drivers/media/usb/uvc/uvc_driver.c | 660 +++++++++++++++++++++++--------------
 1 file changed, 409 insertions(+), 251 deletions(-)
---
base-commit: 398a1b33f1479af35ca915c5efc9b00d6204f8fa
change-id: 20250429-keep-sorted-2ac6f4796726

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

Re: [PATCH 0/2] media: uvcvideo: Enable keep-sorted

Posted by Hans de Goede 7 months, 3 weeks ago

Hi Ricardo,

On 29-Apr-25 15:47, Ricardo Ribalda wrote:
> When committers contribute quirks to the uvc driver, they usually add
> them out of order.
> 
> We can automatically validate that their follow our guidelines with the
> use of keep-sorted.
> 
> This patchset adds support for keep-sorted in the uvc driver. The two
> patches can be squashed if needed.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

I've no objections against these 2 patches, but these need to be
rebased on top of the latest uvc/for-next. Can you send out a new
version please ?

Also for patch 2/2 can we not just add the 2 keep-sorted headers
and then actually run keep-sorted to auto-fix things ?

Or can it not auto-fix ?

Regards,

Hans



> ---
> Ricardo Ribalda (2):
>       media: uvcvideo: Rewrite uvc_ids for keep-sorted.
>       media: uvcvideo: Add keep-sorted statement and sort uvc_ids
> 
>  drivers/media/usb/uvc/uvc_driver.c | 660 +++++++++++++++++++++++--------------
>  1 file changed, 409 insertions(+), 251 deletions(-)
> ---
> base-commit: 398a1b33f1479af35ca915c5efc9b00d6204f8fa
> change-id: 20250429-keep-sorted-2ac6f4796726
> 
> Best regards,

Re: [PATCH 0/2] media: uvcvideo: Enable keep-sorted

Posted by Ricardo Ribalda 7 months, 3 weeks ago

Hi Hans

On Mon, 16 Jun 2025 at 15:05, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Ricardo,
>
> On 29-Apr-25 15:47, Ricardo Ribalda wrote:
> > When committers contribute quirks to the uvc driver, they usually add
> > them out of order.
> >
> > We can automatically validate that their follow our guidelines with the
> > use of keep-sorted.
> >
> > This patchset adds support for keep-sorted in the uvc driver. The two
> > patches can be squashed if needed.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>
> I've no objections against these 2 patches, but these need to be
> rebased on top of the latest uvc/for-next. Can you send out a new
> version please ?

I was waiting for HansV to say that keep-sorted was useful and then
add it to the CI.

Right now it is probably just useful for the Makefiles and uvc.

>
> Also for patch 2/2 can we not just add the 2 keep-sorted headers
> and then actually run keep-sorted to auto-fix things ?

Do you mean removing the annotation after running keep-sorted?

We can do that, but we will be unsorted again in the future after N
patches unless we add it to CI.

If we do not handle this automatically I do not think that there is
much point on this series.


Thanks for looking into it anyway :)

>
> Or can it not auto-fix ?
>
> Regards,
>
> Hans
>
>
>
> > ---
> > Ricardo Ribalda (2):
> >       media: uvcvideo: Rewrite uvc_ids for keep-sorted.
> >       media: uvcvideo: Add keep-sorted statement and sort uvc_ids
> >
> >  drivers/media/usb/uvc/uvc_driver.c | 660 +++++++++++++++++++++++--------------
> >  1 file changed, 409 insertions(+), 251 deletions(-)
> > ---
> > base-commit: 398a1b33f1479af35ca915c5efc9b00d6204f8fa
> > change-id: 20250429-keep-sorted-2ac6f4796726
> >
> > Best regards,
>


-- 
Ricardo Ribalda

Re: [PATCH 0/2] media: uvcvideo: Enable keep-sorted

Posted by Hans de Goede 7 months, 3 weeks ago

Hi Ricardo,

On 16-Jun-25 15:22, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Mon, 16 Jun 2025 at 15:05, Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi Ricardo,
>>
>> On 29-Apr-25 15:47, Ricardo Ribalda wrote:
>>> When committers contribute quirks to the uvc driver, they usually add
>>> them out of order.
>>>
>>> We can automatically validate that their follow our guidelines with the
>>> use of keep-sorted.
>>>
>>> This patchset adds support for keep-sorted in the uvc driver. The two
>>> patches can be squashed if needed.
>>>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>
>> I've no objections against these 2 patches, but these need to be
>> rebased on top of the latest uvc/for-next. Can you send out a new
>> version please ?
> 
> I was waiting for HansV to say that keep-sorted was useful and then
> add it to the CI.

Ok, so should we drop this series from patchwork then ?

> 
> Right now it is probably just useful for the Makefiles and uvc.
> 
>>
>> Also for patch 2/2 can we not just add the 2 keep-sorted headers
>> and then actually run keep-sorted to auto-fix things ?
> 
> Do you mean removing the annotation after running keep-sorted?
> 
> We can do that, but we will be unsorted again in the future after N
> patches unless we add it to CI.
> 
> If we do not handle this automatically I do not think that there is
> much point on this series.

What I meant is only add the annotations and then run something
like:

keepsorted --auto-fix drivers/media/usb/uvc/uvc_driver.c

and submit the result as a separate commit. Assuming that there is such
a thing as --auto-fix. The reason for this is that if the sorting is done
by a tool there is last chance for it to accidentally break things.

Regards,

Hans

Re: [PATCH 0/2] media: uvcvideo: Enable keep-sorted

Posted by Ricardo Ribalda 7 months, 3 weeks ago

On Mon, 16 Jun 2025 at 15:26, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Ricardo,
>
> On 16-Jun-25 15:22, Ricardo Ribalda wrote:
> > Hi Hans
> >
> > On Mon, 16 Jun 2025 at 15:05, Hans de Goede <hansg@kernel.org> wrote:
> >>
> >> Hi Ricardo,
> >>
> >> On 29-Apr-25 15:47, Ricardo Ribalda wrote:
> >>> When committers contribute quirks to the uvc driver, they usually add
> >>> them out of order.
> >>>
> >>> We can automatically validate that their follow our guidelines with the
> >>> use of keep-sorted.
> >>>
> >>> This patchset adds support for keep-sorted in the uvc driver. The two
> >>> patches can be squashed if needed.
> >>>
> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>
> >> I've no objections against these 2 patches, but these need to be
> >> rebased on top of the latest uvc/for-next. Can you send out a new
> >> version please ?
> >
> > I was waiting for HansV to say that keep-sorted was useful and then
> > add it to the CI.
>
> Ok, so should we drop this series from patchwork then ?

If the series does not bother you too much in patchwork let it stay
there until HansV replies to the makefile series.

>
> >
> > Right now it is probably just useful for the Makefiles and uvc.
> >
> >>
> >> Also for patch 2/2 can we not just add the 2 keep-sorted headers
> >> and then actually run keep-sorted to auto-fix things ?
> >
> > Do you mean removing the annotation after running keep-sorted?
> >
> > We can do that, but we will be unsorted again in the future after N
> > patches unless we add it to CI.
> >
> > If we do not handle this automatically I do not think that there is
> > much point on this series.
>
> What I meant is only add the annotations and then run something
> like:
>
> keepsorted --auto-fix drivers/media/usb/uvc/uvc_driver.c
>
> and submit the result as a separate commit. Assuming that there is such
> a thing as --auto-fix. The reason for this is that if the sorting is done
> by a tool there is last chance for it to accidentally break things.

keep-sorted can work in two modes: fix and lint.

If HansV finds it useful I will refactor this patch with the extra step.

Thanks :)

>
> Regards,
>
> Hans
>
>


-- 
Ricardo Ribalda

Re: [PATCH 0/2] media: uvcvideo: Enable keep-sorted

Posted by Hans de Goede 7 months, 3 weeks ago

Hi Ricardo,

On 16-Jun-25 15:31, Ricardo Ribalda wrote:
> On Mon, 16 Jun 2025 at 15:26, Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi Ricardo,
>>
>> On 16-Jun-25 15:22, Ricardo Ribalda wrote:
>>> Hi Hans
>>>
>>> On Mon, 16 Jun 2025 at 15:05, Hans de Goede <hansg@kernel.org> wrote:
>>>>
>>>> Hi Ricardo,
>>>>
>>>> On 29-Apr-25 15:47, Ricardo Ribalda wrote:
>>>>> When committers contribute quirks to the uvc driver, they usually add
>>>>> them out of order.
>>>>>
>>>>> We can automatically validate that their follow our guidelines with the
>>>>> use of keep-sorted.
>>>>>
>>>>> This patchset adds support for keep-sorted in the uvc driver. The two
>>>>> patches can be squashed if needed.
>>>>>
>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>
>>>> I've no objections against these 2 patches, but these need to be
>>>> rebased on top of the latest uvc/for-next. Can you send out a new
>>>> version please ?
>>>
>>> I was waiting for HansV to say that keep-sorted was useful and then
>>> add it to the CI.
>>
>> Ok, so should we drop this series from patchwork then ?
> 
> If the series does not bother you too much in patchwork let it stay
> there until HansV replies to the makefile series.

Sure that works for me.

Regards,

Hans

Re: [PATCH 0/2] media: uvcvideo: Enable keep-sorted

Posted by Hans Verkuil 7 months, 3 weeks ago

On 16/06/2025 15:38, Hans de Goede wrote:
> Hi Ricardo,
> 
> On 16-Jun-25 15:31, Ricardo Ribalda wrote:
>> On Mon, 16 Jun 2025 at 15:26, Hans de Goede <hansg@kernel.org> wrote:
>>>
>>> Hi Ricardo,
>>>
>>> On 16-Jun-25 15:22, Ricardo Ribalda wrote:
>>>> Hi Hans
>>>>
>>>> On Mon, 16 Jun 2025 at 15:05, Hans de Goede <hansg@kernel.org> wrote:
>>>>>
>>>>> Hi Ricardo,
>>>>>
>>>>> On 29-Apr-25 15:47, Ricardo Ribalda wrote:
>>>>>> When committers contribute quirks to the uvc driver, they usually add
>>>>>> them out of order.
>>>>>>
>>>>>> We can automatically validate that their follow our guidelines with the
>>>>>> use of keep-sorted.
>>>>>>
>>>>>> This patchset adds support for keep-sorted in the uvc driver. The two
>>>>>> patches can be squashed if needed.
>>>>>>
>>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>>>>
>>>>> I've no objections against these 2 patches, but these need to be
>>>>> rebased on top of the latest uvc/for-next. Can you send out a new
>>>>> version please ?
>>>>
>>>> I was waiting for HansV to say that keep-sorted was useful and then
>>>> add it to the CI.
>>>
>>> Ok, so should we drop this series from patchwork then ?
>>
>> If the series does not bother you too much in patchwork let it stay
>> there until HansV replies to the makefile series.

I did that. Basically I don't like the keep-sorted annotation unless it
is rolled out kernel-wide. It's not something we should do just in the
media subsystem.

That doesn't mean that a patch fixing the uvc_ids order isn't welcome,
but just drop the annotation.

If we do that, then patch 1/2 is also no longer needed. Although it
feels more logical that match_flags is at the end. I leave that to
HdG and Laurent to decide.

Regards,

	Hans

> 
> Sure that works for me.
> 
> Regards,
> 
> Hans
> 
>

Re: [PATCH 0/2] media: uvcvideo: Enable keep-sorted

Posted by Laurent Pinchart 7 months, 3 weeks ago

On Tue, Jun 17, 2025 at 01:52:50PM +0200, Hans Verkuil wrote:
> On 16/06/2025 15:38, Hans de Goede wrote:
> > On 16-Jun-25 15:31, Ricardo Ribalda wrote:
> >> On Mon, 16 Jun 2025 at 15:26, Hans de Goede <hansg@kernel.org> wrote:
> >>> On 16-Jun-25 15:22, Ricardo Ribalda wrote:
> >>>> On Mon, 16 Jun 2025 at 15:05, Hans de Goede <hansg@kernel.org> wrote:
> >>>>> On 29-Apr-25 15:47, Ricardo Ribalda wrote:
> >>>>>> When committers contribute quirks to the uvc driver, they usually add
> >>>>>> them out of order.
> >>>>>>
> >>>>>> We can automatically validate that their follow our guidelines with the
> >>>>>> use of keep-sorted.
> >>>>>>
> >>>>>> This patchset adds support for keep-sorted in the uvc driver. The two
> >>>>>> patches can be squashed if needed.
> >>>>>>
> >>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >>>>>
> >>>>> I've no objections against these 2 patches, but these need to be
> >>>>> rebased on top of the latest uvc/for-next. Can you send out a new
> >>>>> version please ?
> >>>>
> >>>> I was waiting for HansV to say that keep-sorted was useful and then
> >>>> add it to the CI.
> >>>
> >>> Ok, so should we drop this series from patchwork then ?
> >>
> >> If the series does not bother you too much in patchwork let it stay
> >> there until HansV replies to the makefile series.
> 
> I did that. Basically I don't like the keep-sorted annotation unless it
> is rolled out kernel-wide. It's not something we should do just in the
> media subsystem.
> 
> That doesn't mean that a patch fixing the uvc_ids order isn't welcome,
> but just drop the annotation.
> 
> If we do that, then patch 1/2 is also no longer needed. Although it
> feels more logical that match_flags is at the end. I leave that to
> HdG and Laurent to decide.

.match_flags is first to match the order of the fields in the
usb_device_id structure. Is there a need to move it last, or is only the

	}, {

construct that the tool doesn't like ?

> > Sure that works for me.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/2] media: uvcvideo: Enable keep-sorted

Posted by Ricardo Ribalda 7 months, 3 weeks ago

On Tue, 17 Jun 2025 at 14:54, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Jun 17, 2025 at 01:52:50PM +0200, Hans Verkuil wrote:
> > On 16/06/2025 15:38, Hans de Goede wrote:
> > > On 16-Jun-25 15:31, Ricardo Ribalda wrote:
> > >> On Mon, 16 Jun 2025 at 15:26, Hans de Goede <hansg@kernel.org> wrote:
> > >>> On 16-Jun-25 15:22, Ricardo Ribalda wrote:
> > >>>> On Mon, 16 Jun 2025 at 15:05, Hans de Goede <hansg@kernel.org> wrote:
> > >>>>> On 29-Apr-25 15:47, Ricardo Ribalda wrote:
> > >>>>>> When committers contribute quirks to the uvc driver, they usually add
> > >>>>>> them out of order.
> > >>>>>>
> > >>>>>> We can automatically validate that their follow our guidelines with the
> > >>>>>> use of keep-sorted.
> > >>>>>>
> > >>>>>> This patchset adds support for keep-sorted in the uvc driver. The two
> > >>>>>> patches can be squashed if needed.
> > >>>>>>
> > >>>>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > >>>>>
> > >>>>> I've no objections against these 2 patches, but these need to be
> > >>>>> rebased on top of the latest uvc/for-next. Can you send out a new
> > >>>>> version please ?
> > >>>>
> > >>>> I was waiting for HansV to say that keep-sorted was useful and then
> > >>>> add it to the CI.
> > >>>
> > >>> Ok, so should we drop this series from patchwork then ?
> > >>
> > >> If the series does not bother you too much in patchwork let it stay
> > >> there until HansV replies to the makefile series.
> >
> > I did that. Basically I don't like the keep-sorted annotation unless it
> > is rolled out kernel-wide. It's not something we should do just in the
> > media subsystem.
> >
> > That doesn't mean that a patch fixing the uvc_ids order isn't welcome,
> > but just drop the annotation.
> >
> > If we do that, then patch 1/2 is also no longer needed. Although it
> > feels more logical that match_flags is at the end. I leave that to
> > HdG and Laurent to decide.
>
> .match_flags is first to match the order of the fields in the
> usb_device_id structure. Is there a need to move it last, or is only the
>
>         }, {
>
> construct that the tool doesn't like ?

The },{ construct is fine.

The tool sorts all the content in the block

Eg:
{
 tail= AA;
 head = BBB;
}

is sorted before:
{
 tail = CC;
 head= AAAA;
}
It can be tuned with a regex:
https://github.com/google/keep-sorted?tab=readme-ov-file#regular-expressions
But the syntax is not particularly nice.

This is why I moved "head" to the beginning of every struct.

Anyway, since keep-sorted is not going to be part of the CI unless it
is adopted by other subsystems we can ignore this for now.

Regards!

>
> > > Sure that works for me.
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda