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

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

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.

> 
> The build process was tested with the latest  drivers/platform/x86
> from branch for-next.
> Nonetheless, I will investigate.

I did my test on 6.0 rather than for-next.  But given it's a header 
issue I suspect you have a miss that works with the compiler I'm using.

I was using gcc 11.2.0 on Ubuntu 22.04.

<snip>