Message-Id: <D7OT982LY0H1.1P6XUU7YTDDMB@gmail.com>
Date: Mon, 10 Feb 2025 08:47:13 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: <platform-driver-x86@...r.kernel.org>, "Armin Wolf" <W_Armin@....de>,
 "Mario Limonciello" <mario.limonciello@....com>, "Hans de Goede"
 <hdegoede@...hat.com>, <Dell.Client.Kernel@...l.com>, "LKML"
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v9 11/14] platform/x86: Split the alienware-wmi driver

On Mon Feb 10, 2025 at 6:53 AM -05, Ilpo Järvinen wrote:
> On Fri, 7 Feb 2025, Kurt Borja wrote:
>
>> On Fri Feb 7, 2025 at 10:05 AM -05, Ilpo Järvinen wrote:
>> > On Fri, 7 Feb 2025, Kurt Borja wrote:
>> >
>> >> Split alienware-wmi WMI drivers into different files. This is done
>> >> seamlessly by copying and pasting, however some blocks are reordered.
>> >> 
>> >> Reviewed-by: Armin Wolf <W_Armin@....de>
>> >> Reviewed-by: Mario Limonciello <mario.limonciello@....com>
>> >> Signed-off-by: Kurt Borja <kuurtb@...il.com>
>> >
>> > Hi,
>> >
>> > Can you please check there's no error in driver_data assignments as the 
>> > numbers in removed & added lines do not match:
>> 
>> Hi Ilpo,
>> 
>> There was indeed a wrong assignment to Alienware m16 r1 AMD, I'm not
>> really sure how it happened but it's fixed now!
>> 
>> I'll send a v10. I apologize for the noise.
>> 
>> >
>> > $ git diff-tree -p 73224c076cf2fa2968d61584c62937f6180c8e71 | grep driver_data | rev | sort | rev | uniq -c
>> 
>> Thanks for this amazing trick btw.
>
> Apparently the rev trick wouldn't have been even necessary in this case 
> but writing those things are a second nature to me. When reviewing 
> especially move changes, one has to have a bag full of tricks. :-)

Any trick would have saved time I spent working on this patch :)

>
> It is one of the reasons why I prefer to have move patches do as little 
> extra work as possible because I can use pipelines to verify the pre and 
> post content is identical.
>
> I usually starting by diffing - and + lines in the diff which is how I 
> came across this one too. In the best case there are no code line changes 
> at all but all changes are in the boilerplate, it gives very high 
> confidence on the move being done correctly. When a rebase conflicts, 
> everyone (me included), might introduce unintended changes to move-only 
> patches so I tend to check even my own move patches in similar fashion to 
> avoid making stupid mistakes.

Speaking of this. Let's say I want to add a new model to the DMI list,
how should I go about it? 

If I base it on the fixes branches it's going to conflict when merging
with Linus, and even worse, it would need to be manually added to
alienware-wmi-wmax.c every time it happens.

My solution is to just base the added models on the for-next branch. Of
course users wouldn't get this until v6.15 but I'd prefer not to give
you or some other maintainer extra work.

Another solution is to make two patches one for for-next and one for
stable, but I don't know if people do this to begin with.

What do you think about this?

--
 ~ Kurt

>
> Even the diff of diffs wouldn't get me to 100%, it find that <5% that 
> differs so I can focus on whether it is sound.
>
>> 
>> ~ Kurt
>> 
>> >       1 +               awcc = id->driver_data;
>> >       1 -               awcc = id->driver_data;
>> >       4 +               .driver_data = &generic_quirks,
>> >       5 -               .driver_data = &generic_quirks,
>> >       7 +               .driver_data = &g_series_quirks,
>> >       6 -               .driver_data = &g_series_quirks,
>> >
>> > (That commit id is from my staging tree, not available to you but it's 
>> > this patch.)
>> 

Powered by blists - more mailing lists