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
