On 7/27/23 16:30, Gueter Roeck wrote:
> >> On 7/26/23 08:22, Carsten Spieß wrote:
> >> At _compile-time_ ?
> > How to explain better that it isn't set at runtime?
> I would suggest "can be set with device properties".
Yeah, i like that.
> >> I'd argue that shunt voltage is all but useless, but if you want to have it supported
> >> it _has_ to be in mV.
> > That's a problem.
> >
> > In my case the ER-6P has a 8 milli Ohm (or 8000 micro Ohm) shunt and a powersupply with
> > max. 2.5 A. This gives a max shunt voltage of 20 mV at 2.5 A.
> > The device normaly consumes between 200 and 500 mA. (typ ~250 mA).
> > This results in shunt voltage of 1.6 to 4.0 mV (typ ~2mV).
> > Having no fractions will make it useless.
> >
> > Unfortunately there is no possibility to give a scaling factor.
> > Or returning float values (i know, this can't and shouldn't be changed)
> >
>
> Just like the ABI must not be changed. The sensors command would display your
> 4mV shunt voltage as 4V, which is just as useless.
>
> In practice, the shunt voltage _is_ useless for hardware monitoring purpose
> because it can be calculated from current and shunt resistor value.
> I'd say if you really want it, provide it as debugfs attribute. As hwmon
> attribute it has to be in mV.
O.k. will move to debugfs.
> >> I don't think the sign extensions are correct based on the datasheet.
> >> This will have to use sign_extend.
> > From my understading (see table 11 on page 16 of the ISL28022 datasheet)
> > shunt value is already sign extended, (D15-D12 is sign)
> > bus value (table 12 on page 16) is unsigned.
> >
>
> Not really. For the shunt voltage, 0xf000 has different meanings depending on scale
> and range settings.
Sorry, i don't agree, 0xf000 is -40.96 mV on all scale settings.
> LSB for bus voltage is 4 mV and starts at bit 2 or 3 depending
> on BRNG. The above just happens to be correct if BRNG = 10 OR 11 per datasheet.
> If that is intentional, it needs to get a comment.
Yes, will add comment.
> >> Getting an error message each time the "sensors" command is executed ?
> >> Unacceptable.
> > o.k., will change to set *val = 0;
> >
> Still unacceptable.
O.k. i will limit shunt-resistor-milli-ohms to a minimal value > 0 and drop check here.
> >>> + if (!dev || !data)
> >>> + return -EINVAL;
> >>
> >> How would this ever happen ?
> > Shouldn't, but i'm carefully (i had it once during development due to an error
> > (using dev instead of hwmon_dev) on calling this function
> >
>
> Parameter checks are only acceptable on API functions. This is not an API function.
> Local functions are expected to be consistent. If this function is called with
> a bad argument, that needs to be fixed during development.
O.k., removed.
> >>> +static struct i2c_driver isl28022_driver = {
> >>> + .class = I2C_CLASS_HWMON,
> >>> + .driver = {
> >>> + .name = "isl28022",
> >>> + .of_match_table = of_match_ptr(isl28022_of_match),
> >>
> >> Drop of_match_ptr()
> > Most drivers have this, why drop?
> >
>
> It is needed for device_property_read_u32() to work.
O.k. dropped, i wasn't familiar with device_property_read functions.
Regards Carsten
Attachment:
pgpWxBm1gGVgr.pgp
Description: Digitale Signatur von OpenPGP
