Re: [PATCH v2 3/3] hwmon: ltc2990: support all measurement modes

[Guenter Roeck]

On Wed, Jun 28, 2017 at 07:33:33PM +0200, Mike Looijmans wrote:
> ïOn 28-06-17 19:02, Tom Levens wrote:
> >
> >On Wed, 28 Jun 2017, Guenter Roeck wrote:
> >
> >>On Wed, Jun 28, 2017 at 05:29:38PM +0200, Tom Levens wrote:
> >>>
> >>[ ... ]
> >>
> >>>>
> >>>>>Whatever happened to this patch though? It didn't make it to mainline,
> >>>>>otherwise I'd have found it sooner...
> >>>>>
> >>>>I'll have to look it up, but I guess I didn't get an updated version.
> >>>
> >>>As far as I remember I had a working V3 of this patch, but it is
> >>>entirely
> >>>possible that it was never submitted as I have been busy with other
> >>>projects
> >>>recently. I'll dig it out and check that it is complete.
> >>>
> >>FWIW, I don't see it at
> >>https://patchwork.kernel.org/project/linux-hwmon/list/?submitter=171225&state=*
> >>
> >>
> >>Maybe you were waiting for a reply from Rob. Either case, it might make
> >>sense to only provide valid modes, ie to abstract the mode bits from the
> >>hardware, such as
> >>
> >>0 - internal temp only
> >>1 - Tr1
> >>2 - V1
> >>3 - V1-V2
> >>4 - Tr2
> >>5 - V3
> >>6 - V3-V4
> >>7 to 14 - per bit 0..2
> >>
> >>Guenter
> >>
> >
> >You are right, there was still an open question about how best to handle
> >the mode selection in DT.
> >
> >In the latest version of my patch I have it implemented as an array for
> >setting the two values, for example:
> >
> >     lltc,meas-mode = <7 3>;
> >
> >This sets bits [2..0] = 7 and [4..3] = 3. Of course these could be split
> >into two DT properties, but I was unsure what to name them as both fields
> >are called "mode" in the datasheet and "mode-43"/"mode-20" (or similar) is
> >ugly.
> >
> >With regards to your proposal, it is not clear to me whether the modes
> >which have the same result are exactly equivalent. Does disabling a
> >measurement with the mode[4..3] bits really leaves the part in a safe
> >state for all possible HW connections? With this doubt in my head, I would
> >prefer to keep the option available to the user to select any specific
> >mode. But I am open to suggestions.
> 
> Well, the input restrictions always apply, so disabling V3 measurement
> doesn't imply that you can apply 20V to that input safely now.
> 
> I'd suggest to set unused input to plain voltage measurement. That is
> "passive" and safe for external components.
> 
> So I'd suggest just setting the mode as per device datasheet, I can see no
> real advantage in abstracting it away and forcing users to read yet another
> document to get it right, e.g.:
> 
> lltc,mode = <6>;
> 
> As for the input disabling, since I doubt anyone would use it (why purchase
> a 4-channel device and use only 2), just add two booleans, e.g.
> "disable-inputs-34" and "disable-inputs-12" which set the command bits
> appropriately, and change the mode such that the disabled inputs are voltage
> readout only.
> 
> A case could even be made for changing mode at runtime. This allows using it
> to measure both current and voltage on two inputs, by reading V1, and V3,
> and then switch mode to obtain (accurate) V1-V2 and V4-V3.
> 
The hwmon subsystem doesn't really currently support that, and you'll likely
confuse userspace as well.

> That might be a viable way to handle not setting the mode at all. If the
> mode can be selected via sysfs, the driver can keep the device in a "safe"
> mode until the mode has been selected.
> 

You'll have to present me with a really good use case for me to agree with that
approach.

Guenter

> 
> >Mike, if you would like to test it, the latest version of my code is here:
> >
> >https://github.com/levens/ltc2990/blob/dev/drivers/hwmon/ltc2990.c
> 
> Sure, I even have a board with 2 of these devices now :)
> 
> -- 
> Mike Looijmans
> 
> 
> Kind regards,
> 
> Mike Looijmans
> System Expert
> 
> TOPIC Products
> Materiaalweg 4, NL-5681 RJ Best
> Postbus 440, NL-5680 AK Best
> Telefoon: +31 (0) 499 33 69 79
> E-mail: mike.looijmans@xxxxxxxxxxxxxxxxx
> Website: www.topicproducts.com
> 
> Please consider the environment before printing this e-mail
> 
> 
>