On Fri, Nov 10, 2023 at 04:18:46PM +0100, Nuno Sa wrote:
> The LTC4282 hot swap controller allows a board to be safely inserted and
> removed from a live backplane. Using one or more external N-channel pass
> transistors, board supply voltage and inrush current are ramped up at an
> adjustable rate. An I2C interface and onboard ADC allows for monitoring
> of board current, voltage, power, energy and fault status.
There are plenty of ltc42xx in the same folder and a lot of other chips.
Have you checked that none of the existing driver is not close enough and
brand new driver is indeed required? If so, add a respective paragraph
somewhere (cover letter? TBH I haven't read it, it's fine if it's already
explained there).
...
> +#define LTC4282_STATUS_MSB 0x1F
> +#define LTC4282_VSOURCE 0x3a
> +#define LTC4282_VSOURCE_LOWEST 0x3c
> +#define LTC4282_VSOURCE_HIGHEST 0x3e
You have an inconsistency in the capitalization of the hexadecimal values.
...
> +/*
> + * relaxed version of FIELD_PREP() to be used when mask is not a compile time constant
> + * u32_encode_bits() can't also be used as the compiler needs to be able to evaluate
> + * mask at compile time.
> + */
> +#define LTC4282_FIELD_PREP(m, v) (((v) << (ffs(m) - 1)) & (m))
Can we name it accordingly as done in other places, and TBH it's a time to move
it to the header. (At least I know about two more implementations of this).
...
> +struct ltc4282_state {
> + struct regmap *map;
> + struct device *dev;
> + /* Protect against multiple accesses to the device registers */
> + struct mutex lock;
> + struct gpio_chip gc;
> + u64 saved_energy;
> + long power_max;
> + u32 gpio_map[LTC4282_GPIO_NR];
Why is this needed? The valid_mask is not enough?
> + u32 rsense;
> + u32 vin_mode;
> + u16 vfs_out;
> +};
...
> + /* GPIO_2,3 and the ALERT pin require setting the bit to 1 to pull down the line */
> + if (!gpio->active_high)
Hmm... Why do you need a separate flag for this? Shouldn't be described or
autodetected somehow?
> + val = !val;
...
> + *val = DIV_ROUND_CLOSEST_ULL(be16_to_cpu(in) * (u64)fs, U16_MAX);
I'm wondering if you can do some trick to "divide" actually to 2^16 so, it will
not use division op at all?
...
> + *val = DIV_ROUND_CLOSEST(in * fs, U8_MAX);
Ditto.
...
> + /*
> + * Wait for two ADC conversions so we are sure we get one full VDD
> + * measurement.
> + */
> + msleep(2 * LTC4282_TCONV_US / MILLI);
fsleep() ?
...
> +static int ltc4282_read_power_word(const struct ltc4282_state *st, u32 reg, long *val)
> +{
> + u64 temp = DECA * 40ULL * st->vfs_out * 1 << 16, temp_2;
Too many spaces.
"* BIT(16)" / "* BIT_ULL(16)" ?
...
> + temp = DIV_ROUND_CLOSEST_ULL(power * temp, U16_MAX);
> + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, U16_MAX * (u64)st->rsense);
> + *val = DIV64_U64_ROUND_CLOSEST(temp_2, st->rsense * int_pow(U16_MAX, 2));
Same Q about possible optimizations.
...
> + temp = power * 40 * DECA * st->vfs_out * 256ULL;
BIT_ULL(8) ?
...
> + *val = DIV64_U64_ROUND_CLOSEST(temp * MICRO, int_pow(U8_MAX, 2) * st->rsense);
Same Q about possible optimizations.
...
> + if (check_mul_overflow(DECA * st->vfs_out * 40 * 256, energy, &temp)) {
> + temp = DIV_ROUND_CLOSEST(DECA * st->vfs_out * 40 * 256, U16_MAX);
> + *val = DIV_ROUND_CLOSEST_ULL(temp * energy, st->rsense);
> + return ret;
> + }
BIT(8) ?
...
> + *val = DIV64_U64_ROUND_CLOSEST(temp, U16_MAX * (u64)st->rsense);
Same Q about possible optimizations.
...
> + /*
> + * reset happened... let's read the new energy value that
Reset
Let's
> + * together with the new tick counter should give a sane average
> + * value. Furthermore, we save whatever value we had accumulated
> + * so that the next energy read will have it into account.
> + */
...
> + /* give some time for accumulation... */
> + msleep(2 * LTC4282_TCONV_US / MILLI);
fsleep() ?
...
> + /*
> + * AVG = E / (Tconv * counter)
> + * We get energy in microJoule, hence dividing it by microSeconds gives Watts. Therefore,
> + * multiplying by MICRO gives us microWatts.
> + */
Here you used long lines, somewhere else, much shorter. Please, choose one
limit and be consistent with it.
...
> + temp = val * int_pow(U8_MAX, 2) * st->rsense;
> + power = DIV64_U64_ROUND_CLOSEST(temp, MICRO * DECA * 256ULL * st->vfs_out * 40);
As per above.
...
> + if (val >= fs)
> + in = U8_MAX;
> + else
> + in = DIV_ROUND_CLOSEST(val * U8_MAX, fs);
Can clamp() be used here?
...
> + /*
> + * Make sure vdd is stable. From the datasheet:
> + * The state of the UV and OV comparators is indicated by the STATUS register
> + * bits 0 and 1 and must be stable for at least 50ms to qualify for turn-on.
> + */
> + do {
> + ret = regmap_read_poll_timeout(st->map, LTC4282_STATUS_LSB, reg,
> + reg & LTC4282_VDD_STATUS_MASK, 10000, 50000);
> + if (!ret)
> + continue;
> + if (ret != -ETIMEDOUT)
> + return dev_err_probe(st->dev, ret, "Failed regmap read\n");
I would do it a bit differently.
> + break;
> + } while (n_tries--);
} while (--n_tries); ?
Altogether:
do {
ret = regmap_read_poll_timeout(st->map, LTC4282_STATUS_LSB, reg,
reg & LTC4282_VDD_STATUS_MASK, 10000, 50000);
/* Check if we got timeout which means UV and OV are stable for 50ms */
if (ret == -ETIMEDOUT)
break;
if (ret)
return dev_err_probe(st->dev, ret, "Failed regmap read\n");
} while (--n_tries);
...
> + if (val > LTC4282_CLKOUT_TICK)
> + return dev_err_probe(st->dev, -EINVAL,
> + "Invalid val(%u) for adi,clkout-mode\n", val);
ERANGE?
...
> + if (rate < LTC4282_CLKIN_MIN || rate > LTC4282_CLKIN_MAX)
in_range() ?
> + return dev_err_probe(st->dev, -EINVAL, "Invalid clkin range(%lu) [%lu %lu]\n",
> + rate, LTC4282_CLKIN_MIN, LTC4282_CLKIN_MAX);
...
> + for (gpio = 0; gpio <= LTC4282_GPIO_3; gpio++) {
Is 0 also being defined?
> + ret = device_property_read_u32(dev, ltc4282_gpio_prop[gpio], &func);
> + if (ret)
> + continue;
> + if (func >= ltc4282_gpios[gpio].n_funcs)
> + return dev_err_probe(dev, ret, "Invalid func(%u >= %u) for gpio%u\n",
> + func, ltc4282_gpios[gpio].n_funcs, gpio + 1);
> + if (func == LTC4282_PIN_GPIO) {
> + st->gpio_map[ngpios++] = gpio;
> + if (gpio == LTC4282_GPIO_1) {
> + /* default to input GPIO */
> + ret = regmap_set_bits(st->map, LTC4282_GPIO_CONFIG,
> + LTC4282_GPIO_1_CONFIG_MASK);
> + if (ret)
> + return ret;
> + }
> +
> + continue;
> + }
...
> + switch (val) {
> + case 3300000:
Hmm... Do we have MICROVOLTS_IN_VOLTS or so?
> + st->vin_mode = LTC4282_VIN_3_3V;
> + break;
> + case 5000000:
> + st->vin_mode = LTC4282_VIN_5V;
> + break;
> + case 12000000:
> + st->vin_mode = LTC4282_VIN_12V;
> + break;
> + case 24000000:
> + st->vin_mode = LTC4282_VIN_24V;
> + break;
> + default:
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid val(%u) for vin-mode-microvolt\n", val);
> + }
...
> +static SENSOR_DEVICE_ATTR(in1_crit_fault_log, 0644, ltc4282_show_fault_log,
> + ltc4282_clear_fault_log, LTC4282_OV_FAULT_MASK);
> +static SENSOR_DEVICE_ATTR(in1_lcrit_fault_log, 0644, ltc4282_show_fault_log,
> + ltc4282_clear_fault_log, LTC4282_UV_FAULT_MASK);
> +static SENSOR_DEVICE_ATTR(curr1_crit_fault_log, 0644, ltc4282_show_fault_log,
> + ltc4282_clear_fault_log, LTC4282_OC_FAULT_MASK);
> +static SENSOR_DEVICE_ATTR(power1_fault_log, 0644, ltc4282_show_fault_log,
> + ltc4282_clear_fault_log, LTC4282_POWER_BAD_FAULT_MASK);
> +static SENSOR_DEVICE_ATTR(fet_bad_fault_log, 0644, ltc4282_show_fault_log,
> + ltc4282_clear_fault_log, LTC4282_FET_BAD_FAULT_MASK);
> +static SENSOR_DEVICE_ATTR(fet_short_fault_log, 0644, ltc4282_show_fault_log,
> + ltc4282_clear_fault_log, LTC4282_FET_SHORT_FAULT_MASK);
SENSOR_DEVICE_ATTR_RO() / _RW() ?
...
> +static struct attribute *ltc4282_attrs[] = {
> + &sensor_dev_attr_energy1_input.dev_attr.attr,
> + &sensor_dev_attr_power1_good.dev_attr.attr,
> + &sensor_dev_attr_fet_bad_fault.dev_attr.attr,
> + &sensor_dev_attr_fet_short_fault.dev_attr.attr,
> + &sensor_dev_attr_in1_crit_fault_log.dev_attr.attr,
> + &sensor_dev_attr_in1_lcrit_fault_log.dev_attr.attr,
> + &sensor_dev_attr_curr1_crit_fault_log.dev_attr.attr,
> + &sensor_dev_attr_power1_fault_log.dev_attr.attr,
> + &sensor_dev_attr_fet_bad_fault_log.dev_attr.attr,
> + &sensor_dev_attr_fet_short_fault_log.dev_attr.attr,
> + NULL,
No comma for the terminator line.
> +};
...
> + msleep(3200);
Not a single letter to comment such a huge delay :-(
--
With Best Regards,
Andy Shevchenko
