Re: [PATCH v2 1/2] hwmon: (pmbus/isl68137) add support for voltage divider on Vout

[Grant Peltier <grantpeltier93@gmail.com>]

Hi Guenter,

Thank you for the review!

On Thu, Oct 24, 2024 at 10:48:16AM -0700, Guenter Roeck wrote:
> On 10/22/24 18:58, Grant Peltier wrote:
> > + [...]
> > +	switch (reg) {
> > +	case PMBUS_VOUT_MAX:
> > +		/*
> > +		 * In cases where a voltage divider is attached to the target
> > +		 * rail between Vout and the Vsense pin, Vout related PMBus
> > +		 * commands should be scaled based on the expected voltage
> > +		 * at the Vsense pin.
> > +		 * I.e. Vsense = Vout * R2 / (R1 + R2)
> > +		 */
> > +		fallthrough;
> > +	case PMBUS_VOUT_MARGIN_HIGH:
> > +		fallthrough;
> > +	case PMBUS_VOUT_MARGIN_LOW:
> > +		fallthrough;
> > +	case PMBUS_VOUT_OV_FAULT_LIMIT:
> > +		fallthrough;
> > +	case PMBUS_VOUT_UV_FAULT_LIMIT:
> > +		fallthrough;
> 
> Just add the comment after the last case and drop all the fallthrough;
> Same above.
>

Will fix in v4

> > +	case PMBUS_VOUT_COMMAND:
> > +		if (data->channel[page].vout_voltage_divider[0]
> > +			&& data->channel[page].vout_voltage_divider[1]) {
> 
> It would be better to set defaults instead of having to check this
> for every executed command (for example by setting R1:=0 and R2:=1).
>

Sounds reasonable. I will adjust the channel initialization process to
set defaults instead and will remove the checks in v4.

> > [...]
> > +static int isl68137_probe_child_from_dt(struct device *dev,
> > +					struct device_node *child,
> > +					struct isl68137_data *data)
> > +{
> > +	u32 channel;
> > +	int err;
> > +
> > +	err = of_property_read_u32(child, "reg", &channel);
> > +	if (err) {
> > +		dev_err(dev, "missing reg property of %pOFn\n", child);
> > +		return err;
> > +	}
> > +	if (channel >= MAX_CHANNELS) {
> 
> The actual number of channels (pages) supported by the chip is known here
> and should be checked, either by passing the number of channels or a pointer
> to the entire info structure to this function.
> 

Will fix in v4.

> > +		dev_err(dev, "invalid reg %d of %pOFn\n", channel, child);
> > +		return -EINVAL;
> > +	}
> > +
> > +	of_property_read_u32_array(child, "renesas,vout-voltage-divider",
> 
> Ultimately this potentially applies to _all_ hardware monitoring chips,
> so I would very much prefer a generic voltage divider property definition.
>

There is a parallel conversation on PATCH v3 2/2 about this. Would you
prefer that I match the implementation for maxim20730?

> > +				data->channel[channel].vout_voltage_divider,
> > +				ARRAY_SIZE(data->channel[channel].vout_voltage_divider));
> 
> The returned data should be be validated here.
> 

Fixed in v3.

> > +
> > +	return 0;
> > +}
> > +
> > +static int isl68137_probe_from_dt(struct device *dev,
> > +				  struct isl68137_data *data)
> > +{
> > +	const struct device_node *np = dev->of_node;
> > +	struct device_node *child;
> > +	int err;
> > +
> > +	for_each_child_of_node(np, child) {
> > +		if (strcmp(child->name, "channel"))
> > +			continue;
> > +
> > +		err = isl68137_probe_child_from_dt(dev, child, data);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   static int isl68137_probe(struct i2c_client *client)
> >   {
> > +	struct device *dev = &client->dev;
> >   	struct pmbus_driver_info *info;
> > +	struct isl68137_data *data;
> > +	int i, err;
> > -	info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
> > -	if (!info)
> > +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> >   		return -ENOMEM;
> > -	memcpy(info, &raa_dmpvr_info, sizeof(*info));
> > +
> > +	for (i = 0; i < MAX_CHANNELS; i++)
> > +		memset(data->channel[i].vout_voltage_divider,
> > +			0,
> > +			sizeof(data->channel[i].vout_voltage_divider));
> 
> Under what circumstance would this not already be 0 after devm_kzalloc() ?
>

Mental lapse on my end. Will change to set harmless defaults discussed
above.

> > + [...]
> > +	if (dev->of_node) { 
> This conditional should not be necessary because for_each_child_of_node()
> ultimately calls __of_get_next_child() which checks if the node pointer
> is NULL.
> 

Will remove in v4.

> > +		err = isl68137_probe_from_dt(dev, data);
> > +		if (err)
> > +			return err;
> > + [...]

Thanks again,
Grant