Re: [PATCH RFC] hwmon: Add Support for PMIC5000 Power Management ICs

Hi Guenter,

Thanks for your review!

On 21/8/26 01:34, Guenter Roeck wrote:
>> - Does this belong in HWMON, or should it be part of the regulator
>> 	subsystem?
> 
> It depends on its functionality. Is this just for monitoring, or are the voltages
> controllable ?

This driver is just for monitoring. They could be controllable, but I
haven't implemented that as changing the voltage of active RAM modules
doesn't seem safe (without retraining and so on).

>> +in0_enable		Whether SWA is enabled (RO)
> 
> Does that enable the voltage or its monitoriong ? The enable attribute is
> only to enable monitoring, not to enable the voltage. If this is a
> controllable voltage, the driver should be a regulator driver, as Sashiko
> suggested.

It just queries whether that output is enabled. I'll remove it though as
the disabled channels are hidden by is_visible anyway.

>> +/* PMIC5000 registers. */
>> +// clang-format off
> 
> No way. Are you serious ?

clang-format would break the alignment of the #defines; and there's a
few other drivers with "// clang-format" in them, so I assumed it would
be okay (though none are hwmon). I'll remove it next revision.

>> +
>> +
> 
> Please run checkpatch --strict on your patches and fix what it reports.

Will do!

>> +struct pmic5000_data {
>> +	struct regmap *regmap;
>> +	struct mutex mode_lock;
>> +	struct mutex adc_lock;
> 
> Explain why those are needed on top of the hwmon subsystem lock.

I didn't know the subsystem lock existed, I'll remove them.

>> +	if (channel != 0)
>> +		return -EOPNOTSUPP;
> 
> Unnecessary check.

Sure.

> 
> Unnecessary channel checks (at least if the is_visible function does its job
> and the info data is correct).
> 

Okay.

>> +	case 6: {
> 
> Why "{" ?

No reason, I'll remove it. I had previously declared something there.

>> +	if (channel >= 0 && channel <= 3) {
> 
> channel is always >= 0.
I wasn't too sure, thanks for that!

>> +	/*
>> +	 * The host shall wait minimum of 9 ms delay after the input selection
>> +	 * for ADC readout and the actual readout
>> +	 *
>> +	 * msleep may sleep for up to 20ms, which is fine.
> 
> No, it isn't fine.

Fair enough, I'll change it to fsleep.

> Also, why wait if the channel was not changed ?

Good point.

>> +	*val = 1 << (regval & 0x03);
> 
> 		BIT()

Thanks!

>> +static int pmic5000_write_interval(struct pmic5000_data *data, long val)
>> +{
>> +	struct regmap *regmap = data->regmap;
>> +	u32 regval;
>> +	int err;
>> +
>> +	switch (val) {
>> +	case 1:
>> +		regval = 0;
>> +		break;
>> +	case 2:
>> +		regval = 1;
>> +		break;
>> +	case 4:
>> +		regval = 2;
>> +		break;
>> +	case 8:
>> +		regval = 3;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
> 
> find_closest() would be more appropriate here. We don't usually expect users to know valid
> update intervals.

Thanks!

>> +static int pmic5000_common_probe(struct device *dev, struct regmap *regmap)
> 
> Why pmic5000_common_probe() ? There is only one caller.

Good point, I'll move it.

>> +static const struct of_device_id pmic5000_of_ids[] = {
>> +	{
>> +		.compatible = "jedec,pmic5000",
> 
> jedec,pmic5000 is not documented.

Is there some centralised database for device tree identifiers, or does
it just need to be added to trivial-devices.yaml or something?

Thanks again for your review!
Steve