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
