[PATCH] hwmon: g762: handle cleanup with devm_add_action

[Peng Hao]

Simplify code and use devm_add_action() to handle cleanup.

Signed-off-by: Peng Hao 
---
 drivers/hwmon/g762.c | 30 ++
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/g762.c b/drivers/hwmon/g762.c
index 6d1208b..48e60d8 100644
--- a/drivers/hwmon/g762.c
+++ b/drivers/hwmon/g762.c
@@ -128,7 +128,6 @@ enum g762_regs {
 G762_REG_FAN_CMD2_GEAR_MODE_1)) >> 2))
 
 struct g762_data {
-   struct device *hwmon_dev;
struct i2c_client *client;
struct clk *clk;
 
@@ -1051,9 +1050,17 @@ static inline int g762_fan_init(struct device *dev)
 data->fan_cmd1);
 }
 
+static void g762_remove(void *data)
+{
+   struct g762_data *g762 = data;
+   struct i2c_client *client = g762->client;
+
+   g762_of_clock_disable(client);
+}
 static int g762_probe(struct i2c_client *client, const struct i2c_device_id 
*id)
 {
struct device *dev = >dev;
+   struct device *hwmon_dev;
struct g762_data *data;
int ret;
 
@@ -1086,10 +1093,12 @@ static int g762_probe(struct i2c_client *client, const 
struct i2c_device_id *id)
if (ret)
goto clock_dis;
 
-   data->hwmon_dev = hwmon_device_register_with_groups(dev, client->name,
-   data, g762_groups);
-   if (IS_ERR(data->hwmon_dev)) {
-   ret = PTR_ERR(data->hwmon_dev);
+   devm_add_action(dev, g762_remove, data);
+
+   hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
+  data, g762_groups);
+   if (IS_ERR(hwmon_dev)) {
+   ret = PTR_ERR(hwmon_dev);
goto clock_dis;
}
 
@@ -1101,23 +1110,12 @@ static int g762_probe(struct i2c_client *client, const 
struct i2c_device_id *id)
return ret;
 }
 
-static int g762_remove(struct i2c_client *client)
-{
-   struct g762_data *data = i2c_get_clientdata(client);
-
-   hwmon_device_unregister(data->hwmon_dev);
-   g762_of_clock_disable(client);
-
-   return 0;
-}
-
 static struct i2c_driver g762_driver = {
.driver = {
.name = DRVNAME,
.of_match_table = of_match_ptr(g762_dt_match),
},
.probe= g762_probe,
-   .remove   = g762_remove,
.id_table = g762_id,
 };
 
-- 
1.8.3.1

--
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: [PATCH 05/10] hwmon: generic-pwm-tachometer: Add generic PWM based tachometer

[Mikko Perttunen]

On 02/28/2018 08:12 AM, Rajkumar Rampelli wrote:


On Wednesday 28 February 2018 11:28 AM, Guenter Roeck wrote:

On 02/27/2018 09:38 PM, Rajkumar Rampelli wrote:


On Wednesday 21 February 2018 08:20 PM, Guenter Roeck wrote:

On 02/20/2018 10:58 PM, Rajkumar Rampelli wrote:

Add generic PWM based tachometer driver via HWMON interface
to report the RPM of motor. This drivers get the period/duty
cycle from PWM IP which captures the motor PWM output.

This driver implements a simple interface for monitoring the speed of
a fan and exposes it in roatations per minute (RPM) to the user space
by using the hwmon's sysfs interface

Signed-off-by: Rajkumar Rampelli 
---
  Documentation/hwmon/generic-pwm-tachometer |  17 +
  drivers/hwmon/Kconfig  |  10 +++
  drivers/hwmon/Makefile |   1 +
  drivers/hwmon/generic-pwm-tachometer.c | 112 
+

  4 files changed, 140 insertions(+)
  create mode 100644 Documentation/hwmon/generic-pwm-tachometer
  create mode 100644 drivers/hwmon/generic-pwm-tachometer.c

diff --git a/Documentation/hwmon/generic-pwm-tachometer 
b/Documentation/hwmon/generic-pwm-tachometer

new file mode 100644
index 000..e0713ee
--- /dev/null
+++ b/Documentation/hwmon/generic-pwm-tachometer
@@ -0,0 +1,17 @@
+Kernel driver generic-pwm-tachometer
+
+
+This driver enables the use of a PWM module to monitor a fan. It 
uses the
+generic PWM interface and can be used on SoCs as along as the SoC 
supports

+Tachometer controller that moniors the Fan speed in periods.
+
+Author: Rajkumar Rampelli 
+
+Description
+---
+
+The driver implements a simple interface for monitoring the Fan 
speed using
+PWM module and Tachometer controller. It requests period value 
through PWM
+capture interface to Tachometer and measures the Rotations per 
minute using
+received period value. It exposes the Fan speed in RPM to the user 
space by

+using the hwmon's sysfs interface.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ef23553..8912dcb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1878,6 +1878,16 @@ config SENSORS_XGENE
    If you say yes here you get support for the temperature
    and power sensors for APM X-Gene SoC.
  +config GENERIC_PWM_TACHOMETER
+    tristate "Generic PWM based tachometer driver"
+    depends on PWM
+    help
+  Enables a driver to use PWM signal from motor to use
+  for measuring the motor speed. The RPM is captured by
+  PWM modules which has PWM capture capability and this
+  drivers reads the captured data from PWM IP to convert
+  it to speed in RPM.
+
  if ACPI
    comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index f814b4a..9dcc374 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -175,6 +175,7 @@ obj-$(CONFIG_SENSORS_WM8350)    += wm8350-hwmon.o
  obj-$(CONFIG_SENSORS_XGENE)    += xgene-hwmon.o
    obj-$(CONFIG_PMBUS)    += pmbus/
+obj-$(CONFIG_GENERIC_PWM_TACHOMETER) += generic-pwm-tachometer.o
    ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
  diff --git a/drivers/hwmon/generic-pwm-tachometer.c 
b/drivers/hwmon/generic-pwm-tachometer.c

new file mode 100644
index 000..9354d43
--- /dev/null
+++ b/drivers/hwmon/generic-pwm-tachometer.c
@@ -0,0 +1,112 @@
+/*
+ * Copyright (c) 2017-2018, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct pwm_hwmon_tach {
+    struct device    *dev;
+    struct pwm_device    *pwm;
+    struct device    *hwmon;
+};
+
+static ssize_t show_rpm(struct device *dev, struct 
device_attribute *attr,

+    char *buf)
+{
+    struct pwm_hwmon_tach *ptt = dev_get_drvdata(dev);
+    struct pwm_device *pwm = ptt->pwm;
+    struct pwm_capture result;
+    int err;
+    unsigned int rpm = 0;
+
+    err = pwm_capture(pwm, , 0);
+    if (err < 0) {
+    dev_err(ptt->dev, "Failed to capture PWM: %d\n", err);
+    return err;
+    }
+
+    if (result.period)
+    rpm = DIV_ROUND_CLOSEST_ULL(60ULL * NSEC_PER_SEC,
+    result.period);
+
+    return sprintf(buf, "%u\n", rpm);
+}
+
+static SENSOR_DEVICE_ATTR(rpm, 0444, show_rpm, NULL, 0);
+
+static struct attribute *pwm_tach_attrs[] = {
+    _dev_attr_rpm.dev_attr.attr,
+    NULL,
+};


"rpm" is not a standard hwmon sysfs attribute. If you don't provide
a single 

Re: [PATCH 05/10] hwmon: generic-pwm-tachometer: Add generic PWM based tachometer

[Rajkumar Rampelli]

On Wednesday 28 February 2018 11:28 AM, Guenter Roeck wrote:

On 02/27/2018 09:38 PM, Rajkumar Rampelli wrote:


On Wednesday 21 February 2018 08:20 PM, Guenter Roeck wrote:

On 02/20/2018 10:58 PM, Rajkumar Rampelli wrote:

Add generic PWM based tachometer driver via HWMON interface
to report the RPM of motor. This drivers get the period/duty
cycle from PWM IP which captures the motor PWM output.

This driver implements a simple interface for monitoring the speed of
a fan and exposes it in roatations per minute (RPM) to the user space
by using the hwmon's sysfs interface

Signed-off-by: Rajkumar Rampelli 
---
  Documentation/hwmon/generic-pwm-tachometer |  17 +
  drivers/hwmon/Kconfig  |  10 +++
  drivers/hwmon/Makefile |   1 +
  drivers/hwmon/generic-pwm-tachometer.c | 112 
+

  4 files changed, 140 insertions(+)
  create mode 100644 Documentation/hwmon/generic-pwm-tachometer
  create mode 100644 drivers/hwmon/generic-pwm-tachometer.c

diff --git a/Documentation/hwmon/generic-pwm-tachometer 
b/Documentation/hwmon/generic-pwm-tachometer

new file mode 100644
index 000..e0713ee
--- /dev/null
+++ b/Documentation/hwmon/generic-pwm-tachometer
@@ -0,0 +1,17 @@
+Kernel driver generic-pwm-tachometer
+
+
+This driver enables the use of a PWM module to monitor a fan. It 
uses the
+generic PWM interface and can be used on SoCs as along as the SoC 
supports

+Tachometer controller that moniors the Fan speed in periods.
+
+Author: Rajkumar Rampelli 
+
+Description
+---
+
+The driver implements a simple interface for monitoring the Fan 
speed using
+PWM module and Tachometer controller. It requests period value 
through PWM
+capture interface to Tachometer and measures the Rotations per 
minute using
+received period value. It exposes the Fan speed in RPM to the user 
space by

+using the hwmon's sysfs interface.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ef23553..8912dcb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1878,6 +1878,16 @@ config SENSORS_XGENE
If you say yes here you get support for the temperature
and power sensors for APM X-Gene SoC.
  +config GENERIC_PWM_TACHOMETER
+tristate "Generic PWM based tachometer driver"
+depends on PWM
+help
+  Enables a driver to use PWM signal from motor to use
+  for measuring the motor speed. The RPM is captured by
+  PWM modules which has PWM capture capability and this
+  drivers reads the captured data from PWM IP to convert
+  it to speed in RPM.
+
  if ACPI
comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index f814b4a..9dcc374 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -175,6 +175,7 @@ obj-$(CONFIG_SENSORS_WM8350)+= wm8350-hwmon.o
  obj-$(CONFIG_SENSORS_XGENE)+= xgene-hwmon.o
obj-$(CONFIG_PMBUS)+= pmbus/
+obj-$(CONFIG_GENERIC_PWM_TACHOMETER) += generic-pwm-tachometer.o
ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
  diff --git a/drivers/hwmon/generic-pwm-tachometer.c 
b/drivers/hwmon/generic-pwm-tachometer.c

new file mode 100644
index 000..9354d43
--- /dev/null
+++ b/drivers/hwmon/generic-pwm-tachometer.c
@@ -0,0 +1,112 @@
+/*
+ * Copyright (c) 2017-2018, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct pwm_hwmon_tach {
+struct device*dev;
+struct pwm_device*pwm;
+struct device*hwmon;
+};
+
+static ssize_t show_rpm(struct device *dev, struct 
device_attribute *attr,

+char *buf)
+{
+struct pwm_hwmon_tach *ptt = dev_get_drvdata(dev);
+struct pwm_device *pwm = ptt->pwm;
+struct pwm_capture result;
+int err;
+unsigned int rpm = 0;
+
+err = pwm_capture(pwm, , 0);
+if (err < 0) {
+dev_err(ptt->dev, "Failed to capture PWM: %d\n", err);
+return err;
+}
+
+if (result.period)
+rpm = DIV_ROUND_CLOSEST_ULL(60ULL * NSEC_PER_SEC,
+result.period);
+
+return sprintf(buf, "%u\n", rpm);
+}
+
+static SENSOR_DEVICE_ATTR(rpm, 0444, show_rpm, NULL, 0);
+
+static struct attribute *pwm_tach_attrs[] = {
+_dev_attr_rpm.dev_attr.attr,
+NULL,
+};


"rpm" is not a standard hwmon sysfs attribute. If you don't provide
a single standard hwmon sysfs attribute, having a hwmon driver is 

Re: [PATCH 05/10] hwmon: generic-pwm-tachometer: Add generic PWM based tachometer

[Guenter Roeck]

On 02/27/2018 09:38 PM, Rajkumar Rampelli wrote:


On Wednesday 21 February 2018 08:20 PM, Guenter Roeck wrote:

On 02/20/2018 10:58 PM, Rajkumar Rampelli wrote:

Add generic PWM based tachometer driver via HWMON interface
to report the RPM of motor. This drivers get the period/duty
cycle from PWM IP which captures the motor PWM output.

This driver implements a simple interface for monitoring the speed of
a fan and exposes it in roatations per minute (RPM) to the user space
by using the hwmon's sysfs interface

Signed-off-by: Rajkumar Rampelli 
---
  Documentation/hwmon/generic-pwm-tachometer |  17 +
  drivers/hwmon/Kconfig  |  10 +++
  drivers/hwmon/Makefile |   1 +
  drivers/hwmon/generic-pwm-tachometer.c | 112 +
  4 files changed, 140 insertions(+)
  create mode 100644 Documentation/hwmon/generic-pwm-tachometer
  create mode 100644 drivers/hwmon/generic-pwm-tachometer.c

diff --git a/Documentation/hwmon/generic-pwm-tachometer 
b/Documentation/hwmon/generic-pwm-tachometer
new file mode 100644
index 000..e0713ee
--- /dev/null
+++ b/Documentation/hwmon/generic-pwm-tachometer
@@ -0,0 +1,17 @@
+Kernel driver generic-pwm-tachometer
+
+
+This driver enables the use of a PWM module to monitor a fan. It uses the
+generic PWM interface and can be used on SoCs as along as the SoC supports
+Tachometer controller that moniors the Fan speed in periods.
+
+Author: Rajkumar Rampelli 
+
+Description
+---
+
+The driver implements a simple interface for monitoring the Fan speed using
+PWM module and Tachometer controller. It requests period value through PWM
+capture interface to Tachometer and measures the Rotations per minute using
+received period value. It exposes the Fan speed in RPM to the user space by
+using the hwmon's sysfs interface.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ef23553..8912dcb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1878,6 +1878,16 @@ config SENSORS_XGENE
    If you say yes here you get support for the temperature
    and power sensors for APM X-Gene SoC.
  +config GENERIC_PWM_TACHOMETER
+    tristate "Generic PWM based tachometer driver"
+    depends on PWM
+    help
+  Enables a driver to use PWM signal from motor to use
+  for measuring the motor speed. The RPM is captured by
+  PWM modules which has PWM capture capability and this
+  drivers reads the captured data from PWM IP to convert
+  it to speed in RPM.
+
  if ACPI
    comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index f814b4a..9dcc374 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -175,6 +175,7 @@ obj-$(CONFIG_SENSORS_WM8350)    += wm8350-hwmon.o
  obj-$(CONFIG_SENSORS_XGENE)    += xgene-hwmon.o
    obj-$(CONFIG_PMBUS)    += pmbus/
+obj-$(CONFIG_GENERIC_PWM_TACHOMETER) += generic-pwm-tachometer.o
    ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
  diff --git a/drivers/hwmon/generic-pwm-tachometer.c 
b/drivers/hwmon/generic-pwm-tachometer.c
new file mode 100644
index 000..9354d43
--- /dev/null
+++ b/drivers/hwmon/generic-pwm-tachometer.c
@@ -0,0 +1,112 @@
+/*
+ * Copyright (c) 2017-2018, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct pwm_hwmon_tach {
+    struct device    *dev;
+    struct pwm_device    *pwm;
+    struct device    *hwmon;
+};
+
+static ssize_t show_rpm(struct device *dev, struct device_attribute *attr,
+    char *buf)
+{
+    struct pwm_hwmon_tach *ptt = dev_get_drvdata(dev);
+    struct pwm_device *pwm = ptt->pwm;
+    struct pwm_capture result;
+    int err;
+    unsigned int rpm = 0;
+
+    err = pwm_capture(pwm, , 0);
+    if (err < 0) {
+    dev_err(ptt->dev, "Failed to capture PWM: %d\n", err);
+    return err;
+    }
+
+    if (result.period)
+    rpm = DIV_ROUND_CLOSEST_ULL(60ULL * NSEC_PER_SEC,
+    result.period);
+
+    return sprintf(buf, "%u\n", rpm);
+}
+
+static SENSOR_DEVICE_ATTR(rpm, 0444, show_rpm, NULL, 0);
+
+static struct attribute *pwm_tach_attrs[] = {
+    _dev_attr_rpm.dev_attr.attr,
+    NULL,
+};


"rpm" is not a standard hwmon sysfs attribute. If you don't provide
a single standard hwmon sysfs attribute, having a hwmon driver is pointless.

Guenter Roeck,
I will define a new hwmon sysfs attribute node called 

Re: [PATCH 05/10] hwmon: generic-pwm-tachometer: Add generic PWM based tachometer

[Rajkumar Rampelli]

On Wednesday 21 February 2018 08:20 PM, Guenter Roeck wrote:

On 02/20/2018 10:58 PM, Rajkumar Rampelli wrote:

Add generic PWM based tachometer driver via HWMON interface
to report the RPM of motor. This drivers get the period/duty
cycle from PWM IP which captures the motor PWM output.

This driver implements a simple interface for monitoring the speed of
a fan and exposes it in roatations per minute (RPM) to the user space
by using the hwmon's sysfs interface

Signed-off-by: Rajkumar Rampelli 
---
  Documentation/hwmon/generic-pwm-tachometer |  17 +
  drivers/hwmon/Kconfig  |  10 +++
  drivers/hwmon/Makefile |   1 +
  drivers/hwmon/generic-pwm-tachometer.c | 112 
+

  4 files changed, 140 insertions(+)
  create mode 100644 Documentation/hwmon/generic-pwm-tachometer
  create mode 100644 drivers/hwmon/generic-pwm-tachometer.c

diff --git a/Documentation/hwmon/generic-pwm-tachometer 
b/Documentation/hwmon/generic-pwm-tachometer

new file mode 100644
index 000..e0713ee
--- /dev/null
+++ b/Documentation/hwmon/generic-pwm-tachometer
@@ -0,0 +1,17 @@
+Kernel driver generic-pwm-tachometer
+
+
+This driver enables the use of a PWM module to monitor a fan. It 
uses the
+generic PWM interface and can be used on SoCs as along as the SoC 
supports

+Tachometer controller that moniors the Fan speed in periods.
+
+Author: Rajkumar Rampelli 
+
+Description
+---
+
+The driver implements a simple interface for monitoring the Fan 
speed using
+PWM module and Tachometer controller. It requests period value 
through PWM
+capture interface to Tachometer and measures the Rotations per 
minute using
+received period value. It exposes the Fan speed in RPM to the user 
space by

+using the hwmon's sysfs interface.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ef23553..8912dcb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1878,6 +1878,16 @@ config SENSORS_XGENE
If you say yes here you get support for the temperature
and power sensors for APM X-Gene SoC.
  +config GENERIC_PWM_TACHOMETER
+tristate "Generic PWM based tachometer driver"
+depends on PWM
+help
+  Enables a driver to use PWM signal from motor to use
+  for measuring the motor speed. The RPM is captured by
+  PWM modules which has PWM capture capability and this
+  drivers reads the captured data from PWM IP to convert
+  it to speed in RPM.
+
  if ACPI
comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index f814b4a..9dcc374 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -175,6 +175,7 @@ obj-$(CONFIG_SENSORS_WM8350)+= wm8350-hwmon.o
  obj-$(CONFIG_SENSORS_XGENE)+= xgene-hwmon.o
obj-$(CONFIG_PMBUS)+= pmbus/
+obj-$(CONFIG_GENERIC_PWM_TACHOMETER) += generic-pwm-tachometer.o
ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
  diff --git a/drivers/hwmon/generic-pwm-tachometer.c 
b/drivers/hwmon/generic-pwm-tachometer.c

new file mode 100644
index 000..9354d43
--- /dev/null
+++ b/drivers/hwmon/generic-pwm-tachometer.c
@@ -0,0 +1,112 @@
+/*
+ * Copyright (c) 2017-2018, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or 
modify it

+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of 
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
License for

+ * more details.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct pwm_hwmon_tach {
+struct device*dev;
+struct pwm_device*pwm;
+struct device*hwmon;
+};
+
+static ssize_t show_rpm(struct device *dev, struct device_attribute 
*attr,

+char *buf)
+{
+struct pwm_hwmon_tach *ptt = dev_get_drvdata(dev);
+struct pwm_device *pwm = ptt->pwm;
+struct pwm_capture result;
+int err;
+unsigned int rpm = 0;
+
+err = pwm_capture(pwm, , 0);
+if (err < 0) {
+dev_err(ptt->dev, "Failed to capture PWM: %d\n", err);
+return err;
+}
+
+if (result.period)
+rpm = DIV_ROUND_CLOSEST_ULL(60ULL * NSEC_PER_SEC,
+result.period);
+
+return sprintf(buf, "%u\n", rpm);
+}
+
+static SENSOR_DEVICE_ATTR(rpm, 0444, show_rpm, NULL, 0);
+
+static struct attribute *pwm_tach_attrs[] = {
+_dev_attr_rpm.dev_attr.attr,
+NULL,
+};


"rpm" is not a standard hwmon sysfs attribute. If you don't provide
a single standard hwmon sysfs attribute, having a hwmon driver is 
pointless.

Guenter Roeck,
I will define a new hwmon sysfs attribute node called 
"hwmon_tachometer_attributes" 

Re: [RFC 4/4] input: misc: Add Gateworks System Controller support

[Dmitry Torokhov]

Hi Tim,

On Tue, Feb 27, 2018 at 05:21:14PM -0800, Tim Harvey wrote:
> Add support for dispatching Linux Input events for the various interrupts
> the Gateworks System Controller provides.
> 
> Signed-off-by: Tim Harvey 
> ---
>  drivers/input/misc/Kconfig |   6 ++
>  drivers/input/misc/Makefile|   1 +
>  drivers/input/misc/gsc-input.c | 196 
> +
>  3 files changed, 203 insertions(+)
>  create mode 100644 drivers/input/misc/gsc-input.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 9f082a3..3d18a0e 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -117,6 +117,12 @@ config INPUT_E3X0_BUTTON
> To compile this driver as a module, choose M here: the
> module will be called e3x0_button.
>  
> +config INPUT_GSC
> + tristate "Gateworks System Controller input support"
> + depends on MFD_GSC
> + help
> +   Say Y here if you want Gateworks System Controller input support.
> +

"To compile this driver as a module..."

>  config INPUT_PCSPKR
>   tristate "PC Speaker support"
>   depends on PCSPKR_PLATFORM
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 4b6118d..969debe 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -38,6 +38,7 @@ obj-$(CONFIG_INPUT_GP2A)+= gp2ap002a00f.o
>  obj-$(CONFIG_INPUT_GPIO_BEEPER)  += gpio-beeper.o
>  obj-$(CONFIG_INPUT_GPIO_TILT_POLLED) += gpio_tilt_polled.o
>  obj-$(CONFIG_INPUT_GPIO_DECODER) += gpio_decoder.o
> +obj-$(CONFIG_INPUT_GSC)  += gsc-input.o
>  obj-$(CONFIG_INPUT_HISI_POWERKEY)+= hisi_powerkey.o
>  obj-$(CONFIG_HP_SDC_RTC) += hp_sdc_rtc.o
>  obj-$(CONFIG_INPUT_IMS_PCU)  += ims-pcu.o
> diff --git a/drivers/input/misc/gsc-input.c b/drivers/input/misc/gsc-input.c
> new file mode 100644
> index 000..7cf217c
> --- /dev/null
> +++ b/drivers/input/misc/gsc-input.c
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Gateworks Corporation
> + */

Let's keep the same // comment block fir the copyright notice as well.
An one-line describing what this driver is would be appreciated too.

> +#define DEBUG

Please no.

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +struct gsc_irq {
> + unsigned int irq;
> + const char *name;
> + unsigned int virq;
> +};
> +
> +static struct gsc_irq gsc_irqs[] = {
> + { GSC_IRQ_PB,   "button" },
> + { GSC_IRQ_KEY_ERASED,   "key-erased" },
> + { GSC_IRQ_EEPROM_WP,"eeprom-wp" },
> + { GSC_IRQ_TAMPER,   "tamper" },
> + { GSC_IRQ_SWITCH_HOLD,  "button-held" },
> +};
> +
> +struct gsc_input_info {
> + struct device *dev;
> + struct gsc_dev *gsc;
> + struct input_dev *input;
> +
> + int irq;
> + struct work_struct irq_work;
> + struct mutex mutex;
> +};
> +
> +static void gsc_input_irq_work(struct work_struct *work)
> +{
> + struct gsc_input_info *info = container_of(work, struct gsc_input_info,
> +irq_work);
> + struct gsc_dev *gsc = info->gsc;
> + int i, ret = 0;
> + int key, sts;
> + struct gsc_irq *gsc_irq = NULL;
> +
> + dev_dbg(gsc->dev, "%s irq%d\n", __func__, info->irq);
> + mutex_lock(>mutex);
> +
> + for (i = 0; i < ARRAY_SIZE(gsc_irqs);i++)
> + if (info->irq == gsc_irqs[i].virq)
> + gsc_irq = _irqs[i];
> + if (!gsc_irq) {
> + dev_err(info->dev, "interrupt: irq%d occurred\n", info->irq);
> + mutex_unlock(>mutex);
> + return;
> + }
> +
> + ret = regmap_read(info->gsc->regmap, GSC_IRQ_STATUS, );

Why is this needed? To clear irq? What happens if several events happen
at the same time? Do we lose one of them?

> + if (ret) {
> + dev_err(info->dev, "failed to read status register\n");
> + mutex_unlock(>mutex);
> + return;
> + }
> +
> + key = -1;
> + switch (gsc_irq->virq) {
> + case GSC_IRQ_PB:
> + key = BTN_0;
> + break;
> + case GSC_IRQ_KEY_ERASED:
> + key = BTN_1;
> + break;
> + case GSC_IRQ_EEPROM_WP:
> + key = BTN_2;
> + break;
> + case GSC_IRQ_GPIO:
> + key = BTN_3;
> + break;
> + case GSC_IRQ_TAMPER:
> + key = BTN_4;
> + break;
> + case GSC_IRQ_SWITCH_HOLD:
> + key = BTN_5;

Could we provide the mapping in DTS instead of hard-coding them?

> + break;
> + }
> +
> + if (key != -1) {
> + dev_dbg(>input->dev, "bit%d: key=0x%03x %s\n",
> + gsc_irq->virq, key, gsc_irq->name);
> + input_report_key(info->input, key, 1);

input_sync();

> 

Re: [RFC 3/4] hwmon: add Gateworks System Controller support

[Guenter Roeck]

On 02/27/2018 05:21 PM, Tim Harvey wrote:

The Gateworks System Controller has a hwmon sub-component that exposes
up to 16 ADC's, some of which are temperature sensors, others which are
voltage inputs. The ADC configuration (register mapping and name) is
configured via device-tree and varies board to board.

Signed-off-by: Tim Harvey 
---
  drivers/hwmon/Kconfig |   6 +
  drivers/hwmon/Makefile|   1 +
  drivers/hwmon/gsc-hwmon.c | 299 ++
  3 files changed, 306 insertions(+)
  create mode 100644 drivers/hwmon/gsc-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7ad0176..9cdc3cb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -475,6 +475,12 @@ config SENSORS_F75375S
  This driver can also be built as a module.  If so, the module
  will be called f75375s.
  
+config SENSORS_GSC

+tristate "Gateworks System Controller ADC"
+depends on MFD_GSC
+help
+  Support for the Gateworks System Controller A/D converters.
+
  config SENSORS_MC13783_ADC
  tristate "Freescale MC13783/MC13892 ADC"
  depends on MFD_MC13XXX
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 0fe489f..835a536 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_SENSORS_G760A)   += g760a.o
  obj-$(CONFIG_SENSORS_G762)+= g762.o
  obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
  obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
+obj-$(CONFIG_SENSORS_GSC)  += gsc-hwmon.o
  obj-$(CONFIG_SENSORS_GPIO_FAN)+= gpio-fan.o
  obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o
  obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o
diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c
new file mode 100644
index 000..3e14bea
--- /dev/null
+++ b/drivers/hwmon/gsc-hwmon.c
@@ -0,0 +1,299 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Gateworks Corporation
+ */
+#define DEBUG


Please no.


+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* map channel to channel info */
+struct gsc_hwmon_ch {
+   u8 reg;
+   char name[32];
+};
+static struct gsc_hwmon_ch gsc_temp_ch[16];


16 temperature channels ...


+static struct gsc_hwmon_ch gsc_in_ch[16];
+static struct gsc_hwmon_ch gsc_fan_ch[5];
+
+static int
+gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+  int ch, long *val)
+{
+   struct gsc_dev *gsc = dev_get_drvdata(dev);
+   int sz, ret;
+   u8 reg;
+   u8 buf[3];
+
+   dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
+   switch (type) {
+   case hwmon_in:
+   sz = 3;
+   reg = gsc_in_ch[ch].reg;
+   break;
+   case hwmon_temp:
+   sz = 2;
+   reg = gsc_temp_ch[ch].reg;
+   break;
+   default:
+   return -EOPNOTSUPP;
+   }
+
+   ret = regmap_bulk_read(gsc->regmap_hwmon, reg, , sz);
+   if (!ret) {
+   *val = 0;
+   while (sz-- > 0)
+   *val |= (buf[sz] << (8*sz));
+   if ((type == hwmon_temp) && *val > 0x8000)


Excessive [and inconsistent] ( )

Please make this
if (ret)
return ret;
...
return 0;


+   *val -= 0x;
+   }
+
+   return ret;
+}
+
+static int
+gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int ch, const char **buf)
+{
+   dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
+   switch (type) {
+   case hwmon_in:
+   case hwmon_temp:
+   case hwmon_fan:
+   switch (attr) {
+   case hwmon_in_label:
+   *buf = gsc_in_ch[ch].name;
+   return 0;
+   break;
+   case hwmon_temp_label:
+   *buf = gsc_temp_ch[ch].name;
+   return 0;
+   break;
+   case hwmon_fan_label:
+   *buf = gsc_fan_ch[ch].name;
+   return 0;
+   break;


return followed by break doesn't make sense.


+   default:
+   break;
+   }
+   break;
+   default:
+   break;
+   }
+
+   return -ENOTSUPP;
+}
+
+static int
+gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+  int ch, long val)
+{
+   struct gsc_dev *gsc = dev_get_drvdata(dev);
+   int ret;
+   u8 reg;
+   u8 buf[3];
+
+   dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
+   switch (type) {
+   case hwmon_fan:
+   buf[0] = val & 0xff;
+   buf[1] = (val >> 8) & 0xff;
+   reg = 

Re: [RFC 2/4] mfd: add Gateworks System Controller core driver

[Randy Dunlap]

On 02/27/2018 05:21 PM, Tim Harvey wrote:
> The Gateworks System Controller (GSC) is an I2C slave controller
> implemented with an MSP430 micro-controller whose firmware embeds the
> following features:
>  - I/O expander (16 GPIO's) using PCA955x protocol
>  - Real Time Clock using DS1672 protocol
>  - User EEPROM using AT24 protocol
>  - HWMON using custom protocol
>  - Interrupt controller with tamper detect, user pushbotton
>  - Watchdog controller capable of full board power-cycle
>  - Power Control capable of full board power-cycle
> 
> see http://trac.gateworks.com/wiki/gsc for more details
> 
> Signed-off-by: Tim Harvey 
> ---
>  drivers/mfd/Kconfig |  10 ++
>  drivers/mfd/Makefile|   1 +
>  drivers/mfd/gsc.c   | 330 
> 
>  include/linux/mfd/gsc.h |  79 
>  4 files changed, 420 insertions(+)
>  create mode 100644 drivers/mfd/gsc.c
>  create mode 100644 include/linux/mfd/gsc.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1d20a80..16dd486 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -341,6 +341,16 @@ config MFD_EXYNOS_LPASS
> Select this option to enable support for Samsung Exynos Low Power
> Audio Subsystem.
>  
> +config MFD_GSC
> + tristate "Gateworks System Controller"
> + depends on (I2C && OF) || COMPILE_TEST

Do both I2C and OF have stubs so that a driver will build when they are
both disabled?  I.e., only COMPILE_TEST is enabled?


> + select MFD_CORE
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + help
> +   Enable support for the Gateworks System Controller found
> +   on Gateworks Single Board Computers.
> +
>  config MFD_MC13XXX
>   tristate
>   depends on (SPI_MASTER || I2C)
thanks,
-- 
~Randy
--
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

[RFC 1/4] dt-bindings: mfd: Add Gateworks System Controller bindings

[Tim Harvey]

This patch adds documentation of device-tree bindings for the
Gateworks System Controller (GSC).

Signed-off-by: Tim Harvey 
---
 Documentation/devicetree/bindings/mfd/gsc.txt | 69 +++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/gsc.txt

diff --git a/Documentation/devicetree/bindings/mfd/gsc.txt 
b/Documentation/devicetree/bindings/mfd/gsc.txt
new file mode 100644
index 000..7671347
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/gsc.txt
@@ -0,0 +1,159 @@
+Gateworks System Controller multi-function device
+
+The GSC is a Multifunction I2C slave device with the following submodules:
+- WDT
+- GPIO
+- Pushbutton controller
+- HWMON
+
+Required properties:
+- compatible : Must be "gw,gsc_v1", "gw,gsc_v2", "gw,gsc_v3"
+- reg: I2C address of the device
+- interrupts: interrupt triggered by GSC_IRQ# signal
+- interrupt-parent: Interrupt controller GSC is connected to
+- #interrupt-cells: should be <1>, index of the interrupt within the
+  controller, in accordance with the "one cell" variant of
+  
+
+Optional nodes:
+* watchdog:
+The GSC provides a Watchdog monitor which can power cycle the board's
+primary power supply on most board models when tripped.
+
+Required properties:
+- compatible: must be "gw,gsc-watchdog"
+
+* input:
+The GSC provides an input device capable of dispatching Linux Input events
+for user pushbutton events, tamper switch events, etc.
+
+Required properties:
+- compatible: must be "gw,gsc-input"
+
+* hwmon:
+The GSC provides a set of Analog to Digitcal Converter (ADC) pins used for
+temperature and/or voltage monitoring.
+
+Required properties:
+- compatible: must be "gw,gsc-hwmon"
+
+Example:
+
+   gsc: gsc@20 {
+   compatible = "gw,gsc_v2";
+   reg = <0x20>;
+   interrupt-parent = <>;
+   interrupts = <4 GPIO_ACTIVE_LOW>;
+   interrupt-controller;
+   #interrupt-cells = <1>;
+
+   gsc_input {
+   compatible = "gw,gsc-input";
+   };
+
+   gsc_watchdog {
+   compatible = "gw,gsc-watchdog";
+   };
+
+   gsc_hwmon {
+   compatible = "gw,gsc-hwmon";
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   hwmon@0 { /* A0: Board Temperature */
+   type = <0>;
+   reg = <0x00>;
+   label = "temp";
+   };
+
+   hwmon@1 { /* A1: Input Voltage */
+   type = <1>;
+   reg = <0x02>;
+   label = "Vin";
+   };
+
+   hwmon@2 { /* A2: 5P0 */
+   type = <1>;
+   reg = <0x0b>;
+   label = "5P0";
+   };
+
+   hwmon@4 { /* A4: 0-5V input */
+   type = <1>;
+   reg = <0x14>;
+   label = "ANL0";
+   };
+
+   hwmon@5 { /* A5: 2P5 PCIe/GigE */
+   type = <1>;
+   reg = <0x23>;
+   label = "2P5";
+   };
+
+   hwmon@6 { /* A6: 1P8 Aud/Vid */
+   type = <1>;
+   reg = <0x1d>;
+   label = "1P8";
+   };
+
+   hwmon@7 { /* A7: GPS */
+   type = <1>;
+   reg = <0x26>;
+   label = "GPS";
+   };
+
+   hwmon@12 { /* A12: VDD_CORE */
+   type = <1>;
+   reg = <0x3>;
+   label = "VDD_CORE";
+   };
+
+   hwmon@13 { /* A13: VDD_SOC */
+   type = <1>;
+   reg = <0x11>;
+   label = "VDD_SOC";
+   };
+
+   hwmon@14 { /* A14: 1P0 PCIe SW */
+   type = <1>;
+   reg = <0x20>;
+   label = "1P0";
+   };
+
+   hwmon@15 { /* fan0 */
+   type = <2>;
+   reg = <0x2c>;
+   label = "fan_50p";
+   };
+
+   hwmon@16 { /* fan1 */
+   type = <2>;
+   reg = <0x2e>;
+   

[RFC 4/4] input: misc: Add Gateworks System Controller support

[Tim Harvey]

Add support for dispatching Linux Input events for the various interrupts
the Gateworks System Controller provides.

Signed-off-by: Tim Harvey 
---
 drivers/input/misc/Kconfig |   6 ++
 drivers/input/misc/Makefile|   1 +
 drivers/input/misc/gsc-input.c | 196 +
 3 files changed, 203 insertions(+)
 create mode 100644 drivers/input/misc/gsc-input.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 9f082a3..3d18a0e 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -117,6 +117,12 @@ config INPUT_E3X0_BUTTON
  To compile this driver as a module, choose M here: the
  module will be called e3x0_button.
 
+config INPUT_GSC
+   tristate "Gateworks System Controller input support"
+   depends on MFD_GSC
+   help
+ Say Y here if you want Gateworks System Controller input support.
+
 config INPUT_PCSPKR
tristate "PC Speaker support"
depends on PCSPKR_PLATFORM
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 4b6118d..969debe 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_INPUT_GP2A)  += gp2ap002a00f.o
 obj-$(CONFIG_INPUT_GPIO_BEEPER)+= gpio-beeper.o
 obj-$(CONFIG_INPUT_GPIO_TILT_POLLED)   += gpio_tilt_polled.o
 obj-$(CONFIG_INPUT_GPIO_DECODER)   += gpio_decoder.o
+obj-$(CONFIG_INPUT_GSC)+= gsc-input.o
 obj-$(CONFIG_INPUT_HISI_POWERKEY)  += hisi_powerkey.o
 obj-$(CONFIG_HP_SDC_RTC)   += hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_IMS_PCU)+= ims-pcu.o
diff --git a/drivers/input/misc/gsc-input.c b/drivers/input/misc/gsc-input.c
new file mode 100644
index 000..7cf217c
--- /dev/null
+++ b/drivers/input/misc/gsc-input.c
@@ -0,0 +1,196 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Gateworks Corporation
+ */
+#define DEBUG
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+struct gsc_irq {
+   unsigned int irq;
+   const char *name;
+   unsigned int virq;
+};
+
+static struct gsc_irq gsc_irqs[] = {
+   { GSC_IRQ_PB,   "button" },
+   { GSC_IRQ_KEY_ERASED,   "key-erased" },
+   { GSC_IRQ_EEPROM_WP,"eeprom-wp" },
+   { GSC_IRQ_TAMPER,   "tamper" },
+   { GSC_IRQ_SWITCH_HOLD,  "button-held" },
+};
+
+struct gsc_input_info {
+   struct device *dev;
+   struct gsc_dev *gsc;
+   struct input_dev *input;
+
+   int irq;
+   struct work_struct irq_work;
+   struct mutex mutex;
+};
+
+static void gsc_input_irq_work(struct work_struct *work)
+{
+   struct gsc_input_info *info = container_of(work, struct gsc_input_info,
+  irq_work);
+   struct gsc_dev *gsc = info->gsc;
+   int i, ret = 0;
+   int key, sts;
+   struct gsc_irq *gsc_irq = NULL;
+
+   dev_dbg(gsc->dev, "%s irq%d\n", __func__, info->irq);
+   mutex_lock(>mutex);
+
+   for (i = 0; i < ARRAY_SIZE(gsc_irqs);i++)
+   if (info->irq == gsc_irqs[i].virq)
+   gsc_irq = _irqs[i];
+   if (!gsc_irq) {
+   dev_err(info->dev, "interrupt: irq%d occurred\n", info->irq);
+   mutex_unlock(>mutex);
+   return;
+   }
+
+   ret = regmap_read(info->gsc->regmap, GSC_IRQ_STATUS, );
+   if (ret) {
+   dev_err(info->dev, "failed to read status register\n");
+   mutex_unlock(>mutex);
+   return;
+   }
+
+   key = -1;
+   switch (gsc_irq->virq) {
+   case GSC_IRQ_PB:
+   key = BTN_0;
+   break;
+   case GSC_IRQ_KEY_ERASED:
+   key = BTN_1;
+   break;
+   case GSC_IRQ_EEPROM_WP:
+   key = BTN_2;
+   break;
+   case GSC_IRQ_GPIO:
+   key = BTN_3;
+   break;
+   case GSC_IRQ_TAMPER:
+   key = BTN_4;
+   break;
+   case GSC_IRQ_SWITCH_HOLD:
+   key = BTN_5;
+   break;
+   }
+
+   if (key != -1) {
+   dev_dbg(>input->dev, "bit%d: key=0x%03x %s\n",
+   gsc_irq->virq, key, gsc_irq->name);
+   input_report_key(info->input, key, 1);
+   input_report_key(info->input, key, 0);
+   input_sync(info->input);
+   }
+
+   mutex_unlock(>mutex);
+}
+
+static irqreturn_t gsc_input_irq(int irq, void *data)
+{
+   struct gsc_input_info *info = data;
+
+   dev_dbg(info->gsc->dev, "%s irq%d\n", __func__, irq);
+   info->irq = irq;
+   schedule_work(>irq_work);
+
+   return IRQ_HANDLED;
+}
+
+static int gsc_input_probe(struct platform_device *pdev)
+{
+   struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
+   struct gsc_input_info *info;
+   struct input_dev *input;
+   int 

[RFC 3/4] hwmon: add Gateworks System Controller support

[Tim Harvey]

The Gateworks System Controller has a hwmon sub-component that exposes
up to 16 ADC's, some of which are temperature sensors, others which are
voltage inputs. The ADC configuration (register mapping and name) is
configured via device-tree and varies board to board.

Signed-off-by: Tim Harvey 
---
 drivers/hwmon/Kconfig |   6 +
 drivers/hwmon/Makefile|   1 +
 drivers/hwmon/gsc-hwmon.c | 299 ++
 3 files changed, 306 insertions(+)
 create mode 100644 drivers/hwmon/gsc-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7ad0176..9cdc3cb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -475,6 +475,12 @@ config SENSORS_F75375S
  This driver can also be built as a module.  If so, the module
  will be called f75375s.
 
+config SENSORS_GSC
+tristate "Gateworks System Controller ADC"
+depends on MFD_GSC
+help
+  Support for the Gateworks System Controller A/D converters.
+
 config SENSORS_MC13783_ADC
 tristate "Freescale MC13783/MC13892 ADC"
 depends on MFD_MC13XXX
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 0fe489f..835a536 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_SENSORS_G760A)   += g760a.o
 obj-$(CONFIG_SENSORS_G762) += g762.o
 obj-$(CONFIG_SENSORS_GL518SM)  += gl518sm.o
 obj-$(CONFIG_SENSORS_GL520SM)  += gl520sm.o
+obj-$(CONFIG_SENSORS_GSC)  += gsc-hwmon.o
 obj-$(CONFIG_SENSORS_GPIO_FAN) += gpio-fan.o
 obj-$(CONFIG_SENSORS_HIH6130)  += hih6130.o
 obj-$(CONFIG_SENSORS_ULTRA45)  += ultra45_env.o
diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c
new file mode 100644
index 000..3e14bea
--- /dev/null
+++ b/drivers/hwmon/gsc-hwmon.c
@@ -0,0 +1,299 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Gateworks Corporation
+ */
+#define DEBUG
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* map channel to channel info */
+struct gsc_hwmon_ch {
+   u8 reg;
+   char name[32];
+};
+static struct gsc_hwmon_ch gsc_temp_ch[16];
+static struct gsc_hwmon_ch gsc_in_ch[16];
+static struct gsc_hwmon_ch gsc_fan_ch[5];
+
+static int
+gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+  int ch, long *val)
+{
+   struct gsc_dev *gsc = dev_get_drvdata(dev);
+   int sz, ret;
+   u8 reg;
+   u8 buf[3];
+
+   dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
+   switch (type) {
+   case hwmon_in:
+   sz = 3;
+   reg = gsc_in_ch[ch].reg;
+   break;
+   case hwmon_temp:
+   sz = 2;
+   reg = gsc_temp_ch[ch].reg;
+   break;
+   default:
+   return -EOPNOTSUPP;
+   }
+
+   ret = regmap_bulk_read(gsc->regmap_hwmon, reg, , sz);
+   if (!ret) {
+   *val = 0;
+   while (sz-- > 0)
+   *val |= (buf[sz] << (8*sz));
+   if ((type == hwmon_temp) && *val > 0x8000)
+   *val -= 0x;
+   }
+
+   return ret;
+}
+
+static int
+gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int ch, const char **buf)
+{
+   dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
+   switch (type) {
+   case hwmon_in:
+   case hwmon_temp:
+   case hwmon_fan:
+   switch (attr) {
+   case hwmon_in_label:
+   *buf = gsc_in_ch[ch].name;
+   return 0;
+   break;
+   case hwmon_temp_label:
+   *buf = gsc_temp_ch[ch].name;
+   return 0;
+   break;
+   case hwmon_fan_label:
+   *buf = gsc_fan_ch[ch].name;
+   return 0;
+   break;
+   default:
+   break;
+   }
+   break;
+   default:
+   break;
+   }
+
+   return -ENOTSUPP;
+}
+
+static int
+gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+  int ch, long val)
+{
+   struct gsc_dev *gsc = dev_get_drvdata(dev);
+   int ret;
+   u8 reg;
+   u8 buf[3];
+
+   dev_dbg(dev, "%s type=%d attr=%d ch=%d\n", __func__, type, attr, ch);
+   switch (type) {
+   case hwmon_fan:
+   buf[0] = val & 0xff;
+   buf[1] = (val >> 8) & 0xff;
+   reg = gsc_fan_ch[ch].reg;
+   ret = regmap_bulk_write(gsc->regmap_hwmon, reg, , 2);
+   break;
+   default:
+   ret = -EOPNOTSUPP;
+   break;
+   }
+
+   return ret;
+}
+
+static umode_t
+gsc_hwmon_is_visible(const void *_data, enum 

[RFC 2/4] mfd: add Gateworks System Controller core driver

[Tim Harvey]

The Gateworks System Controller (GSC) is an I2C slave controller
implemented with an MSP430 micro-controller whose firmware embeds the
following features:
 - I/O expander (16 GPIO's) using PCA955x protocol
 - Real Time Clock using DS1672 protocol
 - User EEPROM using AT24 protocol
 - HWMON using custom protocol
 - Interrupt controller with tamper detect, user pushbotton
 - Watchdog controller capable of full board power-cycle
 - Power Control capable of full board power-cycle

see http://trac.gateworks.com/wiki/gsc for more details

Signed-off-by: Tim Harvey 
---
 drivers/mfd/Kconfig |  10 ++
 drivers/mfd/Makefile|   1 +
 drivers/mfd/gsc.c   | 330 
 include/linux/mfd/gsc.h |  79 
 4 files changed, 420 insertions(+)
 create mode 100644 drivers/mfd/gsc.c
 create mode 100644 include/linux/mfd/gsc.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 1d20a80..16dd486 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -341,6 +341,16 @@ config MFD_EXYNOS_LPASS
  Select this option to enable support for Samsung Exynos Low Power
  Audio Subsystem.
 
+config MFD_GSC
+   tristate "Gateworks System Controller"
+   depends on (I2C && OF) || COMPILE_TEST
+   select MFD_CORE
+   select REGMAP_I2C
+   select REGMAP_IRQ
+   help
+ Enable support for the Gateworks System Controller found
+ on Gateworks Single Board Computers.
+
 config MFD_MC13XXX
tristate
depends on (SPI_MASTER || I2C)
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index d9474ad..aede0bc 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_MFD_CROS_EC) += cros_ec_core.o
 obj-$(CONFIG_MFD_CROS_EC_I2C)  += cros_ec_i2c.o
 obj-$(CONFIG_MFD_CROS_EC_SPI)  += cros_ec_spi.o
 obj-$(CONFIG_MFD_EXYNOS_LPASS) += exynos-lpass.o
+obj-$(CONFIG_MFD_GSC)  += gsc.o
 
 rtsx_pci-objs  := rtsx_pcr.o rts5209.o rts5229.o rtl8411.o 
rts5227.o rts5249.o
 obj-$(CONFIG_MFD_RTSX_PCI) += rtsx_pci.o
diff --git a/drivers/mfd/gsc.c b/drivers/mfd/gsc.c
new file mode 100644
index 000..2fe4174
--- /dev/null
+++ b/drivers/mfd/gsc.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Gateworks Corporation
+ *
+ * The Gateworks System Controller (GSC) is a family of a multi-function
+ * "Power Management and System Companion Device" chips originally designed for
+ * use in Gateworks Single Board Computers. The control interface is I2C,
+ * at 100kbps, with an interrupt.
+ *
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * The GSC suffers from an errata where occasionally during
+ * ADC cycles the chip can NAK i2c transactions. To ensure we have reliable
+ * register access we place retries around register access.
+ */
+#define I2C_RETRIES3
+
+static int gsc_regmap_regwrite(void *context, unsigned int reg,
+  unsigned int val)
+{
+   struct i2c_client *client = context;
+   int retry, ret;
+
+   for (retry = 0; retry < I2C_RETRIES; retry++) {
+   ret = i2c_smbus_write_byte_data(client, reg, val);
+   /*
+* -EAGAIN returned when the i2c host controller is busy
+* -EIO returned when i2c device is busy
+*/
+   if (ret != -EAGAIN && ret != -EIO)
+   break;
+   }
+   if (ret < 0) {
+   dev_err(>dev, ">> 0x%02x %d\n", reg, ret);
+   return ret;
+   }
+   dev_dbg(>dev, ">> 0x%02x=0x%02x (%d)\n", reg, val, retry);
+
+return 0;
+}
+
+static int gsc_regmap_regread(void *context, unsigned int reg,
+  unsigned int *val)
+{
+   struct i2c_client *client = context;
+   int retry, ret;
+
+   for (retry = 0; retry < I2C_RETRIES; retry++) {
+   ret = i2c_smbus_read_byte_data(client, reg);
+   /*
+* -EAGAIN returned when the i2c host controller is busy
+* -EIO returned when i2c device is busy
+*/
+   if (ret != -EAGAIN && ret != -EIO)
+   break;
+   }
+   if (ret < 0) {
+   dev_err(>dev, "<< 0x%02x %d\n", reg, ret);
+   return ret;
+   }
+
+   *val = ret & 0xff;
+   dev_dbg(>dev, "<< 0x%02x=0x%02x (%d)\n", reg, *val, retry);
+
+return 0;
+}
+
+static struct regmap_bus regmap_gsc = {
+.reg_write = gsc_regmap_regwrite,
+.reg_read = gsc_regmap_regread,
+};
+
+/*
+ * gsc_powerdown - API to use GSC to power down board for a specific time
+ *
+ * secs - number of seconds to remain powered off
+ */
+static int gsc_powerdown(struct gsc_dev *gsc, unsigned long secs)
+{
+   int ret;
+   unsigned char regs[4];
+
+   

[RFC 0/4] Add support for the Gateworks System Controller

[Tim Harvey]

This series adds support for the Gateworks System Controller used on Gateworks
Laguna, Ventana, and Newport product families.

The GSC is an MSP430 I2C slave controller whose firmware embeds the following
features:
 - I/O expander (16 GPIO's emulating a PCA955x)
 - EEPROM (enumating AT24)
 - RTC (enumating DS1672)
 - HWMON
 - Interrupt controller with tamper detect, user pushbotton
 - Watchdog controller capable of full board power-cycle
 - Power Control capable of full board power-cycle

see http://trac.gateworks.com/wiki/gsc for more details

Tim Harvey (4):
  dt-bindings: mfd: Add Gateworks System Controller bindings
  mfd: add Gateworks System Controller core driver
  hwmon: add Gateworks System Controller support
  input: misc: Add Gateworks System Controller support

 Documentation/devicetree/bindings/mfd/gsc.txt |  69 ++
 drivers/hwmon/Kconfig |   6 +
 drivers/hwmon/Makefile|   1 +
 drivers/hwmon/gsc-hwmon.c | 299 +++
 drivers/input/misc/Kconfig|   6 +
 drivers/input/misc/Makefile   |   1 +
 drivers/input/misc/gsc-input.c| 196 +++
 drivers/mfd/Kconfig   |  10 +
 drivers/mfd/Makefile  |   1 +
 drivers/mfd/gsc.c | 330 ++
 include/linux/mfd/gsc.h   |  79 ++
 11 files changed, 998 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/gsc.txt
 create mode 100644 drivers/hwmon/gsc-hwmon.c
 create mode 100644 drivers/input/misc/gsc-input.c
 create mode 100644 drivers/mfd/gsc.c
 create mode 100644 include/linux/mfd/gsc.h

-- 
2.7.4

--
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: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 27.02.2018 17:38, Daniel Thompson wrote:
> On Tue, Feb 27, 2018 at 01:40:58PM +0200, Claudiu Beznea wrote:
>> On 27.02.2018 12:54, Daniel Thompson wrote:
>>> On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
 On 26.02.2018 11:57, Jani Nikula wrote:
> On Thu, 22 Feb 2018, Daniel Thompson  wrote:
>> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
>>> Add PWM mode to pwm_config() function. The drivers which uses 
>>> pwm_config()
>>> were adapted to this change.
>>>
>>> Signed-off-by: Claudiu Beznea 
>>> ---
>>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>>>  drivers/bus/ts-nbus.c|  2 +-
>>>  drivers/clk/clk-pwm.c|  3 ++-
>>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>>>  drivers/hwmon/pwm-fan.c  |  2 +-
>>>  drivers/input/misc/max77693-haptic.c |  2 +-
>>>  drivers/input/misc/max8997_haptic.c  |  6 +-
>>>  drivers/leds/leds-pwm.c  |  5 -
>>>  drivers/media/rc/ir-rx51.c   |  5 -
>>>  drivers/media/rc/pwm-ir-tx.c |  5 -
>>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>>>  drivers/video/backlight/lp8788_bl.c  |  5 -
>>>  drivers/video/backlight/pwm_bl.c | 11 +--
>>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>>>  include/linux/pwm.h  |  6 --
>>>  16 files changed, 70 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
>>> b/drivers/video/backlight/lm3630a_bl.c
>>> index 2030a6b77a09..696fa25dafd2 100644
>>> --- a/drivers/video/backlight/lm3630a_bl.c
>>> +++ b/drivers/video/backlight/lm3630a_bl.c
>>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
>>> *pchip, int br, int br_max)
>>>  {
>>> unsigned int period = pchip->pdata->pwm_period;
>>> unsigned int duty = br * period / br_max;
>>> +   struct pwm_caps caps = { };
>>>  
>>> -   pwm_config(pchip->pwmd, duty, period);
>>> +   pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
>>> +   pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
>>
>> Well... I admit I've only really looked at the patches that impact 
>> backlight but dispersing this really odd looking bit twiddling 
>> throughout the kernel doesn't strike me a great API design.
>>
>> IMHO callers should not be required to find the first set bit in
>> some specially crafted set of capability bits simply to get sane 
>> default behaviour.
>
> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> error prone.

 Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be 
 OK
 from your side?

 Or, what about using a function like pwm_mode_first() to get the first 
 supported
 mode by PWM channel?

 Or, would you prefer to solve this inside pwm_config() function, let's 
 say, in
 case an invalid mode is passed as argument, to let pwm_config() to choose 
 the
 first available PWM mode for PWM channel passed as argument?
>>>
>>> What is it that actually needs solving?
>>>
>>> If a driver requests normal mode and the PWM driver cannot support it
>>> why not just return an error an move on.
>> Because, simply, I wasn't aware of what these PWM client drivers needs for.
> 
> I'm afraid you have confused me here.
> 
> Didn't you just *add* the whole concept of PWM caps with your patches?
> How could any existing call site expect anything except normal mode.
> Until now there has been no possiblity to request anything else.
Agree. And agree I was confusing in previous email, sorry about that. And
agree that there was nothing before and everything should work with PWM
normal mode.

When I choose to have BIT(ffs(caps.modes)) instead of PWM_MODE(NORMAL) I
was thinking at having these pwm_config() calls working all the time having
in mind that in future the PWM controllers that these drivers use, might
change in terms of PWM supported modes.

Thank you,
Claudiu Beznea

> 
> 
>>> Put another way, what is the use case for secretly adopting a mode the
>>> caller didn't want? Under what circumstances is this a good thing?
>> No one... But I wasn't aware of what the PWM clients needs for from their PWM
>> controllers. At this moment having BIT(ffs(caps.modes)) instead of
>> PWM_MODE(NORMAL) is mostly the same since all the driver that has not 
>> explicitly
>> registered PWM caps will use PWM normal mode.
>>
>> I will use PWM_MODE(NORMAL) instead of this in all the cases if this is OK 
>> from
>> your side.
>>
>> Thank you,
>> Claudiu Beznea
>>>
>>>
>>> Daniel.
>>>
> 
--
To unsubscribe from this list: send the line "unsubscribe 

Re: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Daniel Thompson]

On Tue, Feb 27, 2018 at 01:40:58PM +0200, Claudiu Beznea wrote:
> On 27.02.2018 12:54, Daniel Thompson wrote:
> > On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
> >> On 26.02.2018 11:57, Jani Nikula wrote:
> >>> On Thu, 22 Feb 2018, Daniel Thompson  wrote:
>  On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> > Add PWM mode to pwm_config() function. The drivers which uses 
> > pwm_config()
> > were adapted to this change.
> >
> > Signed-off-by: Claudiu Beznea 
> > ---
> >  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
> >  drivers/bus/ts-nbus.c|  2 +-
> >  drivers/clk/clk-pwm.c|  3 ++-
> >  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
> >  drivers/hwmon/pwm-fan.c  |  2 +-
> >  drivers/input/misc/max77693-haptic.c |  2 +-
> >  drivers/input/misc/max8997_haptic.c  |  6 +-
> >  drivers/leds/leds-pwm.c  |  5 -
> >  drivers/media/rc/ir-rx51.c   |  5 -
> >  drivers/media/rc/pwm-ir-tx.c |  5 -
> >  drivers/video/backlight/lm3630a_bl.c |  4 +++-
> >  drivers/video/backlight/lp855x_bl.c  |  4 +++-
> >  drivers/video/backlight/lp8788_bl.c  |  5 -
> >  drivers/video/backlight/pwm_bl.c | 11 +--
> >  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
> >  include/linux/pwm.h  |  6 --
> >  16 files changed, 70 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/video/backlight/lm3630a_bl.c 
> > b/drivers/video/backlight/lm3630a_bl.c
> > index 2030a6b77a09..696fa25dafd2 100644
> > --- a/drivers/video/backlight/lm3630a_bl.c
> > +++ b/drivers/video/backlight/lm3630a_bl.c
> > @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
> > *pchip, int br, int br_max)
> >  {
> > unsigned int period = pchip->pdata->pwm_period;
> > unsigned int duty = br * period / br_max;
> > +   struct pwm_caps caps = { };
> >  
> > -   pwm_config(pchip->pwmd, duty, period);
> > +   pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> > +   pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
> 
>  Well... I admit I've only really looked at the patches that impact 
>  backlight but dispersing this really odd looking bit twiddling 
>  throughout the kernel doesn't strike me a great API design.
> 
>  IMHO callers should not be required to find the first set bit in
>  some specially crafted set of capability bits simply to get sane 
>  default behaviour.
> >>>
> >>> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> >>> error prone.
> >>
> >> Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be 
> >> OK
> >> from your side?
> >>
> >> Or, what about using a function like pwm_mode_first() to get the first 
> >> supported
> >> mode by PWM channel?
> >>
> >> Or, would you prefer to solve this inside pwm_config() function, let's 
> >> say, in
> >> case an invalid mode is passed as argument, to let pwm_config() to choose 
> >> the
> >> first available PWM mode for PWM channel passed as argument?
> > 
> > What is it that actually needs solving?
> > 
> > If a driver requests normal mode and the PWM driver cannot support it
> > why not just return an error an move on.
> Because, simply, I wasn't aware of what these PWM client drivers needs for.

I'm afraid you have confused me here.

Didn't you just *add* the whole concept of PWM caps with your patches?
How could any existing call site expect anything except normal mode.
Until now there has been no possiblity to request anything else.


> > Put another way, what is the use case for secretly adopting a mode the
> > caller didn't want? Under what circumstances is this a good thing?
> No one... But I wasn't aware of what the PWM clients needs for from their PWM
> controllers. At this moment having BIT(ffs(caps.modes)) instead of
> PWM_MODE(NORMAL) is mostly the same since all the driver that has not 
> explicitly
> registered PWM caps will use PWM normal mode.
> 
> I will use PWM_MODE(NORMAL) instead of this in all the cases if this is OK 
> from
> your side.
> 
> Thank you,
> Claudiu Beznea
> > 
> > 
> > Daniel.
> > 
--
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: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Claudiu Beznea]

On 27.02.2018 12:54, Daniel Thompson wrote:
> On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
>> On 26.02.2018 11:57, Jani Nikula wrote:
>>> On Thu, 22 Feb 2018, Daniel Thompson  wrote:
 On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> were adapted to this change.
>
> Signed-off-by: Claudiu Beznea 
> ---
>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
>  drivers/bus/ts-nbus.c|  2 +-
>  drivers/clk/clk-pwm.c|  3 ++-
>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
>  drivers/hwmon/pwm-fan.c  |  2 +-
>  drivers/input/misc/max77693-haptic.c |  2 +-
>  drivers/input/misc/max8997_haptic.c  |  6 +-
>  drivers/leds/leds-pwm.c  |  5 -
>  drivers/media/rc/ir-rx51.c   |  5 -
>  drivers/media/rc/pwm-ir-tx.c |  5 -
>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
>  drivers/video/backlight/lp8788_bl.c  |  5 -
>  drivers/video/backlight/pwm_bl.c | 11 +--
>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
>  include/linux/pwm.h  |  6 --
>  16 files changed, 70 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/video/backlight/lm3630a_bl.c 
> b/drivers/video/backlight/lm3630a_bl.c
> index 2030a6b77a09..696fa25dafd2 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
> *pchip, int br, int br_max)
>  {
>   unsigned int period = pchip->pdata->pwm_period;
>   unsigned int duty = br * period / br_max;
> + struct pwm_caps caps = { };
>  
> - pwm_config(pchip->pwmd, duty, period);
> + pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> + pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));

 Well... I admit I've only really looked at the patches that impact 
 backlight but dispersing this really odd looking bit twiddling 
 throughout the kernel doesn't strike me a great API design.

 IMHO callers should not be required to find the first set bit in
 some specially crafted set of capability bits simply to get sane 
 default behaviour.
>>>
>>> Agreed. IMHO the regular use case becomes rather tedious, ugly, and
>>> error prone.
>>
>> Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be OK
>> from your side?
>>
>> Or, what about using a function like pwm_mode_first() to get the first 
>> supported
>> mode by PWM channel?
>>
>> Or, would you prefer to solve this inside pwm_config() function, let's say, 
>> in
>> case an invalid mode is passed as argument, to let pwm_config() to choose the
>> first available PWM mode for PWM channel passed as argument?
> 
> What is it that actually needs solving?
> 
> If a driver requests normal mode and the PWM driver cannot support it
> why not just return an error an move on.
Because, simply, I wasn't aware of what these PWM client drivers needs for.

> 
> Put another way, what is the use case for secretly adopting a mode the
> caller didn't want? Under what circumstances is this a good thing?
No one... But I wasn't aware of what the PWM clients needs for from their PWM
controllers. At this moment having BIT(ffs(caps.modes)) instead of
PWM_MODE(NORMAL) is mostly the same since all the driver that has not explicitly
registered PWM caps will use PWM normal mode.

I will use PWM_MODE(NORMAL) instead of this in all the cases if this is OK from
your side.

Thank you,
Claudiu Beznea
> 
> 
> Daniel.
> 
--
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: [PATCH v3 05/10] pwm: add PWM mode to pwm_config()

[Daniel Thompson]

On Mon, Feb 26, 2018 at 04:24:15PM +0200, Claudiu Beznea wrote:
> On 26.02.2018 11:57, Jani Nikula wrote:
> > On Thu, 22 Feb 2018, Daniel Thompson  wrote:
> >> On Thu, Feb 22, 2018 at 02:01:16PM +0200, Claudiu Beznea wrote:
> >>> Add PWM mode to pwm_config() function. The drivers which uses pwm_config()
> >>> were adapted to this change.
> >>>
> >>> Signed-off-by: Claudiu Beznea 
> >>> ---
> >>>  arch/arm/mach-s3c24xx/mach-rx1950.c  | 11 +--
> >>>  drivers/bus/ts-nbus.c|  2 +-
> >>>  drivers/clk/clk-pwm.c|  3 ++-
> >>>  drivers/gpu/drm/i915/intel_panel.c   | 17 ++---
> >>>  drivers/hwmon/pwm-fan.c  |  2 +-
> >>>  drivers/input/misc/max77693-haptic.c |  2 +-
> >>>  drivers/input/misc/max8997_haptic.c  |  6 +-
> >>>  drivers/leds/leds-pwm.c  |  5 -
> >>>  drivers/media/rc/ir-rx51.c   |  5 -
> >>>  drivers/media/rc/pwm-ir-tx.c |  5 -
> >>>  drivers/video/backlight/lm3630a_bl.c |  4 +++-
> >>>  drivers/video/backlight/lp855x_bl.c  |  4 +++-
> >>>  drivers/video/backlight/lp8788_bl.c  |  5 -
> >>>  drivers/video/backlight/pwm_bl.c | 11 +--
> >>>  drivers/video/fbdev/ssd1307fb.c  |  3 ++-
> >>>  include/linux/pwm.h  |  6 --
> >>>  16 files changed, 70 insertions(+), 21 deletions(-)
> >>>
> >>> diff --git a/drivers/video/backlight/lm3630a_bl.c 
> >>> b/drivers/video/backlight/lm3630a_bl.c
> >>> index 2030a6b77a09..696fa25dafd2 100644
> >>> --- a/drivers/video/backlight/lm3630a_bl.c
> >>> +++ b/drivers/video/backlight/lm3630a_bl.c
> >>> @@ -165,8 +165,10 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip 
> >>> *pchip, int br, int br_max)
> >>>  {
> >>>   unsigned int period = pchip->pdata->pwm_period;
> >>>   unsigned int duty = br * period / br_max;
> >>> + struct pwm_caps caps = { };
> >>>  
> >>> - pwm_config(pchip->pwmd, duty, period);
> >>> + pwm_get_caps(pchip->pwmd->chip, pchip->pwmd, );
> >>> + pwm_config(pchip->pwmd, duty, period, BIT(ffs(caps.modes) - 1));
> >>
> >> Well... I admit I've only really looked at the patches that impact 
> >> backlight but dispersing this really odd looking bit twiddling 
> >> throughout the kernel doesn't strike me a great API design.
> >>
> >> IMHO callers should not be required to find the first set bit in
> >> some specially crafted set of capability bits simply to get sane 
> >> default behaviour.
> > 
> > Agreed. IMHO the regular use case becomes rather tedious, ugly, and
> > error prone.
> 
> Using simply PWM_MODE(NORMAL) instead of BIT(ffs(caps.modes) - 1) would be OK
> from your side?
>
> Or, what about using a function like pwm_mode_first() to get the first 
> supported
> mode by PWM channel?
> 
> Or, would you prefer to solve this inside pwm_config() function, let's say, in
> case an invalid mode is passed as argument, to let pwm_config() to choose the
> first available PWM mode for PWM channel passed as argument?

What is it that actually needs solving?

If a driver requests normal mode and the PWM driver cannot support it
why not just return an error an move on.

Put another way, what is the use case for secretly adopting a mode the
caller didn't want? Under what circumstances is this a good thing?


Daniel.
--
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