* Re: [PATCH v6 0/6] Add MediaTek PMIC keys support
From: Dmitry Torokhov @ 2018-03-29 16:15 UTC (permalink / raw)
To: Lee Jones
Cc: Chen Zhong, Matthias Brugger, Sean Wang, Rob Herring,
Alexandre Belloni, Mark Rutland, a.zummo, devicetree,
linus.walleij, jcsing.lee, linux-kernel, krzk, javier,
linux-mediatek, linux-arm-kernel, linux-input, eddie.huang,
beomho.seo, linux-rtc
In-Reply-To: <20180329130402.6fp7nofyp6iesmbv@dell>
On Thu, Mar 29, 2018 at 02:04:02PM +0100, Lee Jones wrote:
> On Thu, 29 Mar 2018, Chen Zhong wrote:
>
> > On Wed, 2018-03-28 at 11:26 +0100, Lee Jones wrote:
> > > On Tue, 27 Mar 2018, Matthias Brugger wrote:
> > >
> > > >
> > > >
> > > > On 03/27/2018 10:05 AM, Lee Jones wrote:
> > > > > On Fri, 23 Mar 2018, Dmitry Torokhov wrote:
> > > > >> On Thu, Mar 22, 2018 at 10:17:53AM +0800, Sean Wang wrote:
> > > > >>> Hi, Dmitry and Lee
> > > > >>>
> > > > >>> The series seems not being got merged. Are they good enough to be ready
> > > > >>> into the your tree?
> > > > >>>
> > > > >>> Recently I've tested the series with focusing on pwrkey event generated
> > > > >>> through interrupt when push and release the key on bpi-r2 board and then
> > > > >>> finally it's working fine. but for homekey it cannot be found on the
> > > > >>> board and thus I cannot have more tests more about it.
> > > > >>>
> > > > >>> Tested-by: Sean Wang <sean.wang@mediatek.com>
> > > > >>
> > > > >> You have my Ack on the input patch; I expect it go through Lee's tree as
> > > > >> there are some dependencies on mfd core piece.
> > > > >
> > > > > Are you happy for me to merge the Input dt-bindings without your Ack?
> > > > >
> > > >
> > > > They got both Acked in v5, but the commit message was not updated:
> > > > https://patchwork.kernel.org/patch/9973721/
> > > > https://patchwork.kernel.org/patch/9973723/
> > >
> > > Thanks Matthias.
> > >
> > > Chen, can you collect all the Acks and repost as a RESEND please?
> > >
> >
> > Thanks Matthias, Lee, Dmitry and Sean for your comments.
> >
> > Hi Lee,
> >
> > I have collected the Acks by Rob and sent the v6:
> > https://patchwork.kernel.org/patch/10026705
> > https://patchwork.kernel.org/patch/10026707
> >
> > Are they enough to be merged?
>
> Oh, I see.
>
> I was asking about Dmitry's Ack.
Oh, sorry, I did not realize you wanted my Ack for bindings. I usually
leave it to Rob and simply ack the driver itself when I am happy with
the code.
I'll go and add my ack to the binding post if that will help merging
the series.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v6 0/6] Add MediaTek PMIC keys support
From: Lee Jones @ 2018-03-29 13:04 UTC (permalink / raw)
To: Chen Zhong
Cc: Matthias Brugger, Dmitry Torokhov, Sean Wang, Rob Herring,
Alexandre Belloni, Mark Rutland, a.zummo, devicetree,
linus.walleij, jcsing.lee, linux-kernel, krzk, javier,
linux-mediatek, linux-arm-kernel, linux-input, eddie.huang,
beomho.seo, linux-rtc
In-Reply-To: <1522290675.17084.8.camel@mhfsdcap03>
On Thu, 29 Mar 2018, Chen Zhong wrote:
> On Wed, 2018-03-28 at 11:26 +0100, Lee Jones wrote:
> > On Tue, 27 Mar 2018, Matthias Brugger wrote:
> >
> > >
> > >
> > > On 03/27/2018 10:05 AM, Lee Jones wrote:
> > > > On Fri, 23 Mar 2018, Dmitry Torokhov wrote:
> > > >> On Thu, Mar 22, 2018 at 10:17:53AM +0800, Sean Wang wrote:
> > > >>> Hi, Dmitry and Lee
> > > >>>
> > > >>> The series seems not being got merged. Are they good enough to be ready
> > > >>> into the your tree?
> > > >>>
> > > >>> Recently I've tested the series with focusing on pwrkey event generated
> > > >>> through interrupt when push and release the key on bpi-r2 board and then
> > > >>> finally it's working fine. but for homekey it cannot be found on the
> > > >>> board and thus I cannot have more tests more about it.
> > > >>>
> > > >>> Tested-by: Sean Wang <sean.wang@mediatek.com>
> > > >>
> > > >> You have my Ack on the input patch; I expect it go through Lee's tree as
> > > >> there are some dependencies on mfd core piece.
> > > >
> > > > Are you happy for me to merge the Input dt-bindings without your Ack?
> > > >
> > >
> > > They got both Acked in v5, but the commit message was not updated:
> > > https://patchwork.kernel.org/patch/9973721/
> > > https://patchwork.kernel.org/patch/9973723/
> >
> > Thanks Matthias.
> >
> > Chen, can you collect all the Acks and repost as a RESEND please?
> >
>
> Thanks Matthias, Lee, Dmitry and Sean for your comments.
>
> Hi Lee,
>
> I have collected the Acks by Rob and sent the v6:
> https://patchwork.kernel.org/patch/10026705
> https://patchwork.kernel.org/patch/10026707
>
> Are they enough to be merged?
Oh, I see.
I was asking about Dmitry's Ack.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v6 0/6] Add MediaTek PMIC keys support
From: Chen Zhong @ 2018-03-29 2:31 UTC (permalink / raw)
To: Lee Jones
Cc: Matthias Brugger, Dmitry Torokhov, Sean Wang, Rob Herring,
Alexandre Belloni, Mark Rutland, a.zummo, devicetree,
linus.walleij, jcsing.lee, linux-kernel, krzk, javier,
linux-mediatek, linux-arm-kernel, linux-input, eddie.huang,
beomho.seo, linux-rtc
In-Reply-To: <20180328102640.yx6nv5abfexmouuf@dell>
On Wed, 2018-03-28 at 11:26 +0100, Lee Jones wrote:
> On Tue, 27 Mar 2018, Matthias Brugger wrote:
>
> >
> >
> > On 03/27/2018 10:05 AM, Lee Jones wrote:
> > > On Fri, 23 Mar 2018, Dmitry Torokhov wrote:
> > >> On Thu, Mar 22, 2018 at 10:17:53AM +0800, Sean Wang wrote:
> > >>> Hi, Dmitry and Lee
> > >>>
> > >>> The series seems not being got merged. Are they good enough to be ready
> > >>> into the your tree?
> > >>>
> > >>> Recently I've tested the series with focusing on pwrkey event generated
> > >>> through interrupt when push and release the key on bpi-r2 board and then
> > >>> finally it's working fine. but for homekey it cannot be found on the
> > >>> board and thus I cannot have more tests more about it.
> > >>>
> > >>> Tested-by: Sean Wang <sean.wang@mediatek.com>
> > >>
> > >> You have my Ack on the input patch; I expect it go through Lee's tree as
> > >> there are some dependencies on mfd core piece.
> > >
> > > Are you happy for me to merge the Input dt-bindings without your Ack?
> > >
> >
> > They got both Acked in v5, but the commit message was not updated:
> > https://patchwork.kernel.org/patch/9973721/
> > https://patchwork.kernel.org/patch/9973723/
>
> Thanks Matthias.
>
> Chen, can you collect all the Acks and repost as a RESEND please?
>
Thanks Matthias, Lee, Dmitry and Sean for your comments.
Hi Lee,
I have collected the Acks by Rob and sent the v6:
https://patchwork.kernel.org/patch/10026705
https://patchwork.kernel.org/patch/10026707
Are they enough to be merged?
Thank you.
^ permalink raw reply
* Re: [PATCH v7 0/2] hid-steam driver with user mode client dection
From: Rodrigo Rivas Costa @ 2018-03-28 21:44 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Pierre-Loup A. Griffais, Clément VUCHENER, Jiri Kosina,
Cameron Gutman, lkml, linux-input
In-Reply-To: <20180328181448.GA1674@casa>
On Wed, Mar 28, 2018 at 08:14:48PM +0200, Rodrigo Rivas Costa wrote:
> On Mon, Mar 26, 2018 at 10:12:19AM +0200, Benjamin Tissoires wrote:
> > Also, I think there will be races if a user changes the value of the
> > parameter while running the system. You might want to add an
> > additional patch that would trigger the mode change on module
> > parameter change.
>
> True, but the races should be easy to remove, with a simple local
> variable. About the trigger, I've never seen a trigger on module
> parameter change. Can you give me a hint or example of how to do that?
Never mind, I've found it: module_param_cb().
Moreover, I'll have to keep a global list of steam_devices, just in case
the parameter is changed...
Regards.
Rodrigo
^ permalink raw reply
* Re: [PATCH v7 1/2] HID: add driver for Valve Steam Controller
From: Rodrigo Rivas Costa @ 2018-03-28 21:00 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Pierre-Loup A. Griffais, Clément VUCHENER, Jiri Kosina,
Cameron Gutman, lkml, linux-input
In-Reply-To: <CAO-hwJKRuQhSh+_X=R_H32a_VxYm079cu1oUEsjO8iK4mC0goA@mail.gmail.com>
On Mon, Mar 26, 2018 at 11:20:30AM +0200, Benjamin Tissoires wrote:
> Hi Rodrigo,
>
> few comments inlined.
>
> On Sun, Mar 25, 2018 at 6:07 PM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > There are two ways to connect the Steam Controller: directly to the USB
> > or with the USB wireless adapter. Both methods are similar, but the
> > wireless adapter can connect up to 4 devices at the same time.
> >
> > The wired device will appear as 3 interfaces: a virtual mouse, a virtual
> > keyboard and a custom HID device.
> >
> > The wireless device will appear as 5 interfaces: a virtual keyboard and
> > 4 custom HID devices, that will remain silent until a device is actually
> > connected.
> >
> > The custom HID device has a report descriptor with all vendor specific
> > usages, so the hid-generic is not very useful. In a PC/SteamBox Valve
> > Steam Client provices a software translation by using hidraw and a
> > creates a uinput virtual gamepad and XTest keyboard/mouse.
> >
> > This driver intercepts the hidraw usage, so it can get out of the way
> > when the Steam Client is in use.
> >
> > Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
> > ---
> > drivers/hid/Kconfig | 8 +
> > drivers/hid/Makefile | 1 +
> > drivers/hid/hid-ids.h | 4 +
> > drivers/hid/hid-steam.c | 840 ++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/hid.h | 1 +
> > 5 files changed, 854 insertions(+)
> > create mode 100644 drivers/hid/hid-steam.c
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 779c5ae47f36..de5f4849bfe4 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -811,6 +811,14 @@ config HID_SPEEDLINK
> > ---help---
> > Support for Speedlink Vicious and Divine Cezanne mouse.
> >
> > +config HID_STEAM
> > + tristate "Steam Controller support"
> > + depends on HID
> > + ---help---
> > + Say Y here if you have a Steam Controller if you want to use it
> > + without running the Steam Client. It supports both the wired and
> > + the wireless adaptor.
> > +
> > config HID_STEELSERIES
> > tristate "Steelseries SRW-S1 steering wheel support"
> > depends on HID
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 235bd2a7b333..e146c257285a 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -94,6 +94,7 @@ obj-$(CONFIG_HID_SAMSUNG) += hid-samsung.o
> > obj-$(CONFIG_HID_SMARTJOYPLUS) += hid-sjoy.o
> > obj-$(CONFIG_HID_SONY) += hid-sony.o
> > obj-$(CONFIG_HID_SPEEDLINK) += hid-speedlink.o
> > +obj-$(CONFIG_HID_STEAM) += hid-steam.o
> > obj-$(CONFIG_HID_STEELSERIES) += hid-steelseries.o
> > obj-$(CONFIG_HID_SUNPLUS) += hid-sunplus.o
> > obj-$(CONFIG_HID_GREENASIA) += hid-gaff.o
> > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > index a0baa5ba5b84..3014991e5d4b 100644
> > --- a/drivers/hid/hid-ids.h
> > +++ b/drivers/hid/hid-ids.h
> > @@ -987,6 +987,10 @@
> > #define USB_VENDOR_ID_STANTUM_SITRONIX 0x1403
> > #define USB_DEVICE_ID_MTP_SITRONIX 0x5001
> >
> > +#define USB_VENDOR_ID_VALVE 0x28de
> > +#define USB_DEVICE_ID_STEAM_CONTROLLER 0x1102
> > +#define USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS 0x1142
> > +
> > #define USB_VENDOR_ID_STEELSERIES 0x1038
> > #define USB_DEVICE_ID_STEELSERIES_SRWS1 0x1410
> >
> > diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> > new file mode 100644
> > index 000000000000..3504d2e2d0e5
> > --- /dev/null
> > +++ b/drivers/hid/hid-steam.c
> > @@ -0,0 +1,840 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * HID driver for Valve Steam Controller
> > + *
> > + * Copyright (c) 2018 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
> > + *
> > + * Supports both the wired and wireless interfaces.
> > + *
> > + * This controller has a builtin emulation of mouse and keyboard: the right pad
> > + * can be used as a mouse, the shoulder buttons are mouse buttons, A and B
> > + * buttons are ENTER and ESCAPE, and so on. This is implemented as additional
> > + * HID interfaces.
> > + *
> > + * This is known as the "lizard mode", because apparently lizards like to use
> > + * the computer from the coach, without a proper mouse and keyboard.
> > + *
> > + * This driver will disable the lizard mode when the input device is opened
> > + * and re-enable it when the input device is closed, so as not to break user
> > + * mode behaviour. The lizard_mode parameter can be used to change that.
> > + *
> > + * There are a few user space applications (notably Steam Client) that use
> > + * the hidraw interface directly to create input devices (XTest, uinput...).
> > + * In order to avoid breaking them this driver creates a layered hidraw device,
> > + * so it can detect when the client is running and then:
> > + * - it will not send any command to the controller.
> > + * - this input device will be disabled, to avoid double input of the same
> > + * user action.
> > + *
> > + * For additional functions, such as changing the right-pad margin or switching
> > + * the led, you can use the user-space tool at:
> > + *
> > + * https://github.com/rodrigorc/steamctrl
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/hid.h>
> > +#include <linux/module.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/mutex.h>
> > +#include <linux/rcupdate.h>
> > +#include <linux/delay.h>
> > +#include <linux/power_supply.h>
> > +#include "hid-ids.h"
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>");
> > +
> > +static int lizard_mode = 1;
> > +module_param(lizard_mode, int, 0644);
> > +MODULE_PARM_DESC(lizard_mode,
> > + "Mouse and keyboard emulation (0 = always disabled; "
> > + "1 (default): enabled when gamepad is not in use; "
> > + "2: let userspace decide)");
>
> As mentioned in 0/2, I think a boolean might be better. Probably
> rename the parameter to something else more explicit too (like
> 'control_lizard_mode').
> Also you might want to hook up to changes to this values so users can
> control it better. But this can be added in a later patch.
As I replied to your previous comment, I really like the options
enable_lizard_mode=true/false. If you add to the mix the no_magic,
then you have the current situation.
I admit that it can be confusing to the user, and that the no_magic may
not have a practical use case. So I'd stick to the enable/disable for
now, if you agree.
> > +
> > +#define STEAM_QUIRK_WIRELESS BIT(0)
> > +
> > +#define STEAM_SERIAL_LEN 10
> > +/* Touch pads are 40 mm in diameter and 65535 units */
> > +#define STEAM_PAD_RESOLUTION 1638
> > +/* Trigger runs are about 5 mm and 256 units */
> > +#define STEAM_TRIGGER_RESOLUTION 51
> > +/* Joystick runs are about 5 mm and 256 units */
> > +#define STEAM_JOYSTICK_RESOLUTION 51
> > +
> > +#define STEAM_PAD_FUZZ 256
> > +
> > +struct steam_device {
> > + spinlock_t lock;
> > + struct hid_device *hdev, *client_hdev;
> > + struct mutex mutex;
> > + bool client_opened, input_opened;
> > + struct input_dev __rcu *input;
> > + unsigned long quirks;
> > + struct work_struct work_connect;
> > + bool connected;
> > + char serial_no[STEAM_SERIAL_LEN + 1];
> > +};
> > +
> > +static int steam_recv_report(struct steam_device *steam,
> > + u8 *data, int size)
> > +{
> > + struct hid_report *r;
> > + u8 *buf;
> > + int ret;
> > +
> > + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> > + if (hid_report_len(r) < 64)
> > + return -EINVAL;
> > +
> > + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + /*
> > + * The report ID is always 0, so strip the first byte from the output.
> > + * hid_report_len() is not counting the report ID, so +1 to the length
> > + * or else we get a EOVERFLOW. We are safe from a buffer overflow
> > + * because hid_alloc_report_buf() allocates +7 bytes.
> > + */
> > + ret = hid_hw_raw_request(steam->hdev, 0x00,
> > + buf, hid_report_len(r) + 1,
> > + HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> > + if (ret > 0)
> > + memcpy(data, buf + 1, min(size, ret - 1));
> > + kfree(buf);
> > + return ret;
> > +}
> > +
> > +static int steam_send_report(struct steam_device *steam,
> > + u8 *cmd, int size)
> > +{
> > + struct hid_report *r;
> > + u8 *buf;
> > + unsigned int retries = 10;
> > + int ret;
> > +
> > + r = steam->hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[0];
> > + if (hid_report_len(r) < 64)
> > + return -EINVAL;
> > +
> > + buf = hid_alloc_report_buf(r, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + /* The report ID is always 0 */
> > + memcpy(buf + 1, cmd, size);
> > +
> > + /*
> > + * Sometimes the wireless controller fails with EPIPE
> > + * when sending a feature report.
> > + * Doing a HID_REQ_GET_REPORT and waiting for a while
> > + * seems to fix that.
> > + */
> > + do {
> > + ret = hid_hw_raw_request(steam->hdev, 0,
> > + buf, size + 1,
> > + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> > + if (ret != -EPIPE)
> > + break;
> > + steam_recv_report(steam, NULL, 0);
> > + msleep(50);
> > + } while (--retries);
> > +
> > + kfree(buf);
> > + if (ret < 0)
> > + hid_err(steam->hdev, "%s: error %d (%*ph)\n", __func__,
> > + ret, size, cmd);
> > + return ret;
> > +}
> > +
> > +static inline int steam_send_report_byte(struct steam_device *steam, u8 cmd)
> > +{
> > + return steam_send_report(steam, &cmd, 1);
> > +}
> > +
> > +static inline int steam_write_register(struct steam_device *steam,
> > + u8 reg, u16 value)
> > +{
> > + u8 cmd[] = {0x87, 0x03, reg, value & 0xFF, value >> 8};
> > +
> > + return steam_send_report(steam, cmd, sizeof(cmd));
> > +}
> > +
> > +static int steam_get_serial(struct steam_device *steam)
> > +{
> > + /*
> > + * Send: 0xae 0x15 0x01
> > + * Recv: 0xae 0x15 0x01 serialnumber (10 chars)
> > + */
> > + int ret;
> > + u8 cmd[] = {0xae, 0x15, 0x01};
> > + u8 reply[3 + STEAM_SERIAL_LEN + 1];
> > +
> > + ret = steam_send_report(steam, cmd, sizeof(cmd));
> > + if (ret < 0)
> > + return ret;
> > + ret = steam_recv_report(steam, reply, sizeof(reply));
> > + if (ret < 0)
> > + return ret;
> > + reply[3 + STEAM_SERIAL_LEN] = 0;
> > + strlcpy(steam->serial_no, reply + 3, sizeof(steam->serial_no));
> > + return 0;
> > +}
> > +
> > +/*
> > + * This command requests the wireless adaptor to post an event
> > + * with the connection status. Useful if this driver is loaded when
> > + * the controller is already connected.
> > + */
> > +static inline int steam_request_conn_status(struct steam_device *steam)
> > +{
> > + return steam_send_report_byte(steam, 0xb4);
> > +}
> > +
> > +static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
> > +{
> > + if (enable) {
> > + /* enable esc, enter, cursors */
> > + steam_send_report_byte(steam, 0x85);
> > + /* enable mouse */
> > + steam_send_report_byte(steam, 0x8e);
> > + } else {
> > + /* disable esc, enter, cursor */
> > + steam_send_report_byte(steam, 0x81);
> > + /* disable mouse */
> > + steam_write_register(steam, 0x08, 0x07);
> > + }
> > +}
> > +
> > +static int steam_input_open(struct input_dev *dev)
> > +{
> > + struct steam_device *steam = input_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = hid_hw_open(steam->hdev);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->input_opened = true;
> > + if (!steam->client_opened && lizard_mode == 1)
> > + steam_set_lizard_mode(steam, false);
> > + mutex_unlock(&steam->mutex);
> > + return 0;
> > +}
> > +
> > +static void steam_input_close(struct input_dev *dev)
> > +{
> > + struct steam_device *steam = input_get_drvdata(dev);
> > +
> > + hid_hw_close(steam->hdev);
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->input_opened = false;
> > + if (!steam->client_opened && lizard_mode == 1)
> > + steam_set_lizard_mode(steam, true);
> > + mutex_unlock(&steam->mutex);
>
> hid_hw_close() should be called after setting the lizard mode.
Done.
> > +}
> > +
> > +static int steam_register(struct steam_device *steam)
> > +{
> > + struct hid_device *hdev = steam->hdev;
> > + struct input_dev *input;
> > + int ret;
> > +
> > + rcu_read_lock();
> > + input = rcu_dereference(steam->input);
> > + rcu_read_unlock();
> > + if (input) {
> > + dbg_hid("%s: already connected\n", __func__);
> > + return 0;
> > + }
> > +
> > + ret = steam_get_serial(steam);
> > + if (ret)
> > + return ret;
> > +
> > + hid_info(hdev, "Steam Controller '%s' connected",
> > + steam->serial_no);
> > +
> > + input = input_allocate_device();
> > + if (!input)
> > + return -ENOMEM;
> > +
> > + input_set_drvdata(input, steam);
> > + input->dev.parent = &hdev->dev;
> > + input->open = steam_input_open;
> > + input->close = steam_input_close;
> > +
> > + input->name = (steam->quirks & STEAM_QUIRK_WIRELESS) ?
> > + "Wireless Steam Controller" :
> > + "Steam Controller";
> > + input->phys = hdev->phys;
> > + input->uniq = steam->serial_no;
> > + input->id.bustype = hdev->bus;
> > + input->id.vendor = hdev->vendor;
> > + input->id.product = hdev->product;
> > + input->id.version = hdev->version;
> > +
> > + input_set_capability(input, EV_KEY, BTN_TR2);
> > + input_set_capability(input, EV_KEY, BTN_TL2);
> > + input_set_capability(input, EV_KEY, BTN_TR);
> > + input_set_capability(input, EV_KEY, BTN_TL);
> > + input_set_capability(input, EV_KEY, BTN_Y);
> > + input_set_capability(input, EV_KEY, BTN_B);
> > + input_set_capability(input, EV_KEY, BTN_X);
> > + input_set_capability(input, EV_KEY, BTN_A);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_UP);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_RIGHT);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_LEFT);
> > + input_set_capability(input, EV_KEY, BTN_DPAD_DOWN);
> > + input_set_capability(input, EV_KEY, BTN_SELECT);
> > + input_set_capability(input, EV_KEY, BTN_MODE);
> > + input_set_capability(input, EV_KEY, BTN_START);
> > + input_set_capability(input, EV_KEY, BTN_GEAR_DOWN);
> > + input_set_capability(input, EV_KEY, BTN_GEAR_UP);
> > + input_set_capability(input, EV_KEY, BTN_THUMBR);
> > + input_set_capability(input, EV_KEY, BTN_THUMBL);
> > + input_set_capability(input, EV_KEY, BTN_THUMB);
> > + input_set_capability(input, EV_KEY, BTN_THUMB2);
> > +
> > + input_set_abs_params(input, ABS_HAT2Y, 0, 255, 0, 0);
> > + input_set_abs_params(input, ABS_HAT2X, 0, 255, 0, 0);
> > + input_set_abs_params(input, ABS_X, -32767, 32767, 0, 0);
> > + input_set_abs_params(input, ABS_Y, -32767, 32767, 0, 0);
> > + input_set_abs_params(input, ABS_RX, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_set_abs_params(input, ABS_RY, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_set_abs_params(input, ABS_HAT0X, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_set_abs_params(input, ABS_HAT0Y, -32767, 32767,
> > + STEAM_PAD_FUZZ, 0);
> > + input_abs_set_res(input, ABS_X, STEAM_JOYSTICK_RESOLUTION);
> > + input_abs_set_res(input, ABS_Y, STEAM_JOYSTICK_RESOLUTION);
> > + input_abs_set_res(input, ABS_RX, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_RY, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT0X, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT0Y, STEAM_PAD_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT2Y, STEAM_TRIGGER_RESOLUTION);
> > + input_abs_set_res(input, ABS_HAT2X, STEAM_TRIGGER_RESOLUTION);
> > +
> > + ret = input_register_device(input);
> > + if (ret)
> > + goto input_register_fail;
> > +
> > + rcu_assign_pointer(steam->input, input);
> > +
> > + return 0;
> > +
> > +input_register_fail:
> > + input_free_device(input);
> > + return ret;
> > +}
> > +
> > +static void steam_unregister(struct steam_device *steam)
> > +{
> > + struct input_dev *input;
> > +
> > + rcu_read_lock();
> > + input = rcu_dereference(steam->input);
> > + rcu_read_unlock();
> > +
> > + if (input) {
> > + RCU_INIT_POINTER(steam->input, NULL);
> > + synchronize_rcu();
> > + hid_info(steam->hdev, "Steam Controller '%s' disconnected",
> > + steam->serial_no);
> > + input_unregister_device(input);
> > + }
> > +}
> > +
> > +static void steam_work_connect_cb(struct work_struct *work)
> > +{
> > + struct steam_device *steam = container_of(work, struct steam_device,
> > + work_connect);
> > + unsigned long flags;
> > + bool connected;
> > + int ret;
> > +
> > + spin_lock_irqsave(&steam->lock, flags);
> > + connected = steam->connected;
> > + spin_unlock_irqrestore(&steam->lock, flags);
> > +
> > + if (connected) {
> > + ret = steam_register(steam);
> > + if (ret) {
> > + hid_err(steam->hdev,
> > + "%s:steam_register failed with error %d\n",
> > + __func__, ret);
> > + }
> > + } else {
> > + steam_unregister(steam);
> > + }
> > +}
> > +
> > +static bool steam_is_valve_interface(struct hid_device *hdev)
> > +{
> > + struct hid_report_enum *rep_enum;
> > +
> > + /*
> > + * The wired device creates 3 interfaces:
> > + * 0: emulated mouse.
> > + * 1: emulated keyboard.
> > + * 2: the real game pad.
> > + * The wireless device creates 5 interfaces:
> > + * 0: emulated keyboard.
> > + * 1-4: slots where up to 4 real game pads will be connected to.
> > + * We know which one is the real gamepad interface because they are the
> > + * only ones with a feature report.
> > + */
> > + rep_enum = &hdev->report_enum[HID_FEATURE_REPORT];
> > + return !list_empty(&rep_enum->report_list);
> > +}
> > +
> > +static int steam_client_ll_parse(struct hid_device *hdev)
> > +{
> > + return 0;
>
> Instead of returning 0 here, you should probably call
> hid_parse_report() on the report descriptors from the parent node.
> Some clients might want to have a look at them or might even rely on
> them.
Ah, but hid_parse_report() just copies the descriptor from the parent,
and I am already doing that from steam_probe(). I'll move the code to
here and change to call hid_parse_report().
> > +}
> > +
> > +static int steam_client_ll_start(struct hid_device *hdev)
> > +{
> > + return 0;
> > +}
> > +
> > +static void steam_client_ll_stop(struct hid_device *hdev)
> > +{
> > +}
> > +
> > +static int steam_client_ll_open(struct hid_device *hdev)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
>
> You probably want to check if steam is not null here. Given the
> ordering of the initialization, you might have someone attempting to
> open the hidraw node before steam is created.
Tricky. Particularly since I've moved hid_parse_report() to
steam_client_ll_parse(), if steam is null there it all will break
apart. And parse is called from hid_add_device(), so I have to reorder
things a bit. Maybe calling hid_add_device() later.
> > + int ret;
> > +
> > + ret = hid_hw_open(steam->hdev);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->client_opened = true;
> > + mutex_unlock(&steam->mutex);
> > + return ret;
> > +}
> > +
> > +static void steam_client_ll_close(struct hid_device *hdev)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
>
> Same here, you might want to check on the validity of steam.
Now that I've reordered the probe, steam cannot be NULL.
> > +
> > + hid_hw_close(steam->hdev);
> > +
> > + mutex_lock(&steam->mutex);
> > + steam->client_opened = false;
> > + if (lizard_mode != 2) {
> > + if (steam->input_opened)
> > + steam_set_lizard_mode(steam, false);
> > + else
> > + steam_set_lizard_mode(steam, lizard_mode);
> > + }
> > + mutex_unlock(&steam->mutex);
>
> You should call hid_hw_close(steam->hdev); after sending the commands
> and not before.
Done.
> > +}
> > +
> > +static int steam_client_ll_raw_request(struct hid_device *hdev,
> > + unsigned char reportnum, u8 *buf,
> > + size_t count, unsigned char report_type,
> > + int reqtype)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
> > +
> > + return hid_hw_raw_request(steam->hdev, reportnum, buf, count,
> > + report_type, reqtype);
> > +}
>
> I wish we could reuse directly the pointer in
> hdev->ll_driver->raw_request to avoid adding an indirection.
> OTOH, the raw_requests are not happening that often, so we should be good.
hid_hw_raw_request() is an inline function that does exactly that, so I
would not worry about that.
However the call to hid_input_report() from steam_raw_event()... I wanted
to call client_hid->driver->raw_event() directly, but I didn't dare.
> > +static struct hid_ll_driver steam_client_ll_driver = {
> > + .parse = steam_client_ll_parse,
> > + .start = steam_client_ll_start,
> > + .stop = steam_client_ll_stop,
> > + .open = steam_client_ll_open,
> > + .close = steam_client_ll_close,
> > + .raw_request = steam_client_ll_raw_request,
> > +};
> > +
> > +static int steam_probe(struct hid_device *hdev,
> > + const struct hid_device_id *id)
> > +{
> > + struct steam_device *steam;
> > + struct hid_device *client_hdev;
> > + int ret;
> > +
> > + ret = hid_parse(hdev);
> > + if (ret) {
> > + hid_err(hdev,
> > + "%s:parse of hid interface failed\n", __func__);
> > + return ret;
> > + }
> > +
> > + /*
> > + * The non-valve interfaces (mouse and keyboard emulation) and
> > + * the client_hid are connected without changes.
> > + */
> > + if (hdev->group == HID_GROUP_STEAM ||
> > + !steam_is_valve_interface(hdev)) {
> > + return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > + }
>
> To make thinks clearer, I would split these calls in 2:
> - if (!steam_is_valve_interface(hdev)) return hid_hw_start(hdev,
> HID_CONNECT_DEFAULT);
> and
> - if (hdev->group == HID_GROUP_STEAM) return hid_hw_start(hdev,
> HID_CONNECT_HIDRAW);
>
> Explicitly having HID_CONNECT_HIDRAW would also make it clearer you
> are just exporting the hidraw interface here. It'll prevent any other
> interface to mess your detection of the hidraw usage.
Ok, done.
> > + /*
> > + * With the real steam controller interface, do not connect hidraw.
> > + * Instead, create the client_hid and connect that.
> > + */
> > + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> > + if (ret)
> > + return ret;
>
> this should likely be at the end of the probe, when you are done
> allocating your data.
Changed.
> > + client_hdev = hid_allocate_device();
> > + if (IS_ERR(client_hdev)) {
> > + ret = PTR_ERR(client_hdev);
> > + goto client_hdev_fail;
> > + }
> > +
> > + client_hdev->ll_driver = &steam_client_ll_driver;
> > + client_hdev->dev.parent = hdev->dev.parent;
> > + client_hdev->bus = hdev->bus;
> > + client_hdev->vendor = hdev->vendor;
> > + client_hdev->product = hdev->product;
> > + strlcpy(client_hdev->name, hdev->name,
> > + sizeof(client_hdev->name));
> > + strlcpy(client_hdev->phys, hdev->phys,
> > + sizeof(client_hdev->phys));
> > + client_hdev->dev_rdesc = kmemdup(hdev->dev_rdesc,
> > + hdev->dev_rsize, GFP_KERNEL);
> > + client_hdev->dev_rsize = hdev->dev_rsize;
> > + /*
> > + * Since we use the same device info than the real interface to
> > + * trick userspace, we will be calling steam_probe recursively.
> > + * We need to recognize the client interface somehow.
> > + */
> > + client_hdev->group = HID_GROUP_STEAM;
>
> I'd extract out the client_hdev initialization in its own function to
> keep .probe() clean.
Yeah, better. Done.
> > + ret = hid_add_device(client_hdev);
> > + if (ret)
> > + goto client_hdev_add_fail;
>
> This should likely be called after initializing steam. It'll keep the
> cleanup path cleaner and make sure all fields are properly initialized
> before they are used.
I've rewritten the probe so much that this does not apply anymore.
>
> > +
> > + steam = devm_kzalloc(&hdev->dev, sizeof(*steam), GFP_KERNEL);
> > + if (!steam) {
> > + ret = -ENOMEM;
> > + goto steam_alloc_fail;
> > + }
> > +
> > + steam->client_hdev = client_hdev;
> > + hid_set_drvdata(client_hdev, steam);
> > +
> > + spin_lock_init(&steam->lock);
> > + mutex_init(&steam->mutex);
> > + steam->hdev = hdev;
> > + hid_set_drvdata(hdev, steam);
> > + steam->quirks = id->driver_data;
> > + INIT_WORK(&steam->work_connect, steam_work_connect_cb);
>
> I'd call hid_hw_start(hdev, ...) here, and then hid_add_device(client_hdev);
Ok.
> To have a better cleanup path, you porbably should allocate
> client_hdev here too (before hid_add_device, of course)
> > +
> > + if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> > + ret = hid_hw_open(hdev);
> > + if (ret) {
> > + hid_err(hdev,
> > + "%s:hid_hw_open for wireless\n",
> > + __func__);
> > + goto hid_hw_open_fail;
> > + }
> > + hid_info(hdev, "Steam wireless receiver connected");
> > + steam_request_conn_status(steam);
> > + } else {
> > + ret = steam_register(steam);
> > + if (ret) {
> > + hid_err(hdev,
> > + "%s:steam_register failed with error %d\n",
> > + __func__, ret);
> > + goto input_register_fail;
> > + }
> > + }
> > + if (lizard_mode != 2)
> > + steam_set_lizard_mode(steam, lizard_mode);
> > +
> > + return 0;
> > +
> > +input_register_fail:
> > +hid_hw_open_fail:
> > + cancel_work_sync(&steam->work_connect);
> > + hid_set_drvdata(hdev, NULL);
> > +steam_alloc_fail:
> > + hid_hw_stop(client_hdev);
> > +client_hdev_add_fail:
> > + hid_destroy_device(client_hdev);
> > +client_hdev_fail:
> > + hid_err(hdev, "%s: failed with error %d\n",
> > + __func__, ret);
> > + return 0;
> > +}
> > +
> > +static void steam_remove(struct hid_device *hdev)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
> > +
> > + if (hdev->group == HID_GROUP_STEAM)
>
> Why don't you call hid_hw_stop() here instead of calling it later in
> the parent device?
I tried to keep together these two lines:
hid_hw_stop(steam->client_hdev);
hid_destroy_device(steam->client_hdev);
but I guess that it is better to the first one here and the last one
there.
> > + return;
> > +
> > + if (!steam) {
> > + hid_hw_stop(hdev);
> > + return;
> > + }
> > +
>
> You should reorder these cleanup calls:
> - you first call hid_destroy_device(), this will clean up properly
> everything related to the hidraw node and should set client_opened to
> false (just to be on the safe side, you might want to overwrite it
> after to be sure not to forward events to the destoryed hid node).
> - then you take care of the rest of the hid device:
> - cancel_work_sync should happen before calling hid_hw_close() and
> hid_hw_stop() on the hdev.
> - steam_unregister(steam);
> - if needed call hid_hw_close()
> - hid_hw_stop()
That was a bit messy. Fixed.
That's all for now... I'll wait a couple of days more, in case I get any
more feedback and reroll.
Best regards.
Rodrigo
> > + if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> > + hid_info(hdev, "Steam wireless receiver disconnected");
> > + hid_hw_close(hdev);
> > + }
> > + hid_hw_stop(hdev);
> > + cancel_work_sync(&steam->work_connect);
> > + hid_hw_stop(steam->client_hdev);
> > + hid_destroy_device(steam->client_hdev);
> > +
> > +}
> > +
> > +static void steam_do_connect_event(struct steam_device *steam, bool connected)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&steam->lock, flags);
> > + steam->connected = connected;
> > + spin_unlock_irqrestore(&steam->lock, flags);
> > +
> > + if (schedule_work(&steam->work_connect) == 0)
> > + dbg_hid("%s: connected=%d event already queued\n",
> > + __func__, connected);
> > +}
> > +
> > +/*
> > + * The size for this message payload is 60.
> > + * The known values are:
> > + * (* values are not sent through wireless)
> > + * (* accelerator/gyro is disabled by default)
> > + * Offset| Type | Mapped to |Meaning
> > + * -------+-------+-----------+--------------------------
> > + * 4-7 | u32 | -- | sequence number
> > + * 8-10 | 24bit | see below | buttons
> > + * 11 | u8 | ABS_HAT2Y | left trigger
> > + * 12 | u8 | ABS_HAT2X | right trigger
> > + * 13-15 | -- | -- | always 0
> > + * 16-17 | s16 | ABS_X/ABS_HAT0X | X value
> > + * 18-19 | s16 | ABS_Y/ABS_HAT0Y | Y value
> > + * 20-21 | s16 | ABS_RX | right-pad X value
> > + * 22-23 | s16 | ABS_RY | right-pad Y value
> > + * 24-25 | s16 | -- | * left trigger
> > + * 26-27 | s16 | -- | * right trigger
> > + * 28-29 | s16 | -- | * accelerometer X value
> > + * 30-31 | s16 | -- | * accelerometer Y value
> > + * 32-33 | s16 | -- | * accelerometer Z value
> > + * 34-35 | s16 | -- | gyro X value
> > + * 36-36 | s16 | -- | gyro Y value
> > + * 38-39 | s16 | -- | gyro Z value
> > + * 40-41 | s16 | -- | quaternion W value
> > + * 42-43 | s16 | -- | quaternion X value
> > + * 44-45 | s16 | -- | quaternion Y value
> > + * 46-47 | s16 | -- | quaternion Z value
> > + * 48-49 | -- | -- | always 0
> > + * 50-51 | s16 | -- | * left trigger (uncalibrated)
> > + * 52-53 | s16 | -- | * right trigger (uncalibrated)
> > + * 54-55 | s16 | -- | * joystick X value (uncalibrated)
> > + * 56-57 | s16 | -- | * joystick Y value (uncalibrated)
> > + * 58-59 | s16 | -- | * left-pad X value
> > + * 60-61 | s16 | -- | * left-pad Y value
> > + * 62-63 | u16 | -- | * battery voltage
> > + *
> > + * The buttons are:
> > + * Bit | Mapped to | Description
> > + * ------+------------+--------------------------------
> > + * 8.0 | BTN_TR2 | right trigger fully pressed
> > + * 8.1 | BTN_TL2 | left trigger fully pressed
> > + * 8.2 | BTN_TR | right shoulder
> > + * 8.3 | BTN_TL | left shoulder
> > + * 8.4 | BTN_Y | button Y
> > + * 8.5 | BTN_B | button B
> > + * 8.6 | BTN_X | button X
> > + * 8.7 | BTN_A | button A
> > + * 9.0 | BTN_DPAD_UP | lef-pad up
> > + * 9.1 | BTN_DPAD_RIGHT | lef-pad right
> > + * 9.2 | BTN_DPAD_LEFT | lef-pad left
> > + * 9.3 | BTN_DPAD_DOWN | lef-pad down
> > + * 9.4 | BTN_SELECT | menu left
> > + * 9.5 | BTN_MODE | steam logo
> > + * 9.6 | BTN_START | menu right
> > + * 9.7 | BTN_GEAR_DOWN | left back lever
> > + * 10.0 | BTN_GEAR_UP | right back lever
> > + * 10.1 | -- | left-pad clicked
> > + * 10.2 | BTN_THUMBR | right-pad clicked
> > + * 10.3 | BTN_THUMB | left-pad touched (but see explanation below)
> > + * 10.4 | BTN_THUMB2 | right-pad touched
> > + * 10.5 | -- | unknown
> > + * 10.6 | BTN_THUMBL | joystick clicked
> > + * 10.7 | -- | lpad_and_joy
> > + */
> > +
> > +static void steam_do_input_event(struct steam_device *steam,
> > + struct input_dev *input, u8 *data)
> > +{
> > + /* 24 bits of buttons */
> > + u8 b8, b9, b10;
> > + bool lpad_touched, lpad_and_joy;
> > +
> > + b8 = data[8];
> > + b9 = data[9];
> > + b10 = data[10];
> > +
> > + input_report_abs(input, ABS_HAT2Y, data[11]);
> > + input_report_abs(input, ABS_HAT2X, data[12]);
> > +
> > + /*
> > + * These two bits tells how to interpret the values X and Y.
> > + * lpad_and_joy tells that the joystick and the lpad are used at the
> > + * same time.
> > + * lpad_touched tells whether X/Y are to be read as lpad coord or
> > + * joystick values.
> > + * (lpad_touched || lpad_and_joy) tells if the lpad is really touched.
> > + */
> > + lpad_touched = b10 & BIT(3);
> > + lpad_and_joy = b10 & BIT(7);
> > + input_report_abs(input, lpad_touched ? ABS_HAT0X : ABS_X,
> > + (s16) le16_to_cpup((__le16 *)(data + 16)));
> > + input_report_abs(input, lpad_touched ? ABS_HAT0Y : ABS_Y,
> > + -(s16) le16_to_cpup((__le16 *)(data + 18)));
> > + /* Check if joystick is centered */
> > + if (lpad_touched && !lpad_and_joy) {
> > + input_report_abs(input, ABS_X, 0);
> > + input_report_abs(input, ABS_Y, 0);
> > + }
> > + /* Check if lpad is untouched */
> > + if (!(lpad_touched || lpad_and_joy)) {
> > + input_report_abs(input, ABS_HAT0X, 0);
> > + input_report_abs(input, ABS_HAT0Y, 0);
> > + }
> > +
> > + input_report_abs(input, ABS_RX,
> > + (s16) le16_to_cpup((__le16 *)(data + 20)));
> > + input_report_abs(input, ABS_RY,
> > + -(s16) le16_to_cpup((__le16 *)(data + 22)));
> > +
> > + input_event(input, EV_KEY, BTN_TR2, !!(b8 & BIT(0)));
> > + input_event(input, EV_KEY, BTN_TL2, !!(b8 & BIT(1)));
> > + input_event(input, EV_KEY, BTN_TR, !!(b8 & BIT(2)));
> > + input_event(input, EV_KEY, BTN_TL, !!(b8 & BIT(3)));
> > + input_event(input, EV_KEY, BTN_Y, !!(b8 & BIT(4)));
> > + input_event(input, EV_KEY, BTN_B, !!(b8 & BIT(5)));
> > + input_event(input, EV_KEY, BTN_X, !!(b8 & BIT(6)));
> > + input_event(input, EV_KEY, BTN_A, !!(b8 & BIT(7)));
> > + input_event(input, EV_KEY, BTN_DPAD_UP, !!(b9 & BIT(0)));
> > + input_event(input, EV_KEY, BTN_DPAD_RIGHT, !!(b9 & BIT(1)));
> > + input_event(input, EV_KEY, BTN_DPAD_LEFT, !!(b9 & BIT(2)));
> > + input_event(input, EV_KEY, BTN_DPAD_DOWN, !!(b9 & BIT(3)));
> > + input_event(input, EV_KEY, BTN_SELECT, !!(b9 & BIT(4)));
> > + input_event(input, EV_KEY, BTN_MODE, !!(b9 & BIT(5)));
> > + input_event(input, EV_KEY, BTN_START, !!(b9 & BIT(6)));
> > + input_event(input, EV_KEY, BTN_GEAR_DOWN, !!(b9 & BIT(7)));
> > + input_event(input, EV_KEY, BTN_GEAR_UP, !!(b10 & BIT(0)));
> > + input_event(input, EV_KEY, BTN_THUMBR, !!(b10 & BIT(2)));
> > + input_event(input, EV_KEY, BTN_THUMBL, !!(b10 & BIT(6)));
> > + input_event(input, EV_KEY, BTN_THUMB, lpad_touched || lpad_and_joy);
> > + input_event(input, EV_KEY, BTN_THUMB2, !!(b10 & BIT(4)));
> > +
> > + input_sync(input);
> > +}
> > +
> > +static int steam_raw_event(struct hid_device *hdev,
> > + struct hid_report *report, u8 *data,
> > + int size)
> > +{
> > + struct steam_device *steam = hid_get_drvdata(hdev);
> > + struct input_dev *input;
> > +
> > + if (!steam)
> > + return 0;
> > +
> > + if (steam->client_opened)
> > + hid_input_report(steam->client_hdev, HID_FEATURE_REPORT,
> > + data, size, 0);
> > + /*
> > + * All messages are size=64, all values little-endian.
> > + * The format is:
> > + * Offset| Meaning
> > + * -------+--------------------------------------------
> > + * 0-1 | always 0x01, 0x00, maybe protocol version?
> > + * 2 | type of message
> > + * 3 | length of the real payload (not checked)
> > + * 4-n | payload data, depends on the type
> > + *
> > + * There are these known types of message:
> > + * 0x01: input data (60 bytes)
> > + * 0x03: wireless connect/disconnect (1 byte)
> > + * 0x04: battery status (11 bytes)
> > + */
> > +
> > + if (size != 64 || data[0] != 1 || data[1] != 0)
> > + return 0;
> > +
> > + switch (data[2]) {
> > + case 0x01:
> > + if (steam->client_opened)
> > + return 0;
> > + rcu_read_lock();
> > + input = rcu_dereference(steam->input);
> > + if (likely(input)) {
> > + steam_do_input_event(steam, input, data);
> > + } else {
> > + dbg_hid("%s: input data without connect event\n",
> > + __func__);
> > + steam_do_connect_event(steam, true);
> > + }
> > + rcu_read_unlock();
> > + break;
> > + case 0x03:
> > + /*
> > + * The payload of this event is a single byte:
> > + * 0x01: disconnected.
> > + * 0x02: connected.
> > + */
> > + switch (data[4]) {
> > + case 0x01:
> > + steam_do_connect_event(steam, false);
> > + break;
> > + case 0x02:
> > + steam_do_connect_event(steam, true);
> > + break;
> > + }
> > + break;
> > + case 0x04:
> > + /* TODO: battery status */
> > + break;
> > + }
> > + return 0;
> > +}
> > +
> > +static const struct hid_device_id steam_controllers[] = {
> > + { /* Wired Steam Controller */
> > + HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> > + USB_DEVICE_ID_STEAM_CONTROLLER)
> > + },
> > + { /* Wireless Steam Controller */
> > + HID_USB_DEVICE(USB_VENDOR_ID_VALVE,
> > + USB_DEVICE_ID_STEAM_CONTROLLER_WIRELESS),
> > + .driver_data = STEAM_QUIRK_WIRELESS
> > + },
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(hid, steam_controllers);
> > +
> > +static struct hid_driver steam_controller_driver = {
> > + .name = "hid-steam",
> > + .id_table = steam_controllers,
> > + .probe = steam_probe,
> > + .remove = steam_remove,
> > + .raw_event = steam_raw_event,
> > +};
> > +
> > +module_hid_driver(steam_controller_driver);
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index d491027a7c22..5e5d76589954 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -364,6 +364,7 @@ struct hid_item {
> > #define HID_GROUP_RMI 0x0100
> > #define HID_GROUP_WACOM 0x0101
> > #define HID_GROUP_LOGITECH_DJ_DEVICE 0x0102
> > +#define HID_GROUP_STEAM 0x0103
> >
> > /*
> > * HID protocol status
> > --
> > 2.16.2
> >
>
> Cheers,
> Benjamin
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: mfd: Add Gateworks System Controller bindings
From: Tim Harvey @ 2018-03-28 20:53 UTC (permalink / raw)
To: Guenter Roeck
Cc: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, linux-kernel, devicetree, linux-arm-kernel,
linux-hwmon, linux-input, linux-watchdog
In-Reply-To: <20180328202321.GA21664@roeck-us.net>
On Wed, Mar 28, 2018 at 1:23 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Wed, Mar 28, 2018 at 12:17:34PM -0700, Tim Harvey wrote:
>> On Wed, Mar 28, 2018 at 9:24 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On Wed, Mar 28, 2018 at 08:14:00AM -0700, Tim Harvey wrote:
>> >> This patch adds documentation of device-tree bindings for the
>> >> Gateworks System Controller (GSC).
>> >>
>> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> >> ---
>> >> v3:
>> >> - replaced _ with -
>> >> - remove input bindings
>> >> - added full description of hwmon
>> >> - fix unit address of hwmon child nodes
>> >>
>> >> ---
>> >> .../devicetree/bindings/mfd/gateworks-gsc.txt | 135 +++++++++++++++++++++
>> >> 1 file changed, 135 insertions(+)
>> >> create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
>> >> new file mode 100644
>> >> index 0000000..8f530ed
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
>> >> @@ -0,0 +1,135 @@
>> >> +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"
>> >> +- 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
>> >> + <devicetree/bindings/interrupt-controller/interrupt.txt>
>> >> +
>> >> +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 watchdog properties:
>> >> +- compatible: must be "gw,gsc-watchdog"
>> >> +
>> >> +* hwmon:
>> >> +The GSC provides a set of Analog to Digitcal Converter (ADC) pins used for
>> >> +temperature and/or voltage monitoring.
>> >> +
>> >> +Required hwmon properties:
>> >> +- compatible: must be "gw,gsc-hwmon"
>> >> +
>> >
>> > "hwmon" is a very Linux specific term. It might make sense to find a more
>> > generic term.
>>
>> The 'hwmon' driver supports child nodes that fall into the following category:
>> - temperature sensor (GSC internal temperature sensor - i2c registers
>> returns value in C*10)
>> - voltage rails (two types here; cooked: i2c registers return
>> pre-scaled value in mV), raw: i2c registers return a raw ADC value
>> that must be scaled based on ADC internal ref voltage and resolution
>> and adjusted for a voltage divider to convert to mV
>> - fan setpoints (I'll explain these below)
>>
>> I called the node 'gw,gsc-hwmon' because the driver fits into the
>> 'hwmon' API. Isn't that appropriate here for the driver compatible
>> string?
>>
>
> Devicetree properties are supposed to be OS independent.
>
>> >
>> >> +Optional hwmon properties:
>> >> +- gw,reference-voltage: ADC reference voltage (mV) used in scaling raw ADCs
>> >
>> > AFAIK devicetree likes to specify voltages in uV.
>>
>> There are currently plenty of dt props specified in mV (grep -r mV
>> Documentation/devicetree/bindings/).
>>
>
> "But so many others are speeding, why do I get a ticket ?"
>
> Please discuss with Rob.
Yes - hoping for feedback on mV vs uV as well as naming of hwmon mfd child node.
>
>> >
>> >> +- gw,resolution: ADC resolution (ie 4096) used in scaling raw ADCs
>> >> +
>> >
>> > 4096 what ?
>>
>> reference-voltage and resolution are used to scale the values from the
>> nodes that report a raw ADC value:
>>
>> V = Vadc * (reference-voltage / resolution)
>>
>> I can provide that in bits if it makes more sense? I can also hard
>
> Yes, I think that would make more sense, and please describe what it means.
>
>> code both the resolution and the vref in the hwmon driver and remove
>> it from dt as currently the only GSC that uses raw ADC values is 12bit
>> with 2.5V ref.
>>
>
> That would be even better.
>
>> >
>> >> +Each hwmon child node defines an ADC input on the chip which the GSC may
>> >> +report cooked values (ie temperature sensor based on thermister), raw values,
>> >> +(ie voltage rail with a pre-scaling resistor divider), or a fan controller
>> >> +setpoint.
>> >> +
>> >> +Required hwmon child properties:
>> >> +- type: one of the following ADC types:
>> >> + "gw,hwmon-temperature" - reports temperature in C*10
>> >> + "gw,hwmon-voltage" - reports a pre-scaled voltage value
>> >> + "gw,hwmon-voltage-raw" - reports a raw ADC that is scaled with
>> >> + vreference, resolution, and optional resistor divider
>> >> + "gw,hwmon-fan" - a fan temperature setpoint in C*10
>> >
>> > What is a "fan temperature setpoint" ?
>> >
>>
>> The GSC supports a fan controller which drives a PWM signal to vary
>> the speed of a fan based on the GSC internal temperature sensor. The
>> FAN controller has 6 setpoints each having a fixed PWM duty-cycle but
>> the temperature at which those setpoints kick in can be varies via
>> registers at the 0x29 slave address (same slave address as the
>> temperature sensor and voltage inputs which is why I have it in the
>> hwmon driver):
>>
>> fan0_point - 50% PWM (default 300)
>> fan1_point - 60% PWM (default 330)
>> fan2_point - 70% PWM (default 360)
>> fan3_point - 80% PWM (default 390)
>> fan4_point - 90% PWM (default 420)
>> fan5_point - 100% PWM (default 450)
>>
>> The values are C/10 thus if the internal GSC temp sensor is below 30C
>> the fan output will be 0% duty cycle and if it hits 30C it will go to
>> 50% until it hits 60% at 33C etc.
>>
> Please do not define your own scaling factors. pwm values are 0..255,
> and temperatures are in milli-degrees C.
>
>> That is the hardware implementation that I'm trying to abstract and
>> define here. You pointed out the fact that the fan*_input ABI is
>> read-only fan PWM and I see that now. What do you suggest I use for
>
> No, it isn't. It is the fan speed in RPM.
>
>> this feature I'm trying to implement driver support for?
>>
>
> pwm[1-*]_auto_point[1-*]_pwm
> pwm[1-*]_auto_point[1-*]_temp
> pwm[1-*]_auto_point[1-*]_temp_hyst
>
> may be relevant. From the context, something like
>
> pwm1_auto_point1_pwm read-only, set to 128
> pwm1_auto_point1_temp 30000
> pwm1_auto_point2_pwm read-only, set to 153
> pwm1_auto_point2_temp 33000
> pwm1_auto_point3_pwm read-only, set to 179
> pwm1_auto_point3_temp 36000
> pwm1_auto_point4_pwm read-only, set to 204
> pwm1_auto_point4_temp 39000
> pwm1_auto_point5_pwm read-only, set to 230
> pwm1_auto_point5_temp 42000
> pwm1_auto_point6_pwm read-only, set to 255
> pwm1_auto_point6_temp 45000
>
> might make sense.
I like that idea! The details of the setpoints do not need to be in
the dts at all. I will need to add a single property to signify that
the board has a fan controller as some don't.
Thanks,
Tim
^ permalink raw reply
* Re: [PATCH v3 3/4] hwmon: add Gateworks System Controller support
From: Guenter Roeck @ 2018-03-28 20:33 UTC (permalink / raw)
To: Tim Harvey
Cc: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, linux-kernel, devicetree, linux-arm-kernel,
linux-hwmon, linux-input, linux-watchdog
In-Reply-To: <CAJ+vNU02R134uoVAkx2VjD1FpUj+Zt0haj_zYcYT5xNWTgF7Ag@mail.gmail.com>
On Wed, Mar 28, 2018 at 01:23:59PM -0700, Tim Harvey wrote:
> On Wed, Mar 28, 2018 at 10:00 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Wed, Mar 28, 2018 at 08:14:02AM -0700, 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.
> >>
> >> Cc: Guenter Roeck <linux@roeck-us.net>
> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >> ---
> >> v3:
> >> - add voltage_raw input type and supporting fields
> >> - add channel validation to is_visible function
> >> - remove unnecessary channel validation from read/write functions
> >>
> >> v2:
> >> - change license comment style
> >> - remove DEBUG
> >> - simplify regmap_bulk_read err check
> >> - remove break after returns in switch statement
> >> - fix fan setpoint buffer address
> >> - remove unnecessary parens
> >> - consistently use struct device *dev pointer
> >> - change license/comment block
> >> - add validation for hwmon child node props
> >> - move parsing of of to own function
> >> - use strlcpy to ensure null termination
> >> - fix static array sizes and removed unnecessary initializers
> >> - dynamically allocate channels
> >> - fix fan input label
> >> - support platform data
> >> - fixed whitespace issues
> >>
> >> drivers/hwmon/Kconfig | 9 +
> >> drivers/hwmon/Makefile | 1 +
> >> drivers/hwmon/gsc-hwmon.c | 368 ++++++++++++++++++++++++++++++++
> >
> > This will require a matching Documentation/hwmon/gsc-hwmon to explain supported
> > attributes.
>
> ok - will add in next submission
>
> >
> <snip>
> >>
> >> +static int
> >> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> >> + int channel, long *val)
> >> +{
> >> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
> >> + struct gsc_hwmon_platform_data *pdata = hwmon->pdata;
> >> + const struct gsc_hwmon_channel *ch;
> >> + int sz, ret;
> >> + u8 buf[3];
> >> +
> >> + dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
> >> + channel);
> >> + switch (type) {
> >> + case hwmon_in:
> >> + ch = hwmon->in_ch[channel];
> >> + break;
> >> + case hwmon_temp:
> >> + ch = hwmon->temp_ch[channel];
> >> + break;
> >> + case hwmon_fan:
> >> + ch = hwmon->fan_ch[channel];
> >> + break;
> >> + default:
> >> + return -EOPNOTSUPP;
> >> + }
> >> +
> >> + sz = (ch->type == type_voltage) ? 3 : 2;
> >> + ret = regmap_bulk_read(hwmon->gsc->regmap_hwmon, ch->reg, buf, sz);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + *val = 0;
> >> + while (sz-- > 0)
> >> + *val |= (buf[sz] << (8*sz));
> >
> > Please use spaces before and after operators.
>
> ok
>
> >
> >> +
> >> + switch (ch->type) {
> >> + case type_temperature:
> >> + if ((type == hwmon_temp) && *val > 0x8000)
> >
> > Please no unnecessary ( ).
> >
>
> ok
>
> > Is there ever a situation where ch->type == type_temperature and type !=
> > hwmon_temp ? Wouldn't that be a bug ?
>
> I should not have been checking (type == hwmon_temp) there... will remove that.
>
> >
> >> + *val -= 0xffff;
> >> + break;
> >> + case type_voltage_raw:
> >> + /* scale based on ref voltage and resolution */
> >> + if (pdata->vreference && pdata->resolution) {
> >> + *val *= pdata->vreference;
> >> + *val /= pdata->resolution;
> >> + }
> >> + /* scale based on optional voltage divider */
> >> + if (ch->vdiv[0] && ch->vdiv[1]) {
> >> + *val *= (ch->vdiv[0] + ch->vdiv[1]);
> >> + *val /= ch->vdiv[1];
> >> + }
> >
> > This accepts both types of scaling. Is that intentional ?
>
> yes that is intentional. One version of the GSC reports cooked
> pre-scaled values that won't fall into either of these. Another
> version of the GSC will report raw ADC values which will need the
> vref/res scaling and those may have an optional voltage divider.
>
> >
> > I don't see any protection against overflows. What if pdata->vreference is
> > larger than 256 on a system with sizeof(long) == 4 and the raw voltage
> > as reported by the chip is 0xffffff ?
> >
> >> + /* adjust by offset */
> >> + *val += ch->voffset;
> >
> > Similar to the above, this can result in an overflow on systems with
> > sizeof(long) == 4.
> >
>
> true - I will add some overflow checking
>
> >> + break;
> >
> > There should be a default case as well as 'case type_voltage:'
> > with a break; statement and a comment indicating that no adjustment
> > is needed.
> >
>
> ok
>
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int
> >> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> >> + u32 attr, int channel, const char **buf)
> >> +{
> >> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
> >> +
> >> + dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
> >> + channel);
> >
> > I seriously wonder if all those dev_dbg() statements add value.
>
> only to me while I was writing/testing; I will remove them
>
> >
> >> + switch (type) {
> >> + case hwmon_in:
> >> + *buf = hwmon->in_ch[channel]->name;
> >> + break;
> >> + case hwmon_temp:
> >> + *buf = hwmon->temp_ch[channel]->name;
> >> + break;
> >> + case hwmon_fan:
> >> + *buf = hwmon->fan_ch[channel]->name;
> >> + break;
> >> + default:
> >> + return -ENOTSUPP;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int
> >> +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> >> + int channel, long val)
> >> +{
> >> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
> >> + u8 buf[2];
> >> +
> >> + dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
> >> + channel);
> >> + switch (type) {
> >> + case hwmon_fan:
> >> + buf[0] = val & 0xff;
> >> + buf[1] = (val >> 8) & 0xff;
> >> + return regmap_bulk_write(hwmon->gsc->regmap_hwmon,
> >> + hwmon->fan_ch[channel]->reg, buf, 2);
> >
> > fanX_input reports the fan speed. By its nature, a fan speed is not writeable.
> > I have no idea what this is supposed to achieve, but whatever it is, it is wrong.
> >
> > Please stick with the ABI.
>
> ok, I see that now. Do you have any recommendation of what I should
> use for these temperature setpoints that control when the fan pwm is
> adjusted?
>
As mentioned in the other thread,
pwm[1-*]_auto_point[1-*]_pwm
pwm[1-*]_auto_point[1-*]_temp
seems to be the best fit. Downside is that we don't have those supported in
the new ABI. You have two options: add the attributes to the ABI and use them,
or define your own sysfs group for it. Either way is fine with me (I would
prefer to have it added to the API, though).
> >
> >> + default:
> >> + break;
> >> + }
> >> +
> >> + return -EOPNOTSUPP;
> >> +}
> >> +
> >> +static umode_t
> >> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
> >> + int ch)
> >> +{
> >> + const struct gsc_hwmon_data *hwmon = _data;
> >> + struct device *dev = hwmon->gsc->dev;
> >> + umode_t mode = 0;
> >> +
> >> + switch (type) {
> >> + case hwmon_fan:
> >> + if (ch >= GSC_HWMON_MAX_FAN_CH)
> >> + return -EOPNOTSUPP;
> >
> > Can that ever happen ?
>
> No, I suppose not because the ch input comes from a hwmon node that
> was created by the driver registering the entities and a check was
> done there to avoid overflow. I will remove these.
>
> >
> >> + mode = S_IRUGO;
> >> + if (attr == hwmon_fan_input)
> >> + mode |= S_IWUSR;
> >
> > fanX_input is a read-only attribute per ABI.
> >
> >> + break;
> >> + case hwmon_temp:
> >> + if (ch >= GSC_HWMON_MAX_TEMP_CH)
> >> + return -EOPNOTSUPP;
> >
> > Can that ever happen ?
> >
> >> + mode = S_IRUGO;
> >> + break;
> >> + case hwmon_in:
> >> + if (ch >= GSC_HWMON_MAX_IN_CH)
> >> + return -EOPNOTSUPP;
> >
> > Can that ever happen ?
> >
> >> + mode = S_IRUGO;
> >> + break;
> >> + default:
> >> + return -EOPNOTSUPP;
> >> + }
> >> + dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__, type,
> >> + attr, ch, mode);
> >> +
> >> + return mode;
> >> +}
> >> +
> >> +static const struct hwmon_ops gsc_hwmon_ops = {
> >> + .is_visible = gsc_hwmon_is_visible,
> >> + .read = gsc_hwmon_read,
> >> + .read_string = gsc_hwmon_read_string,
> >> + .write = gsc_hwmon_write,
> >> +};
> >> +
> >> +static struct gsc_hwmon_platform_data *
> >> +gsc_hwmon_get_devtree_pdata(struct device *dev)
> >> +{
> >> + struct gsc_hwmon_platform_data *pdata;
> >> + struct gsc_hwmon_channel *ch;
> >> + struct fwnode_handle *child;
> >> + const char *type;
> >> + int nchannels;
> >> +
> >> + nchannels = device_get_child_node_count(dev);
> >> + dev_dbg(dev, "channels=%d\n", nchannels);
> >> + if (nchannels == 0)
> >> + return ERR_PTR(-ENODEV);
> >> +
> >> + pdata = devm_kzalloc(dev,
> >> + sizeof(*pdata) + nchannels * sizeof(*ch),
> >> + GFP_KERNEL);
> >> + if (!pdata)
> >> + return ERR_PTR(-ENOMEM);
> >> + ch = (struct gsc_hwmon_channel *)(pdata + 1);
> >> + pdata->channels = ch;
> >> + pdata->nchannels = nchannels;
> >> +
> >> + device_property_read_u32(dev, "gw,reference-voltage",
> >> + &pdata->vreference);
> >> + device_property_read_u32(dev, "gw,resolution", &pdata->resolution);
> >> +
> >> + /* allocate structures for channels and count instances of each type */
> >> + device_for_each_child_node(dev, child) {
> >> + if (fwnode_property_read_string(child, "label", &ch->name)) {
> >> + dev_err(dev, "channel without label\n");
> >> + fwnode_handle_put(child);
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> + if (fwnode_property_read_u32(child, "reg", &ch->reg)) {
> >> + dev_err(dev, "channel without reg\n");
> >> + fwnode_handle_put(child);
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> + if (fwnode_property_read_string(child, "type", &type)) {
> >> + dev_err(dev, "channel without type\n");
> >> + fwnode_handle_put(child);
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> + if (!strcasecmp(type, "gw,hwmon-temperature"))
> >> + ch->type = type_temperature;
> >> + else if (!strcasecmp(type, "gw,hwmon-voltage"))
> >> + ch->type = type_voltage;
> >> + else if (!strcasecmp(type, "gw,hwmon-voltage-raw"))
> >> + ch->type = type_voltage_raw;
> >> + else if (!strcasecmp(type, "gw,hwmon-fan"))
> >> + ch->type = type_fan;
> >> + else {
> >> + dev_err(dev, "channel without type\n");
> >> + fwnode_handle_put(child);
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> +
> >> + fwnode_property_read_u32(child, "gw,voltage-offset",
> >> + &ch->voffset);
> >
> > Note that while it is technically ok to keep voltages internally in mV,
> > devicetree will likely require specificayion in uV. For accuracy, it might be
> > better to perform any calculations on that base and convert to mV for display
> > purposes.
>
> I'm happy to change them if that really is the standard but it doesn't
> look like it is a standard?
>
Again, please discuss with Rob.
> >
> >> + fwnode_property_read_u32_array(child, "gw,voltage-divider",
> >> + ch->vdiv, ARRAY_SIZE(ch->vdiv));
> >> + dev_dbg(dev, "of: reg=0x%02x type=%d %s\n", ch->reg, ch->type,
> >> + ch->name);
> >> + ch++;
> >> + }
> >> +
> >> + return pdata;
> >> +}
> >> +
> >> +static int gsc_hwmon_probe(struct platform_device *pdev)
> >> +{
> >> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
> >> + struct device *dev = &pdev->dev;
> >> + struct gsc_hwmon_platform_data *pdata = dev_get_platdata(dev);
> >> + struct gsc_hwmon_data *hwmon;
> >> + int i, i_in, i_temp, i_fan;
> >> +
> >> + if (!pdata) {
> >> + pdata = gsc_hwmon_get_devtree_pdata(dev);
> >> + if (IS_ERR(pdata))
> >> + return PTR_ERR(pdata);
> >> + }
> >> +
> >> + hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
> >> + if (!hwmon)
> >> + return -ENOMEM;
> >> + hwmon->gsc = gsc;
> >> + hwmon->pdata = pdata;
> >> +
> >> + for (i = 0, i_in = 0, i_temp = 0, i_fan = 0;
> >> + i < hwmon->pdata->nchannels; i++) {
> >> + const struct gsc_hwmon_channel *ch = &pdata->channels[i];
> >> +
> >> + if (ch->reg > GSC_HWMON_MAX_REG) {
> >> + dev_err(dev, "invalid reg: 0x%02x\n", ch->reg);
> >> + return -EINVAL;
> >> + }
> >> + switch (ch->type) {
> >> + case type_temperature:
> >> + if (i_temp == GSC_HWMON_MAX_TEMP_CH) {
> >> + dev_err(dev, "too many temp channels\n");
> >> + return -EINVAL;
> >> + }
> >> + hwmon->temp_ch[i_temp] = ch;
> >> + hwmon->temp_config[i_temp] = HWMON_T_INPUT |
> >> + HWMON_T_LABEL;
> >> + i_temp++;
> >> + break;
> >> + case type_voltage:
> >> + case type_voltage_raw:
> >> + if (i_in == GSC_HWMON_MAX_IN_CH) {
> >> + dev_err(dev, "too many voltage channels\n");
> >> + return -EINVAL;
> >> + }
> >> + hwmon->in_ch[i_in] = ch;
> >> + hwmon->in_config[i_in] =
> >> + HWMON_I_INPUT | HWMON_I_LABEL;
> >> + i_in++;
> >> + break;
> >> + case type_fan:
> >> + if (i_fan == GSC_HWMON_MAX_FAN_CH) {
> >> + dev_err(dev, "too many voltage channels\n");
> >> + return -EINVAL;
> >> + }
> >> + hwmon->fan_ch[i_fan] = ch;
> >> + hwmon->fan_config[i_fan] =
> >> + HWMON_F_INPUT | HWMON_F_LABEL;
> >> + i_fan++;
> >> + break;
> >> + default:
> >> + dev_err(dev, "invalid type: %d\n", ch->type);
> >> + return -EINVAL;
> >> + }
> >> + dev_dbg(dev, "pdata: reg=0x%02x type=%d %s\n", ch->reg,
> >> + ch->type, ch->name);
> >> + }
> >> +
> >> + /* terminate channel config lists */
> >> + hwmon->temp_config[i_temp] = 0;
> >> + hwmon->in_config[i_in] = 0;
> >> + hwmon->fan_config[i_fan] = 0;
> >
> > 'hwmon' was alocated with devm_kzalloc(). Initializing any of its members with 0
> > is unnecessary.
>
> right - will remove
>
> >
> >> +
> >> + /* setup config structures */
> >> + hwmon->chip.ops = &gsc_hwmon_ops;
> >> + hwmon->chip.info = hwmon->info;
> >> + hwmon->info[0] = &hwmon->temp_info;
> >> + hwmon->info[1] = &hwmon->in_info;
> >> + hwmon->info[2] = &hwmon->fan_info;
> >> + hwmon->temp_info.type = hwmon_temp;
> >> + hwmon->temp_info.config = hwmon->temp_config;
> >> + hwmon->in_info.type = hwmon_in;
> >> + hwmon->in_info.config = hwmon->in_config;
> >> + hwmon->fan_info.type = hwmon_fan;
> >> + hwmon->fan_info.config = hwmon->fan_config;
> >> +
> >> + hwmon->dev = devm_hwmon_device_register_with_info(dev,
> >> + KBUILD_MODNAME, hwmon,
> >> + &hwmon->chip, NULL);
> >> + return PTR_ERR_OR_ZERO(hwmon->dev);
> >> +}
> >> +
> >> +static const struct of_device_id gsc_hwmon_of_match[] = {
> >> + { .compatible = "gw,gsc-hwmon", },
> >> + {}
> >> +};
> >> +
> >> +static struct platform_driver gsc_hwmon_driver = {
> >> + .driver = {
> >> + .name = KBUILD_MODNAME,
> >> + .of_match_table = gsc_hwmon_of_match,
> >> + },
> >> + .probe = gsc_hwmon_probe,
> >> +};
> >> +
> >> +module_platform_driver(gsc_hwmon_driver);
> >> +
> >> +MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
> >> +MODULE_DESCRIPTION("GSC hardware monitor driver");
> >> +MODULE_LICENSE("GPL v2");
> >> diff --git a/include/linux/platform_data/gsc_hwmon.h b/include/linux/platform_data/gsc_hwmon.h
> >> new file mode 100644
> >> index 0000000..5e59846
> >> --- /dev/null
> >> +++ b/include/linux/platform_data/gsc_hwmon.h
> >> @@ -0,0 +1,43 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +#ifndef _GSC_HWMON_H
> >> +#define _GSC_HWMON_H
> >> +
> >> +enum gsc_hwmon_type {
> >> + type_temperature,
> >> + type_voltage,
> >> + type_voltage_raw,
> >> + type_fan,
> >> +};
> >> +
> >> +/**
> >> + * struct gsc_hwmon_channel - configuration parameters
> >> + * @reg: I2C register offset
> >> + * @type: channel type
> >> + * @name: channel name
> >> + * @voffset: voltage offset (mV)
> >> + * @vdiv: voltage divider array (2 resistor values in ohms)
> >> + */
> >> +struct gsc_hwmon_channel {
> >> + unsigned int reg;
> >> + unsigned int type;
> >> + const char *name;
> >> + unsigned int voffset;
> >
> > It is interesting that only positive offset values are supported.
> > Is that intentional ?
>
> yes, they are only used for voltage rails that have a diode drop to
> adjust for and will only be positive. They are not used to fine tune
> offsets.
>
> >
> >> + unsigned int vdiv[2];
> >> +};
> >> +
> >> +/**
> >> + * struct gsc_hwmon_platform_data - platform data for gsc_hwmon driver
> >> + * @channels: pointer to array of gsc_hwmon_channel structures
> >> + * describing channels
> >> + * @nchannels: number of elements in @channels array
> >> + * @vreference: voltage reference (mV)
> >> + * @resolution: ADC resolution
> >
> > Resolution in what ?
>
> That's the bit-resolution of the ADC so I'll switch to that notation
> and document it as such. I just realized that the vref/resolution can
> technically change per ADC rail so I'll leave them in but move them
> down into the child nodes and switch resolution to be in bits.
>
> Thanks for the review!
>
> Tim
^ permalink raw reply
* Re: [PATCH v3 3/4] hwmon: add Gateworks System Controller support
From: Tim Harvey @ 2018-03-28 20:23 UTC (permalink / raw)
To: Guenter Roeck
Cc: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, linux-kernel, devicetree, linux-arm-kernel,
linux-hwmon, linux-input, linux-watchdog
In-Reply-To: <20180328170018.GC25325@roeck-us.net>
On Wed, Mar 28, 2018 at 10:00 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Wed, Mar 28, 2018 at 08:14:02AM -0700, 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.
>>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>> v3:
>> - add voltage_raw input type and supporting fields
>> - add channel validation to is_visible function
>> - remove unnecessary channel validation from read/write functions
>>
>> v2:
>> - change license comment style
>> - remove DEBUG
>> - simplify regmap_bulk_read err check
>> - remove break after returns in switch statement
>> - fix fan setpoint buffer address
>> - remove unnecessary parens
>> - consistently use struct device *dev pointer
>> - change license/comment block
>> - add validation for hwmon child node props
>> - move parsing of of to own function
>> - use strlcpy to ensure null termination
>> - fix static array sizes and removed unnecessary initializers
>> - dynamically allocate channels
>> - fix fan input label
>> - support platform data
>> - fixed whitespace issues
>>
>> drivers/hwmon/Kconfig | 9 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/gsc-hwmon.c | 368 ++++++++++++++++++++++++++++++++
>
> This will require a matching Documentation/hwmon/gsc-hwmon to explain supported
> attributes.
ok - will add in next submission
>
<snip>
>>
>> +static int
>> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> + int channel, long *val)
>> +{
>> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> + struct gsc_hwmon_platform_data *pdata = hwmon->pdata;
>> + const struct gsc_hwmon_channel *ch;
>> + int sz, ret;
>> + u8 buf[3];
>> +
>> + dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
>> + channel);
>> + switch (type) {
>> + case hwmon_in:
>> + ch = hwmon->in_ch[channel];
>> + break;
>> + case hwmon_temp:
>> + ch = hwmon->temp_ch[channel];
>> + break;
>> + case hwmon_fan:
>> + ch = hwmon->fan_ch[channel];
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + sz = (ch->type == type_voltage) ? 3 : 2;
>> + ret = regmap_bulk_read(hwmon->gsc->regmap_hwmon, ch->reg, buf, sz);
>> + if (ret)
>> + return ret;
>> +
>> + *val = 0;
>> + while (sz-- > 0)
>> + *val |= (buf[sz] << (8*sz));
>
> Please use spaces before and after operators.
ok
>
>> +
>> + switch (ch->type) {
>> + case type_temperature:
>> + if ((type == hwmon_temp) && *val > 0x8000)
>
> Please no unnecessary ( ).
>
ok
> Is there ever a situation where ch->type == type_temperature and type !=
> hwmon_temp ? Wouldn't that be a bug ?
I should not have been checking (type == hwmon_temp) there... will remove that.
>
>> + *val -= 0xffff;
>> + break;
>> + case type_voltage_raw:
>> + /* scale based on ref voltage and resolution */
>> + if (pdata->vreference && pdata->resolution) {
>> + *val *= pdata->vreference;
>> + *val /= pdata->resolution;
>> + }
>> + /* scale based on optional voltage divider */
>> + if (ch->vdiv[0] && ch->vdiv[1]) {
>> + *val *= (ch->vdiv[0] + ch->vdiv[1]);
>> + *val /= ch->vdiv[1];
>> + }
>
> This accepts both types of scaling. Is that intentional ?
yes that is intentional. One version of the GSC reports cooked
pre-scaled values that won't fall into either of these. Another
version of the GSC will report raw ADC values which will need the
vref/res scaling and those may have an optional voltage divider.
>
> I don't see any protection against overflows. What if pdata->vreference is
> larger than 256 on a system with sizeof(long) == 4 and the raw voltage
> as reported by the chip is 0xffffff ?
>
>> + /* adjust by offset */
>> + *val += ch->voffset;
>
> Similar to the above, this can result in an overflow on systems with
> sizeof(long) == 4.
>
true - I will add some overflow checking
>> + break;
>
> There should be a default case as well as 'case type_voltage:'
> with a break; statement and a comment indicating that no adjustment
> is needed.
>
ok
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
>> + u32 attr, int channel, const char **buf)
>> +{
>> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> +
>> + dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
>> + channel);
>
> I seriously wonder if all those dev_dbg() statements add value.
only to me while I was writing/testing; I will remove them
>
>> + switch (type) {
>> + case hwmon_in:
>> + *buf = hwmon->in_ch[channel]->name;
>> + break;
>> + case hwmon_temp:
>> + *buf = hwmon->temp_ch[channel]->name;
>> + break;
>> + case hwmon_fan:
>> + *buf = hwmon->fan_ch[channel]->name;
>> + break;
>> + default:
>> + return -ENOTSUPP;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int
>> +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> + int channel, long val)
>> +{
>> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> + u8 buf[2];
>> +
>> + dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
>> + channel);
>> + switch (type) {
>> + case hwmon_fan:
>> + buf[0] = val & 0xff;
>> + buf[1] = (val >> 8) & 0xff;
>> + return regmap_bulk_write(hwmon->gsc->regmap_hwmon,
>> + hwmon->fan_ch[channel]->reg, buf, 2);
>
> fanX_input reports the fan speed. By its nature, a fan speed is not writeable.
> I have no idea what this is supposed to achieve, but whatever it is, it is wrong.
>
> Please stick with the ABI.
ok, I see that now. Do you have any recommendation of what I should
use for these temperature setpoints that control when the fan pwm is
adjusted?
>
>> + default:
>> + break;
>> + }
>> +
>> + return -EOPNOTSUPP;
>> +}
>> +
>> +static umode_t
>> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
>> + int ch)
>> +{
>> + const struct gsc_hwmon_data *hwmon = _data;
>> + struct device *dev = hwmon->gsc->dev;
>> + umode_t mode = 0;
>> +
>> + switch (type) {
>> + case hwmon_fan:
>> + if (ch >= GSC_HWMON_MAX_FAN_CH)
>> + return -EOPNOTSUPP;
>
> Can that ever happen ?
No, I suppose not because the ch input comes from a hwmon node that
was created by the driver registering the entities and a check was
done there to avoid overflow. I will remove these.
>
>> + mode = S_IRUGO;
>> + if (attr == hwmon_fan_input)
>> + mode |= S_IWUSR;
>
> fanX_input is a read-only attribute per ABI.
>
>> + break;
>> + case hwmon_temp:
>> + if (ch >= GSC_HWMON_MAX_TEMP_CH)
>> + return -EOPNOTSUPP;
>
> Can that ever happen ?
>
>> + mode = S_IRUGO;
>> + break;
>> + case hwmon_in:
>> + if (ch >= GSC_HWMON_MAX_IN_CH)
>> + return -EOPNOTSUPP;
>
> Can that ever happen ?
>
>> + mode = S_IRUGO;
>> + break;
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> + dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__, type,
>> + attr, ch, mode);
>> +
>> + return mode;
>> +}
>> +
>> +static const struct hwmon_ops gsc_hwmon_ops = {
>> + .is_visible = gsc_hwmon_is_visible,
>> + .read = gsc_hwmon_read,
>> + .read_string = gsc_hwmon_read_string,
>> + .write = gsc_hwmon_write,
>> +};
>> +
>> +static struct gsc_hwmon_platform_data *
>> +gsc_hwmon_get_devtree_pdata(struct device *dev)
>> +{
>> + struct gsc_hwmon_platform_data *pdata;
>> + struct gsc_hwmon_channel *ch;
>> + struct fwnode_handle *child;
>> + const char *type;
>> + int nchannels;
>> +
>> + nchannels = device_get_child_node_count(dev);
>> + dev_dbg(dev, "channels=%d\n", nchannels);
>> + if (nchannels == 0)
>> + return ERR_PTR(-ENODEV);
>> +
>> + pdata = devm_kzalloc(dev,
>> + sizeof(*pdata) + nchannels * sizeof(*ch),
>> + GFP_KERNEL);
>> + if (!pdata)
>> + return ERR_PTR(-ENOMEM);
>> + ch = (struct gsc_hwmon_channel *)(pdata + 1);
>> + pdata->channels = ch;
>> + pdata->nchannels = nchannels;
>> +
>> + device_property_read_u32(dev, "gw,reference-voltage",
>> + &pdata->vreference);
>> + device_property_read_u32(dev, "gw,resolution", &pdata->resolution);
>> +
>> + /* allocate structures for channels and count instances of each type */
>> + device_for_each_child_node(dev, child) {
>> + if (fwnode_property_read_string(child, "label", &ch->name)) {
>> + dev_err(dev, "channel without label\n");
>> + fwnode_handle_put(child);
>> + return ERR_PTR(-EINVAL);
>> + }
>> + if (fwnode_property_read_u32(child, "reg", &ch->reg)) {
>> + dev_err(dev, "channel without reg\n");
>> + fwnode_handle_put(child);
>> + return ERR_PTR(-EINVAL);
>> + }
>> + if (fwnode_property_read_string(child, "type", &type)) {
>> + dev_err(dev, "channel without type\n");
>> + fwnode_handle_put(child);
>> + return ERR_PTR(-EINVAL);
>> + }
>> + if (!strcasecmp(type, "gw,hwmon-temperature"))
>> + ch->type = type_temperature;
>> + else if (!strcasecmp(type, "gw,hwmon-voltage"))
>> + ch->type = type_voltage;
>> + else if (!strcasecmp(type, "gw,hwmon-voltage-raw"))
>> + ch->type = type_voltage_raw;
>> + else if (!strcasecmp(type, "gw,hwmon-fan"))
>> + ch->type = type_fan;
>> + else {
>> + dev_err(dev, "channel without type\n");
>> + fwnode_handle_put(child);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + fwnode_property_read_u32(child, "gw,voltage-offset",
>> + &ch->voffset);
>
> Note that while it is technically ok to keep voltages internally in mV,
> devicetree will likely require specificayion in uV. For accuracy, it might be
> better to perform any calculations on that base and convert to mV for display
> purposes.
I'm happy to change them if that really is the standard but it doesn't
look like it is a standard?
>
>> + fwnode_property_read_u32_array(child, "gw,voltage-divider",
>> + ch->vdiv, ARRAY_SIZE(ch->vdiv));
>> + dev_dbg(dev, "of: reg=0x%02x type=%d %s\n", ch->reg, ch->type,
>> + ch->name);
>> + ch++;
>> + }
>> +
>> + return pdata;
>> +}
>> +
>> +static int gsc_hwmon_probe(struct platform_device *pdev)
>> +{
>> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
>> + struct device *dev = &pdev->dev;
>> + struct gsc_hwmon_platform_data *pdata = dev_get_platdata(dev);
>> + struct gsc_hwmon_data *hwmon;
>> + int i, i_in, i_temp, i_fan;
>> +
>> + if (!pdata) {
>> + pdata = gsc_hwmon_get_devtree_pdata(dev);
>> + if (IS_ERR(pdata))
>> + return PTR_ERR(pdata);
>> + }
>> +
>> + hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
>> + if (!hwmon)
>> + return -ENOMEM;
>> + hwmon->gsc = gsc;
>> + hwmon->pdata = pdata;
>> +
>> + for (i = 0, i_in = 0, i_temp = 0, i_fan = 0;
>> + i < hwmon->pdata->nchannels; i++) {
>> + const struct gsc_hwmon_channel *ch = &pdata->channels[i];
>> +
>> + if (ch->reg > GSC_HWMON_MAX_REG) {
>> + dev_err(dev, "invalid reg: 0x%02x\n", ch->reg);
>> + return -EINVAL;
>> + }
>> + switch (ch->type) {
>> + case type_temperature:
>> + if (i_temp == GSC_HWMON_MAX_TEMP_CH) {
>> + dev_err(dev, "too many temp channels\n");
>> + return -EINVAL;
>> + }
>> + hwmon->temp_ch[i_temp] = ch;
>> + hwmon->temp_config[i_temp] = HWMON_T_INPUT |
>> + HWMON_T_LABEL;
>> + i_temp++;
>> + break;
>> + case type_voltage:
>> + case type_voltage_raw:
>> + if (i_in == GSC_HWMON_MAX_IN_CH) {
>> + dev_err(dev, "too many voltage channels\n");
>> + return -EINVAL;
>> + }
>> + hwmon->in_ch[i_in] = ch;
>> + hwmon->in_config[i_in] =
>> + HWMON_I_INPUT | HWMON_I_LABEL;
>> + i_in++;
>> + break;
>> + case type_fan:
>> + if (i_fan == GSC_HWMON_MAX_FAN_CH) {
>> + dev_err(dev, "too many voltage channels\n");
>> + return -EINVAL;
>> + }
>> + hwmon->fan_ch[i_fan] = ch;
>> + hwmon->fan_config[i_fan] =
>> + HWMON_F_INPUT | HWMON_F_LABEL;
>> + i_fan++;
>> + break;
>> + default:
>> + dev_err(dev, "invalid type: %d\n", ch->type);
>> + return -EINVAL;
>> + }
>> + dev_dbg(dev, "pdata: reg=0x%02x type=%d %s\n", ch->reg,
>> + ch->type, ch->name);
>> + }
>> +
>> + /* terminate channel config lists */
>> + hwmon->temp_config[i_temp] = 0;
>> + hwmon->in_config[i_in] = 0;
>> + hwmon->fan_config[i_fan] = 0;
>
> 'hwmon' was alocated with devm_kzalloc(). Initializing any of its members with 0
> is unnecessary.
right - will remove
>
>> +
>> + /* setup config structures */
>> + hwmon->chip.ops = &gsc_hwmon_ops;
>> + hwmon->chip.info = hwmon->info;
>> + hwmon->info[0] = &hwmon->temp_info;
>> + hwmon->info[1] = &hwmon->in_info;
>> + hwmon->info[2] = &hwmon->fan_info;
>> + hwmon->temp_info.type = hwmon_temp;
>> + hwmon->temp_info.config = hwmon->temp_config;
>> + hwmon->in_info.type = hwmon_in;
>> + hwmon->in_info.config = hwmon->in_config;
>> + hwmon->fan_info.type = hwmon_fan;
>> + hwmon->fan_info.config = hwmon->fan_config;
>> +
>> + hwmon->dev = devm_hwmon_device_register_with_info(dev,
>> + KBUILD_MODNAME, hwmon,
>> + &hwmon->chip, NULL);
>> + return PTR_ERR_OR_ZERO(hwmon->dev);
>> +}
>> +
>> +static const struct of_device_id gsc_hwmon_of_match[] = {
>> + { .compatible = "gw,gsc-hwmon", },
>> + {}
>> +};
>> +
>> +static struct platform_driver gsc_hwmon_driver = {
>> + .driver = {
>> + .name = KBUILD_MODNAME,
>> + .of_match_table = gsc_hwmon_of_match,
>> + },
>> + .probe = gsc_hwmon_probe,
>> +};
>> +
>> +module_platform_driver(gsc_hwmon_driver);
>> +
>> +MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
>> +MODULE_DESCRIPTION("GSC hardware monitor driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/platform_data/gsc_hwmon.h b/include/linux/platform_data/gsc_hwmon.h
>> new file mode 100644
>> index 0000000..5e59846
>> --- /dev/null
>> +++ b/include/linux/platform_data/gsc_hwmon.h
>> @@ -0,0 +1,43 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _GSC_HWMON_H
>> +#define _GSC_HWMON_H
>> +
>> +enum gsc_hwmon_type {
>> + type_temperature,
>> + type_voltage,
>> + type_voltage_raw,
>> + type_fan,
>> +};
>> +
>> +/**
>> + * struct gsc_hwmon_channel - configuration parameters
>> + * @reg: I2C register offset
>> + * @type: channel type
>> + * @name: channel name
>> + * @voffset: voltage offset (mV)
>> + * @vdiv: voltage divider array (2 resistor values in ohms)
>> + */
>> +struct gsc_hwmon_channel {
>> + unsigned int reg;
>> + unsigned int type;
>> + const char *name;
>> + unsigned int voffset;
>
> It is interesting that only positive offset values are supported.
> Is that intentional ?
yes, they are only used for voltage rails that have a diode drop to
adjust for and will only be positive. They are not used to fine tune
offsets.
>
>> + unsigned int vdiv[2];
>> +};
>> +
>> +/**
>> + * struct gsc_hwmon_platform_data - platform data for gsc_hwmon driver
>> + * @channels: pointer to array of gsc_hwmon_channel structures
>> + * describing channels
>> + * @nchannels: number of elements in @channels array
>> + * @vreference: voltage reference (mV)
>> + * @resolution: ADC resolution
>
> Resolution in what ?
That's the bit-resolution of the ADC so I'll switch to that notation
and document it as such. I just realized that the vref/resolution can
technically change per ADC rail so I'll leave them in but move them
down into the child nodes and switch resolution to be in bits.
Thanks for the review!
Tim
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: mfd: Add Gateworks System Controller bindings
From: Guenter Roeck @ 2018-03-28 20:23 UTC (permalink / raw)
To: Tim Harvey
Cc: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, linux-kernel, devicetree, linux-arm-kernel,
linux-hwmon, linux-input, linux-watchdog
In-Reply-To: <CAJ+vNU2aUN4ME3+RA3DNPO75O6vPBcQpuRSfQJ4_Ns_zQAtCCQ@mail.gmail.com>
On Wed, Mar 28, 2018 at 12:17:34PM -0700, Tim Harvey wrote:
> On Wed, Mar 28, 2018 at 9:24 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Wed, Mar 28, 2018 at 08:14:00AM -0700, Tim Harvey wrote:
> >> This patch adds documentation of device-tree bindings for the
> >> Gateworks System Controller (GSC).
> >>
> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >> ---
> >> v3:
> >> - replaced _ with -
> >> - remove input bindings
> >> - added full description of hwmon
> >> - fix unit address of hwmon child nodes
> >>
> >> ---
> >> .../devicetree/bindings/mfd/gateworks-gsc.txt | 135 +++++++++++++++++++++
> >> 1 file changed, 135 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
> >> new file mode 100644
> >> index 0000000..8f530ed
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
> >> @@ -0,0 +1,135 @@
> >> +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"
> >> +- 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
> >> + <devicetree/bindings/interrupt-controller/interrupt.txt>
> >> +
> >> +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 watchdog properties:
> >> +- compatible: must be "gw,gsc-watchdog"
> >> +
> >> +* hwmon:
> >> +The GSC provides a set of Analog to Digitcal Converter (ADC) pins used for
> >> +temperature and/or voltage monitoring.
> >> +
> >> +Required hwmon properties:
> >> +- compatible: must be "gw,gsc-hwmon"
> >> +
> >
> > "hwmon" is a very Linux specific term. It might make sense to find a more
> > generic term.
>
> The 'hwmon' driver supports child nodes that fall into the following category:
> - temperature sensor (GSC internal temperature sensor - i2c registers
> returns value in C*10)
> - voltage rails (two types here; cooked: i2c registers return
> pre-scaled value in mV), raw: i2c registers return a raw ADC value
> that must be scaled based on ADC internal ref voltage and resolution
> and adjusted for a voltage divider to convert to mV
> - fan setpoints (I'll explain these below)
>
> I called the node 'gw,gsc-hwmon' because the driver fits into the
> 'hwmon' API. Isn't that appropriate here for the driver compatible
> string?
>
Devicetree properties are supposed to be OS independent.
> >
> >> +Optional hwmon properties:
> >> +- gw,reference-voltage: ADC reference voltage (mV) used in scaling raw ADCs
> >
> > AFAIK devicetree likes to specify voltages in uV.
>
> There are currently plenty of dt props specified in mV (grep -r mV
> Documentation/devicetree/bindings/).
>
"But so many others are speeding, why do I get a ticket ?"
Please discuss with Rob.
> >
> >> +- gw,resolution: ADC resolution (ie 4096) used in scaling raw ADCs
> >> +
> >
> > 4096 what ?
>
> reference-voltage and resolution are used to scale the values from the
> nodes that report a raw ADC value:
>
> V = Vadc * (reference-voltage / resolution)
>
> I can provide that in bits if it makes more sense? I can also hard
Yes, I think that would make more sense, and please describe what it means.
> code both the resolution and the vref in the hwmon driver and remove
> it from dt as currently the only GSC that uses raw ADC values is 12bit
> with 2.5V ref.
>
That would be even better.
> >
> >> +Each hwmon child node defines an ADC input on the chip which the GSC may
> >> +report cooked values (ie temperature sensor based on thermister), raw values,
> >> +(ie voltage rail with a pre-scaling resistor divider), or a fan controller
> >> +setpoint.
> >> +
> >> +Required hwmon child properties:
> >> +- type: one of the following ADC types:
> >> + "gw,hwmon-temperature" - reports temperature in C*10
> >> + "gw,hwmon-voltage" - reports a pre-scaled voltage value
> >> + "gw,hwmon-voltage-raw" - reports a raw ADC that is scaled with
> >> + vreference, resolution, and optional resistor divider
> >> + "gw,hwmon-fan" - a fan temperature setpoint in C*10
> >
> > What is a "fan temperature setpoint" ?
> >
>
> The GSC supports a fan controller which drives a PWM signal to vary
> the speed of a fan based on the GSC internal temperature sensor. The
> FAN controller has 6 setpoints each having a fixed PWM duty-cycle but
> the temperature at which those setpoints kick in can be varies via
> registers at the 0x29 slave address (same slave address as the
> temperature sensor and voltage inputs which is why I have it in the
> hwmon driver):
>
> fan0_point - 50% PWM (default 300)
> fan1_point - 60% PWM (default 330)
> fan2_point - 70% PWM (default 360)
> fan3_point - 80% PWM (default 390)
> fan4_point - 90% PWM (default 420)
> fan5_point - 100% PWM (default 450)
>
> The values are C/10 thus if the internal GSC temp sensor is below 30C
> the fan output will be 0% duty cycle and if it hits 30C it will go to
> 50% until it hits 60% at 33C etc.
>
Please do not define your own scaling factors. pwm values are 0..255,
and temperatures are in milli-degrees C.
> That is the hardware implementation that I'm trying to abstract and
> define here. You pointed out the fact that the fan*_input ABI is
> read-only fan PWM and I see that now. What do you suggest I use for
No, it isn't. It is the fan speed in RPM.
> this feature I'm trying to implement driver support for?
>
pwm[1-*]_auto_point[1-*]_pwm
pwm[1-*]_auto_point[1-*]_temp
pwm[1-*]_auto_point[1-*]_temp_hyst
may be relevant. From the context, something like
pwm1_auto_point1_pwm read-only, set to 128
pwm1_auto_point1_temp 30000
pwm1_auto_point2_pwm read-only, set to 153
pwm1_auto_point2_temp 33000
pwm1_auto_point3_pwm read-only, set to 179
pwm1_auto_point3_temp 36000
pwm1_auto_point4_pwm read-only, set to 204
pwm1_auto_point4_temp 39000
pwm1_auto_point5_pwm read-only, set to 230
pwm1_auto_point5_temp 42000
pwm1_auto_point6_pwm read-only, set to 255
pwm1_auto_point6_temp 45000
might make sense.
> I did notice that nouveau_hwmon.c has a single temperature setpoint
> similar to this that they support with SENSOR_DEVICE_ATTR:
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/nouveau/nouveau_hwmon.c#L63
>
Whatever nouveau_hwmon.c does is, for all practical purposes, absolutely
irrelevant. This code has never been reviewed by a hwmon maintainer.
It may (or may not) be complete junk. Please do not use anything outside
drivers/hwmon as example.
> >> +- reg: offset of the ADC register
> >> +- label: name of the ADC input or FAN setpoint
> >> +
> >> +Optional hwmon child properties:
> >> +- gw,voltage-divider: An array of two integers containing the resistor
> >> + values R1 and R2 of the optinal resistor divider on a raw ADC
> >> +- gw,voltage-offset: a mV voltage offset to apply to a raw ADC (ie to
> >> + compensate for a diode drop)
> >> +
> >> +Example:
> >> +
> >> + gsc: gsc@20 {
> >> + compatible = "gw,gsc";
> >> + reg = <0x20>;
> >> + interrupt-parent = <&gpio1>;
> >> + interrupts = <4 GPIO_ACTIVE_LOW>;
> >> + interrupt-controller;
> >> + #interrupt-cells = <1>;
> >> +
> >> + watchdog {
> >> + compatible = "gw,gsc-watchdog";
> >> + };
> >> +
> >> + hwmon {
> >> + compatible = "gw,gsc-hwmon";
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> + gw,reference-voltage = <2500>;
> >> + gw,resolution = <4096>;
> >> +
> >> + hwmon@0 { /* A0: Board Temperature */
> >> + type = "gw,hwmon-temperature";
> >> + reg = <0x00>;
> >> + label = "temp";
> >> + };
> >> +
> >> + hwmon@2 { /* A1: Input Voltage (raw ADC) */
> >> + type = "gw,hwmon-voltage-raw";
> >> + reg = <0x02>;
> >> + label = "vdd_vin";
> >> + gw,voltage-divider = <22100 1000>;
> >> + gw,voltage-offset = <800>;
> >> + };
> >> +
> >> + hwmon@b { /* A2: Battery voltage */
> >> + type = "gw,hwmon-voltage";
> >> + reg = <0x0b>;
> >> + label = "vdd_bat";
> >> + };
> >> +
> >> + hwmon@2c { /* fan temperature setpoint for 50% duty */
> >> + type = "gw,hwmon-fan";
> >> + reg = <0x2c>;
> >> + label = "fan_50p";
> >> + };
> >> +
> >> + hwmon@2e { /* fan1 */
> >> + type = "gw,hwmon-fan";
> >> + reg = <0x2e>;
> >> + label = "fan_60p";
> >> + };
> >> +
> >> + hwmon@30 { /* fan2 */
> >> + type = "gw,hwmon-fan";
> >> + reg = <0x30>;
> >> + label = "fan_70p";
> >> + };
> >> +
> >> + hwmon@32 { /* fan3 */
> >> + type = "gw,hwmon-fan";
> >> + reg = <0x32>;
> >> + label = "fan_80p";
> >> + };
> >> +
> >> + hwmon@34 { /* fan4 */
> >> + type = "gw,hwmon-fan";
> >> + reg = <0x34>;
> >> + label = "fan_90p";
> >> + };
> >> +
> >> + hwmon@36 { /* fan5 */
> >> + type = "gw,hwmon-fan";
> >> + reg = <0x36>;
> >> + label = "fan_100p";
> >> + };
> >
> > No idea what this is supposed to be doing, but whatever it is,
> > it appears to be wrong. I'll comment more on it in the hwmon driver.
> >
>
> ok - I'll respond to that thread.
>
> Thanks for the review!
>
> Tim
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: mfd: Add Gateworks System Controller bindings
From: Tim Harvey @ 2018-03-28 19:17 UTC (permalink / raw)
To: Guenter Roeck
Cc: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, linux-kernel, devicetree, linux-arm-kernel,
linux-hwmon, linux-input, linux-watchdog
In-Reply-To: <20180328162416.GB25325@roeck-us.net>
On Wed, Mar 28, 2018 at 9:24 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Wed, Mar 28, 2018 at 08:14:00AM -0700, Tim Harvey wrote:
>> This patch adds documentation of device-tree bindings for the
>> Gateworks System Controller (GSC).
>>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>> v3:
>> - replaced _ with -
>> - remove input bindings
>> - added full description of hwmon
>> - fix unit address of hwmon child nodes
>>
>> ---
>> .../devicetree/bindings/mfd/gateworks-gsc.txt | 135 +++++++++++++++++++++
>> 1 file changed, 135 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
>> new file mode 100644
>> index 0000000..8f530ed
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
>> @@ -0,0 +1,135 @@
>> +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"
>> +- 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
>> + <devicetree/bindings/interrupt-controller/interrupt.txt>
>> +
>> +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 watchdog properties:
>> +- compatible: must be "gw,gsc-watchdog"
>> +
>> +* hwmon:
>> +The GSC provides a set of Analog to Digitcal Converter (ADC) pins used for
>> +temperature and/or voltage monitoring.
>> +
>> +Required hwmon properties:
>> +- compatible: must be "gw,gsc-hwmon"
>> +
>
> "hwmon" is a very Linux specific term. It might make sense to find a more
> generic term.
The 'hwmon' driver supports child nodes that fall into the following category:
- temperature sensor (GSC internal temperature sensor - i2c registers
returns value in C*10)
- voltage rails (two types here; cooked: i2c registers return
pre-scaled value in mV), raw: i2c registers return a raw ADC value
that must be scaled based on ADC internal ref voltage and resolution
and adjusted for a voltage divider to convert to mV
- fan setpoints (I'll explain these below)
I called the node 'gw,gsc-hwmon' because the driver fits into the
'hwmon' API. Isn't that appropriate here for the driver compatible
string?
>
>> +Optional hwmon properties:
>> +- gw,reference-voltage: ADC reference voltage (mV) used in scaling raw ADCs
>
> AFAIK devicetree likes to specify voltages in uV.
There are currently plenty of dt props specified in mV (grep -r mV
Documentation/devicetree/bindings/).
>
>> +- gw,resolution: ADC resolution (ie 4096) used in scaling raw ADCs
>> +
>
> 4096 what ?
reference-voltage and resolution are used to scale the values from the
nodes that report a raw ADC value:
V = Vadc * (reference-voltage / resolution)
I can provide that in bits if it makes more sense? I can also hard
code both the resolution and the vref in the hwmon driver and remove
it from dt as currently the only GSC that uses raw ADC values is 12bit
with 2.5V ref.
>
>> +Each hwmon child node defines an ADC input on the chip which the GSC may
>> +report cooked values (ie temperature sensor based on thermister), raw values,
>> +(ie voltage rail with a pre-scaling resistor divider), or a fan controller
>> +setpoint.
>> +
>> +Required hwmon child properties:
>> +- type: one of the following ADC types:
>> + "gw,hwmon-temperature" - reports temperature in C*10
>> + "gw,hwmon-voltage" - reports a pre-scaled voltage value
>> + "gw,hwmon-voltage-raw" - reports a raw ADC that is scaled with
>> + vreference, resolution, and optional resistor divider
>> + "gw,hwmon-fan" - a fan temperature setpoint in C*10
>
> What is a "fan temperature setpoint" ?
>
The GSC supports a fan controller which drives a PWM signal to vary
the speed of a fan based on the GSC internal temperature sensor. The
FAN controller has 6 setpoints each having a fixed PWM duty-cycle but
the temperature at which those setpoints kick in can be varies via
registers at the 0x29 slave address (same slave address as the
temperature sensor and voltage inputs which is why I have it in the
hwmon driver):
fan0_point - 50% PWM (default 300)
fan1_point - 60% PWM (default 330)
fan2_point - 70% PWM (default 360)
fan3_point - 80% PWM (default 390)
fan4_point - 90% PWM (default 420)
fan5_point - 100% PWM (default 450)
The values are C/10 thus if the internal GSC temp sensor is below 30C
the fan output will be 0% duty cycle and if it hits 30C it will go to
50% until it hits 60% at 33C etc.
That is the hardware implementation that I'm trying to abstract and
define here. You pointed out the fact that the fan*_input ABI is
read-only fan PWM and I see that now. What do you suggest I use for
this feature I'm trying to implement driver support for?
I did notice that nouveau_hwmon.c has a single temperature setpoint
similar to this that they support with SENSOR_DEVICE_ATTR:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/nouveau/nouveau_hwmon.c#L63
>> +- reg: offset of the ADC register
>> +- label: name of the ADC input or FAN setpoint
>> +
>> +Optional hwmon child properties:
>> +- gw,voltage-divider: An array of two integers containing the resistor
>> + values R1 and R2 of the optinal resistor divider on a raw ADC
>> +- gw,voltage-offset: a mV voltage offset to apply to a raw ADC (ie to
>> + compensate for a diode drop)
>> +
>> +Example:
>> +
>> + gsc: gsc@20 {
>> + compatible = "gw,gsc";
>> + reg = <0x20>;
>> + interrupt-parent = <&gpio1>;
>> + interrupts = <4 GPIO_ACTIVE_LOW>;
>> + interrupt-controller;
>> + #interrupt-cells = <1>;
>> +
>> + watchdog {
>> + compatible = "gw,gsc-watchdog";
>> + };
>> +
>> + hwmon {
>> + compatible = "gw,gsc-hwmon";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + gw,reference-voltage = <2500>;
>> + gw,resolution = <4096>;
>> +
>> + hwmon@0 { /* A0: Board Temperature */
>> + type = "gw,hwmon-temperature";
>> + reg = <0x00>;
>> + label = "temp";
>> + };
>> +
>> + hwmon@2 { /* A1: Input Voltage (raw ADC) */
>> + type = "gw,hwmon-voltage-raw";
>> + reg = <0x02>;
>> + label = "vdd_vin";
>> + gw,voltage-divider = <22100 1000>;
>> + gw,voltage-offset = <800>;
>> + };
>> +
>> + hwmon@b { /* A2: Battery voltage */
>> + type = "gw,hwmon-voltage";
>> + reg = <0x0b>;
>> + label = "vdd_bat";
>> + };
>> +
>> + hwmon@2c { /* fan temperature setpoint for 50% duty */
>> + type = "gw,hwmon-fan";
>> + reg = <0x2c>;
>> + label = "fan_50p";
>> + };
>> +
>> + hwmon@2e { /* fan1 */
>> + type = "gw,hwmon-fan";
>> + reg = <0x2e>;
>> + label = "fan_60p";
>> + };
>> +
>> + hwmon@30 { /* fan2 */
>> + type = "gw,hwmon-fan";
>> + reg = <0x30>;
>> + label = "fan_70p";
>> + };
>> +
>> + hwmon@32 { /* fan3 */
>> + type = "gw,hwmon-fan";
>> + reg = <0x32>;
>> + label = "fan_80p";
>> + };
>> +
>> + hwmon@34 { /* fan4 */
>> + type = "gw,hwmon-fan";
>> + reg = <0x34>;
>> + label = "fan_90p";
>> + };
>> +
>> + hwmon@36 { /* fan5 */
>> + type = "gw,hwmon-fan";
>> + reg = <0x36>;
>> + label = "fan_100p";
>> + };
>
> No idea what this is supposed to be doing, but whatever it is,
> it appears to be wrong. I'll comment more on it in the hwmon driver.
>
ok - I'll respond to that thread.
Thanks for the review!
Tim
^ permalink raw reply
* Re: [PATCH v7 0/2] hid-steam driver with user mode client dection
From: Rodrigo Rivas Costa @ 2018-03-28 18:14 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Pierre-Loup A. Griffais, Clément VUCHENER, Jiri Kosina,
Cameron Gutman, lkml, linux-input
In-Reply-To: <CAO-hwJ+XxB9QnZSAALPZTWmAMDsrzq-dhjtMDOvdXHWtDv=H7g@mail.gmail.com>
On Mon, Mar 26, 2018 at 10:12:19AM +0200, Benjamin Tissoires wrote:
> Hi Rodrigo,
>
> On Sun, Mar 25, 2018 at 6:07 PM, Rodrigo Rivas Costa
> <rodrigorivascosta@gmail.com> wrote:
> > This is a reroll of the Steam Controller driver.
> >
> > This time the client usage is detected by using exposing a custom hidraw
> > device (hid_ll_driver) to userland and forwarding that to the real one.
>
> That is actually more clever than what I had in mind.
> This way you reliably know if the hidraw node has been opened without
> touching hid-core.c or hidraw.c, well played :)
8-)
> > About how the lizard-mode/hidraw-client issue is handled, I have added a
> > module parameter (I don't know if that is the best option, but it helps me
> > illustrate different approaches to the problem):
> >
> > Independently of the lizard_mode parameter value:
>
> I find the values of the lizard_mode parameter hard to understand. And
> honestly, I do not think there is much a difference between 0 and 1.
> AFAICT, the difference between these two values is that in the first
> case you are disabling the lizard mode whether or not the joystick is
> in used, and in the latter, you disable it only when the joystick is
> in use. Does that gives any benefits to users?
Well, the rules I'm trying to apply are, in order of priority:
1. Never send a lizard command when hidraw is in use.
2. Disable the lizard mode when the input node is in use.
Now, the freedom is in what to do when the input node and hidraw are
both unused, hence the parameters. The two easy options are:
* lizard_mode=1: lizard mode is on, same experience as without
hid-steam, so it is the default.
* lizard_mode=0: lizard mode is off. I always use a mouse and
keyboard together with the controller, so I like this one.
But that leaves a marginal use case uncovered. What if I want to manage
the lizard mode using a user mode tool, such as my steamctrl from
github? This tool opens hidraw and sends a lizard_mode command (enable
or disable), but it does not matter, because as soon as hidraw is closed
hid-steam will reset the lizard mode to the value of the lizard_mode
parameter.
That is why I added lizard_mode=2: it instructs hid-steam not to send
any lizard mode command at all. You do not get any magic (I even break
rule 2 above) but you can use steamctrl freely.
Summing up:
* lizard_mode = 0: disabled.
* lizard_mode = 1: enabled (except when input is in use).
* lizard_mode = 2: no magic (use a user mode tool or expect weirdness).
I don't think anybody is actually using a user-mode tool for that so
maybe mode 2 can be discarded.
Note that Steam disables the lizard_mode when it starts and restores it
when it closes, unconditionally, so default option lizard_mode=1 will be
indistinguishable from hid-generic for the average user... except if
Steam crashes, then you will have lizard mode reenabled automatically,
which is nice.
The other two lizard_mode={0,1} I think they are quite useful, and I'd
like to keep them.
> I would think having a boolean "let the kernel handle the lizard mode
> with some magic inside" would be simpler to understand. You do not
> really want to support too many configurations and I think it would be
> wiser to say either disable the new mode or just enable it.
That would be the lizard_mode={0,1} I talked about? Both of them have
magic. The magicless option was the 2.
> Also, I think there will be races if a user changes the value of the
> parameter while running the system. You might want to add an
> additional patch that would trigger the mode change on module
> parameter change.
True, but the races should be easy to remove, with a simple local
variable. About the trigger, I've never seen a trigger on module
parameter change. Can you give me a hint or example of how to do that.
> As far as I can tell, I am happy with such hack. There are a few
> points I'd like to raise in the patch 1, but I'll inline my comments
> there.
I'll address them promptly.
Thanks.
Rodrigo
> Cheers,
> Benjamin
>
> >
> > 1. When a hidraw client is running:
> > a. I will not send any lizard-mode command to the device, so the client is
> > in full control.
> > b. The input device will not send any input message. However it will not
> > dissappear nor return any error message. Maybe it could return ENODEV if
> > they try to open the input device when hidraw client is running?
> >
> > If lizard_mode == 0 ('disabled'):
> > 2.0. When a hidraw client is not running, lizard_mode is disabled.
> >
> > If lizard_mode == 1, ('auto', the default):
> > 2.1. When a hidraw client is not running:
> > a. When the input device is not in use, lizard mode is enabled.
> > b. When the input device is in use, lizard mode is disabled.
> >
> > If lizard_mode == 2 ('usermode'):
> > 2.2. This driver does not send any lizard_mode-related command. So it is up
> > to user mode to configure it, with steamctrl or whatever.
> >
> > Note that when Steam Client opens it always disables lizard-mode (it creates
> > keyboard/mouse XTest inputs with the same function, though). And when it closed
> > (but not when it crashes, I think) it always re-enables the lizard mode.
> >
> > About the input buttons/axes mapping, as per Clément suggestion, I tried to
> > conform to Documentation/gamepad.rst, but with a few caveats and doubts:
> > * BTN_NORTH/BTN_WEST are alias of BTN_X/BTN_Y, but those buttons in this
> > controller have the labels changed (BTN_NORTH is actually Y). I don't know
> > the best option, so for now I'm mapping 'Y' to BTN_Y and 'X' to BTN_X.
> > * I'm mapping the lpad clicks to BTN_DPAD_{UP,RIGHT,DOWN,LEFT} but I'm not
> > sure if that is such a good idea: it cannot do diagonals, for example.
> > Maybe we could fake the whole dpad from the touch position?
> > * I'm mapping pressing the joystick to BTN_THUMBL and clicking the rpad to
> > BTN_THUMBR. Clicking the lpad is unmapped because that is used for the
> > dpad, depending on where it is clicked.
> > * Currently I'm mapping the lpad-touch to BTN_THUMB and rpad-touch to
> > BTN_THUMB2, but I don't know if that is so useful. lpad-touch will overlap
> > with any use of the dpad. And rpad-touch will overlap with rpad-click...
> >
> > Changes in v7:
> > * All the automatic lizard_mode stuff.
> > * Added the lizard_mode parameter.
> > * The patchset is reduced to 2 commits. The separation of the
> > steam_get_serial command no longer makes sense, since I need the
> > steam_send_cmd in the first commit to implement the lizard mode.
> > * Change the input mapping to conform to Documentation/gamepad.rst.
> >
> > (v6 was a RFC, it does not count).
> >
> > Changes in v5:
> > * Fix license SPDX to GPL-2.0+.
> > * Minor stylistic changes (BIT(3) instead 0x08 and so on).
> >
> > Changes in v4:
> > * Add command to check the wireless connection status on probe, without
> > waiting for a message (thanks to Clément Vuchener for the tip).
> > * Removed the error code on redundant connection/disconnection messages. That
> > was harmless but polluted dmesg.
> > * Added buttons for touching the left-pad and right-pad.
> > * Fixed a misplaced #include from 2/4 to 1/4.
> >
> > Changes in v3:
> > * Use RCU to do the dynamic connec/disconnect of wireless devices.
> > * Remove entries in hid-quirks.c as they are no longer needed. This allows
> > this module to be blacklisted without side effects.
> > * Do not bypass the virtual keyboard/mouse HID devices to avoid breaking
> > existing use cases (lizard mode). A user-space tool to do that is
> > linked.
> > * Fully separated axes for joystick and left-pad. As it happens.
> > * Add fuzz values for left/right pad axes, they are a little wiggly.
> >
> > Changes in v2:
> > * Remove references to USB. Now the interesting interfaces are selected by
> > looking for the ones with feature reports.
> > * Feature reports buffers are allocated with hid_alloc_report_buf().
> > * Feature report length is checked, to avoid overflows in case of
> > corrupt/malicius USB devices.
> > * Resolution added to the ABS axes.
> > * A lot of minor cleanups.
> >
> > Rodrigo Rivas Costa (2):
> > HID: add driver for Valve Steam Controller
> > HID: steam: add battery device.
> >
> > drivers/hid/Kconfig | 8 +
> > drivers/hid/Makefile | 1 +
> > drivers/hid/hid-ids.h | 4 +
> > drivers/hid/hid-steam.c | 978 ++++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/hid.h | 1 +
> > 5 files changed, 992 insertions(+)
> > create mode 100644 drivers/hid/hid-steam.c
> >
> > --
> > 2.16.2
> >
^ permalink raw reply
* Re: [PATCH v3 3/4] hwmon: add Gateworks System Controller support
From: Guenter Roeck @ 2018-03-28 17:00 UTC (permalink / raw)
To: Tim Harvey
Cc: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, linux-kernel, devicetree, linux-arm-kernel,
linux-hwmon, linux-input, linux-watchdog
In-Reply-To: <1522250043-8065-4-git-send-email-tharvey@gateworks.com>
On Wed, Mar 28, 2018 at 08:14:02AM -0700, 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.
>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
> v3:
> - add voltage_raw input type and supporting fields
> - add channel validation to is_visible function
> - remove unnecessary channel validation from read/write functions
>
> v2:
> - change license comment style
> - remove DEBUG
> - simplify regmap_bulk_read err check
> - remove break after returns in switch statement
> - fix fan setpoint buffer address
> - remove unnecessary parens
> - consistently use struct device *dev pointer
> - change license/comment block
> - add validation for hwmon child node props
> - move parsing of of to own function
> - use strlcpy to ensure null termination
> - fix static array sizes and removed unnecessary initializers
> - dynamically allocate channels
> - fix fan input label
> - support platform data
> - fixed whitespace issues
>
> drivers/hwmon/Kconfig | 9 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/gsc-hwmon.c | 368 ++++++++++++++++++++++++++++++++
This will require a matching Documentation/hwmon/gsc-hwmon to explain supported
attributes.
> include/linux/platform_data/gsc_hwmon.h | 43 ++++
> 4 files changed, 421 insertions(+)
> create mode 100644 drivers/hwmon/gsc-hwmon.c
> create mode 100644 include/linux/platform_data/gsc_hwmon.h
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 7ad0176..85d9aa3 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -475,6 +475,15 @@ 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_GATEWORKS_GSC
> + help
> + Support for the Gateworks System Controller A/D converters.
> +
> + To compile this driver as a module, choose M here:
> + the module will be called gsc-hwmon.
> +
> 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 0000000..10da46f
> --- /dev/null
> +++ b/drivers/hwmon/gsc-hwmon.c
> @@ -0,0 +1,368 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * Copyright (C) 2018 Gateworks Corporation
> + *
> + * This driver registers Linux HWMON attributes for GSC ADC's
> + */
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/mfd/gsc.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#include <linux/platform_data/gsc_hwmon.h>
> +
> +#define GSC_HWMON_MAX_TEMP_CH 16
> +#define GSC_HWMON_MAX_IN_CH 16
> +#define GSC_HWMON_MAX_FAN_CH 6
> +
> +struct gsc_hwmon_data {
> + struct gsc_dev *gsc;
> + struct device *dev;
> + struct gsc_hwmon_platform_data *pdata;
> + const struct gsc_hwmon_channel *temp_ch[GSC_HWMON_MAX_TEMP_CH];
> + const struct gsc_hwmon_channel *in_ch[GSC_HWMON_MAX_IN_CH];
> + const struct gsc_hwmon_channel *fan_ch[GSC_HWMON_MAX_FAN_CH];
> + u32 temp_config[GSC_HWMON_MAX_TEMP_CH + 1];
> + u32 in_config[GSC_HWMON_MAX_IN_CH + 1];
> + u32 fan_config[GSC_HWMON_MAX_FAN_CH + 1];
> + struct hwmon_channel_info temp_info;
> + struct hwmon_channel_info in_info;
> + struct hwmon_channel_info fan_info;
> + const struct hwmon_channel_info *info[4];
> + struct hwmon_chip_info chip;
> + int vreference;
> + int resolution;
> +};
> +
> +static int
> +gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long *val)
> +{
> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
> + struct gsc_hwmon_platform_data *pdata = hwmon->pdata;
> + const struct gsc_hwmon_channel *ch;
> + int sz, ret;
> + u8 buf[3];
> +
> + dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
> + channel);
> + switch (type) {
> + case hwmon_in:
> + ch = hwmon->in_ch[channel];
> + break;
> + case hwmon_temp:
> + ch = hwmon->temp_ch[channel];
> + break;
> + case hwmon_fan:
> + ch = hwmon->fan_ch[channel];
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + sz = (ch->type == type_voltage) ? 3 : 2;
> + ret = regmap_bulk_read(hwmon->gsc->regmap_hwmon, ch->reg, buf, sz);
> + if (ret)
> + return ret;
> +
> + *val = 0;
> + while (sz-- > 0)
> + *val |= (buf[sz] << (8*sz));
Please use spaces before and after operators.
> +
> + switch (ch->type) {
> + case type_temperature:
> + if ((type == hwmon_temp) && *val > 0x8000)
Please no unnecessary ( ).
Is there ever a situation where ch->type == type_temperature and type !=
hwmon_temp ? Wouldn't that be a bug ?
> + *val -= 0xffff;
> + break;
> + case type_voltage_raw:
> + /* scale based on ref voltage and resolution */
> + if (pdata->vreference && pdata->resolution) {
> + *val *= pdata->vreference;
> + *val /= pdata->resolution;
> + }
> + /* scale based on optional voltage divider */
> + if (ch->vdiv[0] && ch->vdiv[1]) {
> + *val *= (ch->vdiv[0] + ch->vdiv[1]);
> + *val /= ch->vdiv[1];
> + }
This accepts both types of scaling. Is that intentional ?
I don't see any protection against overflows. What if pdata->vreference is
larger than 256 on a system with sizeof(long) == 4 and the raw voltage
as reported by the chip is 0xffffff ?
> + /* adjust by offset */
> + *val += ch->voffset;
Similar to the above, this can result in an overflow on systems with
sizeof(long) == 4.
> + break;
There should be a default case as well as 'case type_voltage:'
with a break; statement and a comment indicating that no adjustment
is needed.
> + }
> +
> + return 0;
> +}
> +
> +static int
> +gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, const char **buf)
> +{
> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
> +
> + dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
> + channel);
I seriously wonder if all those dev_dbg() statements add value.
> + switch (type) {
> + case hwmon_in:
> + *buf = hwmon->in_ch[channel]->name;
> + break;
> + case hwmon_temp:
> + *buf = hwmon->temp_ch[channel]->name;
> + break;
> + case hwmon_fan:
> + *buf = hwmon->fan_ch[channel]->name;
> + break;
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long val)
> +{
> + struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
> + u8 buf[2];
> +
> + dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
> + channel);
> + switch (type) {
> + case hwmon_fan:
> + buf[0] = val & 0xff;
> + buf[1] = (val >> 8) & 0xff;
> + return regmap_bulk_write(hwmon->gsc->regmap_hwmon,
> + hwmon->fan_ch[channel]->reg, buf, 2);
fanX_input reports the fan speed. By its nature, a fan speed is not writeable.
I have no idea what this is supposed to achieve, but whatever it is, it is wrong.
Please stick with the ABI.
> + default:
> + break;
> + }
> +
> + return -EOPNOTSUPP;
> +}
> +
> +static umode_t
> +gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
> + int ch)
> +{
> + const struct gsc_hwmon_data *hwmon = _data;
> + struct device *dev = hwmon->gsc->dev;
> + umode_t mode = 0;
> +
> + switch (type) {
> + case hwmon_fan:
> + if (ch >= GSC_HWMON_MAX_FAN_CH)
> + return -EOPNOTSUPP;
Can that ever happen ?
> + mode = S_IRUGO;
> + if (attr == hwmon_fan_input)
> + mode |= S_IWUSR;
fanX_input is a read-only attribute per ABI.
> + break;
> + case hwmon_temp:
> + if (ch >= GSC_HWMON_MAX_TEMP_CH)
> + return -EOPNOTSUPP;
Can that ever happen ?
> + mode = S_IRUGO;
> + break;
> + case hwmon_in:
> + if (ch >= GSC_HWMON_MAX_IN_CH)
> + return -EOPNOTSUPP;
Can that ever happen ?
> + mode = S_IRUGO;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__, type,
> + attr, ch, mode);
> +
> + return mode;
> +}
> +
> +static const struct hwmon_ops gsc_hwmon_ops = {
> + .is_visible = gsc_hwmon_is_visible,
> + .read = gsc_hwmon_read,
> + .read_string = gsc_hwmon_read_string,
> + .write = gsc_hwmon_write,
> +};
> +
> +static struct gsc_hwmon_platform_data *
> +gsc_hwmon_get_devtree_pdata(struct device *dev)
> +{
> + struct gsc_hwmon_platform_data *pdata;
> + struct gsc_hwmon_channel *ch;
> + struct fwnode_handle *child;
> + const char *type;
> + int nchannels;
> +
> + nchannels = device_get_child_node_count(dev);
> + dev_dbg(dev, "channels=%d\n", nchannels);
> + if (nchannels == 0)
> + return ERR_PTR(-ENODEV);
> +
> + pdata = devm_kzalloc(dev,
> + sizeof(*pdata) + nchannels * sizeof(*ch),
> + GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> + ch = (struct gsc_hwmon_channel *)(pdata + 1);
> + pdata->channels = ch;
> + pdata->nchannels = nchannels;
> +
> + device_property_read_u32(dev, "gw,reference-voltage",
> + &pdata->vreference);
> + device_property_read_u32(dev, "gw,resolution", &pdata->resolution);
> +
> + /* allocate structures for channels and count instances of each type */
> + device_for_each_child_node(dev, child) {
> + if (fwnode_property_read_string(child, "label", &ch->name)) {
> + dev_err(dev, "channel without label\n");
> + fwnode_handle_put(child);
> + return ERR_PTR(-EINVAL);
> + }
> + if (fwnode_property_read_u32(child, "reg", &ch->reg)) {
> + dev_err(dev, "channel without reg\n");
> + fwnode_handle_put(child);
> + return ERR_PTR(-EINVAL);
> + }
> + if (fwnode_property_read_string(child, "type", &type)) {
> + dev_err(dev, "channel without type\n");
> + fwnode_handle_put(child);
> + return ERR_PTR(-EINVAL);
> + }
> + if (!strcasecmp(type, "gw,hwmon-temperature"))
> + ch->type = type_temperature;
> + else if (!strcasecmp(type, "gw,hwmon-voltage"))
> + ch->type = type_voltage;
> + else if (!strcasecmp(type, "gw,hwmon-voltage-raw"))
> + ch->type = type_voltage_raw;
> + else if (!strcasecmp(type, "gw,hwmon-fan"))
> + ch->type = type_fan;
> + else {
> + dev_err(dev, "channel without type\n");
> + fwnode_handle_put(child);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + fwnode_property_read_u32(child, "gw,voltage-offset",
> + &ch->voffset);
Note that while it is technically ok to keep voltages internally in mV,
devicetree will likely require specificayion in uV. For accuracy, it might be
better to perform any calculations on that base and convert to mV for display
purposes.
> + fwnode_property_read_u32_array(child, "gw,voltage-divider",
> + ch->vdiv, ARRAY_SIZE(ch->vdiv));
> + dev_dbg(dev, "of: reg=0x%02x type=%d %s\n", ch->reg, ch->type,
> + ch->name);
> + ch++;
> + }
> +
> + return pdata;
> +}
> +
> +static int gsc_hwmon_probe(struct platform_device *pdev)
> +{
> + struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = &pdev->dev;
> + struct gsc_hwmon_platform_data *pdata = dev_get_platdata(dev);
> + struct gsc_hwmon_data *hwmon;
> + int i, i_in, i_temp, i_fan;
> +
> + if (!pdata) {
> + pdata = gsc_hwmon_get_devtree_pdata(dev);
> + if (IS_ERR(pdata))
> + return PTR_ERR(pdata);
> + }
> +
> + hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
> + if (!hwmon)
> + return -ENOMEM;
> + hwmon->gsc = gsc;
> + hwmon->pdata = pdata;
> +
> + for (i = 0, i_in = 0, i_temp = 0, i_fan = 0;
> + i < hwmon->pdata->nchannels; i++) {
> + const struct gsc_hwmon_channel *ch = &pdata->channels[i];
> +
> + if (ch->reg > GSC_HWMON_MAX_REG) {
> + dev_err(dev, "invalid reg: 0x%02x\n", ch->reg);
> + return -EINVAL;
> + }
> + switch (ch->type) {
> + case type_temperature:
> + if (i_temp == GSC_HWMON_MAX_TEMP_CH) {
> + dev_err(dev, "too many temp channels\n");
> + return -EINVAL;
> + }
> + hwmon->temp_ch[i_temp] = ch;
> + hwmon->temp_config[i_temp] = HWMON_T_INPUT |
> + HWMON_T_LABEL;
> + i_temp++;
> + break;
> + case type_voltage:
> + case type_voltage_raw:
> + if (i_in == GSC_HWMON_MAX_IN_CH) {
> + dev_err(dev, "too many voltage channels\n");
> + return -EINVAL;
> + }
> + hwmon->in_ch[i_in] = ch;
> + hwmon->in_config[i_in] =
> + HWMON_I_INPUT | HWMON_I_LABEL;
> + i_in++;
> + break;
> + case type_fan:
> + if (i_fan == GSC_HWMON_MAX_FAN_CH) {
> + dev_err(dev, "too many voltage channels\n");
> + return -EINVAL;
> + }
> + hwmon->fan_ch[i_fan] = ch;
> + hwmon->fan_config[i_fan] =
> + HWMON_F_INPUT | HWMON_F_LABEL;
> + i_fan++;
> + break;
> + default:
> + dev_err(dev, "invalid type: %d\n", ch->type);
> + return -EINVAL;
> + }
> + dev_dbg(dev, "pdata: reg=0x%02x type=%d %s\n", ch->reg,
> + ch->type, ch->name);
> + }
> +
> + /* terminate channel config lists */
> + hwmon->temp_config[i_temp] = 0;
> + hwmon->in_config[i_in] = 0;
> + hwmon->fan_config[i_fan] = 0;
'hwmon' was alocated with devm_kzalloc(). Initializing any of its members with 0
is unnecessary.
> +
> + /* setup config structures */
> + hwmon->chip.ops = &gsc_hwmon_ops;
> + hwmon->chip.info = hwmon->info;
> + hwmon->info[0] = &hwmon->temp_info;
> + hwmon->info[1] = &hwmon->in_info;
> + hwmon->info[2] = &hwmon->fan_info;
> + hwmon->temp_info.type = hwmon_temp;
> + hwmon->temp_info.config = hwmon->temp_config;
> + hwmon->in_info.type = hwmon_in;
> + hwmon->in_info.config = hwmon->in_config;
> + hwmon->fan_info.type = hwmon_fan;
> + hwmon->fan_info.config = hwmon->fan_config;
> +
> + hwmon->dev = devm_hwmon_device_register_with_info(dev,
> + KBUILD_MODNAME, hwmon,
> + &hwmon->chip, NULL);
> + return PTR_ERR_OR_ZERO(hwmon->dev);
> +}
> +
> +static const struct of_device_id gsc_hwmon_of_match[] = {
> + { .compatible = "gw,gsc-hwmon", },
> + {}
> +};
> +
> +static struct platform_driver gsc_hwmon_driver = {
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = gsc_hwmon_of_match,
> + },
> + .probe = gsc_hwmon_probe,
> +};
> +
> +module_platform_driver(gsc_hwmon_driver);
> +
> +MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
> +MODULE_DESCRIPTION("GSC hardware monitor driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/platform_data/gsc_hwmon.h b/include/linux/platform_data/gsc_hwmon.h
> new file mode 100644
> index 0000000..5e59846
> --- /dev/null
> +++ b/include/linux/platform_data/gsc_hwmon.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _GSC_HWMON_H
> +#define _GSC_HWMON_H
> +
> +enum gsc_hwmon_type {
> + type_temperature,
> + type_voltage,
> + type_voltage_raw,
> + type_fan,
> +};
> +
> +/**
> + * struct gsc_hwmon_channel - configuration parameters
> + * @reg: I2C register offset
> + * @type: channel type
> + * @name: channel name
> + * @voffset: voltage offset (mV)
> + * @vdiv: voltage divider array (2 resistor values in ohms)
> + */
> +struct gsc_hwmon_channel {
> + unsigned int reg;
> + unsigned int type;
> + const char *name;
> + unsigned int voffset;
It is interesting that only positive offset values are supported.
Is that intentional ?
> + unsigned int vdiv[2];
> +};
> +
> +/**
> + * struct gsc_hwmon_platform_data - platform data for gsc_hwmon driver
> + * @channels: pointer to array of gsc_hwmon_channel structures
> + * describing channels
> + * @nchannels: number of elements in @channels array
> + * @vreference: voltage reference (mV)
> + * @resolution: ADC resolution
Resolution in what ?
> + */
> +struct gsc_hwmon_platform_data {
> + const struct gsc_hwmon_channel *channels;
> + int nchannels;
> + unsigned int resolution;
> + unsigned int vreference;
> +};
> +
> +#endif
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: mfd: Add Gateworks System Controller bindings
From: Guenter Roeck @ 2018-03-28 16:24 UTC (permalink / raw)
To: Tim Harvey
Cc: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, linux-kernel, devicetree, linux-arm-kernel,
linux-hwmon, linux-input, linux-watchdog
In-Reply-To: <1522250043-8065-2-git-send-email-tharvey@gateworks.com>
On Wed, Mar 28, 2018 at 08:14:00AM -0700, Tim Harvey wrote:
> This patch adds documentation of device-tree bindings for the
> Gateworks System Controller (GSC).
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
> v3:
> - replaced _ with -
> - remove input bindings
> - added full description of hwmon
> - fix unit address of hwmon child nodes
>
> ---
> .../devicetree/bindings/mfd/gateworks-gsc.txt | 135 +++++++++++++++++++++
> 1 file changed, 135 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
> new file mode 100644
> index 0000000..8f530ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
> @@ -0,0 +1,135 @@
> +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"
> +- 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
> + <devicetree/bindings/interrupt-controller/interrupt.txt>
> +
> +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 watchdog properties:
> +- compatible: must be "gw,gsc-watchdog"
> +
> +* hwmon:
> +The GSC provides a set of Analog to Digitcal Converter (ADC) pins used for
> +temperature and/or voltage monitoring.
> +
> +Required hwmon properties:
> +- compatible: must be "gw,gsc-hwmon"
> +
"hwmon" is a very Linux specific term. It might make sense to find a more
generic term.
> +Optional hwmon properties:
> +- gw,reference-voltage: ADC reference voltage (mV) used in scaling raw ADCs
AFAIK devicetree likes to specify voltages in uV.
> +- gw,resolution: ADC resolution (ie 4096) used in scaling raw ADCs
> +
4096 what ?
> +Each hwmon child node defines an ADC input on the chip which the GSC may
> +report cooked values (ie temperature sensor based on thermister), raw values,
> +(ie voltage rail with a pre-scaling resistor divider), or a fan controller
> +setpoint.
> +
> +Required hwmon child properties:
> +- type: one of the following ADC types:
> + "gw,hwmon-temperature" - reports temperature in C*10
> + "gw,hwmon-voltage" - reports a pre-scaled voltage value
> + "gw,hwmon-voltage-raw" - reports a raw ADC that is scaled with
> + vreference, resolution, and optional resistor divider
> + "gw,hwmon-fan" - a fan temperature setpoint in C*10
What is a "fan temperature setpoint" ?
> +- reg: offset of the ADC register
> +- label: name of the ADC input or FAN setpoint
> +
> +Optional hwmon child properties:
> +- gw,voltage-divider: An array of two integers containing the resistor
> + values R1 and R2 of the optinal resistor divider on a raw ADC
> +- gw,voltage-offset: a mV voltage offset to apply to a raw ADC (ie to
> + compensate for a diode drop)
> +
> +Example:
> +
> + gsc: gsc@20 {
> + compatible = "gw,gsc";
> + reg = <0x20>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <4 GPIO_ACTIVE_LOW>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + watchdog {
> + compatible = "gw,gsc-watchdog";
> + };
> +
> + hwmon {
> + compatible = "gw,gsc-hwmon";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + gw,reference-voltage = <2500>;
> + gw,resolution = <4096>;
> +
> + hwmon@0 { /* A0: Board Temperature */
> + type = "gw,hwmon-temperature";
> + reg = <0x00>;
> + label = "temp";
> + };
> +
> + hwmon@2 { /* A1: Input Voltage (raw ADC) */
> + type = "gw,hwmon-voltage-raw";
> + reg = <0x02>;
> + label = "vdd_vin";
> + gw,voltage-divider = <22100 1000>;
> + gw,voltage-offset = <800>;
> + };
> +
> + hwmon@b { /* A2: Battery voltage */
> + type = "gw,hwmon-voltage";
> + reg = <0x0b>;
> + label = "vdd_bat";
> + };
> +
> + hwmon@2c { /* fan temperature setpoint for 50% duty */
> + type = "gw,hwmon-fan";
> + reg = <0x2c>;
> + label = "fan_50p";
> + };
> +
> + hwmon@2e { /* fan1 */
> + type = "gw,hwmon-fan";
> + reg = <0x2e>;
> + label = "fan_60p";
> + };
> +
> + hwmon@30 { /* fan2 */
> + type = "gw,hwmon-fan";
> + reg = <0x30>;
> + label = "fan_70p";
> + };
> +
> + hwmon@32 { /* fan3 */
> + type = "gw,hwmon-fan";
> + reg = <0x32>;
> + label = "fan_80p";
> + };
> +
> + hwmon@34 { /* fan4 */
> + type = "gw,hwmon-fan";
> + reg = <0x34>;
> + label = "fan_90p";
> + };
> +
> + hwmon@36 { /* fan5 */
> + type = "gw,hwmon-fan";
> + reg = <0x36>;
> + label = "fan_100p";
> + };
No idea what this is supposed to be doing, but whatever it is,
it appears to be wrong. I'll comment more on it in the hwmon driver.
Guenter
> + };
> + };
> --
> 2.7.4
>
^ permalink raw reply
* [PATCH v3 4/4] watchdog: add Gateworks System Controller support
From: Tim Harvey @ 2018-03-28 15:14 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, Guenter Roeck
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-hwmon,
linux-input, linux-watchdog
In-Reply-To: <1522250043-8065-1-git-send-email-tharvey@gateworks.com>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
drivers/watchdog/Kconfig | 10 ++++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/gsc_wdt.c | 146 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 157 insertions(+)
create mode 100644 drivers/watchdog/gsc_wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index ca200d1..c9d4b2e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -150,6 +150,16 @@ config GPIO_WATCHDOG_ARCH_INITCALL
arch_initcall.
If in doubt, say N.
+config GSC_WATCHDOG
+ tristate "Gateworks System Controller (GSC) Watchdog support"
+ depends on MFD_GATEWORKS_GSC
+ select WATCHDOG_CORE
+ help
+ Say Y here to include support for the GSC Watchdog.
+
+ This driver can also be built as a module. If so the module
+ will be called gsc_wdt.
+
config MENF21BMC_WATCHDOG
tristate "MEN 14F021P00 BMC Watchdog"
depends on MFD_MENF21BMC || COMPILE_TEST
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 715a210..499327e 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -215,6 +215,7 @@ obj-$(CONFIG_DA9055_WATCHDOG) += da9055_wdt.o
obj-$(CONFIG_DA9062_WATCHDOG) += da9062_wdt.o
obj-$(CONFIG_DA9063_WATCHDOG) += da9063_wdt.o
obj-$(CONFIG_GPIO_WATCHDOG) += gpio_wdt.o
+obj-$(CONFIG_GSC_WATCHDOG) += gsc_wdt.o
obj-$(CONFIG_TANGOX_WATCHDOG) += tangox_wdt.o
obj-$(CONFIG_WDAT_WDT) += wdat_wdt.o
obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o
diff --git a/drivers/watchdog/gsc_wdt.c b/drivers/watchdog/gsc_wdt.c
new file mode 100644
index 0000000..b43d083
--- /dev/null
+++ b/drivers/watchdog/gsc_wdt.c
@@ -0,0 +1,146 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright (C) 2018 Gateworks Corporation
+ *
+ * This driver registers a Linux Watchdog for the GSC
+ */
+#include <linux/mfd/gsc.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/watchdog.h>
+
+#define WDT_DEFAULT_TIMEOUT 60
+
+struct gsc_wdt {
+ struct watchdog_device wdt_dev;
+ struct gsc_dev *gsc;
+};
+
+static int gsc_wdt_start(struct watchdog_device *wdd)
+{
+ struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
+ unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE);
+ int ret;
+
+ dev_dbg(wdd->parent, "%s timeout=%d\n", __func__, wdd->timeout);
+
+ /* clear first as regmap_update_bits will not write if no change */
+ ret = regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0);
+ if (ret)
+ return ret;
+ return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, reg);
+}
+
+static int gsc_wdt_stop(struct watchdog_device *wdd)
+{
+ struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
+ unsigned int reg = (1 << GSC_CTRL_1_WDT_ENABLE);
+
+ dev_dbg(wdd->parent, "%s\n", __func__);
+
+ return regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1, reg, 0);
+}
+
+static int gsc_wdt_set_timeout(struct watchdog_device *wdd,
+ unsigned int timeout)
+{
+ struct gsc_wdt *wdt = watchdog_get_drvdata(wdd);
+ unsigned int long_sel = 0;
+
+ dev_dbg(wdd->parent, "%s: %d\n", __func__, timeout);
+
+ switch (timeout) {
+ case 60:
+ long_sel = (1 << GSC_CTRL_1_WDT_TIME);
+ case 30:
+ regmap_update_bits(wdt->gsc->regmap, GSC_CTRL_1,
+ (1 << GSC_CTRL_1_WDT_TIME),
+ (long_sel << GSC_CTRL_1_WDT_TIME));
+ wdd->timeout = timeout;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static const struct watchdog_info gsc_wdt_info = {
+ .options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+ .identity = "GSC Watchdog"
+};
+
+static const struct watchdog_ops gsc_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = gsc_wdt_start,
+ .stop = gsc_wdt_stop,
+ .set_timeout = gsc_wdt_set_timeout,
+};
+
+static int gsc_wdt_probe(struct platform_device *pdev)
+{
+ struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
+ struct device *dev = &pdev->dev;
+ struct gsc_wdt *wdt;
+ int ret;
+ unsigned int reg;
+
+ wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
+ if (!wdt)
+ return -ENOMEM;
+
+ /* ensure GSC fw supports WD functionality */
+ if (gsc->fwver < 44) {
+ dev_err(dev, "fw v44 or newer required for wdt function\n");
+ return -EINVAL;
+ }
+
+ /* ensure WD bit enabled */
+ if (regmap_read(gsc->regmap, GSC_CTRL_1, ®))
+ return -EIO;
+ if (!(reg & (1 << GSC_CTRL_1_WDT_ENABLE))) {
+ dev_err(dev, "not enabled - must be manually enabled\n");
+ return -EINVAL;
+ }
+
+ platform_set_drvdata(pdev, wdt);
+
+ wdt->gsc = gsc;
+ wdt->wdt_dev.info = &gsc_wdt_info;
+ wdt->wdt_dev.ops = &gsc_wdt_ops;
+ wdt->wdt_dev.status = 0;
+ wdt->wdt_dev.min_timeout = 30;
+ wdt->wdt_dev.max_timeout = 60;
+ wdt->wdt_dev.parent = dev;
+
+ watchdog_set_nowayout(&wdt->wdt_dev, 1);
+ watchdog_init_timeout(&wdt->wdt_dev, WDT_DEFAULT_TIMEOUT, dev);
+
+ watchdog_set_drvdata(&wdt->wdt_dev, wdt);
+ ret = devm_watchdog_register_device(dev, &wdt->wdt_dev);
+ if (ret)
+ return ret;
+
+ dev_info(dev, "watchdog driver (timeout=%d sec)\n",
+ wdt->wdt_dev.timeout);
+
+ return 0;
+}
+
+static const struct of_device_id gsc_wdt_dt_ids[] = {
+ { .compatible = "gw,gsc-watchdog", },
+ {}
+};
+
+static struct platform_driver gsc_wdt_driver = {
+ .probe = gsc_wdt_probe,
+ .driver = {
+ .name = "gsc-wdt",
+ .of_match_table = gsc_wdt_dt_ids,
+ },
+};
+
+module_platform_driver(gsc_wdt_driver);
+
+MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
+MODULE_DESCRIPTION("Gateworks System Controller Watchdog driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related
* [PATCH v3 3/4] hwmon: add Gateworks System Controller support
From: Tim Harvey @ 2018-03-28 15:14 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, Guenter Roeck
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-hwmon,
linux-input, linux-watchdog
In-Reply-To: <1522250043-8065-1-git-send-email-tharvey@gateworks.com>
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.
Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
v3:
- add voltage_raw input type and supporting fields
- add channel validation to is_visible function
- remove unnecessary channel validation from read/write functions
v2:
- change license comment style
- remove DEBUG
- simplify regmap_bulk_read err check
- remove break after returns in switch statement
- fix fan setpoint buffer address
- remove unnecessary parens
- consistently use struct device *dev pointer
- change license/comment block
- add validation for hwmon child node props
- move parsing of of to own function
- use strlcpy to ensure null termination
- fix static array sizes and removed unnecessary initializers
- dynamically allocate channels
- fix fan input label
- support platform data
- fixed whitespace issues
drivers/hwmon/Kconfig | 9 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/gsc-hwmon.c | 368 ++++++++++++++++++++++++++++++++
include/linux/platform_data/gsc_hwmon.h | 43 ++++
4 files changed, 421 insertions(+)
create mode 100644 drivers/hwmon/gsc-hwmon.c
create mode 100644 include/linux/platform_data/gsc_hwmon.h
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 7ad0176..85d9aa3 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -475,6 +475,15 @@ 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_GATEWORKS_GSC
+ help
+ Support for the Gateworks System Controller A/D converters.
+
+ To compile this driver as a module, choose M here:
+ the module will be called gsc-hwmon.
+
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 0000000..10da46f
--- /dev/null
+++ b/drivers/hwmon/gsc-hwmon.c
@@ -0,0 +1,368 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright (C) 2018 Gateworks Corporation
+ *
+ * This driver registers Linux HWMON attributes for GSC ADC's
+ */
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/mfd/gsc.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#include <linux/platform_data/gsc_hwmon.h>
+
+#define GSC_HWMON_MAX_TEMP_CH 16
+#define GSC_HWMON_MAX_IN_CH 16
+#define GSC_HWMON_MAX_FAN_CH 6
+
+struct gsc_hwmon_data {
+ struct gsc_dev *gsc;
+ struct device *dev;
+ struct gsc_hwmon_platform_data *pdata;
+ const struct gsc_hwmon_channel *temp_ch[GSC_HWMON_MAX_TEMP_CH];
+ const struct gsc_hwmon_channel *in_ch[GSC_HWMON_MAX_IN_CH];
+ const struct gsc_hwmon_channel *fan_ch[GSC_HWMON_MAX_FAN_CH];
+ u32 temp_config[GSC_HWMON_MAX_TEMP_CH + 1];
+ u32 in_config[GSC_HWMON_MAX_IN_CH + 1];
+ u32 fan_config[GSC_HWMON_MAX_FAN_CH + 1];
+ struct hwmon_channel_info temp_info;
+ struct hwmon_channel_info in_info;
+ struct hwmon_channel_info fan_info;
+ const struct hwmon_channel_info *info[4];
+ struct hwmon_chip_info chip;
+ int vreference;
+ int resolution;
+};
+
+static int
+gsc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+ int channel, long *val)
+{
+ struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
+ struct gsc_hwmon_platform_data *pdata = hwmon->pdata;
+ const struct gsc_hwmon_channel *ch;
+ int sz, ret;
+ u8 buf[3];
+
+ dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
+ channel);
+ switch (type) {
+ case hwmon_in:
+ ch = hwmon->in_ch[channel];
+ break;
+ case hwmon_temp:
+ ch = hwmon->temp_ch[channel];
+ break;
+ case hwmon_fan:
+ ch = hwmon->fan_ch[channel];
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ sz = (ch->type == type_voltage) ? 3 : 2;
+ ret = regmap_bulk_read(hwmon->gsc->regmap_hwmon, ch->reg, buf, sz);
+ if (ret)
+ return ret;
+
+ *val = 0;
+ while (sz-- > 0)
+ *val |= (buf[sz] << (8*sz));
+
+ switch (ch->type) {
+ case type_temperature:
+ if ((type == hwmon_temp) && *val > 0x8000)
+ *val -= 0xffff;
+ break;
+ case type_voltage_raw:
+ /* scale based on ref voltage and resolution */
+ if (pdata->vreference && pdata->resolution) {
+ *val *= pdata->vreference;
+ *val /= pdata->resolution;
+ }
+ /* scale based on optional voltage divider */
+ if (ch->vdiv[0] && ch->vdiv[1]) {
+ *val *= (ch->vdiv[0] + ch->vdiv[1]);
+ *val /= ch->vdiv[1];
+ }
+ /* adjust by offset */
+ *val += ch->voffset;
+ break;
+ }
+
+ return 0;
+}
+
+static int
+gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, const char **buf)
+{
+ struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
+
+ dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
+ channel);
+ switch (type) {
+ case hwmon_in:
+ *buf = hwmon->in_ch[channel]->name;
+ break;
+ case hwmon_temp:
+ *buf = hwmon->temp_ch[channel]->name;
+ break;
+ case hwmon_fan:
+ *buf = hwmon->fan_ch[channel]->name;
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ return 0;
+}
+
+static int
+gsc_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+ int channel, long val)
+{
+ struct gsc_hwmon_data *hwmon = dev_get_drvdata(dev);
+ u8 buf[2];
+
+ dev_dbg(dev, "%s type=%d attr=%d channel=%d\n", __func__, type, attr,
+ channel);
+ switch (type) {
+ case hwmon_fan:
+ buf[0] = val & 0xff;
+ buf[1] = (val >> 8) & 0xff;
+ return regmap_bulk_write(hwmon->gsc->regmap_hwmon,
+ hwmon->fan_ch[channel]->reg, buf, 2);
+ default:
+ break;
+ }
+
+ return -EOPNOTSUPP;
+}
+
+static umode_t
+gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
+ int ch)
+{
+ const struct gsc_hwmon_data *hwmon = _data;
+ struct device *dev = hwmon->gsc->dev;
+ umode_t mode = 0;
+
+ switch (type) {
+ case hwmon_fan:
+ if (ch >= GSC_HWMON_MAX_FAN_CH)
+ return -EOPNOTSUPP;
+ mode = S_IRUGO;
+ if (attr == hwmon_fan_input)
+ mode |= S_IWUSR;
+ break;
+ case hwmon_temp:
+ if (ch >= GSC_HWMON_MAX_TEMP_CH)
+ return -EOPNOTSUPP;
+ mode = S_IRUGO;
+ break;
+ case hwmon_in:
+ if (ch >= GSC_HWMON_MAX_IN_CH)
+ return -EOPNOTSUPP;
+ mode = S_IRUGO;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ dev_dbg(dev, "%s type=%d attr=%d ch=%d mode=0x%x\n", __func__, type,
+ attr, ch, mode);
+
+ return mode;
+}
+
+static const struct hwmon_ops gsc_hwmon_ops = {
+ .is_visible = gsc_hwmon_is_visible,
+ .read = gsc_hwmon_read,
+ .read_string = gsc_hwmon_read_string,
+ .write = gsc_hwmon_write,
+};
+
+static struct gsc_hwmon_platform_data *
+gsc_hwmon_get_devtree_pdata(struct device *dev)
+{
+ struct gsc_hwmon_platform_data *pdata;
+ struct gsc_hwmon_channel *ch;
+ struct fwnode_handle *child;
+ const char *type;
+ int nchannels;
+
+ nchannels = device_get_child_node_count(dev);
+ dev_dbg(dev, "channels=%d\n", nchannels);
+ if (nchannels == 0)
+ return ERR_PTR(-ENODEV);
+
+ pdata = devm_kzalloc(dev,
+ sizeof(*pdata) + nchannels * sizeof(*ch),
+ GFP_KERNEL);
+ if (!pdata)
+ return ERR_PTR(-ENOMEM);
+ ch = (struct gsc_hwmon_channel *)(pdata + 1);
+ pdata->channels = ch;
+ pdata->nchannels = nchannels;
+
+ device_property_read_u32(dev, "gw,reference-voltage",
+ &pdata->vreference);
+ device_property_read_u32(dev, "gw,resolution", &pdata->resolution);
+
+ /* allocate structures for channels and count instances of each type */
+ device_for_each_child_node(dev, child) {
+ if (fwnode_property_read_string(child, "label", &ch->name)) {
+ dev_err(dev, "channel without label\n");
+ fwnode_handle_put(child);
+ return ERR_PTR(-EINVAL);
+ }
+ if (fwnode_property_read_u32(child, "reg", &ch->reg)) {
+ dev_err(dev, "channel without reg\n");
+ fwnode_handle_put(child);
+ return ERR_PTR(-EINVAL);
+ }
+ if (fwnode_property_read_string(child, "type", &type)) {
+ dev_err(dev, "channel without type\n");
+ fwnode_handle_put(child);
+ return ERR_PTR(-EINVAL);
+ }
+ if (!strcasecmp(type, "gw,hwmon-temperature"))
+ ch->type = type_temperature;
+ else if (!strcasecmp(type, "gw,hwmon-voltage"))
+ ch->type = type_voltage;
+ else if (!strcasecmp(type, "gw,hwmon-voltage-raw"))
+ ch->type = type_voltage_raw;
+ else if (!strcasecmp(type, "gw,hwmon-fan"))
+ ch->type = type_fan;
+ else {
+ dev_err(dev, "channel without type\n");
+ fwnode_handle_put(child);
+ return ERR_PTR(-EINVAL);
+ }
+
+ fwnode_property_read_u32(child, "gw,voltage-offset",
+ &ch->voffset);
+ fwnode_property_read_u32_array(child, "gw,voltage-divider",
+ ch->vdiv, ARRAY_SIZE(ch->vdiv));
+ dev_dbg(dev, "of: reg=0x%02x type=%d %s\n", ch->reg, ch->type,
+ ch->name);
+ ch++;
+ }
+
+ return pdata;
+}
+
+static int gsc_hwmon_probe(struct platform_device *pdev)
+{
+ struct gsc_dev *gsc = dev_get_drvdata(pdev->dev.parent);
+ struct device *dev = &pdev->dev;
+ struct gsc_hwmon_platform_data *pdata = dev_get_platdata(dev);
+ struct gsc_hwmon_data *hwmon;
+ int i, i_in, i_temp, i_fan;
+
+ if (!pdata) {
+ pdata = gsc_hwmon_get_devtree_pdata(dev);
+ if (IS_ERR(pdata))
+ return PTR_ERR(pdata);
+ }
+
+ hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
+ if (!hwmon)
+ return -ENOMEM;
+ hwmon->gsc = gsc;
+ hwmon->pdata = pdata;
+
+ for (i = 0, i_in = 0, i_temp = 0, i_fan = 0;
+ i < hwmon->pdata->nchannels; i++) {
+ const struct gsc_hwmon_channel *ch = &pdata->channels[i];
+
+ if (ch->reg > GSC_HWMON_MAX_REG) {
+ dev_err(dev, "invalid reg: 0x%02x\n", ch->reg);
+ return -EINVAL;
+ }
+ switch (ch->type) {
+ case type_temperature:
+ if (i_temp == GSC_HWMON_MAX_TEMP_CH) {
+ dev_err(dev, "too many temp channels\n");
+ return -EINVAL;
+ }
+ hwmon->temp_ch[i_temp] = ch;
+ hwmon->temp_config[i_temp] = HWMON_T_INPUT |
+ HWMON_T_LABEL;
+ i_temp++;
+ break;
+ case type_voltage:
+ case type_voltage_raw:
+ if (i_in == GSC_HWMON_MAX_IN_CH) {
+ dev_err(dev, "too many voltage channels\n");
+ return -EINVAL;
+ }
+ hwmon->in_ch[i_in] = ch;
+ hwmon->in_config[i_in] =
+ HWMON_I_INPUT | HWMON_I_LABEL;
+ i_in++;
+ break;
+ case type_fan:
+ if (i_fan == GSC_HWMON_MAX_FAN_CH) {
+ dev_err(dev, "too many voltage channels\n");
+ return -EINVAL;
+ }
+ hwmon->fan_ch[i_fan] = ch;
+ hwmon->fan_config[i_fan] =
+ HWMON_F_INPUT | HWMON_F_LABEL;
+ i_fan++;
+ break;
+ default:
+ dev_err(dev, "invalid type: %d\n", ch->type);
+ return -EINVAL;
+ }
+ dev_dbg(dev, "pdata: reg=0x%02x type=%d %s\n", ch->reg,
+ ch->type, ch->name);
+ }
+
+ /* terminate channel config lists */
+ hwmon->temp_config[i_temp] = 0;
+ hwmon->in_config[i_in] = 0;
+ hwmon->fan_config[i_fan] = 0;
+
+ /* setup config structures */
+ hwmon->chip.ops = &gsc_hwmon_ops;
+ hwmon->chip.info = hwmon->info;
+ hwmon->info[0] = &hwmon->temp_info;
+ hwmon->info[1] = &hwmon->in_info;
+ hwmon->info[2] = &hwmon->fan_info;
+ hwmon->temp_info.type = hwmon_temp;
+ hwmon->temp_info.config = hwmon->temp_config;
+ hwmon->in_info.type = hwmon_in;
+ hwmon->in_info.config = hwmon->in_config;
+ hwmon->fan_info.type = hwmon_fan;
+ hwmon->fan_info.config = hwmon->fan_config;
+
+ hwmon->dev = devm_hwmon_device_register_with_info(dev,
+ KBUILD_MODNAME, hwmon,
+ &hwmon->chip, NULL);
+ return PTR_ERR_OR_ZERO(hwmon->dev);
+}
+
+static const struct of_device_id gsc_hwmon_of_match[] = {
+ { .compatible = "gw,gsc-hwmon", },
+ {}
+};
+
+static struct platform_driver gsc_hwmon_driver = {
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = gsc_hwmon_of_match,
+ },
+ .probe = gsc_hwmon_probe,
+};
+
+module_platform_driver(gsc_hwmon_driver);
+
+MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
+MODULE_DESCRIPTION("GSC hardware monitor driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/platform_data/gsc_hwmon.h b/include/linux/platform_data/gsc_hwmon.h
new file mode 100644
index 0000000..5e59846
--- /dev/null
+++ b/include/linux/platform_data/gsc_hwmon.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _GSC_HWMON_H
+#define _GSC_HWMON_H
+
+enum gsc_hwmon_type {
+ type_temperature,
+ type_voltage,
+ type_voltage_raw,
+ type_fan,
+};
+
+/**
+ * struct gsc_hwmon_channel - configuration parameters
+ * @reg: I2C register offset
+ * @type: channel type
+ * @name: channel name
+ * @voffset: voltage offset (mV)
+ * @vdiv: voltage divider array (2 resistor values in ohms)
+ */
+struct gsc_hwmon_channel {
+ unsigned int reg;
+ unsigned int type;
+ const char *name;
+ unsigned int voffset;
+ unsigned int vdiv[2];
+};
+
+/**
+ * struct gsc_hwmon_platform_data - platform data for gsc_hwmon driver
+ * @channels: pointer to array of gsc_hwmon_channel structures
+ * describing channels
+ * @nchannels: number of elements in @channels array
+ * @vreference: voltage reference (mV)
+ * @resolution: ADC resolution
+ */
+struct gsc_hwmon_platform_data {
+ const struct gsc_hwmon_channel *channels;
+ int nchannels;
+ unsigned int resolution;
+ unsigned int vreference;
+};
+
+#endif
--
2.7.4
^ permalink raw reply related
* [PATCH v3 2/4] mfd: add Gateworks System Controller core driver
From: Tim Harvey @ 2018-03-28 15:14 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, Guenter Roeck
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-hwmon,
linux-input, linux-watchdog, Randy Dunlap
In-Reply-To: <1522250043-8065-1-git-send-email-tharvey@gateworks.com>
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
Cc: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
v3:
- rename gsc->gateworks-gsc
- remove uncecessary include for linux/mfd/core.h
- upercase I2C in comments
- remove i2c debug
- remove uncecessary comments
- don't use KBUILD_MODNAME for name
- remove unnecessary v1/v2/v3 tracking
- unregister hwmon i2c adapter on remove
v2:
- change license comment block style
- remove COMPILE_TEST (Randy)
- fixed whitespace issues
- replaced a printk with dev_err
---
drivers/mfd/Kconfig | 13 ++
drivers/mfd/Makefile | 1 +
drivers/mfd/gateworks-gsc.c | 285 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/gsc.h | 75 ++++++++++++
4 files changed, 374 insertions(+)
create mode 100644 drivers/mfd/gateworks-gsc.c
create mode 100644 include/linux/mfd/gsc.h
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 1d20a80..013df63 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -341,6 +341,19 @@ config MFD_EXYNOS_LPASS
Select this option to enable support for Samsung Exynos Low Power
Audio Subsystem.
+config MFD_GATEWORKS_GSC
+ tristate "Gateworks System Controller"
+ depends on (I2C && OF)
+ select MFD_CORE
+ select REGMAP_I2C
+ select REGMAP_IRQ
+ help
+ Enable support for the Gateworks System Controller found
+ on Gateworks Single Board Computers.
+
+ To compile this driver as a module, choose M here: the
+ module will be called gsc.
+
config MFD_MC13XXX
tristate
depends on (SPI_MASTER || I2C)
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index d9474ad..da0868ff 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_GATEWORKS_GSC) += gateworks-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/gateworks-gsc.c b/drivers/mfd/gateworks-gsc.c
new file mode 100644
index 0000000..e081613
--- /dev/null
+++ b/drivers/mfd/gateworks-gsc.c
@@ -0,0 +1,285 @@
+/* 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 <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/gsc.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/*
+ * 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_RETRIES 3
+
+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;
+ }
+
+ 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;
+ }
+ *val = ret & 0xff;
+
+ 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];
+
+ dev_info(&gsc->i2c->dev, "GSC powerdown for %ld seconds\n",
+ secs);
+ regs[0] = secs & 0xff;
+ regs[1] = (secs >> 8) & 0xff;
+ regs[2] = (secs >> 16) & 0xff;
+ regs[3] = (secs >> 24) & 0xff;
+ ret = regmap_bulk_write(gsc->regmap, GSC_TIME_ADD, regs, 4);
+
+ return ret;
+}
+
+static ssize_t gsc_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct gsc_dev *gsc = dev_get_drvdata(dev);
+ const char *name = attr->attr.name;
+ int rz = 0;
+
+ if (strcasecmp(name, "fw_version") == 0)
+ rz = sprintf(buf, "%d\n", gsc->fwver);
+ else if (strcasecmp(name, "fw_crc") == 0)
+ rz = sprintf(buf, "0x%04x\n", gsc->fwcrc);
+
+ return rz;
+}
+
+static ssize_t gsc_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct gsc_dev *gsc = dev_get_drvdata(dev);
+ const char *name = attr->attr.name;
+ int ret;
+
+ if (strcasecmp(name, "powerdown") == 0) {
+ long value;
+
+ ret = kstrtol(buf, 0, &value);
+ if (ret == 0)
+ gsc_powerdown(gsc, value);
+ } else
+ dev_err(dev, "invalid name '%s\n", name);
+
+ return count;
+}
+
+static struct device_attribute attr_fwver =
+ __ATTR(fw_version, 0440, gsc_show, NULL);
+static struct device_attribute attr_fwcrc =
+ __ATTR(fw_crc, 0440, gsc_show, NULL);
+static struct device_attribute attr_pwrdown =
+ __ATTR(powerdown, 0220, NULL, gsc_store);
+
+static struct attribute *gsc_attrs[] = {
+ &attr_fwver.attr,
+ &attr_fwcrc.attr,
+ &attr_pwrdown.attr,
+ NULL,
+};
+
+static struct attribute_group attr_group = {
+ .attrs = gsc_attrs,
+};
+
+static const struct of_device_id gsc_of_match[] = {
+ { .compatible = "gw,gsc", },
+ { }
+};
+
+static const struct regmap_config gsc_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .cache_type = REGCACHE_NONE,
+ .max_register = 0xf,
+};
+
+static const struct regmap_config gsc_regmap_hwmon_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .cache_type = REGCACHE_NONE,
+ .max_register = 0x37,
+};
+
+static const struct regmap_irq gsc_irqs[] = {
+ REGMAP_IRQ_REG(GSC_IRQ_PB, 0, BIT(GSC_IRQ_PB)),
+ REGMAP_IRQ_REG(GSC_IRQ_KEY_ERASED, 0, BIT(GSC_IRQ_KEY_ERASED)),
+ REGMAP_IRQ_REG(GSC_IRQ_EEPROM_WP, 0, BIT(GSC_IRQ_EEPROM_WP)),
+ REGMAP_IRQ_REG(GSC_IRQ_RESV, 0, BIT(GSC_IRQ_RESV)),
+ REGMAP_IRQ_REG(GSC_IRQ_GPIO, 0, BIT(GSC_IRQ_GPIO)),
+ REGMAP_IRQ_REG(GSC_IRQ_TAMPER, 0, BIT(GSC_IRQ_TAMPER)),
+ REGMAP_IRQ_REG(GSC_IRQ_WDT_TIMEOUT, 0, BIT(GSC_IRQ_WDT_TIMEOUT)),
+ REGMAP_IRQ_REG(GSC_IRQ_SWITCH_HOLD, 0, BIT(GSC_IRQ_SWITCH_HOLD)),
+};
+
+static const struct regmap_irq_chip gsc_irq_chip = {
+ .name = "gateworks-gsc",
+ .irqs = gsc_irqs,
+ .num_irqs = ARRAY_SIZE(gsc_irqs),
+ .num_regs = 1,
+ .status_base = GSC_IRQ_STATUS,
+ .mask_base = GSC_IRQ_ENABLE,
+ .mask_invert = true,
+ .ack_base = GSC_IRQ_STATUS,
+ .ack_invert = true,
+};
+
+static int
+gsc_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct gsc_dev *gsc;
+ int ret;
+ unsigned int reg;
+
+ gsc = devm_kzalloc(dev, sizeof(*gsc), GFP_KERNEL);
+ if (!gsc)
+ return -ENOMEM;
+
+ gsc->dev = &client->dev;
+ gsc->i2c = client;
+ gsc->irq = client->irq;
+ i2c_set_clientdata(client, gsc);
+
+ gsc->regmap = devm_regmap_init(dev, ®map_gsc, client,
+ &gsc_regmap_config);
+ if (IS_ERR(gsc->regmap))
+ return PTR_ERR(gsc->regmap);
+
+ if (regmap_read(gsc->regmap, GSC_FW_VER, ®))
+ return -EIO;
+ gsc->fwver = reg;
+
+ regmap_read(gsc->regmap, GSC_FW_CRC, ®);
+ gsc->fwcrc = reg;
+ regmap_read(gsc->regmap, GSC_FW_CRC + 1, ®);
+ gsc->fwcrc |= reg << 8;
+
+ gsc->i2c_hwmon = i2c_new_dummy(client->adapter, GSC_HWMON);
+ if (!gsc->i2c_hwmon) {
+ dev_err(dev, "Failed to allocate I2C device for HWMON\n");
+ return -ENODEV;
+ }
+ i2c_set_clientdata(gsc->i2c_hwmon, gsc);
+
+ gsc->regmap_hwmon = devm_regmap_init(dev, ®map_gsc, gsc->i2c_hwmon,
+ &gsc_regmap_hwmon_config);
+ if (IS_ERR(gsc->regmap_hwmon)) {
+ ret = PTR_ERR(gsc->regmap_hwmon);
+ dev_err(dev, "failed to allocate register map: %d\n", ret);
+ goto err_regmap;
+ }
+
+ ret = devm_regmap_add_irq_chip(dev, gsc->regmap, gsc->irq,
+ IRQF_ONESHOT | IRQF_SHARED |
+ IRQF_TRIGGER_FALLING, 0,
+ &gsc_irq_chip, &gsc->irq_chip_data);
+ if (ret)
+ goto err_regmap;
+
+ dev_info(dev, "Gateworks System Controller v%d: fw 0x%04x\n",
+ gsc->fwver, gsc->fwcrc);
+
+ ret = sysfs_create_group(&dev->kobj, &attr_group);
+ if (ret)
+ dev_err(dev, "failed to create sysfs attrs\n");
+
+ ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+ if (ret)
+ goto err_sysfs;
+
+ return 0;
+
+err_sysfs:
+ sysfs_remove_group(&dev->kobj, &attr_group);
+err_regmap:
+ i2c_unregister_device(gsc->i2c_hwmon);
+
+ return ret;
+}
+
+static int gsc_remove(struct i2c_client *client)
+{
+ struct gsc_dev *gsc = i2c_get_clientdata(client);
+
+ sysfs_remove_group(&client->dev.kobj, &attr_group);
+ i2c_unregister_device(gsc->i2c_hwmon);
+
+ return 0;
+}
+
+static struct i2c_driver gsc_driver = {
+ .driver = {
+ .name = "gateworks-gsc",
+ .of_match_table = of_match_ptr(gsc_of_match),
+ },
+ .probe = gsc_probe,
+ .remove = gsc_remove,
+};
+
+module_i2c_driver(gsc_driver);
+
+MODULE_AUTHOR("Tim Harvey <tharvey@gateworks.com>");
+MODULE_DESCRIPTION("I2C Core interface for GSC");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/gsc.h b/include/linux/mfd/gsc.h
new file mode 100644
index 0000000..dcae61b
--- /dev/null
+++ b/include/linux/mfd/gsc.h
@@ -0,0 +1,75 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright (C) 2018 Gateworks Corporation
+ */
+#ifndef __LINUX_MFD_GSC_H_
+#define __LINUX_MFD_GSC_H_
+
+/* Device Addresses */
+#define GSC_MISC 0x20
+#define GSC_UPDATE 0x21
+#define GSC_GPIO 0x23
+#define GSC_HWMON 0x29
+#define GSC_EEPROM0 0x50
+#define GSC_EEPROM1 0x51
+#define GSC_EEPROM2 0x52
+#define GSC_EEPROM3 0x53
+#define GSC_RTC 0x68
+
+/* Register offsets */
+#define GSC_CTRL_0 0x00
+#define GSC_CTRL_1 0x01
+#define GSC_TIME 0x02
+#define GSC_TIME_ADD 0x06
+#define GSC_IRQ_STATUS 0x0A
+#define GSC_IRQ_ENABLE 0x0B
+#define GSC_FW_CRC 0x0C
+#define GSC_FW_VER 0x0E
+#define GSC_WP 0x0F
+
+/* Bit definitions */
+#define GSC_CTRL_0_PB_HARD_RESET 0
+#define GSC_CTRL_0_PB_CLEAR_SECURE_KEY 1
+#define GSC_CTRL_0_PB_SOFT_POWER_DOWN 2
+#define GSC_CTRL_0_PB_BOOT_ALTERNATE 3
+#define GSC_CTRL_0_PERFORM_CRC 4
+#define GSC_CTRL_0_TAMPER_DETECT 5
+#define GSC_CTRL_0_SWITCH_HOLD 6
+
+#define GSC_CTRL_1_SLEEP_ENABLE 0
+#define GSC_CTRL_1_ACTIVATE_SLEEP 1
+#define GSC_CTRL_1_LATCH_SLEEP_ADD 2
+#define GSC_CTRL_1_SLEEP_NOWAKEPB 3
+#define GSC_CTRL_1_WDT_TIME 4
+#define GSC_CTRL_1_WDT_ENABLE 5
+#define GSC_CTRL_1_SWITCH_BOOT_ENABLE 6
+#define GSC_CTRL_1_SWITCH_BOOT_CLEAR 7
+
+#define GSC_IRQ_PB 0
+#define GSC_IRQ_KEY_ERASED 1
+#define GSC_IRQ_EEPROM_WP 2
+#define GSC_IRQ_RESV 3
+#define GSC_IRQ_GPIO 4
+#define GSC_IRQ_TAMPER 5
+#define GSC_IRQ_WDT_TIMEOUT 6
+#define GSC_IRQ_SWITCH_HOLD 7
+
+/* Max registers */
+#define GSC_HWMON_MAX_REG 56
+
+struct gsc_dev {
+ struct device *dev;
+
+ struct i2c_client *i2c; /* 0x20: interrupt controller, WDT */
+ struct i2c_client *i2c_hwmon; /* 0x29: hwmon, fan controller */
+
+ struct regmap *regmap;
+ struct regmap *regmap_hwmon;
+ struct regmap_irq_chip_data *irq_chip_data;
+
+ int irq;
+ unsigned int fwver;
+ unsigned short fwcrc;
+};
+
+#endif /* __LINUX_MFD_GSC_H_ */
--
2.7.4
^ permalink raw reply related
* [PATCH v3 1/4] dt-bindings: mfd: Add Gateworks System Controller bindings
From: Tim Harvey @ 2018-03-28 15:14 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, Guenter Roeck
Cc: linux-hwmon, devicetree, linux-watchdog, linux-kernel,
linux-input, linux-arm-kernel
In-Reply-To: <1522250043-8065-1-git-send-email-tharvey@gateworks.com>
This patch adds documentation of device-tree bindings for the
Gateworks System Controller (GSC).
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
v3:
- replaced _ with -
- remove input bindings
- added full description of hwmon
- fix unit address of hwmon child nodes
---
.../devicetree/bindings/mfd/gateworks-gsc.txt | 135 +++++++++++++++++++++
1 file changed, 135 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
diff --git a/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
new file mode 100644
index 0000000..8f530ed
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
@@ -0,0 +1,135 @@
+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"
+- 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
+ <devicetree/bindings/interrupt-controller/interrupt.txt>
+
+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 watchdog properties:
+- compatible: must be "gw,gsc-watchdog"
+
+* hwmon:
+The GSC provides a set of Analog to Digitcal Converter (ADC) pins used for
+temperature and/or voltage monitoring.
+
+Required hwmon properties:
+- compatible: must be "gw,gsc-hwmon"
+
+Optional hwmon properties:
+- gw,reference-voltage: ADC reference voltage (mV) used in scaling raw ADCs
+- gw,resolution: ADC resolution (ie 4096) used in scaling raw ADCs
+
+Each hwmon child node defines an ADC input on the chip which the GSC may
+report cooked values (ie temperature sensor based on thermister), raw values,
+(ie voltage rail with a pre-scaling resistor divider), or a fan controller
+setpoint.
+
+Required hwmon child properties:
+- type: one of the following ADC types:
+ "gw,hwmon-temperature" - reports temperature in C*10
+ "gw,hwmon-voltage" - reports a pre-scaled voltage value
+ "gw,hwmon-voltage-raw" - reports a raw ADC that is scaled with
+ vreference, resolution, and optional resistor divider
+ "gw,hwmon-fan" - a fan temperature setpoint in C*10
+- reg: offset of the ADC register
+- label: name of the ADC input or FAN setpoint
+
+Optional hwmon child properties:
+- gw,voltage-divider: An array of two integers containing the resistor
+ values R1 and R2 of the optinal resistor divider on a raw ADC
+- gw,voltage-offset: a mV voltage offset to apply to a raw ADC (ie to
+ compensate for a diode drop)
+
+Example:
+
+ gsc: gsc@20 {
+ compatible = "gw,gsc";
+ reg = <0x20>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <4 GPIO_ACTIVE_LOW>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ watchdog {
+ compatible = "gw,gsc-watchdog";
+ };
+
+ hwmon {
+ compatible = "gw,gsc-hwmon";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ gw,reference-voltage = <2500>;
+ gw,resolution = <4096>;
+
+ hwmon@0 { /* A0: Board Temperature */
+ type = "gw,hwmon-temperature";
+ reg = <0x00>;
+ label = "temp";
+ };
+
+ hwmon@2 { /* A1: Input Voltage (raw ADC) */
+ type = "gw,hwmon-voltage-raw";
+ reg = <0x02>;
+ label = "vdd_vin";
+ gw,voltage-divider = <22100 1000>;
+ gw,voltage-offset = <800>;
+ };
+
+ hwmon@b { /* A2: Battery voltage */
+ type = "gw,hwmon-voltage";
+ reg = <0x0b>;
+ label = "vdd_bat";
+ };
+
+ hwmon@2c { /* fan temperature setpoint for 50% duty */
+ type = "gw,hwmon-fan";
+ reg = <0x2c>;
+ label = "fan_50p";
+ };
+
+ hwmon@2e { /* fan1 */
+ type = "gw,hwmon-fan";
+ reg = <0x2e>;
+ label = "fan_60p";
+ };
+
+ hwmon@30 { /* fan2 */
+ type = "gw,hwmon-fan";
+ reg = <0x30>;
+ label = "fan_70p";
+ };
+
+ hwmon@32 { /* fan3 */
+ type = "gw,hwmon-fan";
+ reg = <0x32>;
+ label = "fan_80p";
+ };
+
+ hwmon@34 { /* fan4 */
+ type = "gw,hwmon-fan";
+ reg = <0x34>;
+ label = "fan_90p";
+ };
+
+ hwmon@36 { /* fan5 */
+ type = "gw,hwmon-fan";
+ reg = <0x36>;
+ label = "fan_100p";
+ };
+ };
+ };
--
2.7.4
^ permalink raw reply related
* [PATCH v3 0/4] Add support for the Gateworks System Controller
From: Tim Harvey @ 2018-03-28 15:13 UTC (permalink / raw)
To: Lee Jones, Rob Herring, Mark Rutland, Mark Brown, Dmitry Torokhov,
Wim Van Sebroeck, Guenter Roeck
Cc: linux-kernel, devicetree, linux-arm-kernel, linux-hwmon,
linux-input, linux-watchdog
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
---
v3:
- removed unnecessary input driver
- added wdt driver
- bindings: encorporated feedback from mailng list
- hwmon:
- encoroprated feedback from mailng list
- added support for raw ADC voltage input used in newer GSC firmware
v2:
- change license comment block style
- remove COMPILE_TEST
- fixed whitespace issues
- replaced a printk with dev_err
- remove DEBUG
- simplify regmap_bulk_read err check
- remove break after returns in switch statement
- fix fan setpoint buffer address
- remove unnecessary parens
- consistently use struct device *dev pointer
- add validation for hwmon child node props
- move parsing of of to own function
- use strlcpy to ensure null termination
- fix static array sizes and removed unnecessary initializers
- dynamically allocate channels
- fix fan input label
- support platform data
Tim Harvey (4):
dt-bindings: mfd: Add Gateworks System Controller bindings
mfd: add Gateworks System Controller core driver
hwmon: add Gateworks System Controller support
watchdog: add Gateworks System Controller support
.../devicetree/bindings/mfd/gateworks-gsc.txt | 135 ++++++++
drivers/hwmon/Kconfig | 9 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/gsc-hwmon.c | 368 +++++++++++++++++++++
drivers/mfd/Kconfig | 13 +
drivers/mfd/Makefile | 1 +
drivers/mfd/gateworks-gsc.c | 285 ++++++++++++++++
drivers/watchdog/Kconfig | 10 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/gsc_wdt.c | 146 ++++++++
include/linux/mfd/gsc.h | 75 +++++
include/linux/platform_data/gsc_hwmon.h | 43 +++
12 files changed, 1087 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/gateworks-gsc.txt
create mode 100644 drivers/hwmon/gsc-hwmon.c
create mode 100644 drivers/mfd/gateworks-gsc.c
create mode 100644 drivers/watchdog/gsc_wdt.c
create mode 100644 include/linux/mfd/gsc.h
create mode 100644 include/linux/platform_data/gsc_hwmon.h
--
2.7.4
^ permalink raw reply
* Re: [PATCH] HID: google: Enable PM Full On mode when adjusting backlight
From: Jiri Kosina @ 2018-03-28 14:13 UTC (permalink / raw)
To: Nicolas Boichat
Cc: Benjamin Tissoires, linux-kernel, linux-input, groeck, dtor,
Haridhar Kalvala
In-Reply-To: <20180328061610.206090-1-drinkcat@chromium.org>
On Wed, 28 Mar 2018, Nicolas Boichat wrote:
> From: Haridhar Kalvala <haridhar.kalvala@intel.com>
>
> hammer LED backlight brightness is not getting set when USB
> device is in suspend state.
>
> This patch fixes the issue by requesting USB HID device to be
> in FULLON mode, so that sending hardware output report and
> hardware raw request won't fail to set brightness, and set
> device back to NORMAL mode once this call returns.
>
> Signed-off-by: Haridhar Kalvala <haridhar.kalvala@intel.com>
> Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Applied to for-4.17/google-hammer. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v6 0/6] Add MediaTek PMIC keys support
From: Lee Jones @ 2018-03-28 10:26 UTC (permalink / raw)
To: Matthias Brugger
Cc: Dmitry Torokhov, Sean Wang, Rob Herring, Alexandre Belloni,
Mark Rutland, a.zummo, devicetree, linus.walleij, jcsing.lee,
linux-kernel, krzk, javier, linux-mediatek, linux-arm-kernel,
linux-input, eddie.huang, Chen.Zhong, beomho.seo, linux-rtc
In-Reply-To: <3ece050c-7573-ef4e-2c89-61645a243a33@gmail.com>
On Tue, 27 Mar 2018, Matthias Brugger wrote:
>
>
> On 03/27/2018 10:05 AM, Lee Jones wrote:
> > On Fri, 23 Mar 2018, Dmitry Torokhov wrote:
> >> On Thu, Mar 22, 2018 at 10:17:53AM +0800, Sean Wang wrote:
> >>> Hi, Dmitry and Lee
> >>>
> >>> The series seems not being got merged. Are they good enough to be ready
> >>> into the your tree?
> >>>
> >>> Recently I've tested the series with focusing on pwrkey event generated
> >>> through interrupt when push and release the key on bpi-r2 board and then
> >>> finally it's working fine. but for homekey it cannot be found on the
> >>> board and thus I cannot have more tests more about it.
> >>>
> >>> Tested-by: Sean Wang <sean.wang@mediatek.com>
> >>
> >> You have my Ack on the input patch; I expect it go through Lee's tree as
> >> there are some dependencies on mfd core piece.
> >
> > Are you happy for me to merge the Input dt-bindings without your Ack?
> >
>
> They got both Acked in v5, but the commit message was not updated:
> https://patchwork.kernel.org/patch/9973721/
> https://patchwork.kernel.org/patch/9973723/
Thanks Matthias.
Chen, can you collect all the Acks and repost as a RESEND please?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] Input: ALPS - fix DualPoint flag for 74 03 28 devices
From: Pali Rohár @ 2018-03-28 7:38 UTC (permalink / raw)
To: Masaki Ota
Cc: Dmitry Torokhov, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Aaron Ma
In-Reply-To: <TYXPR01MB0719F95C7B4E9FD0FF313BEFC7AC0@TYXPR01MB0719.jpnprd01.prod.outlook.com>
Ideally this information should be put as a comment into the code. It is
really not obvious.
On Tuesday 27 March 2018 04:37:19 Masaki Ota wrote:
> Hi,
>
> We can get OTP page 0 value by EA EA E9 commands, but we cannot get it by EA EA EA E9.
> As far as I remember, Device initialization finish at EA command, then sends EA EA E9 commands.
> In this case we cannot get correct OTP page 0 value.
> So I changed this order. (We can get OTP page 1 value by both of EA F0 F0 E9 and F0 F0 E9.)
>
> Best Regards,
> Masaki Ota
> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Monday, March 26, 2018 6:26 AM
> To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Aaron Ma <aaron.ma@canonical.com>
> Subject: Re: [PATCH] Input: ALPS - fix DualPoint flag for 74 03 28 devices
>
> On Tuesday 20 March 2018 11:47:26 Dmitry Torokhov wrote:
> > On Mon, Jan 29, 2018 at 2:51 PM, dmitry.torokhov@gmail.com
> > <dmitry.torokhov@gmail.com> wrote:
> > > Hi,
> > >
> > > On Thu, Nov 16, 2017 at 07:27:02AM +0000, Masaki Ota wrote:
> > >> Hi, Pali, Aaron,
> > >>
> > >> Current code is correct device setting, previous code is wrong.
> > >> If the trackstick does not work(DUALPOINT flag disable), Device Firmware setting is wrong.
> > >>
> > >> But recently I received the same report from Thinkpad L570 user, and I checked this device and found this device Firmware setting is wrong. Sorry for our mistake.
> > >> Is your laptop L570 ?
> > >>
> > >> I will add code that supports the trackstick for this device.
> > >
> > > Sorry for resurrecting this old thread, I am just trying to
> > > understand what went wrong here. Is the sequence of "f0 f0 e9" and
> > > "ea ea e9" is important in getting the correct OTP data and we
> > > originally got this order wrong? It is not clear from the original
> > > patch and discussion that this change was intentional.
> >
> > Could I please get an answer to my question?
> >
> > Thanks!
>
> Masaki, this question is for you ↑↑↑
>
> > >
> > > Thanks.
> > >
> > >>
> > >> Best Regards,
> > >> Masaki Ota
> > >> -----Original Message-----
> > >> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > >> Sent: Wednesday, November 15, 2017 5:35 PM
> > >> To: 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>
> > >> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org;
> > >> dmitry.torokhov@gmail.com; Aaron Ma <aaron.ma@canonical.com>
> > >> Subject: Re: [PATCH] Input: ALPS - fix DualPoint flag for 74 03 28
> > >> devices
> > >>
> > >> On Wednesday 15 November 2017 14:34:04 Aaron Ma wrote:
> > >> > There is a regression of commit 4a646580f793 ("Input: ALPS - fix
> > >> > two-finger scroll breakage"), ALPS device fails with log:
> > >> >
> > >> > psmouse serio1: alps: Rejected trackstick packet from non
> > >> > DualPoint device
> > >> >
> > >> > ALPS device with id "74 03 28" report OTP[0] data 0xCE after
> > >> > commit 4a646580f793, after restore the OTP reading order, it
> > >> > becomes to 0x10 as before and reports the right flag.
> > >> >
> > >> > Fixes: 4a646580f793 ("Input: ALPS - fix two-finger scroll
> > >> > breakage")
> > >> > Cc: <stable@vger.kernel.org>
> > >> > Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> > >> > ---
> > >> > drivers/input/mouse/alps.c | 4 ++--
> > >> > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >> >
> > >> > diff --git a/drivers/input/mouse/alps.c
> > >> > b/drivers/input/mouse/alps.c index 579b899add26..c59b8f7ca2fc
> > >> > 100644
> > >> > --- a/drivers/input/mouse/alps.c
> > >> > +++ b/drivers/input/mouse/alps.c
> > >> > @@ -2562,8 +2562,8 @@ static int alps_set_defaults_ss4_v2(struct
> > >> > psmouse *psmouse,
> > >> >
> > >> > memset(otp, 0, sizeof(otp));
> > >> >
> > >> > - if (alps_get_otp_values_ss4_v2(psmouse, 1, &otp[1][0]) ||
> > >> > - alps_get_otp_values_ss4_v2(psmouse, 0, &otp[0][0]))
> > >> > + if (alps_get_otp_values_ss4_v2(psmouse, 0, &otp[0][0]) ||
> > >> > + alps_get_otp_values_ss4_v2(psmouse, 1, &otp[1][0]))
> > >> > return -1;
> > >> >
> > >> > alps_update_device_area_ss4_v2(otp, priv);
> > >>
> > >> Masaki Ota, please look at this patch as it partially revert your
> > >> commit
> > >> 4a646580f793 ("Input: ALPS - fix two-finger scroll breakage"). Something smells here.
> > >>
> > >> --
> > >> Pali Rohár
> > >> pali.rohar@gmail.com
> > >
> > > --
> > > Dmitry
> >
> >
> >
>
> --
> Pali Rohár
> pali.rohar@gmail.com
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH] Input: ALPS - add support for 73 03 28 devices (Thinkpad L570)
From: Pali Rohár @ 2018-03-28 7:37 UTC (permalink / raw)
To: Dennis Wassenberg
Cc: Masaki Ota, Dmitry Torokhov, Takashi Iwai, Kees Cook, Nir Perry,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <TYXPR01MB07194E0C045B134D1788E2C0C7A30@TYXPR01MB0719.jpnprd01.prod.outlook.com>
Ah, this si again same issue... It was already discussed here:
https://patchwork.kernel.org/patch/10081557/
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* [PATCH] HID: google: Enable PM Full On mode when adjusting backlight
From: Nicolas Boichat @ 2018-03-28 6:16 UTC (permalink / raw)
To: Jiri Kosina
Cc: Benjamin Tissoires, linux-kernel, linux-input, groeck, dtor,
Haridhar Kalvala
From: Haridhar Kalvala <haridhar.kalvala@intel.com>
hammer LED backlight brightness is not getting set when USB
device is in suspend state.
This patch fixes the issue by requesting USB HID device to be
in FULLON mode, so that sending hardware output report and
hardware raw request won't fail to set brightness, and set
device back to NORMAL mode once this call returns.
Signed-off-by: Haridhar Kalvala <haridhar.kalvala@intel.com>
Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---
drivers/hid/hid-google-hammer.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
Changes from version on Chromium OS gerrit
(https://chromium-review.googlesource.com/735259):
- Reworded and reflowed commit message, as well as comment in code.
diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
index 6486469ce0f64..7b8e17b03cb86 100644
--- a/drivers/hid/hid-google-hammer.c
+++ b/drivers/hid/hid-google-hammer.c
@@ -41,6 +41,16 @@ static int hammer_kbd_brightness_set_blocking(struct led_classdev *cdev,
led->buf[0] = 0;
led->buf[1] = br;
+ /*
+ * Request USB HID device to be in Full On mode, so that sending
+ * hardware output report and hardware raw request won't fail.
+ */
+ ret = hid_hw_power(led->hdev, PM_HINT_FULLON);
+ if (ret < 0) {
+ hid_err(led->hdev, "failed: device not resumed %d\n", ret);
+ return ret;
+ }
+
ret = hid_hw_output_report(led->hdev, led->buf, sizeof(led->buf));
if (ret == -ENOSYS)
ret = hid_hw_raw_request(led->hdev, 0, led->buf,
@@ -50,6 +60,10 @@ static int hammer_kbd_brightness_set_blocking(struct led_classdev *cdev,
if (ret < 0)
hid_err(led->hdev, "failed to set keyboard backlight: %d\n",
ret);
+
+ /* Request USB HID device back to Normal Mode. */
+ hid_hw_power(led->hdev, PM_HINT_NORMAL);
+
return ret;
}
--
2.17.0.rc1.321.gba9d0f2565-goog
^ permalink raw reply related
* RE: [PATCH] Input: ALPS - add support for 73 03 28 devices (Thinkpad L570)
From: Masaki Ota @ 2018-03-28 0:24 UTC (permalink / raw)
To: Dennis Wassenberg, Pali Rohár
Cc: Dmitry Torokhov, Takashi Iwai, Kees Cook, Nir Perry,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <bbb6a2ea-c8d1-beae-1f83-c234f03d4d77@secunet.com>
Hi, Dennis
I know your issue, and I added the solution for Thinkpad L/E system last year.
BTW, Pali also knows about it.
On Wednesday 29 November 2017 17:33:58 Masaki Ota wrote:
From: Masaki Ota <masaki.ota@jp.alps.com
- The issue is that Thinkpad L570 TrackStick does not work. Because the main interface of Thinkpad L570 device is SMBus, so ALPS overlooked PS2 interface Firmware setting of TrackStick. The detail is that TrackStick otp bit is disabled.
- Add the code that checks 0xD7 address value. This value is device number information, so we can identify the device by checking this value.
- If we check 0xD7 value, we need to enable Command mode and after check the value we need to disable Command mode, then we have to enable the device(0xF4 command).
- Thinkpad L570 device number is 0x0C or 0x1D. If it is TRUE, enable ALPS_DUALPOINT flag.
Signed-off-by: Masaki Ota <masaki.ota@jp.alps.com
---
drivers/input/mouse/alps.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/input/mouse/alps.c
b/drivers/input/mouse/alps.c index 850b00e3ad8e..6f092bdd9fc5
100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -2541,13 +2541,31 @@ static int
alps_update_btn_info_ss4_v2(unsigned char otp[][4], }
static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
- struct alps_data *priv)
+ struct alps_data *priv,
+ struct psmouse *psmouse)
{
bool is_dual = false;
+ int reg_val = 0;
+ struct ps2dev *ps2dev = &psmouse-ps2dev;
- if (IS_SS4PLUS_DEV(priv-dev_id))
+ if (IS_SS4PLUS_DEV(priv-dev_id)) {
is_dual = (otp[0][0] 4) & 0x01;
+ if (!is_dual) {
+ /* For support TrackStick of Thinkpad L/E series */
+ if (alps_exit_command_mode(psmouse) == 0 &&
+ alps_enter_command_mode(psmouse) == 0) {
+ reg_val = alps_command_mode_read_reg(psmouse,
+ 0xD7);
+ }
+ alps_exit_command_mode(psmouse);
+ ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
+
+ if (reg_val == 0x0C || reg_val == 0x1D)
+ is_dual = true;
+ }
+ }
+
if (is_dual)
priv-flags |= ALPS_DUALPOINT |
ALPS_DUALPOINT_WITH_PRESSURE; @@ -2570,7 +2588,7 @@ static
int alps_set_defaults_ss4_v2(struct psmouse *psmouse,
alps_update_btn_info_ss4_v2(otp, priv);
- alps_update_dual_info_ss4_v2(otp, priv);
+ alps_update_dual_info_ss4_v2(otp, priv, psmouse);
return 0;
}
Best Regards,
Masaki Ota
-----Original Message-----
From: Dennis Wassenberg [mailto:dennis.wassenberg@secunet.com]
Sent: Tuesday, March 27, 2018 10:56 PM
To: Pali Rohár <pali.rohar@gmail.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>; 太田 真喜 Masaki Ota <masaki.ota@jp.alps.com>; Takashi Iwai <tiwai@suse.de>; Kees Cook <keescook@chromium.org>; Nir Perry <nirperry@gmail.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Input: ALPS - add support for 73 03 28 devices (Thinkpad L570)
Hi,
oh ok, understood. Thanks for this hint.
So maybe there is something wrong with the alps_update_dual_info_ss4_v2 function or the reporting of the hardware.
alps_update_dual_info_ss4_v2 detects the ThinkPad L570 as ss4plus device but not as dualpoint device. This means that the ALPS_DUALPOINT and the ALPS_DUALPOINT_WITH_PRESSURE flag will not be set which results in a non function trackstick and hardware mouse buttons. Each time I touch the trackstick I get the message: "alps: Rejected trackstick packet from non DualPoint device".
The value of otp[0][0] inside alps_update_dual_info_ss4_v2 is 0xCE. Are there any ideas why it is not detected as dualpoint device?
Thank you & best regards,
Dennis
On 23.03.2018 15:33, Pali Rohár wrote:
> On Friday 23 March 2018 15:23:55 Dennis Wassenberg wrote:
>> The Lenovo Thinkpad L570 uses V8 protocol.
>> Add 0x73 0x03 0x28 devices to use V8 protovol which makes trackstick
>> and mouse buttons work with Lenovo Thinkpad L570.
>>
>> Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com>
>> ---
>> drivers/input/mouse/alps.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
>> index dbe57da..5523d4e 100644
>> --- a/drivers/input/mouse/alps.c
>> +++ b/drivers/input/mouse/alps.c
>> @@ -136,6 +136,8 @@
>> { { 0x73, 0x02, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
>> { { 0x73, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_FW_BK_2 } }, /* Ahtec Laptop */
>> { { 0x73, 0x02, 0x50 }, { ALPS_PROTO_V2, 0xcf, 0xcf, ALPS_FOUR_BUTTONS } }, /* Dell Vostro 1400 */
>> + { { 0x73, 0x03, 0x28 }, { ALPS_PROTO_V8, 0x18, 0x18,
>> + ALPS_DUALPOINT | ALPS_DUALPOINT_WITH_PRESSURE | ALPS_BUTTONPAD } }, /* Lenovo L570 */
>> };
>>
>> static const struct alps_protocol_info alps_v3_protocol_data = {
>
> Hi! alps_model_data table is used for fixed identification of v1 and
> v2 protocols. Why you need to add there v8 protocol which
> autodetection is already done in alps_identify() function? There is already code:
>
> } else if (e7[0] == 0x73 && e7[1] == 0x03 &&
> (e7[2] == 0x14 || e7[2] == 0x28)) {
> protocol = &alps_v8_protocol_data;
>
> Which matches above your E7 detection 0x73, 0x03, 0x28.
>
> Also you patch matches basically all v8 device and therefore has
> potential to break proper v8 autodetection for other v8 devices...
>
^ permalink raw reply
* Re: [PATCH] Input: ALPS - add support for 73 03 28 devices (Thinkpad L570)
From: Dennis Wassenberg @ 2018-03-27 13:56 UTC (permalink / raw)
To: Pali Rohár
Cc: Dmitry Torokhov, Masaki Ota, Takashi Iwai, Kees Cook, Nir Perry,
linux-input, linux-kernel
In-Reply-To: <20180323143336.v26rvflt3oq5xppn@pali>
Hi,
oh ok, understood. Thanks for this hint.
So maybe there is something wrong with the alps_update_dual_info_ss4_v2
function or the reporting of the hardware.
alps_update_dual_info_ss4_v2 detects the ThinkPad L570 as ss4plus device
but not as dualpoint device. This means that the ALPS_DUALPOINT and the
ALPS_DUALPOINT_WITH_PRESSURE flag will not be set which results in a non
function trackstick and hardware mouse buttons. Each time I touch the
trackstick I get the message: "alps: Rejected trackstick packet from non
DualPoint device".
The value of otp[0][0] inside alps_update_dual_info_ss4_v2 is 0xCE. Are
there any ideas why it is not detected as dualpoint device?
Thank you & best regards,
Dennis
On 23.03.2018 15:33, Pali Rohár wrote:
> On Friday 23 March 2018 15:23:55 Dennis Wassenberg wrote:
>> The Lenovo Thinkpad L570 uses V8 protocol.
>> Add 0x73 0x03 0x28 devices to use V8 protovol which makes
>> trackstick and mouse buttons work with Lenovo Thinkpad L570.
>>
>> Signed-off-by: Dennis Wassenberg <dennis.wassenberg@secunet.com>
>> ---
>> drivers/input/mouse/alps.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
>> index dbe57da..5523d4e 100644
>> --- a/drivers/input/mouse/alps.c
>> +++ b/drivers/input/mouse/alps.c
>> @@ -136,6 +136,8 @@
>> { { 0x73, 0x02, 0x0a }, { ALPS_PROTO_V2, 0xf8, 0xf8, 0 } },
>> { { 0x73, 0x02, 0x14 }, { ALPS_PROTO_V2, 0xf8, 0xf8, ALPS_FW_BK_2 } }, /* Ahtec Laptop */
>> { { 0x73, 0x02, 0x50 }, { ALPS_PROTO_V2, 0xcf, 0xcf, ALPS_FOUR_BUTTONS } }, /* Dell Vostro 1400 */
>> + { { 0x73, 0x03, 0x28 }, { ALPS_PROTO_V8, 0x18, 0x18,
>> + ALPS_DUALPOINT | ALPS_DUALPOINT_WITH_PRESSURE | ALPS_BUTTONPAD } }, /* Lenovo L570 */
>> };
>>
>> static const struct alps_protocol_info alps_v3_protocol_data = {
>
> Hi! alps_model_data table is used for fixed identification of v1 and v2
> protocols. Why you need to add there v8 protocol which autodetection is
> already done in alps_identify() function? There is already code:
>
> } else if (e7[0] == 0x73 && e7[1] == 0x03 &&
> (e7[2] == 0x14 || e7[2] == 0x28)) {
> protocol = &alps_v8_protocol_data;
>
> Which matches above your E7 detection 0x73, 0x03, 0x28.
>
> Also you patch matches basically all v8 device and therefore has
> potential to break proper v8 autodetection for other v8 devices...
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox