Re: [PATCH 1/2] hwmon: (isl28022) new driver for ISL28022 power monitor

["Carsten Spieß" <mail@carsten-spiess.de>]

[-- Attachment #1: Type: text/plain, Size: 3275 bytes --]


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 #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]