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, ®val);
>
> 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, ®val);
> > + 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, ®val);
> > + 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:
pgp0jkxrKg9xk.pgp
Description: Digitale Signatur von OpenPGP
