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

[Guenter Roeck]

On 7/27/23 00:35, Carsten Spieß wrote:
> 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.

You mean it is ok to exceed the speed limit because others do it
as well ?

[ Sorry, I have heard the "Other drivers do it" pseudo-argument too many times.
  That doesn't mean it is the right thing to do. ]

Why not just leave it off ? What value does it add ? Besides,
even "via devicetree" isn't really correct because it can also
be set via ACPI or by a platform driver when using device_ properties.
I would suggest "can be set with device properties".

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

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

No, I missed the "+ channel".

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

Not really. For the shunt voltage, 0xf000 has different meanings depending on scale
and range 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.

> > > +			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;
Still unacceptable.

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

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.

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

This is not an old driver. It is more generic than of_ functions and
should be used in new drivers.

> What about backporting this driver to e.g. 5.15, will it affect it?

Device property callback functions existed for a long time. The function
exists in v4.14.y, which is the oldest supported kernel. Either case,
lack of support in an older kernel version is not an argument for avoiding
a new API. Anyone who backports a driver to an older kernel is responsible
for handling the backport, which may include new API functions.

Specific example: Your driver will have to use the .probe callback.
That has one argument in the latest kernel. In v5.15.y, it has two arguments.
You'll have to use the .probe_new callback there. Yes, technically you could
try sneaking in the use of .probe_new in your driver, but that callback will
be removed in v6.6. So you'll _have_ to do some backport, if you want it or not.

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

It is needed for device_property_read_u32() to work. And, again, "other drivers
do it" is not a valid argument.

Guenter