Re: PMBus driver for FSP/3Y Power device with non-standard VOUT values (LINEAR11 vs LINEAR16)

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Mar 14, 2019 at 04:08:32PM +0000, Grönke, Christian wrote:
> Hello Guenter,
> 
> > -----Ursprüngliche Nachricht-----
> > Von: Guenter Roeck <groeck7@gmail.com> Im Auftrag von Guenter Roeck
> > Gesendet: Donnerstag, 14. März 2019 04:19
> > An: Grönke, Christian <C.Groenke@infodas.de>
> > Cc: linux-hwmon@vger.kernel.org
> > Betreff: Re: PMBus driver for FSP/3Y Power device with non-standard VOUT
> > values (LINEAR11 vs LINEAR16)
> > 
> > Hi Christian,
> > 
> > On Wed, Mar 13, 2019 at 05:35:38PM +0000, Grönke, Christian wrote:
> > [ ... ]
> > > >
> > > > Our last e-mails overlapped; can you explore what it would take to
> > > > add support for ieee754 half precision floating point ?  We would
> > > > need a new set of conversion functions in the core (ie
> > > > pmbus_reg2data_ieee754 and pmbus_data2reg_ieee754), and your driver
> > > > would have to implement the
> > > > linear11 <-> ieee754 conversion. That may be a bit more work, but it
> > > > would also be cleaner, and it should not affect precision.
> > >
> > > I give it a look tomorrow. If this presents a proper and clean way, to
> > > avoid adding the hack in the generic code it might be worth it.
> > >
> > 
> > Please use the attached patch as starting point. Obviously I could not
> > do much testing, but unit testing returned reasonable results. Note that
> > we don't have to follow the pmbus spec to the letter - it is sufficient
> > to declare the VOUT values to be in ieee754 format for your driver.
> 
> Thanks a lot for the patch. It did indeed help.
> 
> The framework code seemed to work fine. I used your code for the conversion:
>     linear11 -> 'scaled integer' -> ieee754
> It provided a way to test the code and was easy for me as my tries to do
> some other bit magic weren't successful. That means I partly tested the code
> from pmbus_data2reg_ieee754 as my read_word function uses this for the
> conversion. Of course not the module local function...
> 
> I have question wrt to the code:
> [...]
> > +static u16 pmbus_data2reg_ieee754(struct pmbus_data *data,
> > +				  struct pmbus_sensor *sensor, long val) {
> [...]
> > +
> > +	/* Ensure that resulting number is within range */
> > +	if (mantissa > 0x7ff)
> > +		mantissa = 0x7ff;
> > +	else if (mantissa < 0x400)
> > +		mantissa = 0x400;
> 
> Setting the mantissa to 0x400...
> 
> > +	/* Convert to sign, 5 bit exponent, 10 bit mantissa */
> > +	return sign | (mantissa & 0x3ff) | ((exponent << 10) & 0x7c00); }
> 
> ... would mean it is cut to 0 here. I am not sure this is the intention.
> I guess it should be 0x3FF then. Or in case it is supposed to be 0, then
> 'mantissa = 0;' would be easier to understand.
> 

It is supposed to end up being 0 at the end. The valid range is
0x400..0x7ff, where bit 10 represents the '1' in the normalized number.
Bit 10 has to be removed when applying the value to the final ieee754
representation. I thought that was easier to understand than setting
the values to 0x3ff and 0x0 (which would look even more confusing
to the casual reader). I think I'll add a comment at the top
explaining the logic.

Thanks,
Guenter