[0/2] Add support for LTC4282

Hi all,

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.

I'm aware that there are ABI in the driver that will surely raise questions.
So, I'll try to add some comments about those:

- For the fault_log stuff please see the comment in the code. There might be
some scenarios where one might really want to latch off the device until a
fault is manually cleared.
- I also see value in the FET interfaces as they are real faults but maybe
the naming is poor.
- I'm not so sure about the power1_good and the power1_fault_log. The
power1_good is more of a real status bit. If the bit is 0, it does not
necessarily means that there's something wrong. If someone removed
(on purpose) the "load", then this will be 0 and there's nothing wrong.
The fault_log is also not one of those bits that will keep the device to
latch on again. However, they might really indicate some misbehave. But,
OTOH (again :)), maybe the GPIO support for this is enough...
- There's also the handling for the overflow bits. I don't think it makes
much sense to export those so I tried to be clever and automatically handle
it the driver. The power_average is the thing making the whole thing more
complicated. If it was only the energy, we could defer it completely to
userspace...
- And there's also the rsense as a mandatory property. Designs like this
completely depend on the calculated rsense so I have no idea (and if it
makes sense) what default should I use if the property is not given.

I'm also cc'ing the GPIO folks for the GPIO bits. And I'm also not so sure
about it. I'm just treating the pins as if I can set value + direction. However,
the only thing that we can do is to PULL_LOW and set the pins in HIGH_Z. So,
I dunno I'm doing the right thing. I wonder if I should just give the ability
to configure the pins through FW with the .set_config hook and then just
allow to read the pin level? The GPIO1 is also odd since is only the one
that directly allows you to control the direction but then, again, you
can just pull it low or high-z.

One last comment is about lines length. I know some maintainer still want
the 80 col limit but since I'm not so sure on the policy in hwmon I just
went for 100. I'm pretty sure I'll need more iterations to get the driver
in, so I'm happy to change it to 80 if required.

Nuno Sa (2):
  dt-bindings: hwmon: Add LTC4282 bindings
  hwmon: ltc4282: add support for the LTC4282 chip

 .../bindings/hwmon/adi,ltc4282.yaml           |  228 +++
 Documentation/hwmon/ltc4282.rst               |  101 ++
 MAINTAINERS                                   |    8 +
 drivers/hwmon/Kconfig                         |   11 +
 drivers/hwmon/Makefile                        |    1 +
 drivers/hwmon/ltc4282.c                       | 1518 +++++++++++++++++
 6 files changed, 1867 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adi,ltc4282.yaml
 create mode 100644 Documentation/hwmon/ltc4282.rst
 create mode 100644 drivers/hwmon/ltc4282.c

On Mon, Nov 13, 2023 at 10:32:17AM +0100, Nuno Sá wrote:
> On Fri, 2023-11-10 at 18:42 +0000, Conor Dooley wrote:
> > On Fri, Nov 10, 2023 at 04:18:45PM +0100, Nuno Sa wrote:

> > > +  adi,clkout-mode:
> > > +    description: |
> > > +      Controls in which mode the CLKOUT PIN should work:
> > > +        0 - Configures the CLKOUT pin to output the internal system clock
> > > +        1 - Configures the CLKOUT pin to output the internal conversion
> > > time
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    enum: [0, 1]
> > 
> > I really am not a fan of these types of properties. Part of me says that
> > if you're outputting clocks from this device, then you should be a clock
> > controller. How do consumers of this @clkout@ pin get the rate of the
> > clock?
> 
> I explained it to Guenter as he also argued about this. I'll wait for more
> feedback but it's likely this will just turn into a clock provider, yes.
> 
> > I'd kinda be expecting to see a clocks property with a maxItems of 1 and
> > clock-names with two, mutually exclusive, options.
> > 
> > The other part says, and it applies in multiple places here, that having
> > integer properties with non-integer meanings is a poor ABI. I'd be vastly
> > happier if the various instances in this file became enums of strings,
> > or $re┤evant-unit so that a dts containing these properties is
> > immediately understandable.
> 
> Well, I think you're mentioning the 'gpio-mode' 'and under/over-voltage-
> dividers'. I think for both it's clear that having the relevant units is not
> feasible (at least I'm not seeing a way of properly do it). As for the strings,
> well, I don't have any much to argue other than:

Yeah, sorry - I was kinda making a general point there about not liking
having integer values mapped to some sort of meaning, when units or some
other sort of more meaningful property is possible.
I really do not like these sorts of properties that you go and put
"gpio-mode = <3>;" or whatever in the devicetree.
I know its not quite units, but you could use 5, 10 & 15 as the
allowable values for the divider property and I think that'd be fine.

Oh and now that I think of it - for the divider property, how does
"adi,undervoltage-dividers = 0" differ from omitting the property
altogether? It's not entirely apparently from the binding what that
actually means. If they do differ, I think you need to mention what
the omission of the property means, and if they do not, then that = 0
case should be removed IMO.

> 1) It's pattern seen in a lot of bindings - yes, that's not an excuse to copy
> bad/wrong things over new bindings - but, honestly, it's the first time I have
> someone complaining about it so I never thought it was wrong.
> 
> 2) It makes much more easier to handle the properties in the driver (yeah, I
> know that, as far as you're concerned, this does not matter to you :))

Yeah, with one hat on, sure, I don't care. Realistically I am aware that
having these integers makes your life a little easier though.
> 
> So yeah, if you insist on it, no strong reasons on my side to not do it. As long
> as I see some consistency down the road :)).

From me at least, I try to push people away from these sorts of integer
properties and will continue to do so.

Cheers,
Conor.