Re: [PATCH v1 1/1] Introduction of HP-BIOSCFG driver

["Limonciello, Mario" <mario.limonciello@amd.com>]

On 10/17/2022 10:37, Hans de Goede wrote:
> Hi,
> 
> On 10/17/22 17:20, Jorge Lopez wrote:
>> Hi Hans,
>>
>> On Mon, Oct 17, 2022 at 9:37 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> On 10/17/22 16:29, Limonciello, Mario wrote:
>>>> FYI When you submit v3, you don't need to add "new patches on top" for your feedbacks to the new driver, they can roll into the patch introducing hp-cfg.  Just make sure you include a changelog under your cut line to indicate you changed these from vX->vY
>>>>
>>>> I suspect that Hans will also want you to split the driver up into smaller bite-size patches to make his review easier as well, but I'll let him advise how he wants it done.
>>>>
>>>> On 10/17/2022 09:11, Jorge Lopez wrote:
>>>>> ''Hi Mario,
>>>>>
>>>>> Please see comments to previous source comments.
>>>> <snip>
>>>>
>>>>>>> Thanks.  If you make this change for v2, I can make the matching change
>>>>>>> in fwupd so that if it notices current_value permissions like this that
>>>>>>> it shows read only there too.
>>>>>>
>>>>>> Submitted the recommended changes for review in v2
>>>>>>
>>>>
>>>> Thanks, looks good.
>>>>
>>>>>> Submitted a patch to improve the friendly display name for
>>>>>> few numbers of attributes associated with ‘Schedule Power-ON.’  BIOS
>>>>>> assign names such ‘Tuesday’ to an attribute. The name is correct, but
>>>>>> it is not descriptive enough for the user.  Under those
>>>>>> conditions a portion of the path data value is appended to the attribute
>>>>>> name to create a user-friendly display name.
>>>>>>
>>>>>> For instance, the attribute name is ‘Tuesday,’ and the display name
>>>>>> value is ‘Schedule Power-ON – Tuesday’
>>>>
>>>> Looks good
>>>>
>>>>>>>>>
>>>>>>>>> Presumably if this is going into it's own directory you should move all
>>>>>>>>> platform-x86 HP drivers to this directory earlier in the series too.
>>>>>
>>>>> The other drivers named HP-WMI and HP_ACCEL  were written by third
>>>>> party members and not by HP.   It is for this reason and because of
>>>>> the number of files, only hp-bioscfg was placed in a separate
>>>>> directory.   Let me know If my reasoning is not valid enough  and I
>>>>> will keep the files in a separate directory and move the selection to
>>>>> the main list.    In addition, Moving  HP-WMI and HP_ACCEL drivers
>>>>> from x86 directories fall outside of the scope of these changes,
>>>>> Correct?
>>>>>
>>>>
>>>> There is no distinction who writes a driver.  I think either you keep this driver in the root of drivers/platform/x86 or you put all the HP drivers in drivers/platform/x86/hp.
>>>>
>>>> I think if you're going to put this driver in the sub-directory "hp", then the first patch in this series should be to move those drivers to that sub-directory.  The second patch should be to introduce your new driver.
>>>
>>> I see this driver has a lot of separate files, so what should happen here IMHO is:
>>>
>>> 1. a preparation patch adding a hp subdir moving the existing hp drivers there
>>
>> This will be a separate patch but not an obstacle to gain approval of
>> hp-bioscfg driver, correct?
> 
> Right, this is just shuffling things around a bit because as we get
> more and more drivers having them all in one dir becomes a bit
> unwieldly.

The AMD drivers just underwent this very recently too, as did Intel 
about 6 months before.  If you want an example of what they did you can 
use this to reference the AMD one:

https://github.com/torvalds/linux/commit/ef233eafe5adc54ddc39a1b6cc483dddc744bf97

> 
>>
>>> 2. but this driver in a subdir of the hp subdir, so put all its files under:
>>>
>>> drivers/platform/x86/hp/hp-bioscfg
>>>
>>> so as to keep the files together and separate from other hp drivers.
>>
>> Can you please clarify..
>>
>> Do I need to start a new review with only two patches described earlier?
>>
>> 1. a preparation patch adding a hp subdir moving the existing hp drivers there
>> 2. Squash (current version v1 and v2 changes) into one
> 
> Ah, no, splitting step 2 as you did into multiple patches is fine,
> reviewing multiple small patches typically also is easier. So keeping
> the addition of the hp-bioscfg split into multiple patches is good.

The split right now is along changes requested on v1 -> v2 rather than 
along making the driver into functional pieces.

> 
> The point which I was trying to make is to put all the files
> for hp-bioscfg in their own sub-sub-dir and not mix them with
> the other driver files in a single hp dir.
> 
> Regards,
> 
> Hans
>