Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip

From: Guenter Roeck <linux@roeck-us.net>
To: "Nuno Sá" <noname.nuno@gmail.com>,
	"Andy Shevchenko" <andy@kernel.org>,
	"Nuno Sa" <nuno.sa@analog.com>
Cc: linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org,
	devicetree@vger.kernel.org, Bartosz Golaszewski <brgl@bgdev.pl>,
	Jonathan Corbet <corbet@lwn.net>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Jean Delvare <jdelvare@suse.com>,
	Conor Dooley <conor+dt@kernel.org>
Subject: Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip
Date: Mon, 20 Nov 2023 07:00:52 -0800	[thread overview]
Message-ID: <e3cf1182-47c6-4b2d-b5ec-1eee07974b51@roeck-us.net> (raw)
In-Reply-To: <581aec9c6313e3885aae8b1e12dfcc9f392716db.camel@gmail.com>

On 11/13/23 02:13, Nuno Sá wrote:
> On Fri, 2023-11-10 at 18:50 +0200, Andy Shevchenko wrote:
>> On Fri, Nov 10, 2023 at 04:18:46PM +0100, Nuno Sa wrote:
>>> The LTC4282 hot swap controller allows a board to be safely inserted and
>>> removed from a live backplane. Using one or more external N-channel pass
>>> transistors, board supply voltage and inrush current are ramped up at an
>>> adjustable rate. An I2C interface and onboard ADC allows for monitoring
>>> of board current, voltage, power, energy and fault status.
>>
>> There are plenty of ltc42xx in the same folder and a lot of other chips.
>> Have you checked that none of the existing driver is not close enough and
>> brand new driver is indeed required? If so, add a respective paragraph
>> somewhere (cover letter? TBH I haven't read it, it's fine if it's already
>> explained there).
>>
> 
> I can double check but not the last time I looked into those drivers

They are all different.

>>
>>> +	/*
>>> +	 * AVG = E / (Tconv * counter)
>>> +	 * We get energy in microJoule, hence dividing it by microSeconds
>>> gives Watts. Therefore,
>>> +	 * multiplying by MICRO gives us microWatts.
>>> +	 */
>>
>> Here you used long lines, somewhere else, much shorter. Please, choose one
>> limit and be consistent with it.
>>
> 
> Yeah, maybe not too consistent. I'm still not sure what's the convention in
> hwmon (I mention it in the cover).
> 

Patches should pass "checkpatch --strict". Other than that, I'd expect consistency
and common sense, but that isn't exactly spelled out anywhere and would be
difficult to define. Personally, I would suggest something like "use limit of 80
columns for comments and otherwise try to stick with 80 columns unless it results
in odd line breaks. In that case, feel free to go up to 100 columns". Do I need to
spell that out in Documentation/hwmon/submitting-patches.rst ?

Not that anyone seems to read that document :-(. Sorry, frustrated maintainer
speaking.

Guenter