Re: [RFC] hwmon: (ibmpowernv) Add support for reset-history sensors

[Guenter Roeck]

On 07/25/2017 10:55 PM, Shilpasri G Bhat wrote:

In P9, OCC allows for clearing the sensor min-max history. This patch
exports attribute to reset history when set will clear the history of
all the sensors owned by CSM and belonging to the chip.

Signed-off-by: Shilpasri G Bhat 
---
This patch is on top of this patchset:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1453891.html

This patch creates a non-standard attribute called as reset_historyX
which clears the lowest and highest of all the sensors like power,
temperature, voltage belonging to the chip.

  drivers/hwmon/ibmpowernv.c | 31 +++
  1 file changed, 31 insertions(+)

diff --git a/drivers/hwmon/ibmpowernv.c b/drivers/hwmon/ibmpowernv.c
index 5ccdd0b..611e472 100644
--- a/drivers/hwmon/ibmpowernv.c
+++ b/drivers/hwmon/ibmpowernv.c
@@ -51,6 +51,7 @@ enum sensors {
POWER_SUPPLY,
POWER_INPUT,
CURRENT,
+   RESET_HISTORY,
MAX_SENSOR_TYPE,
  };
  
@@ -78,6 +79,7 @@ enum sensors {

{ "in"},
{ "power" },
{ "curr"  },
+   { "reset_history" },
  };
  
  struct sensor_data {

@@ -126,6 +128,25 @@ static ssize_t show_label(struct device *dev, struct 
device_attribute *devattr,
return sprintf(buf, "%s\n", sdata->label);
  }
  
+static ssize_t store_reset_history(struct device *dev,

+  struct device_attribute *devattr,
+  const char *buf, size_t count)
+{
+   struct sensor_data *sdata = container_of(devattr, struct sensor_data,
+dev_attr);
+   int rc;
+   int reset;
+
+   rc = kstrtoint(buf, 0, );
+   if (rc)
+   return rc;
+
+   if (reset == 1)
+   rc = opal_sensor_groups_clear_history(sdata->id);
+


Which of course doesn't exist, so it is hard to determine if this is
all that is offered.

I'd rather stick with the existing ABI, which resets the history either
per sensor(inX_reset_history) or per group (in_reset_history).

Guenter


+   return rc ? rc : count;
+}
+
  static int __init get_logical_cpu(int hwcpu)
  {
int cpu;
@@ -458,6 +479,16 @@ static int create_device_attrs(struct platform_device 
*pdev)
  
  		create_hwmon_attr([count], attr_name, show_sensor);
  
+		if (type == RESET_HISTORY) {

+   snprintf(sdata[count].name, MAX_ATTR_LEN, "%s%d",
+sensor_groups[type].name,
+sdata[count].hwmon_index);
+
+   sdata[count].dev_attr.attr.mode = 0220;
+   sdata[count].dev_attr.store = store_reset_history;
+   sdata[count].dev_attr.show = NULL;
+   }
+
pgroups[type]->attrs[sensor_groups[type].attr_count++] =
[count++].dev_attr.attr;
  



--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [v3, 1/2] hwmon: (it87) Split out chip registers setting code on probe path

[Guenter Roeck]

On Wed, Aug 02, 2017 at 01:06:23AM +0200, Maciej S. Szmigiero wrote:
> This commit splits out chip registers setting code on probe path to
> separate functions so they can be reused for setting the device properly
> again when system resumes from suspend.
> 
> While we are at it let's also make clear that on IT8720 and IT8782 it's
> the VCCH5V line that is (possibly) routed to in7.
> This will make it consistent with a similar message that it printed on
> IT8783.
> 
> Signed-off-by: Maciej S. Szmigiero 

I finally got to this and tried to apply it, but it fails spectacularly.
Even patch fails to apply each and every chunk. No idea what is going on.
How did you generate the patch, and what was your baseline ?

Guenter

> ---
> Changes from v1: Move code of common probe / resume steps to new functions
> so we don't need to make large parts of probe function conditional on a
> newly added 'resume' parameter.
> 
> Changes from v2: Code move of common probe / resume steps to new functions
> and actual resume functionality split into two, separate patches.
> 
> Made a message about VCCH5V being routed to in7 consistent across all
> chips.
> 
>   drivers/hwmon/it87.c | 138 
> ---
>   1 file changed, 88 insertions(+), 50 deletions(-)
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> diff --git a/drivers/hwmon/it87.c b/drivers/hwmon/it87.c
> index 4dfc7238313e..818f123ac475 100644
> --- a/drivers/hwmon/it87.c
> +++ b/drivers/hwmon/it87.c
> @@ -2761,13 +2761,13 @@ static int __init it87_find(int sioaddr, unsigned 
> short *address,
>   uart6 = sio_data->type == it8782 && (reg & BIT(2));
>   
>   /*
> -  * The IT8720F has no VIN7 pin, so VCCH should always be
> +  * The IT8720F has no VIN7 pin, so VCCH5V should always be
>* routed internally to VIN7 with an internal divider.
>* Curiously, there still is a configuration bit to control
>* this, which means it can be set incorrectly. And even
>* more curiously, many boards out there are improperly
>* configured, even though the IT8720F datasheet claims
> -  * that the internal routing of VCCH to VIN7 is the default
> +  * that the internal routing of VCCH5V to VIN7 is the default
>* setting. So we force the internal routing in this case.
>*
>* On IT8782F, VIN7 is multiplexed with one of the UART6 pins.
> @@ -2777,7 +2777,7 @@ static int __init it87_find(int sioaddr, unsigned short 
> *address,
>   if ((sio_data->type == it8720 || uart6) && !(reg & BIT(1))) {
>   reg |= BIT(1);
>   superio_outb(sioaddr, IT87_SIO_PINX2_REG, reg);
> - pr_notice("Routing internal VCCH to in7\n");
> + pr_notice("Routing internal VCCH5V to in7\n");
>   }
>   if (reg & BIT(0))
>   sio_data->internal |= BIT(0);
> @@ -2828,13 +2828,89 @@ static int __init it87_find(int sioaddr, unsigned 
> short *address,
>   return err;
>   }
>   
> +/*
> + * Some chips seem to have default value 0xff for all limit
> + * registers. For low voltage limits it makes no sense and triggers
> + * alarms, so change to 0 instead. For high temperature limits, it
> + * means -1 degree C, which surprisingly doesn't trigger an alarm,
> + * but is still confusing, so change to 127 degrees C.
> + */
> +static void it87_check_limit_regs(struct it87_data *data)
> +{
> + int i, reg;
> +
> + for (i = 0; i < NUM_VIN_LIMIT; i++) {
> + reg = it87_read_value(data, IT87_REG_VIN_MIN(i));
> + if (reg == 0xff)
> + it87_write_value(data, IT87_REG_VIN_MIN(i), 0);
> + }
> + for (i = 0; i < NUM_TEMP_LIMIT; i++) {
> + reg = it87_read_value(data, IT87_REG_TEMP_HIGH(i));
> + if (reg == 0xff)
> + it87_write_value(data, IT87_REG_TEMP_HIGH(i), 127);
> + }
> +}
> +
> +/* Check if voltage monitors are reset manually or by some reason */
> +static void it87_check_voltage_monitors_reset(struct it87_data *data)
> +{
> + int reg;
> +
> + reg = it87_read_value(data, IT87_REG_VIN_ENABLE);
> + if ((reg & 0xff) == 0) {
> + /* Enable all voltage monitors */
> + it87_write_value(data, IT87_REG_VIN_ENABLE, 0xff);
> + }
> +}
> +
> +/* Check if tachometers are reset manually or by some reason */
> +static void it87_check_tachometers_reset(struct platform_device *pdev)
> +{
> + struct it87_sio_data *sio_data = dev_get_platdata(>dev);
> + struct it87_data *data = platform_get_drvdata(pdev);
> + u8 mask, fan_main_ctrl;
> +
> + mask = 

Re: [v2] hwmon: adt7475: constify attribute_group structures

[Arvind Yadav]

yes, Sorry for noise. After rebase, it' was showing unchanged.


On Wednesday 09 August 2017 07:35 AM, Guenter Roeck wrote:

On Mon, Aug 07, 2017 at 03:06:29PM +0530, Arvind Yadav wrote:

attribute_group are not supposed to change at runtime. All functions
working with attribute_group provided by  work with
const attribute_group. So mark the non-const structs as const.

Signed-off-by: Arvind Yadav 

Kind of confused by this one. I applied a similar patch from you
back in July ??

Guenter


---
change in v2:
 subject was not correct. Removed 'wusbhc' and '.'.

  drivers/hwmon/adt7475.c | 16 
  1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 1baa213..9ef8499 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -1319,14 +1319,14 @@ static SENSOR_DEVICE_ATTR_2(pwm3_stall_disable, S_IRUGO 
| S_IWUSR,
NULL
  };
  
-static struct attribute_group adt7475_attr_group = { .attrs = adt7475_attrs };

-static struct attribute_group fan4_attr_group = { .attrs = fan4_attrs };
-static struct attribute_group pwm2_attr_group = { .attrs = pwm2_attrs };
-static struct attribute_group in0_attr_group = { .attrs = in0_attrs };
-static struct attribute_group in3_attr_group = { .attrs = in3_attrs };
-static struct attribute_group in4_attr_group = { .attrs = in4_attrs };
-static struct attribute_group in5_attr_group = { .attrs = in5_attrs };
-static struct attribute_group vid_attr_group = { .attrs = vid_attrs };
+static const struct attribute_group adt7475_attr_group = { .attrs = 
adt7475_attrs };
+static const struct attribute_group fan4_attr_group = { .attrs = fan4_attrs };
+static const struct attribute_group pwm2_attr_group = { .attrs = pwm2_attrs };
+static const struct attribute_group in0_attr_group = { .attrs = in0_attrs };
+static const struct attribute_group in3_attr_group = { .attrs = in3_attrs };
+static const struct attribute_group in4_attr_group = { .attrs = in4_attrs };
+static const struct attribute_group in5_attr_group = { .attrs = in5_attrs };
+static const struct attribute_group vid_attr_group = { .attrs = vid_attrs };
  
  static int adt7475_detect(struct i2c_client *client,

  struct i2c_board_info *info)

~arvind
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [8/9, v2] hwmon: (core) constify thermal_zone_of_device_ops structures

[Guenter Roeck]

On Tue, Aug 08, 2017 at 05:09:02PM +0200, Julia Lawall wrote:
> The thermal_zone_of_device_ops structure is only passed as the fourth
> argument to devm_thermal_zone_of_sensor_register, which is declared
> as const.  Thus the thermal_zone_of_device_ops structure itself can
> be const.
> 
> Done with the help of Coccinelle.
> 
> Signed-off-by: Julia Lawall 

Applied to hwmon-next.

Thanks,
Guenter

> ---
> 
> v2: New patch
> 
>  drivers/hwmon/hwmon.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index 2ac578a..c9790e2 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -135,7 +135,7 @@ static int hwmon_thermal_get_temp(void *data, int *temp)
>   return 0;
>  }
>  
> -static struct thermal_zone_of_device_ops hwmon_thermal_ops = {
> +static const struct thermal_zone_of_device_ops hwmon_thermal_ops = {
>   .get_temp = hwmon_thermal_get_temp,
>  };
>  
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC, 1/3] drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by default

[Guenter Roeck]

On Mon, Aug 07, 2017 at 10:25:46PM -0500, eaja...@linux.vnet.ibm.com wrote:
> From: "Edward A. James" 
> 
> Pmbus core always reads byte data from the status register, even if
> configured to use STATUS_WORD. Use a function pointer so we always do
> either byte or word access depending on which register we're trying to
> access. Also switch to use STATUS_WORD by default.
> 
> Signed-off-by: Edward A. James 
> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 69 
> +++-
>  1 file changed, 54 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c 
> b/drivers/hwmon/pmbus/pmbus_core.c
> index f1eff6b..8511aba 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.
>*/
>   u8 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,36 @@ static bool pmbus_check_register(struct i2c_client 
> *client,
>   return rv >= 0;
>  }
>  
> +/* Check specified page status register accessibility. Need a separate

I am not in favor of networking-style multi-line comments.

> + * function rather than the two above so we can use the correct status
> + * register, and we can optimize out the second status register read.
> + */
> +static bool pmbus_check_status_register(struct i2c_client *client, int page)
> +{
> + int status, rc = 0;
> + struct pmbus_data *data = i2c_get_clientdata(client);
> +
> + status = data->read_status(client, page);
> + if (status < 0)
> + goto out;
> +
> + if (!(data->flags & PMBUS_SKIP_STATUS_CHECK)) {
> + if (status & PB_STATUS_CML) {
> + status = _pmbus_read_byte_data(client, -1,
> +PMBUS_STATUS_CML);
> + if (status < 0 ||
> + (status & PB_CML_FAULT_INVALID_COMMAND))
> + goto out;
> + }
> + }
> +
> + rc = 1;

If you want to use bool, please use bool.

> +
> +out:
> + pmbus_clear_fault_page(client, -1);
> + return rc;
> +}
> +
>  bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg)
>  {
>   return pmbus_check_register(client, _pmbus_read_byte_data, page, reg);
> @@ -393,9 +424,8 @@ static struct pmbus_data *pmbus_update_device(struct 
> device *dev)
>   int i, j;
>  
>   for (i = 0; i < info->pages; i++) {
> - data->status[PB_STATUS_BASE + i]
> - = _pmbus_read_byte_data(client, i,
> - data->status_register);
> + data->status[PB_STATUS_BASE + i] =
> + data->read_status(client, i);

This doesn't really work anymore, since the status register now has to hold 16
bit. Sure, the next patch tries to fix that, but that leaves a 1-patch gap.

Changing the width of data->status to 16 bit results in a "waste" of a whopping
32 * 6 + 2 or around 200 bytes. I don't think that warrants the complexity you
are introducing with the next patch. Are you sure that adds less than 200 bytes
of code ?)

>   for (j = 0; j < ARRAY_SIZE(pmbus_status); j++) {
>   struct _pmbus_status *s = _status[j];
>  
> @@ -1051,8 +1081,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 +1758,16 @@ static int pmbus_identify_common(struct i2c_client 
> *client,
>   return 0;
>  }
>  
> +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_read_status_byte(struct 

Re: [RFC 0/3] drivers/hwmon/pmbus: Use STATUS_WORD and add status sensors

[Guenter Roeck]

On Tue, Aug 08, 2017 at 02:26:15PM -0500, Christopher Bostic wrote:
> 
> 
> On 8/8/17 11:12 AM, Eddie James wrote:
> >
> >
> >On 08/07/2017 11:00 PM, Guenter Roeck wrote:
> >>On 08/07/2017 08:25 PM, Eddie James wrote:
> >>>From: "Edward A. James" 
> >>>
> >>>Hi Guenter,
> >>>
> >>>I'm looking for some feedback for some extensions to the pmbus core.
> >>>We're
> >>>looking for some additional functionality, particularly with
> >>>STATUS_WORD and
> >>>obtaining raw status data.
> >>>
> >>>The first two patches enable the use of the STATUS_WORD register in
> >>>the pmbus
> >>>core. This allows the use of more default alarm/fault attributes for
> >>>default
> >>>pmbus sensors by allowing the use of the higher byte status bits.
> >>>
> >>>The third patch adds "status" attributes to each class of hwmon sensor
> >>>created
> >>>by pmbus. For example, in1_status and temp1_status. These will display
> >>>the
> >>>associated raw status register (e.g. STATUS_INPUT and
> >>>STATUS_TEMPERATURE). I
> >>>realize this is not really "normal" for hwmon or pmbus. These are
> >>>potentially
> >>>very useful in hardware diagnostic situations where it might be
> >>>impossible
> >>>to tell the origin of a failure from a simple alarm or fault bit set.
> >>>We really
> >>>want to access the status registers, and for a multi-page pmbus
> >>>device, this is
> >>>pretty tricky from userspace.
> >>>
> >>>Please let me know your thoughts,
> >>>Thanks,
> >>
> >>I don't mind providing such data with debugfs, for example, but I don't
> >>see
> >>the point in providing it as part of the ABI. Which, in part, since it
> >>requires
> >>a lot of thought on my side, is part of the reason why I didn't provide
> >>feedback to your earlier patches yet. Sorry, I've been exceptionally
> >>busy
> >>lately, and non-standard requests tend to end up at the end of the queue
> >>:-(.
> Hi Guenter,
> 
> In this case we're pulling fail data out of the hardware to diagnose what
> happened, so just wanted to verify that debugfs is still ok. I was assuming
> it was for internal kernel debugging and not hardware fault logging.
> 
News to me, but who am I to know. Do you have a reference for that ?

debugfs is often used to display register contents. Are you sure it is even
possible to clearly distinguish kernel debugging from hardware fault logging
(or, rather, in this case from displaying raw status register values) ?

Anyway, debugfs is fine with me.

Guenter

> Thanks,
> Chris
> 
> 
> >
> >No problem! Thanks for the quick response on this.
> >
> >>
> >>Any reason why debugfs is not sufficient and/or acceptable for your use
> >>case ?
> >>You _are_ talking about diagnostic situations, which seems to be an
> >>exact fit
> >>for debugfs.
> >
> >Agreed, great idea, I think debugfs will work perfectly. I probably should
> >have thought of that sooner...
> >
> >How about the first two patches in the series? They are unrelated to
> >adding any attributes. Mainly I would
> >like to have the PB_STATUS_INPUT bit available to trigger the default
> >boolean alarm attribute, as our hardware doesn't support any limits.
> >
> >Thanks again,
> >Eddie
> >
> >>
> >>Guenter
> >>
> >>>
> >>>Edward A. James (3):
> >>>   drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by
> >>> default
> >>>   drivers/hmwon/pmbus: store STATUS_WORD in status registers
> >>>   drivers/hwmon/pmbus: Add sensor status to pmbus attributes
> >>>
> >>>  drivers/hwmon/pmbus/pmbus_core.c | 153
> >>>+--
> >>>  1 file changed, 130 insertions(+), 23 deletions(-)
> >>>
> >>
> >
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/3] drivers/hwmon/pmbus: Use STATUS_WORD and add status sensors

[Christopher Bostic]

On 8/8/17 11:12 AM, Eddie James wrote:



On 08/07/2017 11:00 PM, Guenter Roeck wrote:

On 08/07/2017 08:25 PM, Eddie James wrote:

From: "Edward A. James" 

Hi Guenter,

I'm looking for some feedback for some extensions to the pmbus core. 
We're
looking for some additional functionality, particularly with 
STATUS_WORD and

obtaining raw status data.

The first two patches enable the use of the STATUS_WORD register in 
the pmbus
core. This allows the use of more default alarm/fault attributes for 
default

pmbus sensors by allowing the use of the higher byte status bits.

The third patch adds "status" attributes to each class of hwmon 
sensor created
by pmbus. For example, in1_status and temp1_status. These will 
display the
associated raw status register (e.g. STATUS_INPUT and 
STATUS_TEMPERATURE). I
realize this is not really "normal" for hwmon or pmbus. These are 
potentially
very useful in hardware diagnostic situations where it might be 
impossible
to tell the origin of a failure from a simple alarm or fault bit 
set. We really
want to access the status registers, and for a multi-page pmbus 
device, this is

pretty tricky from userspace.

Please let me know your thoughts,
Thanks,


I don't mind providing such data with debugfs, for example, but I 
don't see
the point in providing it as part of the ABI. Which, in part, since 
it requires

a lot of thought on my side, is part of the reason why I didn't provide
feedback to your earlier patches yet. Sorry, I've been exceptionally 
busy
lately, and non-standard requests tend to end up at the end of the 
queue :-(.

Hi Guenter,

In this case we're pulling fail data out of the hardware to diagnose 
what happened, so just wanted to verify that debugfs is still ok. I was 
assuming it was for internal kernel debugging and not hardware fault 
logging.


Thanks,
Chris




No problem! Thanks for the quick response on this.



Any reason why debugfs is not sufficient and/or acceptable for your 
use case ?
You _are_ talking about diagnostic situations, which seems to be an 
exact fit

for debugfs.


Agreed, great idea, I think debugfs will work perfectly. I probably 
should have thought of that sooner...


How about the first two patches in the series? They are unrelated to 
adding any attributes. Mainly I would
like to have the PB_STATUS_INPUT bit available to trigger the default 
boolean alarm attribute, as our hardware doesn't support any limits.


Thanks again,
Eddie



Guenter



Edward A. James (3):
   drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by
 default
   drivers/hmwon/pmbus: store STATUS_WORD in status registers
   drivers/hwmon/pmbus: Add sensor status to pmbus attributes

  drivers/hwmon/pmbus/pmbus_core.c | 153 
+--

  1 file changed, 130 insertions(+), 23 deletions(-)







--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/3] drivers/hwmon/pmbus: Use STATUS_WORD and add status sensors

[Eddie James]

On 08/07/2017 11:00 PM, Guenter Roeck wrote:

On 08/07/2017 08:25 PM, Eddie James wrote:

From: "Edward A. James" 

Hi Guenter,

I'm looking for some feedback for some extensions to the pmbus core. 
We're
looking for some additional functionality, particularly with 
STATUS_WORD and

obtaining raw status data.

The first two patches enable the use of the STATUS_WORD register in 
the pmbus
core. This allows the use of more default alarm/fault attributes for 
default

pmbus sensors by allowing the use of the higher byte status bits.

The third patch adds "status" attributes to each class of hwmon 
sensor created
by pmbus. For example, in1_status and temp1_status. These will 
display the
associated raw status register (e.g. STATUS_INPUT and 
STATUS_TEMPERATURE). I
realize this is not really "normal" for hwmon or pmbus. These are 
potentially
very useful in hardware diagnostic situations where it might be 
impossible
to tell the origin of a failure from a simple alarm or fault bit set. 
We really
want to access the status registers, and for a multi-page pmbus 
device, this is

pretty tricky from userspace.

Please let me know your thoughts,
Thanks,


I don't mind providing such data with debugfs, for example, but I 
don't see
the point in providing it as part of the ABI. Which, in part, since it 
requires

a lot of thought on my side, is part of the reason why I didn't provide
feedback to your earlier patches yet. Sorry, I've been exceptionally busy
lately, and non-standard requests tend to end up at the end of the 
queue :-(.


No problem! Thanks for the quick response on this.



Any reason why debugfs is not sufficient and/or acceptable for your 
use case ?
You _are_ talking about diagnostic situations, which seems to be an 
exact fit

for debugfs.


Agreed, great idea, I think debugfs will work perfectly. I probably 
should have thought of that sooner...


How about the first two patches in the series? They are unrelated to 
adding any attributes. Mainly I would
like to have the PB_STATUS_INPUT bit available to trigger the default 
boolean alarm attribute, as our hardware doesn't support any limits.


Thanks again,
Eddie



Guenter



Edward A. James (3):
   drivers/hwmon/pmbus: Access word data for STATUS_WORD and use it by
 default
   drivers/hmwon/pmbus: store STATUS_WORD in status registers
   drivers/hwmon/pmbus: Add sensor status to pmbus attributes

  drivers/hwmon/pmbus/pmbus_core.c | 153 
+--

  1 file changed, 130 insertions(+), 23 deletions(-)





--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/9 v2] constify thermal_zone_of_device_ops structures

[Julia Lawall]

The thermal_zone_of_device_ops structures are only passed as the fourth
argument to thermal_zone_of_sensor_register or
devm_thermal_zone_of_sensor_register, both of which are declared as const.
Thus the thermal_zone_of_device_ops structures themselves can be const.

v2: add structures passed to devm_thermal_zone_of_sensor_register also.

Done with the help of Coccinelle.

---

 drivers/hwmon/hwmon.c  |2 +-
 drivers/hwmon/scpi-hwmon.c |2 +-
 drivers/input/touchscreen/sun4i-ts.c   |2 +-
 drivers/thermal/broadcom/bcm2835_thermal.c |2 +-
 drivers/thermal/hisi_thermal.c |2 +-
 drivers/thermal/qoriq_thermal.c|2 +-
 drivers/thermal/rcar_gen3_thermal.c|2 +-
 drivers/thermal/samsung/exynos_tmu.c   |2 +-
 drivers/thermal/zx2967_thermal.c   |2 +-
 9 files changed, 9 insertions(+), 9 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 9/9 v2] hwmon: (scpi) constify thermal_zone_of_device_ops structures

[Julia Lawall]

The thermal_zone_of_device_ops structure is only passed as the fourth
argument to devm_thermal_zone_of_sensor_register, which is declared
as const.  Thus the thermal_zone_of_device_ops structure itself can
be const.

Done with the help of Coccinelle.

Signed-off-by: Julia Lawall 

---

v2: New patch

 drivers/hwmon/scpi-hwmon.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c
index a586480..7e49da5 100644
--- a/drivers/hwmon/scpi-hwmon.c
+++ b/drivers/hwmon/scpi-hwmon.c
@@ -120,7 +120,7 @@ static int scpi_read_temp(void *dev, int *temp)
return sprintf(buf, "%s\n", sensor->info.name);
 }
 
-static struct thermal_zone_of_device_ops scpi_sensor_ops = {
+static const struct thermal_zone_of_device_ops scpi_sensor_ops = {
.get_temp = scpi_read_temp,
 };
 

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html