Re: [PATCH 2/2] hwmon: ltc4282: add support for the LTC4282 chip

[Andy Shevchenko <andy@kernel.org>]

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