Re: Commit messages (was: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y)

[Ban Feng]

Hi Paul and Guenter,

Appreciate your valuable comments, and I'll add some commit messages
to describe this chip for pwm and fan.

Thanks,
Ban

On Thu, Feb 29, 2024 at 12:25 AM Paul Menzel <pmenzel@xxxxxxxxxxxxx> wrote:
>
> Dear Guenter,
>
>
> Thank you for your reply.
>
> Am 28.02.24 um 17:03 schrieb Guenter Roeck:
> > On 2/28/24 03:03, Paul Menzel wrote:
>
> >> Am 28.02.24 um 10:03 schrieb Guenter Roeck:
> >>> On 2/27/24 23:57, Paul Menzel wrote:
> >>
> >>>> Am 27.02.24 um 01:56 schrieb baneric926@xxxxxxxxx:
> >>>>> From: Ban Feng <kcfeng0@xxxxxxxxxxx>
> >>>>>
> >>>>> NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
> >>>>
> >>>> Please reference the datasheet.
> >>>
> >>> Note that something like
> >>>
> >>> Datasheet: Available from Nuvoton upon request
> >>>
> >>> is quite common for hardware monitoring chips and acceptable.
> >>
> >> Yes, it would be nice to document it though. (And finally for vendors
> >> to just make them available for download.)
> >
> > Nuvoton is nice enough and commonly makes datasheets available on request.
> > The only exception I have seen so far is where they were forced into an NDA
> > by a large chip and board vendor, which prevented them from publishing a
> > specific datasheet.
>
> Nice, that they are better in this regard than others.
>
> > Others are much worse. Many PMIC vendors don't publish their datasheets at
> > all, and sometimes chips don't even officially exist (notorious for chips
> > intended for the automotive market). Just look at the whole discussion
> > around MAX31335.
> >
> > Anyway, there are lots of examples in Documentation/hwmon/. I don't see
> > the need to add further documentation, and I specifically don't want to
> > make it official that "Datasheet not public" is acceptable as well.
> > We really don't have a choice unless we want to exclude a whole class
> > of chips from the kernel, but that doesn't make it better.
>
> I know folks figure it out eventually, but I found it helpful to have
> the datesheet name in the commit message to know what to search for, ask
> for, or in case of difference between datasheet revision what to compare
> against.
>
> >>>> Could you please give a high level description of the driver design?
> >>>
> >>> Can you be more specific ? I didn't have time yet to look into details,
> >>> but at first glance this looks like a standard hardware monitoring
> >>> driver.
> >>> One could argue that the high level design of such drivers is described
> >>> in Documentation/hwmon/hwmon-kernel-api.rst.
> >>>
> >>> I don't usually ask for a additional design information for hwmon drivers
> >>> unless some chip interaction is unusual and needs to be explained,
> >>> and then I prefer to have it explained in the code. Given that, I am
> >>> quite curious and would like to understand what you are looking for.
> >> For a 10+ lines commit, in my opinion the commit message should say
> >> something about the implementation. Even it is just, as you wrote, a
> >> note, that it follows the standard design.
> >
> > Again, I have not looked into the submission, but usually we ask for that
> > to be documented in Documentation/hwmon/. I find that much better than
> > a soon-to-be-forgotten commit message. I don't mind something like
> > "The NCT7363Y is a fan controller with up to 16 independent fan input
> >   monitors and up to 16 independent PWM outputs. It also supports up
> >   to 16 GPIO pins"
> > or in other words a description of the chip, not the implementation.
> > That a driver hwmon driver uses the hardware monitoring API seems to be
> > obvious to me, so I don't see the value of adding it to the commit
> > description. I would not mind having something there, but I don't
> > see it as mandatory.
> >
> > On the  other side, granted, that is just _my_ personal opinion.
> > Do we have a common guideline for what exactly should be in commit
> > descriptions for driver submissions ? I guess I should look that up.
>
> `Documentation/hwmon/submitting-patches.rst` refers to
> `Documentation/process/submitting-patches.rst`, and there *Describe your
> changes* seems to have been written for documenting bug fixes or
> enhancements and not new additions. It for example contains:
>
> > Once the problem is established, describe what you are actually doing
> > about it in technical detail.  It's important to describe the change
> > in plain English for the reviewer to verify that the code is behaving
> > as you intend it to.
>
> I agree with your description, but I am also convinced if you write 500
> lines of code, that you can write ten lines of commit messages giving a
> broad overview. In this case, saying that it follows the standard driver
> model would be good enough for me.
>
> Also, at least for me, often having to bisect stuff and using `git
> blame` to look at old commits, commit messages are very valuable to me,
> and not “forgotten”. ;-)
>
>
> Kind regards,
>
> Paul