* [PATCH v2 1/4] hwmon: (pmbus): Switch status registers to 16 bit
2017-08-10 21:57 [PATCH v2 0/4] hwmon: (pmbus): Core extension for STATUS_WORD and debugfs Eddie James
@ 2017-08-10 21:57 ` Eddie James
2017-08-13 13:36 ` [v2,1/4] " Guenter Roeck
2017-08-10 21:57 ` [PATCH v2 2/4] hwmon: (pmbus): Access word data for STATUS_WORD Eddie James
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Eddie James @ 2017-08-10 21:57 UTC (permalink / raw)
To: linux
Cc: jdelvare, linux-hwmon, linux-kernel, joel, jk, andrew, cbostic,
eajames, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
Switch the storage of status registers to 16 bit values. This allows us
to store all the bits of STATUS_WORD.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
drivers/hwmon/pmbus/pmbus_core.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index f1eff6b..4ec7586 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -112,7 +112,7 @@ struct pmbus_data {
* A single status register covers multiple attributes,
* so we keep them all together.
*/
- u8 status[PB_NUM_STATUS_REG];
+ u16 status[PB_NUM_STATUS_REG];
u8 status_register;
u8 currpage;
@@ -716,10 +716,10 @@ static int pmbus_get_boolean(struct pmbus_data *data, struct pmbus_boolean *b,
{
struct pmbus_sensor *s1 = b->s1;
struct pmbus_sensor *s2 = b->s2;
- u16 reg = (index >> 8) & 0xffff;
- u8 mask = index & 0xff;
+ u16 reg = (index >> 16) & 0xffff;
+ u16 mask = index & 0xffff;
int ret, status;
- u8 regval;
+ u16 regval;
status = data->status[reg];
if (status < 0)
@@ -860,7 +860,7 @@ static int pmbus_add_boolean(struct pmbus_data *data,
const char *name, const char *type, int seq,
struct pmbus_sensor *s1,
struct pmbus_sensor *s2,
- u16 reg, u8 mask)
+ u16 reg, u16 mask)
{
struct pmbus_boolean *boolean;
struct sensor_device_attribute *a;
@@ -876,7 +876,7 @@ static int pmbus_add_boolean(struct pmbus_data *data,
boolean->s1 = s1;
boolean->s2 = s2;
pmbus_attr_init(a, boolean->name, S_IRUGO, pmbus_show_boolean, NULL,
- (reg << 8) | mask);
+ (reg << 16) | mask);
return pmbus_add_attribute(data, &a->dev_attr.attr);
}
@@ -962,7 +962,7 @@ struct pmbus_limit_attr {
*/
struct pmbus_sensor_attr {
u16 reg; /* sensor register */
- u8 gbit; /* generic status bit */
+ u16 gbit; /* generic status bit */
u8 nlimit; /* # of limit registers */
enum pmbus_sensor_classes class;/* sensor class */
const char *label; /* sensor label */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [v2,1/4] hwmon: (pmbus): Switch status registers to 16 bit
2017-08-10 21:57 ` [PATCH v2 1/4] hwmon: (pmbus): Switch status registers to 16 bit Eddie James
@ 2017-08-13 13:36 ` Guenter Roeck
0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2017-08-13 13:36 UTC (permalink / raw)
To: eajames
Cc: jdelvare, linux-hwmon, linux-kernel, joel, jk, andrew, cbostic,
Edward A. James
On Thu, Aug 10, 2017 at 04:57:47PM -0500, eajames@linux.vnet.ibm.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Switch the storage of status registers to 16 bit values. This allows us
> to store all the bits of STATUS_WORD.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
Applied to hwmon-next.
Thanks,
Guenter
> ---
> drivers/hwmon/pmbus/pmbus_core.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index f1eff6b..4ec7586 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -112,7 +112,7 @@ struct pmbus_data {
> * A single status register covers multiple attributes,
> * so we keep them all together.
> */
> - u8 status[PB_NUM_STATUS_REG];
> + u16 status[PB_NUM_STATUS_REG];
> u8 status_register;
>
> u8 currpage;
> @@ -716,10 +716,10 @@ static int pmbus_get_boolean(struct pmbus_data *data, struct pmbus_boolean *b,
> {
> struct pmbus_sensor *s1 = b->s1;
> struct pmbus_sensor *s2 = b->s2;
> - u16 reg = (index >> 8) & 0xffff;
> - u8 mask = index & 0xff;
> + u16 reg = (index >> 16) & 0xffff;
> + u16 mask = index & 0xffff;
> int ret, status;
> - u8 regval;
> + u16 regval;
>
> status = data->status[reg];
> if (status < 0)
> @@ -860,7 +860,7 @@ static int pmbus_add_boolean(struct pmbus_data *data,
> const char *name, const char *type, int seq,
> struct pmbus_sensor *s1,
> struct pmbus_sensor *s2,
> - u16 reg, u8 mask)
> + u16 reg, u16 mask)
> {
> struct pmbus_boolean *boolean;
> struct sensor_device_attribute *a;
> @@ -876,7 +876,7 @@ static int pmbus_add_boolean(struct pmbus_data *data,
> boolean->s1 = s1;
> boolean->s2 = s2;
> pmbus_attr_init(a, boolean->name, S_IRUGO, pmbus_show_boolean, NULL,
> - (reg << 8) | mask);
> + (reg << 16) | mask);
>
> return pmbus_add_attribute(data, &a->dev_attr.attr);
> }
> @@ -962,7 +962,7 @@ struct pmbus_limit_attr {
> */
> struct pmbus_sensor_attr {
> u16 reg; /* sensor register */
> - u8 gbit; /* generic status bit */
> + u16 gbit; /* generic status bit */
> u8 nlimit; /* # of limit registers */
> enum pmbus_sensor_classes class;/* sensor class */
> const char *label; /* sensor label */
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/4] hwmon: (pmbus): Access word data for STATUS_WORD
2017-08-10 21:57 [PATCH v2 0/4] hwmon: (pmbus): Core extension for STATUS_WORD and debugfs Eddie James
2017-08-10 21:57 ` [PATCH v2 1/4] hwmon: (pmbus): Switch status registers to 16 bit Eddie James
@ 2017-08-10 21:57 ` Eddie James
2017-08-13 13:38 ` [v2,2/4] " Guenter Roeck
2017-08-10 21:57 ` [PATCH v2 3/4] hwmon: (pmbus): Add generic alarm bit for iin and pin Eddie James
2017-08-10 21:57 ` [PATCH v2 4/4] hwmon: (pmbus): Add debugfs for status registers Eddie James
3 siblings, 1 reply; 10+ messages in thread
From: Eddie James @ 2017-08-10 21:57 UTC (permalink / raw)
To: linux
Cc: jdelvare, linux-hwmon, linux-kernel, joel, jk, andrew, cbostic,
eajames, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
Pmbus always reads byte data from the status register, even if
configured to use STATUS_WORD. Use a function pointer to read the
correct amount of data from the registers.
Also switch to try STATUS_WORD first before STATUS_BYTE on init.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
drivers/hwmon/pmbus/pmbus_core.c | 54 +++++++++++++++++++++++++++++-----------
1 file changed, 40 insertions(+), 14 deletions(-)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 4ec7586..277d170 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -113,7 +113,8 @@ struct pmbus_data {
* so we keep them all together.
*/
u16 status[PB_NUM_STATUS_REG];
- u8 status_register;
+
+ int (*read_status)(struct i2c_client *client, int page);
u8 currpage;
};
@@ -324,7 +325,7 @@ static int pmbus_check_status_cml(struct i2c_client *client)
struct pmbus_data *data = i2c_get_clientdata(client);
int status, status2;
- status = _pmbus_read_byte_data(client, -1, data->status_register);
+ status = data->read_status(client, -1);
if (status < 0 || (status & PB_STATUS_CML)) {
status2 = _pmbus_read_byte_data(client, -1, PMBUS_STATUS_CML);
if (status2 < 0 || (status2 & PB_CML_FAULT_INVALID_COMMAND))
@@ -348,6 +349,23 @@ static bool pmbus_check_register(struct i2c_client *client,
return rv >= 0;
}
+static bool pmbus_check_status_register(struct i2c_client *client, int page)
+{
+ int status;
+ struct pmbus_data *data = i2c_get_clientdata(client);
+
+ status = data->read_status(client, page);
+ if (status >= 0 && !(data->flags & PMBUS_SKIP_STATUS_CHECK) &&
+ (status & PB_STATUS_CML)) {
+ status = _pmbus_read_byte_data(client, -1, PMBUS_STATUS_CML);
+ if (status < 0 || (status & PB_CML_FAULT_INVALID_COMMAND))
+ status = -EIO;
+ }
+
+ pmbus_clear_fault_page(client, -1);
+ return status >= 0;
+}
+
bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg)
{
return pmbus_check_register(client, _pmbus_read_byte_data, page, reg);
@@ -394,8 +412,7 @@ static struct pmbus_data *pmbus_update_device(struct device *dev)
for (i = 0; i < info->pages; i++) {
data->status[PB_STATUS_BASE + i]
- = _pmbus_read_byte_data(client, i,
- data->status_register);
+ = data->read_status(client, i);
for (j = 0; j < ARRAY_SIZE(pmbus_status); j++) {
struct _pmbus_status *s = &pmbus_status[j];
@@ -1051,8 +1068,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
* the generic status register for this page is accessible.
*/
if (!ret && attr->gbit &&
- pmbus_check_byte_register(client, page,
- data->status_register)) {
+ pmbus_check_status_register(client, page)) {
ret = pmbus_add_boolean(data, name, "alarm", index,
NULL, NULL,
PB_STATUS_BASE + page,
@@ -1729,6 +1745,16 @@ static int pmbus_identify_common(struct i2c_client *client,
return 0;
}
+static int pmbus_read_status_byte(struct i2c_client *client, int page)
+{
+ return _pmbus_read_byte_data(client, page, PMBUS_STATUS_BYTE);
+}
+
+static int pmbus_read_status_word(struct i2c_client *client, int page)
+{
+ return _pmbus_read_word_data(client, page, PMBUS_STATUS_WORD);
+}
+
static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
struct pmbus_driver_info *info)
{
@@ -1736,16 +1762,16 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
int page, ret;
/*
- * Some PMBus chips don't support PMBUS_STATUS_BYTE, so try
- * to use PMBUS_STATUS_WORD instead if that is the case.
+ * Some PMBus chips don't support PMBUS_STATUS_WORD, so try
+ * to use PMBUS_STATUS_BYTE instead if that is the case.
* Bail out if both registers are not supported.
*/
- data->status_register = PMBUS_STATUS_BYTE;
- ret = i2c_smbus_read_byte_data(client, PMBUS_STATUS_BYTE);
- if (ret < 0 || ret == 0xff) {
- data->status_register = PMBUS_STATUS_WORD;
- ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
- if (ret < 0 || ret == 0xffff) {
+ data->read_status = pmbus_read_status_word;
+ ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
+ if (ret < 0 || ret == 0xffff) {
+ data->read_status = pmbus_read_status_byte;
+ ret = i2c_smbus_read_byte_data(client, PMBUS_STATUS_BYTE);
+ if (ret < 0 || ret == 0xff) {
dev_err(dev, "PMBus status register not found\n");
return -ENODEV;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [v2,2/4] hwmon: (pmbus): Access word data for STATUS_WORD
2017-08-10 21:57 ` [PATCH v2 2/4] hwmon: (pmbus): Access word data for STATUS_WORD Eddie James
@ 2017-08-13 13:38 ` Guenter Roeck
0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2017-08-13 13:38 UTC (permalink / raw)
To: eajames
Cc: jdelvare, linux-hwmon, linux-kernel, joel, jk, andrew, cbostic,
Edward A. James
On Thu, Aug 10, 2017 at 04:57:48PM -0500, eajames@linux.vnet.ibm.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Pmbus always reads byte data from the status register, even if
> configured to use STATUS_WORD. Use a function pointer to read the
> correct amount of data from the registers.
> Also switch to try STATUS_WORD first before STATUS_BYTE on init.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
Applied to hwmon-next.
Thanks,
Guenter
> ---
> drivers/hwmon/pmbus/pmbus_core.c | 54 +++++++++++++++++++++++++++++-----------
> 1 file changed, 40 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 4ec7586..277d170 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -113,7 +113,8 @@ struct pmbus_data {
> * so we keep them all together.
> */
> u16 status[PB_NUM_STATUS_REG];
> - u8 status_register;
> +
> + int (*read_status)(struct i2c_client *client, int page);
>
> u8 currpage;
> };
> @@ -324,7 +325,7 @@ static int pmbus_check_status_cml(struct i2c_client *client)
> struct pmbus_data *data = i2c_get_clientdata(client);
> int status, status2;
>
> - status = _pmbus_read_byte_data(client, -1, data->status_register);
> + status = data->read_status(client, -1);
> if (status < 0 || (status & PB_STATUS_CML)) {
> status2 = _pmbus_read_byte_data(client, -1, PMBUS_STATUS_CML);
> if (status2 < 0 || (status2 & PB_CML_FAULT_INVALID_COMMAND))
> @@ -348,6 +349,23 @@ static bool pmbus_check_register(struct i2c_client *client,
> return rv >= 0;
> }
>
> +static bool pmbus_check_status_register(struct i2c_client *client, int page)
> +{
> + int status;
> + struct pmbus_data *data = i2c_get_clientdata(client);
> +
> + status = data->read_status(client, page);
> + if (status >= 0 && !(data->flags & PMBUS_SKIP_STATUS_CHECK) &&
> + (status & PB_STATUS_CML)) {
> + status = _pmbus_read_byte_data(client, -1, PMBUS_STATUS_CML);
> + if (status < 0 || (status & PB_CML_FAULT_INVALID_COMMAND))
> + status = -EIO;
> + }
> +
> + pmbus_clear_fault_page(client, -1);
> + return status >= 0;
> +}
> +
> bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg)
> {
> return pmbus_check_register(client, _pmbus_read_byte_data, page, reg);
> @@ -394,8 +412,7 @@ static struct pmbus_data *pmbus_update_device(struct device *dev)
>
> for (i = 0; i < info->pages; i++) {
> data->status[PB_STATUS_BASE + i]
> - = _pmbus_read_byte_data(client, i,
> - data->status_register);
> + = data->read_status(client, i);
> for (j = 0; j < ARRAY_SIZE(pmbus_status); j++) {
> struct _pmbus_status *s = &pmbus_status[j];
>
> @@ -1051,8 +1068,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
> * the generic status register for this page is accessible.
> */
> if (!ret && attr->gbit &&
> - pmbus_check_byte_register(client, page,
> - data->status_register)) {
> + pmbus_check_status_register(client, page)) {
> ret = pmbus_add_boolean(data, name, "alarm", index,
> NULL, NULL,
> PB_STATUS_BASE + page,
> @@ -1729,6 +1745,16 @@ static int pmbus_identify_common(struct i2c_client *client,
> return 0;
> }
>
> +static int pmbus_read_status_byte(struct i2c_client *client, int page)
> +{
> + return _pmbus_read_byte_data(client, page, PMBUS_STATUS_BYTE);
> +}
> +
> +static int pmbus_read_status_word(struct i2c_client *client, int page)
> +{
> + return _pmbus_read_word_data(client, page, PMBUS_STATUS_WORD);
> +}
> +
> static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
> struct pmbus_driver_info *info)
> {
> @@ -1736,16 +1762,16 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
> int page, ret;
>
> /*
> - * Some PMBus chips don't support PMBUS_STATUS_BYTE, so try
> - * to use PMBUS_STATUS_WORD instead if that is the case.
> + * Some PMBus chips don't support PMBUS_STATUS_WORD, so try
> + * to use PMBUS_STATUS_BYTE instead if that is the case.
> * Bail out if both registers are not supported.
> */
> - data->status_register = PMBUS_STATUS_BYTE;
> - ret = i2c_smbus_read_byte_data(client, PMBUS_STATUS_BYTE);
> - if (ret < 0 || ret == 0xff) {
> - data->status_register = PMBUS_STATUS_WORD;
> - ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
> - if (ret < 0 || ret == 0xffff) {
> + data->read_status = pmbus_read_status_word;
> + ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
> + if (ret < 0 || ret == 0xffff) {
> + data->read_status = pmbus_read_status_byte;
> + ret = i2c_smbus_read_byte_data(client, PMBUS_STATUS_BYTE);
> + if (ret < 0 || ret == 0xff) {
> dev_err(dev, "PMBus status register not found\n");
> return -ENODEV;
> }
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/4] hwmon: (pmbus): Add generic alarm bit for iin and pin
2017-08-10 21:57 [PATCH v2 0/4] hwmon: (pmbus): Core extension for STATUS_WORD and debugfs Eddie James
2017-08-10 21:57 ` [PATCH v2 1/4] hwmon: (pmbus): Switch status registers to 16 bit Eddie James
2017-08-10 21:57 ` [PATCH v2 2/4] hwmon: (pmbus): Access word data for STATUS_WORD Eddie James
@ 2017-08-10 21:57 ` Eddie James
2017-08-13 13:39 ` [v2,3/4] " Guenter Roeck
2017-08-10 21:57 ` [PATCH v2 4/4] hwmon: (pmbus): Add debugfs for status registers Eddie James
3 siblings, 1 reply; 10+ messages in thread
From: Eddie James @ 2017-08-10 21:57 UTC (permalink / raw)
To: linux
Cc: jdelvare, linux-hwmon, linux-kernel, joel, jk, andrew, cbostic,
eajames, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
Add PB_STATUS_INPUT as the generic alarm bit for iin and pin. We also
need to redo the status register checking before setting up the boolean
attribute, since it won't necessarily check STATUS_WORD if the device
doesn't support it, which we need for this bit.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
drivers/hwmon/pmbus/pmbus_core.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 277d170..ef135d8 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -114,6 +114,7 @@ struct pmbus_data {
*/
u16 status[PB_NUM_STATUS_REG];
+ bool has_status_word; /* device uses STATUS_WORD register */
int (*read_status)(struct i2c_client *client, int page);
u8 currpage;
@@ -1045,6 +1046,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
const struct pmbus_sensor_attr *attr)
{
struct pmbus_sensor *base;
+ bool upper = !!(attr->gbit & 0xff00); /* need to check STATUS_WORD */
int ret;
if (attr->label) {
@@ -1065,9 +1067,11 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
/*
* Add generic alarm attribute only if there are no individual
* alarm attributes, if there is a global alarm bit, and if
- * the generic status register for this page is accessible.
+ * the generic status register (word or byte, depending on
+ * which global bit is set) for this page is accessible.
*/
if (!ret && attr->gbit &&
+ (!upper || (upper && data->has_status_word)) &&
pmbus_check_status_register(client, page)) {
ret = pmbus_add_boolean(data, name, "alarm", index,
NULL, NULL,
@@ -1324,6 +1328,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
.func = PMBUS_HAVE_IIN,
.sfunc = PMBUS_HAVE_STATUS_INPUT,
.sbase = PB_STATUS_INPUT_BASE,
+ .gbit = PB_STATUS_INPUT,
.limit = iin_limit_attrs,
.nlimit = ARRAY_SIZE(iin_limit_attrs),
}, {
@@ -1408,6 +1413,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
.func = PMBUS_HAVE_PIN,
.sfunc = PMBUS_HAVE_STATUS_INPUT,
.sbase = PB_STATUS_INPUT_BASE,
+ .gbit = PB_STATUS_INPUT,
.limit = pin_limit_attrs,
.nlimit = ARRAY_SIZE(pin_limit_attrs),
}, {
@@ -1775,6 +1781,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
dev_err(dev, "PMBus status register not found\n");
return -ENODEV;
}
+ } else {
+ data->has_status_word = true;
}
/* Enable PEC if the controller supports it */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [v2,3/4] hwmon: (pmbus): Add generic alarm bit for iin and pin
2017-08-10 21:57 ` [PATCH v2 3/4] hwmon: (pmbus): Add generic alarm bit for iin and pin Eddie James
@ 2017-08-13 13:39 ` Guenter Roeck
0 siblings, 0 replies; 10+ messages in thread
From: Guenter Roeck @ 2017-08-13 13:39 UTC (permalink / raw)
To: eajames
Cc: jdelvare, linux-hwmon, linux-kernel, joel, jk, andrew, cbostic,
Edward A. James
On Thu, Aug 10, 2017 at 04:57:49PM -0500, eajames@linux.vnet.ibm.com wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add PB_STATUS_INPUT as the generic alarm bit for iin and pin. We also
> need to redo the status register checking before setting up the boolean
> attribute, since it won't necessarily check STATUS_WORD if the device
> doesn't support it, which we need for this bit.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
Applied to hwmon-next.
Thanks,
Guenter
> ---
> drivers/hwmon/pmbus/pmbus_core.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 277d170..ef135d8 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -114,6 +114,7 @@ struct pmbus_data {
> */
> u16 status[PB_NUM_STATUS_REG];
>
> + bool has_status_word; /* device uses STATUS_WORD register */
> int (*read_status)(struct i2c_client *client, int page);
>
> u8 currpage;
> @@ -1045,6 +1046,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
> const struct pmbus_sensor_attr *attr)
> {
> struct pmbus_sensor *base;
> + bool upper = !!(attr->gbit & 0xff00); /* need to check STATUS_WORD */
> int ret;
>
> if (attr->label) {
> @@ -1065,9 +1067,11 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
> /*
> * Add generic alarm attribute only if there are no individual
> * alarm attributes, if there is a global alarm bit, and if
> - * the generic status register for this page is accessible.
> + * the generic status register (word or byte, depending on
> + * which global bit is set) for this page is accessible.
> */
> if (!ret && attr->gbit &&
> + (!upper || (upper && data->has_status_word)) &&
> pmbus_check_status_register(client, page)) {
> ret = pmbus_add_boolean(data, name, "alarm", index,
> NULL, NULL,
> @@ -1324,6 +1328,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
> .func = PMBUS_HAVE_IIN,
> .sfunc = PMBUS_HAVE_STATUS_INPUT,
> .sbase = PB_STATUS_INPUT_BASE,
> + .gbit = PB_STATUS_INPUT,
> .limit = iin_limit_attrs,
> .nlimit = ARRAY_SIZE(iin_limit_attrs),
> }, {
> @@ -1408,6 +1413,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
> .func = PMBUS_HAVE_PIN,
> .sfunc = PMBUS_HAVE_STATUS_INPUT,
> .sbase = PB_STATUS_INPUT_BASE,
> + .gbit = PB_STATUS_INPUT,
> .limit = pin_limit_attrs,
> .nlimit = ARRAY_SIZE(pin_limit_attrs),
> }, {
> @@ -1775,6 +1781,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
> dev_err(dev, "PMBus status register not found\n");
> return -ENODEV;
> }
> + } else {
> + data->has_status_word = true;
> }
>
> /* Enable PEC if the controller supports it */
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 4/4] hwmon: (pmbus): Add debugfs for status registers
2017-08-10 21:57 [PATCH v2 0/4] hwmon: (pmbus): Core extension for STATUS_WORD and debugfs Eddie James
` (2 preceding siblings ...)
2017-08-10 21:57 ` [PATCH v2 3/4] hwmon: (pmbus): Add generic alarm bit for iin and pin Eddie James
@ 2017-08-10 21:57 ` Eddie James
2017-08-11 13:52 ` Guenter Roeck
3 siblings, 1 reply; 10+ messages in thread
From: Eddie James @ 2017-08-10 21:57 UTC (permalink / raw)
To: linux
Cc: jdelvare, linux-hwmon, linux-kernel, joel, jk, andrew, cbostic,
eajames, Edward A. James
From: "Edward A. James" <eajames@us.ibm.com>
Export all the available status registers through debugfs, if the client
driver wants them.
Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
drivers/hwmon/pmbus/pmbus.h | 6 ++
drivers/hwmon/pmbus/pmbus_core.c | 201 +++++++++++++++++++++++++++++++++++++++
2 files changed, 207 insertions(+)
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index bfcb13b..5f91a19 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -383,6 +383,12 @@ struct pmbus_driver_info {
/* Regulator functionality, if supported by this chip driver. */
int num_regulators;
const struct regulator_desc *reg_desc;
+
+ /*
+ * Controls whether or not to create debugfs entries for this driver's
+ * devices.
+ */
+ bool debugfs;
};
/* Regulator ops */
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ef135d8..b11ebec 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -19,6 +19,7 @@
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
+#include <linux/debugfs.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
@@ -101,6 +102,7 @@ struct pmbus_data {
int num_attributes;
struct attribute_group group;
const struct attribute_group *groups[2];
+ struct dentry *debugfs; /* debugfs device directory */
struct pmbus_sensor *sensors;
@@ -120,6 +122,12 @@ struct pmbus_data {
u8 currpage;
};
+struct pmbus_debugfs_entry {
+ struct i2c_client *client;
+ u8 page;
+ u8 reg;
+};
+
void pmbus_clear_cache(struct i2c_client *client)
{
struct pmbus_data *data = i2c_get_clientdata(client);
@@ -1893,6 +1901,184 @@ static int pmbus_regulator_register(struct pmbus_data *data)
}
#endif
+static struct dentry *pmbus_debugfs_dir; /* pmbus debugfs directory */
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static int pmbus_debugfs_get(void *data, u64 *val)
+{
+ struct pmbus_debugfs_entry *entry = data;
+
+ *val = _pmbus_read_byte_data(entry->client, entry->page, entry->reg);
+
+ return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops, pmbus_debugfs_get, NULL,
+ "0x%02llx\n");
+
+static int pmbus_debugfs_get_status(void *data, u64 *val)
+{
+ struct pmbus_debugfs_entry *entry = data;
+ struct pmbus_data *pdata = i2c_get_clientdata(entry->client);
+
+ *val = pdata->read_status(entry->client, entry->page);
+
+ return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops_status, pmbus_debugfs_get_status,
+ NULL, "0x%04llx\n");
+
+static int pmbus_init_debugfs(struct i2c_client *client,
+ struct pmbus_data *data)
+{
+ int i, idx = 0;
+ char name[PMBUS_NAME_SIZE];
+ struct pmbus_debugfs_entry *entries;
+
+ /* Check if client driver requested debugfs support. */
+ if (!data->info->debugfs)
+ return 0;
+
+ /* Create the root pmbus debugfs directory if it's not created yet. */
+ if (!pmbus_debugfs_dir) {
+ pmbus_debugfs_dir = debugfs_create_dir("pmbus", NULL);
+ if (IS_ERR_OR_NULL(pmbus_debugfs_dir)) {
+ pmbus_debugfs_dir = NULL;
+ return -ENODEV;
+ }
+ }
+
+ /*
+ * Create the debugfs directory for this device. Use the hwmon device
+ * name to avoid conflicts (hwmon numbers are globally unique).
+ */
+ data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
+ pmbus_debugfs_dir);
+ if (IS_ERR_OR_NULL(data->debugfs)) {
+ data->debugfs = NULL;
+ return -ENODEV;
+ }
+
+ /* Allocate the max possible entries we need. */
+ entries = devm_kzalloc(data->dev,
+ sizeof(*entries) * (data->info->pages * 10),
+ GFP_KERNEL);
+ if (!entries)
+ return -ENOMEM;
+
+ for (i = 0; i < data->info->pages; ++i) {
+ /* Check accessibility of status register if it's not page 0 */
+ if (!i || pmbus_check_status_register(client, i)) {
+ /* No need to set reg as we have special read op. */
+ entries[idx].client = client;
+ entries[idx].page = i;
+ snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
+ debugfs_create_file(name, 0444, data->debugfs,
+ &entries[idx++],
+ &pmbus_debugfs_ops_status);
+ }
+
+ if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
+ entries[idx].client = client;
+ entries[idx].page = i;
+ entries[idx].reg = PMBUS_STATUS_VOUT;
+ snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
+ debugfs_create_file(name, 0444, data->debugfs,
+ &entries[idx++],
+ &pmbus_debugfs_ops);
+ }
+
+ if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
+ entries[idx].client = client;
+ entries[idx].page = i;
+ entries[idx].reg = PMBUS_STATUS_IOUT;
+ snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
+ debugfs_create_file(name, 0444, data->debugfs,
+ &entries[idx++],
+ &pmbus_debugfs_ops);
+ }
+
+ if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
+ entries[idx].client = client;
+ entries[idx].page = i;
+ entries[idx].reg = PMBUS_STATUS_INPUT;
+ snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
+ debugfs_create_file(name, 0444, data->debugfs,
+ &entries[idx++],
+ &pmbus_debugfs_ops);
+ }
+
+ if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
+ entries[idx].client = client;
+ entries[idx].page = i;
+ entries[idx].reg = PMBUS_STATUS_TEMPERATURE;
+ snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
+ debugfs_create_file(name, 0444, data->debugfs,
+ &entries[idx++],
+ &pmbus_debugfs_ops);
+ }
+
+ if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
+ entries[idx].client = client;
+ entries[idx].page = i;
+ entries[idx].reg = PMBUS_STATUS_CML;
+ snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
+ debugfs_create_file(name, 0444, data->debugfs,
+ &entries[idx++],
+ &pmbus_debugfs_ops);
+ }
+
+ if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) {
+ entries[idx].client = client;
+ entries[idx].page = i;
+ entries[idx].reg = PMBUS_STATUS_OTHER;
+ snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
+ debugfs_create_file(name, 0444, data->debugfs,
+ &entries[idx++],
+ &pmbus_debugfs_ops);
+ }
+
+ if (pmbus_check_byte_register(client, i,
+ PMBUS_STATUS_MFR_SPECIFIC)) {
+ entries[idx].client = client;
+ entries[idx].page = i;
+ entries[idx].reg = PMBUS_STATUS_MFR_SPECIFIC;
+ snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
+ debugfs_create_file(name, 0444, data->debugfs,
+ &entries[idx++],
+ &pmbus_debugfs_ops);
+ }
+
+ if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
+ entries[idx].client = client;
+ entries[idx].page = i;
+ entries[idx].reg = PMBUS_STATUS_FAN_12;
+ snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
+ debugfs_create_file(name, 0444, data->debugfs,
+ &entries[idx++],
+ &pmbus_debugfs_ops);
+ }
+
+ if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
+ entries[idx].client = client;
+ entries[idx].page = i;
+ entries[idx].reg = PMBUS_STATUS_FAN_34;
+ snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
+ debugfs_create_file(name, 0444, data->debugfs,
+ &entries[idx++],
+ &pmbus_debugfs_ops);
+ }
+ }
+
+ return 0;
+}
+#else
+static int pmbus_init_debugfs(struct i2c_client *client,
+ struct pmbus_data *data)
+{
+ return 0;
+}
+#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */
+
int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
struct pmbus_driver_info *info)
{
@@ -1952,6 +2138,10 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
if (ret)
goto out_unregister;
+ ret = pmbus_init_debugfs(client, data);
+ if (ret)
+ dev_warn(dev, "Failed to register debugfs\n");
+
return 0;
out_unregister:
@@ -1965,6 +2155,17 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
int pmbus_do_remove(struct i2c_client *client)
{
struct pmbus_data *data = i2c_get_clientdata(client);
+
+ debugfs_remove_recursive(data->debugfs);
+ if (pmbus_debugfs_dir && simple_empty(pmbus_debugfs_dir)) {
+ /*
+ * Remove the root pmbus debugfs directory if we've removed
+ * all the device directories underneath already.
+ */
+ debugfs_remove(pmbus_debugfs_dir);
+ pmbus_debugfs_dir = NULL;
+ }
+
hwmon_device_unregister(data->hwmon_dev);
kfree(data->group.attrs);
return 0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v2 4/4] hwmon: (pmbus): Add debugfs for status registers
2017-08-10 21:57 ` [PATCH v2 4/4] hwmon: (pmbus): Add debugfs for status registers Eddie James
@ 2017-08-11 13:52 ` Guenter Roeck
2017-08-14 14:58 ` Eddie James
0 siblings, 1 reply; 10+ messages in thread
From: Guenter Roeck @ 2017-08-11 13:52 UTC (permalink / raw)
To: Eddie James
Cc: jdelvare, linux-hwmon, linux-kernel, joel, jk, andrew, cbostic,
Edward A. James
On 08/10/2017 02:57 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Export all the available status registers through debugfs, if the client
> driver wants them.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
> drivers/hwmon/pmbus/pmbus.h | 6 ++
> drivers/hwmon/pmbus/pmbus_core.c | 201 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 207 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index bfcb13b..5f91a19 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -383,6 +383,12 @@ struct pmbus_driver_info {
> /* Regulator functionality, if supported by this chip driver. */
> int num_regulators;
> const struct regulator_desc *reg_desc;
> +
> + /*
> + * Controls whether or not to create debugfs entries for this driver's
> + * devices.
> + */
> + bool debugfs;
> };
>
> /* Regulator ops */
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index ef135d8..b11ebec 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -19,6 +19,7 @@
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> +#include <linux/debugfs.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/init.h>
> @@ -101,6 +102,7 @@ struct pmbus_data {
> int num_attributes;
> struct attribute_group group;
> const struct attribute_group *groups[2];
> + struct dentry *debugfs; /* debugfs device directory */
>
> struct pmbus_sensor *sensors;
>
> @@ -120,6 +122,12 @@ struct pmbus_data {
> u8 currpage;
> };
>
> +struct pmbus_debugfs_entry {
> + struct i2c_client *client;
> + u8 page;
> + u8 reg;
> +};
> +
> void pmbus_clear_cache(struct i2c_client *client)
> {
> struct pmbus_data *data = i2c_get_clientdata(client);
> @@ -1893,6 +1901,184 @@ static int pmbus_regulator_register(struct pmbus_data *data)
> }
> #endif
>
> +static struct dentry *pmbus_debugfs_dir; /* pmbus debugfs directory */
> +
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +static int pmbus_debugfs_get(void *data, u64 *val)
> +{
> + struct pmbus_debugfs_entry *entry = data;
> +
> + *val = _pmbus_read_byte_data(entry->client, entry->page, entry->reg);
> +
> + return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops, pmbus_debugfs_get, NULL,
> + "0x%02llx\n");
> +
> +static int pmbus_debugfs_get_status(void *data, u64 *val)
> +{
> + struct pmbus_debugfs_entry *entry = data;
> + struct pmbus_data *pdata = i2c_get_clientdata(entry->client);
> +
> + *val = pdata->read_status(entry->client, entry->page);
> +
> + return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops_status, pmbus_debugfs_get_status,
> + NULL, "0x%04llx\n");
> +
> +static int pmbus_init_debugfs(struct i2c_client *client,
> + struct pmbus_data *data)
> +{
> + int i, idx = 0;
> + char name[PMBUS_NAME_SIZE];
> + struct pmbus_debugfs_entry *entries;
> +
> + /* Check if client driver requested debugfs support. */
> + if (!data->info->debugfs)
> + return 0;
> +
Let's just make this unconditional and drop the flag. I don't see the value
in enabling it per driver. The code is there anyway, might as well support it.
> + /* Create the root pmbus debugfs directory if it's not created yet. */
> + if (!pmbus_debugfs_dir) {
> + pmbus_debugfs_dir = debugfs_create_dir("pmbus", NULL);
> + if (IS_ERR_OR_NULL(pmbus_debugfs_dir)) {
> + pmbus_debugfs_dir = NULL;
> + return -ENODEV;
> + }
> + }
> +
> + /*
> + * Create the debugfs directory for this device. Use the hwmon device
> + * name to avoid conflicts (hwmon numbers are globally unique).
> + */
> + data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
> + pmbus_debugfs_dir);
> + if (IS_ERR_OR_NULL(data->debugfs)) {
> + data->debugfs = NULL;
> + return -ENODEV;
> + }
> +
> + /* Allocate the max possible entries we need. */
> + entries = devm_kzalloc(data->dev,
> + sizeof(*entries) * (data->info->pages * 10),
> + GFP_KERNEL);
> + if (!entries)
> + return -ENOMEM;
> +
> + for (i = 0; i < data->info->pages; ++i) {
> + /* Check accessibility of status register if it's not page 0 */
> + if (!i || pmbus_check_status_register(client, i)) {
> + /* No need to set reg as we have special read op. */
> + entries[idx].client = client;
> + entries[idx].page = i;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
scnprintf might be better.
> + debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops_status);
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
> + entries[idx].client = client;
> + entries[idx].page = i;
> + entries[idx].reg = PMBUS_STATUS_VOUT;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
> + debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops);
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
> + entries[idx].client = client;
> + entries[idx].page = i;
> + entries[idx].reg = PMBUS_STATUS_IOUT;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
> + debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops);
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
> + entries[idx].client = client;
> + entries[idx].page = i;
> + entries[idx].reg = PMBUS_STATUS_INPUT;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
> + debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops);
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
> + entries[idx].client = client;
> + entries[idx].page = i;
> + entries[idx].reg = PMBUS_STATUS_TEMPERATURE;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
> + debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops);
> + }
> +
> + if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
> + entries[idx].client = client;
> + entries[idx].page = i;
> + entries[idx].reg = PMBUS_STATUS_CML;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
> + debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops);
> + }
> +
> + if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) {
> + entries[idx].client = client;
> + entries[idx].page = i;
> + entries[idx].reg = PMBUS_STATUS_OTHER;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
> + debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops);
> + }
> +
> + if (pmbus_check_byte_register(client, i,
> + PMBUS_STATUS_MFR_SPECIFIC)) {
> + entries[idx].client = client;
> + entries[idx].page = i;
> + entries[idx].reg = PMBUS_STATUS_MFR_SPECIFIC;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
> + debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops);
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
> + entries[idx].client = client;
> + entries[idx].page = i;
> + entries[idx].reg = PMBUS_STATUS_FAN_12;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
> + debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops);
> + }
> +
> + if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
> + entries[idx].client = client;
> + entries[idx].page = i;
> + entries[idx].reg = PMBUS_STATUS_FAN_34;
> + snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
> + debugfs_create_file(name, 0444, data->debugfs,
> + &entries[idx++],
> + &pmbus_debugfs_ops);
> + }
> + }
> +
> + return 0;
> +}
> +#else
> +static int pmbus_init_debugfs(struct i2c_client *client,
> + struct pmbus_data *data)
> +{
> + return 0;
> +}
> +#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */
> +
> int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> struct pmbus_driver_info *info)
> {
> @@ -1952,6 +2138,10 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> if (ret)
> goto out_unregister;
>
> + ret = pmbus_init_debugfs(client, data);
> + if (ret)
> + dev_warn(dev, "Failed to register debugfs\n");
> +
> return 0;
>
> out_unregister:
> @@ -1965,6 +2155,17 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> int pmbus_do_remove(struct i2c_client *client)
> {
> struct pmbus_data *data = i2c_get_clientdata(client);
> +
> + debugfs_remove_recursive(data->debugfs);
> + if (pmbus_debugfs_dir && simple_empty(pmbus_debugfs_dir)) {
Almost. Problem is that it is racy against insertions.
We would need a mutex to protect write accesses to pmbus_debugfs_dir.
Wonder if we can just add module_init/module_exit to pmbus_core.c
and initialize/remove the directory from there.
Guenter
> + /*
> + * Remove the root pmbus debugfs directory if we've removed
> + * all the device directories underneath already.
> + */
> + debugfs_remove(pmbus_debugfs_dir);
> + pmbus_debugfs_dir = NULL;
> + }
> +
> hwmon_device_unregister(data->hwmon_dev);
> kfree(data->group.attrs);
> return 0;
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 4/4] hwmon: (pmbus): Add debugfs for status registers
2017-08-11 13:52 ` Guenter Roeck
@ 2017-08-14 14:58 ` Eddie James
0 siblings, 0 replies; 10+ messages in thread
From: Eddie James @ 2017-08-14 14:58 UTC (permalink / raw)
To: Guenter Roeck
Cc: jdelvare, linux-hwmon, linux-kernel, joel, jk, andrew, cbostic,
Edward A. James
On 08/11/2017 08:52 AM, Guenter Roeck wrote:
> On 08/10/2017 02:57 PM, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Export all the available status registers through debugfs, if the client
>> driver wants them.
>>
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>> drivers/hwmon/pmbus/pmbus.h | 6 ++
>> drivers/hwmon/pmbus/pmbus_core.c | 201
>> +++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 207 insertions(+)
>>
>> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
>> index bfcb13b..5f91a19 100644
>> --- a/drivers/hwmon/pmbus/pmbus.h
>> +++ b/drivers/hwmon/pmbus/pmbus.h
>> @@ -383,6 +383,12 @@ struct pmbus_driver_info {
>> /* Regulator functionality, if supported by this chip driver. */
>> int num_regulators;
>> const struct regulator_desc *reg_desc;
>> +
>> + /*
>> + * Controls whether or not to create debugfs entries for this
>> driver's
>> + * devices.
>> + */
>> + bool debugfs;
>> };
>> /* Regulator ops */
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c
>> b/drivers/hwmon/pmbus/pmbus_core.c
>> index ef135d8..b11ebec 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -19,6 +19,7 @@
>> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>> */
>> +#include <linux/debugfs.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/init.h>
>> @@ -101,6 +102,7 @@ struct pmbus_data {
>> int num_attributes;
>> struct attribute_group group;
>> const struct attribute_group *groups[2];
>> + struct dentry *debugfs; /* debugfs device directory */
>> struct pmbus_sensor *sensors;
>> @@ -120,6 +122,12 @@ struct pmbus_data {
>> u8 currpage;
>> };
>> +struct pmbus_debugfs_entry {
>> + struct i2c_client *client;
>> + u8 page;
>> + u8 reg;
>> +};
>> +
>> void pmbus_clear_cache(struct i2c_client *client)
>> {
>> struct pmbus_data *data = i2c_get_clientdata(client);
>> @@ -1893,6 +1901,184 @@ static int pmbus_regulator_register(struct
>> pmbus_data *data)
>> }
>> #endif
>> +static struct dentry *pmbus_debugfs_dir; /* pmbus debugfs
>> directory */
>> +
>> +#if IS_ENABLED(CONFIG_DEBUG_FS)
>> +static int pmbus_debugfs_get(void *data, u64 *val)
>> +{
>> + struct pmbus_debugfs_entry *entry = data;
>> +
>> + *val = _pmbus_read_byte_data(entry->client, entry->page,
>> entry->reg);
>> +
>> + return 0;
>> +}
>> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops, pmbus_debugfs_get, NULL,
>> + "0x%02llx\n");
>> +
>> +static int pmbus_debugfs_get_status(void *data, u64 *val)
>> +{
>> + struct pmbus_debugfs_entry *entry = data;
>> + struct pmbus_data *pdata = i2c_get_clientdata(entry->client);
>> +
>> + *val = pdata->read_status(entry->client, entry->page);
>> +
>> + return 0;
>> +}
>> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops_status,
>> pmbus_debugfs_get_status,
>> + NULL, "0x%04llx\n");
>> +
>> +static int pmbus_init_debugfs(struct i2c_client *client,
>> + struct pmbus_data *data)
>> +{
>> + int i, idx = 0;
>> + char name[PMBUS_NAME_SIZE];
>> + struct pmbus_debugfs_entry *entries;
>> +
>> + /* Check if client driver requested debugfs support. */
>> + if (!data->info->debugfs)
>> + return 0;
>> +
>
> Let's just make this unconditional and drop the flag. I don't see the
> value
> in enabling it per driver. The code is there anyway, might as well
> support it.
>
>> + /* Create the root pmbus debugfs directory if it's not created
>> yet. */
>> + if (!pmbus_debugfs_dir) {
>> + pmbus_debugfs_dir = debugfs_create_dir("pmbus", NULL);
>> + if (IS_ERR_OR_NULL(pmbus_debugfs_dir)) {
>> + pmbus_debugfs_dir = NULL;
>> + return -ENODEV;
>> + }
>> + }
>> +
>> + /*
>> + * Create the debugfs directory for this device. Use the hwmon
>> device
>> + * name to avoid conflicts (hwmon numbers are globally unique).
>> + */
>> + data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
>> + pmbus_debugfs_dir);
>> + if (IS_ERR_OR_NULL(data->debugfs)) {
>> + data->debugfs = NULL;
>> + return -ENODEV;
>> + }
>> +
>> + /* Allocate the max possible entries we need. */
>> + entries = devm_kzalloc(data->dev,
>> + sizeof(*entries) * (data->info->pages * 10),
>> + GFP_KERNEL);
>> + if (!entries)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < data->info->pages; ++i) {
>> + /* Check accessibility of status register if it's not page 0 */
>> + if (!i || pmbus_check_status_register(client, i)) {
>> + /* No need to set reg as we have special read op. */
>> + entries[idx].client = client;
>> + entries[idx].page = i;
>> + snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
>
> scnprintf might be better.
>
>> + debugfs_create_file(name, 0444, data->debugfs,
>> + &entries[idx++],
>> + &pmbus_debugfs_ops_status);
>> + }
>> +
>> + if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
>> + entries[idx].client = client;
>> + entries[idx].page = i;
>> + entries[idx].reg = PMBUS_STATUS_VOUT;
>> + snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
>> + debugfs_create_file(name, 0444, data->debugfs,
>> + &entries[idx++],
>> + &pmbus_debugfs_ops);
>> + }
>> +
>> + if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
>> + entries[idx].client = client;
>> + entries[idx].page = i;
>> + entries[idx].reg = PMBUS_STATUS_IOUT;
>> + snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
>> + debugfs_create_file(name, 0444, data->debugfs,
>> + &entries[idx++],
>> + &pmbus_debugfs_ops);
>> + }
>> +
>> + if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
>> + entries[idx].client = client;
>> + entries[idx].page = i;
>> + entries[idx].reg = PMBUS_STATUS_INPUT;
>> + snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
>> + debugfs_create_file(name, 0444, data->debugfs,
>> + &entries[idx++],
>> + &pmbus_debugfs_ops);
>> + }
>> +
>> + if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
>> + entries[idx].client = client;
>> + entries[idx].page = i;
>> + entries[idx].reg = PMBUS_STATUS_TEMPERATURE;
>> + snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
>> + debugfs_create_file(name, 0444, data->debugfs,
>> + &entries[idx++],
>> + &pmbus_debugfs_ops);
>> + }
>> +
>> + if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
>> + entries[idx].client = client;
>> + entries[idx].page = i;
>> + entries[idx].reg = PMBUS_STATUS_CML;
>> + snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
>> + debugfs_create_file(name, 0444, data->debugfs,
>> + &entries[idx++],
>> + &pmbus_debugfs_ops);
>> + }
>> +
>> + if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) {
>> + entries[idx].client = client;
>> + entries[idx].page = i;
>> + entries[idx].reg = PMBUS_STATUS_OTHER;
>> + snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
>> + debugfs_create_file(name, 0444, data->debugfs,
>> + &entries[idx++],
>> + &pmbus_debugfs_ops);
>> + }
>> +
>> + if (pmbus_check_byte_register(client, i,
>> + PMBUS_STATUS_MFR_SPECIFIC)) {
>> + entries[idx].client = client;
>> + entries[idx].page = i;
>> + entries[idx].reg = PMBUS_STATUS_MFR_SPECIFIC;
>> + snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
>> + debugfs_create_file(name, 0444, data->debugfs,
>> + &entries[idx++],
>> + &pmbus_debugfs_ops);
>> + }
>> +
>> + if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
>> + entries[idx].client = client;
>> + entries[idx].page = i;
>> + entries[idx].reg = PMBUS_STATUS_FAN_12;
>> + snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
>> + debugfs_create_file(name, 0444, data->debugfs,
>> + &entries[idx++],
>> + &pmbus_debugfs_ops);
>> + }
>> +
>> + if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
>> + entries[idx].client = client;
>> + entries[idx].page = i;
>> + entries[idx].reg = PMBUS_STATUS_FAN_34;
>> + snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
>> + debugfs_create_file(name, 0444, data->debugfs,
>> + &entries[idx++],
>> + &pmbus_debugfs_ops);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +#else
>> +static int pmbus_init_debugfs(struct i2c_client *client,
>> + struct pmbus_data *data)
>> +{
>> + return 0;
>> +}
>> +#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */
>> +
>> int pmbus_do_probe(struct i2c_client *client, const struct
>> i2c_device_id *id,
>> struct pmbus_driver_info *info)
>> {
>> @@ -1952,6 +2138,10 @@ int pmbus_do_probe(struct i2c_client *client,
>> const struct i2c_device_id *id,
>> if (ret)
>> goto out_unregister;
>> + ret = pmbus_init_debugfs(client, data);
>> + if (ret)
>> + dev_warn(dev, "Failed to register debugfs\n");
>> +
>> return 0;
>> out_unregister:
>> @@ -1965,6 +2155,17 @@ int pmbus_do_probe(struct i2c_client *client,
>> const struct i2c_device_id *id,
>> int pmbus_do_remove(struct i2c_client *client)
>> {
>> struct pmbus_data *data = i2c_get_clientdata(client);
>> +
>> + debugfs_remove_recursive(data->debugfs);
>> + if (pmbus_debugfs_dir && simple_empty(pmbus_debugfs_dir)) {
>
> Almost. Problem is that it is racy against insertions.
> We would need a mutex to protect write accesses to pmbus_debugfs_dir.
> Wonder if we can just add module_init/module_exit to pmbus_core.c
> and initialize/remove the directory from there.
Got it, this worked well. Will send up v3 for this patch. Thanks for
merging the others!
Eddie
>
> Guenter
>
>> + /*
>> + * Remove the root pmbus debugfs directory if we've removed
>> + * all the device directories underneath already.
>> + */
>> + debugfs_remove(pmbus_debugfs_dir);
>> + pmbus_debugfs_dir = NULL;
>> + }
>> +
>> hwmon_device_unregister(data->hwmon_dev);
>> kfree(data->group.attrs);
>> return 0;
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread