Hi Hans, On Mon, Oct 17, 2022 at 9:37 AM Hans de Goede <hdegoede@xxxxxxxxxx> 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? > 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 > > Note other then just skimming the comments I have not looked at this driver > at yet I will try to make time for this soon. > > Mario, thank you for your review work on this. > > Regards, > > Hans > > >
