Ira Snyder wrote:
> Add Linux support for the Linear Technology LTC4245 Multiple Supply Hot
> Swap controller I2C monitoring interface.
>
> Signed-off-by: Ira W. Snyder <iws at ovro.caltech.edu>
> ---
>
> I've incorporated the suggestions from the latest posting of this
> driver.
>
> I didn't see an easy way to combine the LTC4245_VOLTAGE and
> LTC4245_ALARM defines into a single #define. The in[1-4]_min_alarm bits
> are in the FAULT1 register, while the in[5-8]_min_alarm are in the
> FAULT2 register. I could do it with one more macro parameter, but I
> thought that was getting ugly and confusing. Comments?
>
Looks good now, I've only got one minor issue left (which I'm afraid I missed
in my reviews before).
> Also, I used a local variable named "current" in a few places. Checking
> the driver with sparse, I get a warning that I'm shadowing the "current"
> variable in arch/powerpc/include/asm/current.h. Should I change the name
> of my variable, or ignore the warning? There isn't any harm in it...
>
Just ignore the warning.
<snip>
> +/* Return the voltage from the given register in millivolts */
> +static u32 ltc4245_get_voltage(struct device *dev, u8 reg)
> +{
Why dont you make this an s32 (or just an int) and then ..
> + struct ltc4245_data *data = ltc4245_update_device(dev);
> + const s32 val = data->vregs[reg - 0x10];
> + const u8 regval = val;
> + u32 voltage = 0;
> +
> + if (unlikely(val < 0))
> + return 0;
> +
> + switch (reg) {
> + case LTC4245_12VIN:
> + case LTC4245_12VOUT:
> + voltage = regval * 55;
> + break;
> + case LTC4245_5VIN:
> + case LTC4245_5VOUT:
> + voltage = regval * 22;
> + break;
> + case LTC4245_3VIN:
> + case LTC4245_3VOUT:
> + voltage = regval * 15;
> + break;
> + case LTC4245_VEEIN:
> + case LTC4245_VEEOUT:
> + voltage = regval * 55;
Make this * -55, and ...
<snip>
> +
> +static ssize_t ltc4245_show_voltage(struct device *dev,
> + struct device_attribute *da,
> + char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> + const u32 voltage = ltc4245_get_voltage(dev, attr->index);
> +
> + /* The VEEIN and VEEOUT registers are for -12V, so
> + * we add the negative sign on the output */
> + if (attr->index == LTC4245_VEEIN || attr->index == LTC4245_VEEOUT)
> + return snprintf(buf, PAGE_SIZE, "%d\n", voltage * -1);
> +
Remove this special case, and make the %u below %d ?
> + return snprintf(buf, PAGE_SIZE, "%u\n", voltage);
> +}
> +
<snip>
Regards,
Hans
