> > + case hwmon_power:
> > + /* External calibration of receive power requires
> > + * floating point arithmetic. Doing that in the kernel
> > + * is not easy, so just skip it. If the module does
> > + * not require external calibration, we can however
> > + * show receiver power, since FP is then not needed.
> > + */
> > + if (sfp->id.ext.diagmon & SFP_DIAGMON_EXT_CAL &&
> > + channel == 1)
> > + return 0;
>
> It would be nice if it was possible to convert the floting point to
> a fixed point calculation. Would that be possible ?
Maybe. I decided to leave it for later.
The kernel has some support for emulating floating point hardware, by
doing floating point operations in software. I didn't find any
examples of using that code outside of emulation, but i wondered if it
would be possible to use it here. We don't need high performance here,
when just reading a sensor once per second.
> > +/* Sensors values are stored as two bytes, MSB second */
> > +static int sfp_hwmon_read_sensor(struct sfp *sfp, int reg, long *value)
> > +{
> > + u8 val[2];
> > + int err;
> > +
> > + err = sfp_read(sfp, true, reg, val, 2);
> > + if (err < 0)
> > + return err;
> > +
> > + *value = val[0] << 8 | val[1];
> > +
>
> Any chance to use something like __be16 and be16_to_cpu() ?
> You do that elsewhere - why not here ?
Yes. I want to look at this again. I don't like it either.
> > + for (i = j = 0; sfp->hwmon_name[i]; i++) {
> > + if (isalnum(sfp->hwmon_name[i])) {
> > + if (i != j)
> > + sfp->hwmon_name[j] = sfp->hwmon_name[i];
> > + j++;
> > + }
> > + }
>
> It might be better and simpler to replace invalid characters with '_'
> instead of dropping them. Also note that '_' is a valid character.
> Strictly speaking only "-* \t\n" are invalid.
I borrowed this code from the marvell10g driver. I don't know where it
borrowed it from. Is there a hwmon core function which we can pass an
arbitrary name to and it returned a sanitised one? Maybe we should add
one?
> > + sfp->hwmon_name[j] = '\0';
> > +
> Is it possible that j == 0 ?
Hummm....
sfp->hwmon_name is derived from dev_name(sfp->dev), which comes from
pdev->dev in the probe function. That comes from the device tree node
name. I suppose it is possible to name the node $@#$@, but i suspect
Rob would NACK it :-)
I can add a check for j==0 and return -EINVAL.
> > + sfp->hwmon_dev = devm_hwmon_device_register_with_info(sfp->dev,
> > + sfp->hwmon_name, sfp, &sfp_hwmon_chip_info,
> > + NULL);
> > +
> > + return PTR_ERR_OR_ZERO(sfp->hwmon_dev);
> > +}
> > +
> > +static void sfp_hwmon_remove(struct sfp *sfp)
> > +{
> > + devm_hwmon_device_unregister(sfp->hwmon_dev);
>
> If registartion and removal are not tied to a device, it doesn't make sense
> to use devm_ functions. Either use hwmon_device_register_with_info()
> and hwmon_device_unregister(), or drop the remove function.
Yes. I can change it. We have a few different lifetimes involved
here. You can consider the driver probe being for the SFP cage. The
SFP module being inserted into the cage is a different life time, and
the lifetime of the hwmon device.
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
