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

[Carsten Spieß]

On 7/26/23 18:14, Gueter Roeck wrote:
> On 7/26/23 08:22, Carsten Spieß wrote:
> > +The shunt value in micro-ohms, shunt gain and averaging can be set
> > +via device tree at compile-time.  
> 
> At _compile-time_ ?
How to explain better that it isn't set at runtime?
Other drivers use the same term.

> 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)

> Why not support limit attributes ?
Due to limited time, left for later enhancement.


> > +#define ISL28022_REG_SHUNT	0x01
> > +#define ISL28022_REG_BUS	0x02


> > +	switch (type) {
> > +	case hwmon_in:
> > +		switch (attr) {
> > +		case hwmon_in_input:
> > +			err = regmap_read(data->regmap,
> > +					  ISL28022_REG_SHUNT + channel, &regval);  
> 
> That never reads REG_BUS.
Hmm, 
channel 0: ISL28022_REG_SHUNT + 0 == 0x01
channel 1: ISL28022_REG_SHUNT + 1 == 0x02 == ISL28022_REG_BUS
or do i miss something?

> > +			if (err < 0)
> > +				return err;
> > +			*val = (channel == 0) ?
> > +					(long)((s16)((u16)regval)) * 10 :
> > +					(long)(((u16)regval) & 0xFFFC);  
> 
> 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.

> > +			err = regmap_read(data->regmap,
> > +					  ISL28022_REG_CURRENT, &regval);
> > +			if (err < 0)
> > +				return err;
> > +			if (!data->shunt)
> > +				return -EINVAL;  
> 
> Getting an error message each time the "sensors" command is executed ?
> Unacceptable.
o.k., will change to set *val = 0;

> > +			err = regmap_read(data->regmap,
> > +					  ISL28022_REG_POWER, &regval);
> > +			if (err < 0)
> > +				return err;
> > +			if (!data->shunt)
> > +				return -EINVAL;  
> 
> Unacceptable.
o.k., as above

> > +			*val = ((long)regval * 409600000L * (long)data->gain) /
> > +				(long)(8 * data->shunt);  
> 
> I don't think this was checked for overflows.
Yes, i must first divide then multiply.
I have to think about how to keep accuracy on high shunt resistor values.

> > +static int isl28022_config(struct device *dev)
> > +{
> > +	struct isl28022_data *data = dev_get_drvdata(dev);
> > +
> > +	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
 
> > +	regmap_write(data->regmap, ISL28022_REG_CONFIG, data->config);
> > +	regmap_write(data->regmap, ISL28022_REG_CALIB, data->calib);  
> 
> Error checking needed.
o.k. will add.

> > +static int isl28022_probe(struct i2c_client *client)
> > +{

> > +	if (!i2c_check_functionality(client->adapter,
> > +				     I2C_FUNC_SMBUS_BYTE_DATA |
> > +				     I2C_FUNC_SMBUS_WORD_DATA))
> > +		return -EIO;  
> 
> This is not an IO error. Return -ENODEV as most other drivers do.
o.k.

> > +	of_property_read_u32(dev->of_node, "shunt-gain", &data->gain);
> > +	of_property_read_u32(dev->of_node, "average", &data->average);  
> 
> Check for errors and provide defaults if properties are not set.
o.k.

> Also please use device_property_read_u32() to enable use from non-devicetree
> systems.
o.k. Never used this, have to look for an example on using it.
Many (old) drivers are using the of_property_* functions, is it intended to replace it.
What about backporting this driver to e.g. 5.15, will it affect it?

> > +	status = isl28022_config(hwmon_dev);
> > +	if (status)
> > +		return status;  
> 
> That has to happen before the call to devm_hwmon_device_register_with_info()
> to avoid race conditions.
o.k.

> > +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?

Regards, Carsten

Attachment: pgpPoCxeq0NMo.pgp
Description: Digitale Signatur von OpenPGP