Messages in this thread | | | Date | Thu, 13 Oct 2022 15:22:19 +0200 | Subject | Re: [PATCH v6 6/6] arm64: defconfig: Add tps65219 as modules | From | jerome Neanne <> |
| |
On 13/10/2022 14:32, Krzysztof Kozlowski wrote: > On 12/10/2022 13:56, Kevin Hilman wrote: >> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: >> >>> On 12/10/2022 04:39, jerome Neanne wrote: >>>>> You explained what you did, which is easily visible. You did not explain >>>>> why you are doing it. >>>>> >>>>> Best regards, >>>>> Krzysztof >>>>> >>>> Thanks for pointing me to the detailed guidelines >>>> I'm new to upstream and not well aware of all good practices. >>>> >>>> Would below commit message be more suitable: >>>> >>>> Add support for the TPS65219 PMIC by enabling MFD, regulator and >>>> power-button drivers. All drivers enabled as modules. >>> >>> This still says only what you did. I still does not explain why. >> >> Jerome, maybe adding a bit of preamble like: >> >> "Development boards from TI include the TPS65219 PMIC. Add support..." > > I would propose: "Development boards from TI with xxx SoC include the > ..." because the point is that you use this defconfig for boards for > given SoC (supported by upstream). > > Other way would be "Foo-bar development board includes the TP..." > >> >> Krzysztof, I'm the first to argue for descriptive/verbose changelogs, >> but IMO, this is getting a little bit nit-picky. >> >> The series adds a new driver, DTS and defconfig patches to enable >> support the new driver. The "why" for changes to defconfig changes like >> this are kind of implied/obvious, and there is lots of precedent for >> changelogs of defconfig changes for simple drivers to simply say "enable >> X and Y". > > While I understand the entire patchset, the defconfig goes via separate > tree/branch and must stand on its own. Later (one month, one year, one > decade) someone will look at history and wonder why the heck we enabled > TPS65219. > >> >> If my above suggesion is not enough, please make a suggestion for what >> you think would qualify as an appropritate changelong that answers "why" >> for a simple driver change. > > It is enough :) > > Best regards, > Krzysztof > Got it! I'll rephrase following your suggestion
| |