Message-ID: <a174f532-0595-30b9-b9af-04e67bb9be20@roeck-us.net>
Date:   Sat, 29 Feb 2020 08:30:05 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Grant Peltier <grantpeltier93@...il.com>
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
        openbmc@...ts.ozlabs.org, adam.vaughn.xh@...esas.com,
        zaitsev@...gle.com
Subject: Re: [PATCH] hwmon: (pmbus) Add support for 2nd Gen Renesas digital
 multiphase

On 2/29/20 7:48 AM, Grant Peltier wrote:
> On Fri, Feb 28, 2020 at 05:55:44PM -0800, Guenter Roeck wrote:
>> On 2/28/20 3:52 PM, Grant Peltier wrote:
>>> Hi Guenter,
>>>
>>> Thank you for your expedient review. I will need to consult with my
>>> coworkers to determine a more appropriate driver name. In the meantime I
>>> will make the desired changes and I will also create a document for the
>>> driver, which I will submit as a linked but separate patch.
>>>
>>> With regard to the part numbers, this family of parts is currently in
>>> the process of being released and we have not yet published all of the
>>> corresponding datasheets. However, I have been assured that all of the
>>> parts listed are slated to have a datasheet published publicly in the near
>>> future.
>>>
>> That would be great.
>>
>> As for the driver name, I had a look into drivers/hwmon/pmbus/isl68137.c,
>> and I don't immediately see why the new chips would warrant a new driver.
>> The only differences seem to be that VMON is a new command, and of course
>> only the ISL68137 supports AVL. But then there is, for example, ISL68127,
>> which is again quite similar. The only other difference as far as I can
>> see is input voltage scaling, but that doesn't warrant a separate driver
>> (and, of course, I have no means to validate if input voltage scaling
>> is indeed different for all the new chips).
>>
>> Overall I would suggest to extend the isl68137 driver. I would also
>> suggest to not add separate tables for each of the rail configurations
>> but use the three-phase entry as starting point, copy it, and adjust its
>> values as needed.
>>
>> For the multi-phase chips, I question if reporting the input voltage
>> for each phase make sense. Is it really a different voltage ? For IIN
>> and PIN, the question is if the registers are indeed paged, since they
>> are not paged in the older chips.
>>
>> Guenter
> 
> The ISL68137 is part of the first generation of our digital multiphase
> parts which are all exclusively 2-rail (2-page) devices. There are a
> couple of reasons that we are opting for a new driver for the new
> generation of devices:
> 
> 1) Gen 2 has multiple rail configurations (1, 2, or 3) with different scaling
> parameters than Gen 1

That would only mean a single additional entry for the Gen1 devices.
This is not a valid argument for a separate driver, especially since
the difference in scaling only affects a single parameter as far as I
can see.

> 2) We are planning to support some of the non-generic PMBus functions of
> the Gen 2 devices using the debugfs interface.
> 
This is not a valid argument either. We have, for example, a single driver
for all Linear chips, even though they have quite some difference across
individual chips. The above can simply be solved by not instantiating
debugfs for Gen1 devices, and marking VMON as supported for Gen2 devices
only. If the Gen1 devices are all pretty much the same, you'd only need
a single additional table entry for those in the driver (if you insist
using a table).

For the debugfs functions, please keep in mind that those will have to be
documented in a way that lets people without access to datasheets
understand what they are for. We can't have cryptic debugfs functions
which, if misused, blow up the chip.

> I am currently working on point 2 and those features are not
> quite ready to be included in a patch set but we wanted to move forward
> with the hwmon functionality for now as that is useful on it's own.
> 
> Fair point on the global vs paged commands. I will modify the page
> functions so that global commands are only read from page 0.
> 

Also a fair point showing that not having the datasheet hurts. I see the same
problem with recent TI devices. In some cases I _know_ that a driver is buggy
or much less than perfect, because I have access to a datasheet through
my employer, but I can't talk about it because what I know is under NDA.
Then I end up spending a lot of time trying to find leaks that let me comment.
I am _not_ happy about this situation, not at all.

Please understand that I won't accept a driver adding support for a chip
if there is no public information available that the chip exists in the
first place. Imagine a situation where you are requesting to add support
for a chip, and that chip isn't mentioned anywhere, not even in a datasheet
I (hypothetically) may have access to through my employer. How would _you_
handle such a situation if you were in my place ?

You could of course send me all the datasheets under NDA to my work e-mail,
but that wouldn't really be much better since I still would not be able to
comment in public. On the other side, I'd have at least some confirmation
that those magic chips do indeed exist, so you might possibly want to
consider that.

Thanks,
Guenter

Powered by blists - more mailing lists