Re: [PATCH 10/10] arm64: dts: imx8mp-skov: increase I2C clock frequency for RTC

[Frank Li <Frank.li@nxp.com>]

On Fri, Dec 20, 2024 at 10:12:49AM +0100, Ahmad Fatoum wrote:
> Hello Frank,
>
> On 19.12.24 20:16, Frank Li wrote:
> > On Thu, Dec 19, 2024 at 06:51:35PM +0100, Ahmad Fatoum wrote:
> >> On 19.12.24 18:47, Frank Li wrote:
> >>> On Thu, Dec 19, 2024 at 08:25:34AM +0100, Ahmad Fatoum wrote:
> >>>> From: Oleksij Rempel <o.rempel@pengutronix.de>
> >>>>
> >>>> The NXP PCF85063TP RTC we use is capable of up to 400 kHz of SCL clock
> >>>> frequency, so let's make use of this instead of the 100 kHz bus frequency
> >>>> we are currently using.
> >>>
> >>> Increase I2C frequency to 400khz from 100kHz because NXP PCF85063TP RTC
> >>> support it.
> >>
> >> Unlike your other suggestions, these is no information lost by rewriting
> >> the commit message as you suggest. I don't mind, but must admit it feels
> >> like bikeshedding. What is your concrete objection to my commit message?
> >
> > According to
> > https://docs.kernel.org/process/submitting-patches.html
> >
> > Describe your changes in imperative mood, e.g. “make xyzzy do frotz”
> > instead of “[This patch] makes xyzzy do frotz” or
> > “[I] changed xyzzy to do frotz”, as if you are giving orders to the
> > codebase to change its behaviour.
>
> I have to disagree with your interpretation. The imperative mood is primarily
> expected for commit message titles, but if you take a look at normal commit
> message _bodies_ that make it into the kernel, you'll find that a whole lot
> of them start off like I do: Describe the situation and then what's done about
> it.

I was got many similar feedback from difference maintainer to be required
change my commit message _bodies_ in past years.

https://lore.kernel.org/imx/3c9fe85a-5f86-4df6-92fb-e4ceb033f161@kernel.org/
https://lore.kernel.org/imx/117dd6f3-8829-4000-a05b-6cb421ca7de6@kernel.org/
https://lore.kernel.org/linux-pci/20240807025423.GF3412@thinkpad/
https://lore.kernel.org/linux-pci/YnvyrSTAxJmGxful@lpieralisi/

At beginning, I can't understand this. But after follow these expertor
suggestion, I found some beanfit.
- sentense will be shorter and easy to capture most important part. for
example:

  "The NXP PCF85063TP RTC we use is capable of up to 400 kHz of SCL clock
  frequency, so let's make use of this instead of the 100 kHz bus frequency
  we are currently using."

  34 words

  "Increase I2C frequency to 400khz from 100kHz because NXP PCF85063TP RTC
   support it"

  13 words

  Linux is big community, even reduce 1 minutes to read it, multi by
totall reviewer/reader will be very big numbers.

- empahse the important change first to help understand whole patch
quickly.

You can choose what you want if maintainer don't reject it. Generally I
just send such kind comments for v1 patch, because it is less possible to
be accepted by by maintainer to accept (except some critial fixes).

>
> I personally find that this is often easier to follow than just having two
> imperatives back-to-back.

Basic principle is clear, simple, and straightforward.

Frank

>
> I have just Cc'd you on an RFC patch to amend the documentation you linked
> to reflect this. I am happy to continue the discussion over there:
>
> https://lore.kernel.org/workflows/20241220-submitting-patches-imperative-v1-0-ee874c1859b3@pengutronix.de/T/#t
>
> Thanks,
> Ahmad
>
> >
> > Generally, avoid use
> >
> > "this patch ..."
> > "let's ...."
> > "we do ... for ... "
> >
> > Just simple said
> >
> > Do ... to ...
> > Do ... because ...
> >
> > Frank
> >
> >>
> >> Thanks,
> >> Ahmad
> >>
> >>
> >>>
> >>> Frank
> >>>
> >>>>
> >>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> >>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >>>> ---
> >>>>  arch/arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi
> >>>> index a683f46fcbab..ec7857db7bf0 100644
> >>>> --- a/arch/arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi
> >>>> +++ b/arch/arm64/boot/dts/freescale/imx8mp-skov-reva.dtsi
> >>>> @@ -333,7 +333,7 @@ reg_nvcc_sd2: LDO5 {
> >>>>  };
> >>>>
> >>>>  &i2c3 {
> >>>> -	clock-frequency = <100000>;
> >>>> +	clock-frequency = <400000>;
> >>>>  	pinctrl-names = "default";
> >>>>  	pinctrl-0 = <&pinctrl_i2c3>;
> >>>>  	status = "okay";
> >>>>
> >>>> --
> >>>> 2.39.5
> >>>>
> >>>
> >>
> >>
> >> --
> >> Pengutronix e.K.                           |                             |
> >> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> >
>
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |