| Message ID | 20221011140549.16761-7-jneanne@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for TI TPS65219 PMIC. | expand |
On 11/10/2022 10:05, Jerome Neanne wrote: > This adds defconfig option to support TPS65219 PMIC, MFD, Regulators Do not use "This commit/patch". https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > and power-button. You explained what you did, which is easily visible. You did not explain why you are doing it. Best regards, Krzysztof
On 11/10/2022 16:48, Krzysztof Kozlowski wrote: > On 11/10/2022 10:05, Jerome Neanne wrote: >> This adds defconfig option to support TPS65219 PMIC, MFD, Regulators > > Do not use "This commit/patch". > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > >> and power-button. > > 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. Best regards Jerome
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. Best regards, Krzysztof
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..." 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". 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. Kevin
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
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
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index d5b2d2dd4904..d64e00355fcd 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -406,6 +406,7 @@ CONFIG_TOUCHSCREEN_GOODIX=m CONFIG_TOUCHSCREEN_EDT_FT5X06=m CONFIG_INPUT_MISC=y CONFIG_INPUT_PM8941_PWRKEY=y +CONFIG_INPUT_TPS65219_PWRBUTTON=m CONFIG_INPUT_PM8XXX_VIBRATOR=m CONFIG_INPUT_PWM_BEEPER=m CONFIG_INPUT_PWM_VIBRA=m @@ -639,6 +640,7 @@ CONFIG_MFD_SPMI_PMIC=y CONFIG_MFD_RK808=y CONFIG_MFD_SEC_CORE=y CONFIG_MFD_SL28CPLD=y +CONFIG_MFD_TPS65219=m CONFIG_MFD_ROHM_BD718XX=y CONFIG_MFD_WCD934X=m CONFIG_REGULATOR_FIXED_VOLTAGE=y @@ -666,6 +668,7 @@ CONFIG_REGULATOR_QCOM_SPMI=y CONFIG_REGULATOR_RK808=y CONFIG_REGULATOR_S2MPS11=y CONFIG_REGULATOR_TPS65132=m +CONFIG_REGULATOR_TPS65219=m CONFIG_REGULATOR_VCTRL=m CONFIG_RC_CORE=m CONFIG_RC_DECODERS=y