Messages in this thread Patch in this message | | | Date | Thu, 24 Nov 2022 06:18:57 -0800 | From | Guenter Roeck <> | Subject | Re: [PATCH v5] hwmon: (pmbus/core): Implement regulator get_status |
| |
On 11/24/22 00:34, Naresh Solanki wrote: > From: Patrick Rudolph <patrick.rudolph@9elements.com> > > Add get_status for pmbus_regulator_ops. > > --- > Changes: > - use lock throughout the function > - Avoid line continuation upto 100 column > - Optimize use of & and | operator > - Check for VOUT, IOUT, TEMPERATURE bit in status word before checking > respective status register for fault. > - Report regulator current status. > - Utilize get_error_flag to check for regulator errors. > - Check for return value of function get_error_flag > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com> > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > --- > drivers/hwmon/pmbus/pmbus_core.c | 44 ++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 20ca26e19db7..0b13214c662f 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -2855,6 +2855,49 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned > return 0; > } > > +static int pmbus_regulator_get_status(struct regulator_dev *rdev) > +{ > + struct device *dev = rdev_get_dev(rdev); > + struct i2c_client *client = to_i2c_client(dev->parent); > + struct pmbus_data *data = i2c_get_clientdata(client); > + u8 page = rdev_get_id(rdev); > + int status, ret; > + > + mutex_lock(&data->update_lock); > + status = pmbus_get_status(client, page, PMBUS_STATUS_WORD); > + if (status < 0) { > + ret = status; > + goto unlock; > + } > + > + if (status & PB_STATUS_OFF) { > + ret = REGULATOR_STATUS_OFF; > + goto unlock; > + } > + > + /* If regulator is ON & reports power good then return ON */ > + if (!(status & PB_STATUS_POWER_GOOD_N)) { > + ret = REGULATOR_STATUS_ON; > + goto unlock; > + } > + > + if (rdev->desc->ops->get_error_flags) {
Looking into this again, why is this check necessary ? Isn't this the regulator_ops from below ? Also, why not just call pmbus_regulator_get_error_flags() directly ?
> + ret = rdev->desc->ops->get_error_flags(rdev, &status); > + if (ret) > + goto unlock; > + > + if (status & (REGULATOR_ERROR_UNDER_VOLTAGE | REGULATOR_ERROR_OVER_CURRENT | > + REGULATOR_ERROR_REGULATION_OUT | REGULATOR_ERROR_FAIL | > + REGULATOR_ERROR_OVER_TEMP)) > + ret = REGULATOR_STATUS_ERROR;
If the condition above is false, the return value will be 0, or REGULATOR_STATUS_OFF. Is that intentional ?
> + } else > + ret = REGULATOR_STATUS_UNDEFINED; > +
CHECK: braces {} should be used on all arms of this statement #72: FILE: drivers/hwmon/pmbus/pmbus_core.c:2884: + if (rdev->desc->ops->get_error_flags) { [...] + } else
Guenter
> +unlock: > + mutex_unlock(&data->update_lock); > + return ret; > +} > + > static int pmbus_regulator_get_low_margin(struct i2c_client *client, int page) > { > struct pmbus_data *data = i2c_get_clientdata(client); > @@ -2995,6 +3038,7 @@ const struct regulator_ops pmbus_regulator_ops = { > .disable = pmbus_regulator_disable, > .is_enabled = pmbus_regulator_is_enabled, > .get_error_flags = pmbus_regulator_get_error_flags, > + .get_status = pmbus_regulator_get_status, > .get_voltage = pmbus_regulator_get_voltage, > .set_voltage = pmbus_regulator_set_voltage, > .list_voltage = pmbus_regulator_list_voltage, > > base-commit: 2c71b3246ec3246522e8cb7c8191dc7a5d62cc70
| |