* [PATCH] HID parser: change usages table allocation to kvzalloc
From: Marcin Mikula @ 2023-09-27 10:58 UTC (permalink / raw)
To: linux-input; +Cc: Marcin Mikula
There are devices which deliver buggy HID descriptors, repoting
Usage Maximum for particular Usage Page of max value (0xFFFF).
By decision of kernel HID subsystem developers such number will be limited
to arbitrary value of HID_MAX_USAGES (12288), and the following log will
be printed to warn about such condition:
"kernel: hid-generic 0005:057A:0087.001A: ignoring exceeding usage max"
but still the amount of memory allocated for HID_MAX_USAGES modified value
can be quite high as for kmalloc allocation.
On some systems with limited memory (seen in ARM based embedded system),
in high memory pressure condition an attempt to kmalloc() such value
can fail.
As a consequence this HID Usage Page will not parsed, so will not be
handled and the events will be ignored.
From the user perspective part of the input described with this Usage Page
simply will not work, which is a severe condition.
Change of kzalloc to kvzalloc makes sure that this allocation will
not fail.
Example of the kernel callstack when allocation fails for reference:
kernel: kworker/1:1: page allocation failure: order:7,
mode:0x40dc0(GFP_KERNEL|__GFP_COMP|__GFP_ZERO), nodemask=(null),cpuset=/,
mems_allowed=0
Workqueue: events uhid_device_add_worker
Call trace:
dump_backtrace+0x0/0x1b4
show_stack+0x24/0x30
dump_stack+0xac/0x10c
warn_alloc+0xd8/0x158
__alloc_pages_slowpath+0x2e0/0x914
__alloc_pages_nodemask+0x1b0/0x278
kmalloc_order+0x30/0x7c
kmalloc_order_trace+0x3c/0xd4
__kmalloc+0x50/0x188
hid_add_field+0x160/0x250
hid_parser_main+0x220/0x25c
hid_open_report+0x138/0x278
hid_generic_probe+0x40/0x5c
hid_device_probe+0x130/0x154
really_probe+0x274/0x3f0
driver_probe_device+0x118/0x128
__device_attach_driver+0xc4/0x108
bus_for_each_drv+0xa4/0xc8
__device_attach+0xf4/0x17c
device_initial_probe+0x24/0x30
bus_probe_device+0x38/0x98
device_add+0x4a0/0x578
hid_add_device+0x100/0x294
uhid_device_add_worker+0x28/0x70
process_one_work+0x19c/0x294
worker_thread+0x1e4/0x274
kthread+0xdc/0xec
ret_from_fork+0x10/0x18
Signed-off-by: Marcin Mikula <marcin@helix.pl>
---
drivers/hid/hid-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 8992e3c1e769..0886868ee176 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -95,7 +95,7 @@ static struct hid_field *hid_register_field(struct hid_report *report, unsigned
return NULL;
}
- field = kzalloc((sizeof(struct hid_field) +
+ field = kvzalloc((sizeof(struct hid_field) +
usages * sizeof(struct hid_usage) +
3 * usages * sizeof(unsigned int)), GFP_KERNEL);
if (!field)
@@ -661,7 +661,7 @@ static void hid_free_report(struct hid_report *report)
kfree(report->field_entries);
for (n = 0; n < report->maxfield; n++)
- kfree(report->field[n]);
+ kvfree(report->field[n]);
kfree(report);
}
--
2.25.1
^ permalink raw reply related
* [regression] Resume broken on T14s Gen1 (AMD) due to "Input: psmouse - add delay when deactivating for SMBus mode"
From: Thorsten Leemhuis @ 2023-09-27 8:54 UTC (permalink / raw)
To: Jeffery Miller
Cc: Andrew Duggan, Dmitry Torokhov, Benjamin Tissoires, linux-input,
linux-kernel, Linux kernel regressions list
Hi Jeffery! Your change 92e24e0e57f72e ("Input: psmouse - add delay when
deactivating for SMBus mode") [merged for v6.6-rc1] broke resume on my
T14s Gen1 (AMD): the system didn't really resume again at all (the
display almost always didn't re-initialize) and there was nothing in the
logs. I found your commit to be the culprit using a bisection and
confirmed that reverting it on top of Linux 6.6-rc3 makes thing work
again for me.
My dmesg from a kernel with the revert:
https://www.leemhuis.info/files/misc/dmesg
My config:
https://www.leemhuis.info/files/misc/config
Funny detail: this is the full-blown Fedora rawhide config, apart from
disabling UCSI (it caused another problem that was fixed after -rc1); I
first had tried to use localmodconfig to strip down the config, but then
the problem did not happen for some reason (or I did something stupid).
Ciao, Thorsten
#regzbot introduced 92e24e0e57f72e
#regzbot title Input: psmouse - Resume broken on T14s Gen1 (AMD) due to
a new delay when deactivating for SMBus mode
^ permalink raw reply
* Pozycjonowanie- informacja
From: Dominik Perkowski @ 2023-09-27 8:30 UTC (permalink / raw)
To: linux-input
Dzień dobry,
jakiś czas temu zgłosiła się do nas firma, której strona internetowa nie pozycjonowała się wysoko w wyszukiwarce Google.
Na podstawie wykonanego przez nas audytu SEO zoptymalizowaliśmy treści na stronie pod kątem wcześniej opracowanych słów kluczowych. Nasz wewnętrzny system codziennie analizuje prawidłowe działanie witryny. Dzięki indywidualnej strategii, firma zdobywa coraz więcej Klientów.
Czy chcieliby Państwo zwiększyć liczbę osób odwiedzających stronę internetową firmy? Mógłbym przedstawić ofertę?
Pozdrawiam serdecznie,
Dominik Perkowski
^ permalink raw reply
* Re: [RFC PATCH 2/2] hid: lenovo: move type checks to lenovo_features_set_cptkbd()
From: Jamie Lentin @ 2023-09-27 8:19 UTC (permalink / raw)
To: Martin Kepplinger; +Cc: jikos, benjamin.tissoires, linux-kernel, linux-input
In-Reply-To: <20230925102302.13094-2-martink@posteo.de>
On 2023-09-25 11:23, Martin Kepplinger wrote:
> These custom commands will be sent to both the USB keyboard & mouse
> devices but only the mouse will respond. Avoid sending known-useless
> messages by always prepending the filter before sending them.
>
> Suggested-by: Jamie Lentin <jm@lentin.co.uk>
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> ---
> drivers/hid/hid-lenovo.c | 27 +++++++++------------------
> 1 file changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 29aa6d372bad..922f3e5462f4 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -521,6 +521,14 @@ static void lenovo_features_set_cptkbd(struct
> hid_device *hdev)
> int ret;
> struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
>
> + /* All the custom action happens on the USBMOUSE device for USB */
> + if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
> + (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
> + hdev->type != HID_TYPE_USBMOUSE) {
> + hid_dbg(hdev, "Ignoring keyboard half of device\n");
> + return;
> + }
> +
> /*
> * Tell the keyboard a driver understands it, and turn F7, F9, F11
> into
> * regular keys
> @@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct hid_device
> *hdev)
> int ret;
> struct lenovo_drvdata *cptkbd_data;
>
> - /* All the custom action happens on the USBMOUSE device for USB */
> - if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
> - (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD)) &&
> - hdev->type != HID_TYPE_USBMOUSE) {
> - hid_dbg(hdev, "Ignoring keyboard half of device\n");
> - return 0;
> - }
> -
I like the idea of doing it once then forgetting about it, but removing
this will mean that the "keyboard half" will have it's own set of
non-functional sysfs parameters I think? Currently:-
# evtest
. . .
/dev/input/event10: ThinkPad Compact Bluetooth Keyboard with
TrackPoint
/dev/input/event11: Lenovo ThinkPad Compact USB Keyboard with
TrackPoint
/dev/input/event12: Lenovo ThinkPad Compact USB Keyboard with
TrackPoint
# ls -1 /sys/class/input/event*/device/device/fn_lock
/sys/class/input/event10/device/device/fn_lock
/sys/class/input/event12/device/device/fn_lock
(note 11 is missing.)
I think the easiest (but ugly) thing to do is to copy-paste this lump of
code to the top of lenovo_reset_resume.
Cheers,
> cptkbd_data = devm_kzalloc(&hdev->dev,
> sizeof(*cptkbd_data),
> GFP_KERNEL);
> @@ -1264,16 +1264,7 @@ static int lenovo_probe(struct hid_device *hdev,
> #ifdef CONFIG_PM
> static int lenovo_reset_resume(struct hid_device *hdev)
> {
> - switch (hdev->product) {
> - case USB_DEVICE_ID_LENOVO_CUSBKBD:
> - if (hdev->type == HID_TYPE_USBMOUSE) {
> - lenovo_features_set_cptkbd(hdev);
> - }
> -
> - break;
> - default:
> - break;
> - }
> + lenovo_features_set_cptkbd(hdev);
>
> return 0;
> }
^ permalink raw reply
* [PATCH v7 1/3] input: pm8xxx-vib: refactor to easily support new SPMI vibrator
From: Fenglin Wu via B4 Relay @ 2023-09-27 6:05 UTC (permalink / raw)
To: linux-arm-msm, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski
Cc: linux-input, linux-kernel, devicetree, quic_collinsd,
quic_subbaram, quic_kamalw, jestar, Luca Weiss, Fenglin Wu
In-Reply-To: <20230927-pm8xxx-vibrator-v7-0-b5d8c92ce818@quicinc.com>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
Currently, all vibrator control register addresses are hard coded,
including the base address and the offset, it's not flexible to support
new SPMI vibrator module which is usually included in different PMICs
with different base address. Refactor this by defining register offset
with HW type combination, and register base address which is defined
in 'reg' property is added for SPMI vibrators.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
drivers/input/misc/pm8xxx-vibrator.c | 130 ++++++++++++++++++++++-------------
1 file changed, 81 insertions(+), 49 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 04cb87efd799..ba9be374f892 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -12,36 +12,44 @@
#include <linux/regmap.h>
#include <linux/slab.h>
+#define SSBI_VIB_DRV_REG 0x4A
+#define SSBI_VIB_DRV_EN_MANUAL_MASK GENMASK(7, 2)
+#define SSBI_VIB_DRV_LEVEL_MASK GENMASK(7, 3)
+#define SSBI_VIB_DRV_SHIFT 3
+
+#define SPMI_VIB_DRV_REG 0x41
+#define SPMI_VIB_DRV_LEVEL_MASK GENMASK(4, 0)
+#define SPMI_VIB_DRV_SHIFT 0
+
+#define SPMI_VIB_EN_REG 0x46
+#define SPMI_VIB_EN_BIT BIT(7)
+
#define VIB_MAX_LEVEL_mV (3100)
#define VIB_MIN_LEVEL_mV (1200)
#define VIB_MAX_LEVELS (VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
#define MAX_FF_SPEED 0xff
-struct pm8xxx_regs {
- unsigned int enable_addr;
- unsigned int enable_mask;
+enum vib_hw_type {
+ SSBI_VIB,
+ SPMI_VIB,
+};
- unsigned int drv_addr;
- unsigned int drv_mask;
- unsigned int drv_shift;
- unsigned int drv_en_manual_mask;
+struct pm8xxx_vib_data {
+ enum vib_hw_type hw_type;
+ unsigned int enable_addr;
+ unsigned int drv_addr;
};
-static const struct pm8xxx_regs pm8058_regs = {
- .drv_addr = 0x4A,
- .drv_mask = 0xf8,
- .drv_shift = 3,
- .drv_en_manual_mask = 0xfc,
+static const struct pm8xxx_vib_data ssbi_vib_data = {
+ .hw_type = SSBI_VIB,
+ .drv_addr = SSBI_VIB_DRV_REG,
};
-static struct pm8xxx_regs pm8916_regs = {
- .enable_addr = 0xc046,
- .enable_mask = BIT(7),
- .drv_addr = 0xc041,
- .drv_mask = 0x1F,
- .drv_shift = 0,
- .drv_en_manual_mask = 0,
+static const struct pm8xxx_vib_data spmi_vib_data = {
+ .hw_type = SPMI_VIB,
+ .enable_addr = SPMI_VIB_EN_REG,
+ .drv_addr = SPMI_VIB_DRV_REG,
};
/**
@@ -49,7 +57,8 @@ static struct pm8xxx_regs pm8916_regs = {
* @vib_input_dev: input device supporting force feedback
* @work: work structure to set the vibration parameters
* @regmap: regmap for register read/write
- * @regs: registers' info
+ * @data: vibrator HW info
+ * @reg_base: the register base of the module
* @speed: speed of vibration set from userland
* @active: state of vibrator
* @level: level of vibration to set in the chip
@@ -59,7 +68,8 @@ struct pm8xxx_vib {
struct input_dev *vib_input_dev;
struct work_struct work;
struct regmap *regmap;
- const struct pm8xxx_regs *regs;
+ const struct pm8xxx_vib_data *data;
+ unsigned int reg_base;
int speed;
int level;
bool active;
@@ -75,24 +85,39 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
{
int rc;
unsigned int val = vib->reg_vib_drv;
- const struct pm8xxx_regs *regs = vib->regs;
+ u32 mask, shift;
+
+ switch (vib->data->hw_type) {
+ case SSBI_VIB:
+ mask = SSBI_VIB_DRV_LEVEL_MASK;
+ shift = SSBI_VIB_DRV_SHIFT;
+ break;
+ case SPMI_VIB:
+ mask = SPMI_VIB_DRV_LEVEL_MASK;
+ shift = SPMI_VIB_DRV_SHIFT;
+ break;
+ default:
+ return -EINVAL;
+
+ }
if (on)
- val |= (vib->level << regs->drv_shift) & regs->drv_mask;
+ val |= (vib->level << shift) & mask;
else
- val &= ~regs->drv_mask;
+ val &= ~mask;
- rc = regmap_write(vib->regmap, regs->drv_addr, val);
+ rc = regmap_update_bits(vib->regmap, vib->reg_base + vib->data->drv_addr, mask, val);
if (rc < 0)
return rc;
vib->reg_vib_drv = val;
- if (regs->enable_mask)
- rc = regmap_update_bits(vib->regmap, regs->enable_addr,
- regs->enable_mask, on ? ~0 : 0);
+ if (vib->data->hw_type == SSBI_VIB)
+ return 0;
- return rc;
+ mask = SPMI_VIB_EN_BIT;
+ val = on ? SPMI_VIB_EN_BIT : 0;
+ return regmap_update_bits(vib->regmap, vib->reg_base + vib->data->enable_addr, mask, val);
}
/**
@@ -102,13 +127,6 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
static void pm8xxx_work_handler(struct work_struct *work)
{
struct pm8xxx_vib *vib = container_of(work, struct pm8xxx_vib, work);
- const struct pm8xxx_regs *regs = vib->regs;
- int rc;
- unsigned int val;
-
- rc = regmap_read(vib->regmap, regs->drv_addr, &val);
- if (rc < 0)
- return;
/*
* pmic vibrator supports voltage ranges from 1.2 to 3.1V, so
@@ -168,9 +186,9 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
{
struct pm8xxx_vib *vib;
struct input_dev *input_dev;
+ const struct pm8xxx_vib_data *data;
int error;
- unsigned int val;
- const struct pm8xxx_regs *regs;
+ unsigned int val, reg_base;
vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
if (!vib)
@@ -187,19 +205,33 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
INIT_WORK(&vib->work, pm8xxx_work_handler);
vib->vib_input_dev = input_dev;
- regs = of_device_get_match_data(&pdev->dev);
+ data = of_device_get_match_data(&pdev->dev);
+ if (!data)
+ return -EINVAL;
- /* operate in manual mode */
- error = regmap_read(vib->regmap, regs->drv_addr, &val);
- if (error < 0)
- return error;
+ if (data->hw_type != SSBI_VIB) {
+ error = fwnode_property_read_u32(pdev->dev.fwnode, "reg", ®_base);
+ if (error < 0) {
+ dev_err(&pdev->dev, "Failed to read reg address, rc=%d\n", error);
+ return error;
+ }
+
+ vib->reg_base += reg_base;
+ }
- val &= regs->drv_en_manual_mask;
- error = regmap_write(vib->regmap, regs->drv_addr, val);
+ error = regmap_read(vib->regmap, vib->reg_base + data->drv_addr, &val);
if (error < 0)
return error;
- vib->regs = regs;
+ /* operate in manual mode */
+ if (data->hw_type == SSBI_VIB) {
+ val &= SSBI_VIB_DRV_EN_MANUAL_MASK;
+ error = regmap_write(vib->regmap, vib->reg_base + data->drv_addr, val);
+ if (error < 0)
+ return error;
+ }
+
+ vib->data = data;
vib->reg_vib_drv = val;
input_dev->name = "pm8xxx_vib_ffmemless";
@@ -239,9 +271,9 @@ static int pm8xxx_vib_suspend(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(pm8xxx_vib_pm_ops, pm8xxx_vib_suspend, NULL);
static const struct of_device_id pm8xxx_vib_id_table[] = {
- { .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
- { .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
- { .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
+ { .compatible = "qcom,pm8058-vib", .data = &ssbi_vib_data },
+ { .compatible = "qcom,pm8921-vib", .data = &ssbi_vib_data },
+ { .compatible = "qcom,pm8916-vib", .data = &spmi_vib_data },
{ }
};
MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
--
2.25.1
^ permalink raw reply related
* [PATCH v7 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu via B4 Relay @ 2023-09-27 6:05 UTC (permalink / raw)
To: linux-arm-msm, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski
Cc: linux-input, linux-kernel, devicetree, quic_collinsd,
quic_subbaram, quic_kamalw, jestar, Luca Weiss, Fenglin Wu
In-Reply-To: <20230927-pm8xxx-vibrator-v7-0-b5d8c92ce818@quicinc.com>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
Add new SPMI vibrator module which is very similar to the SPMI vibrator
module inside PM8916 but just has a finer drive voltage step (1mV vs
100mV) hence its drive level control is expanded to across 2 registers.
The vibrator module can be found in Qualcomm PMIC PMI632, then following
PM7250B, PM7325B, PM7550BA PMICs.
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
Tested-by: Luca Weiss <luca.weiss@fairphone.com> # sdm632-fairphone-fp3 (pmi632)
---
drivers/input/misc/pm8xxx-vibrator.c | 42 +++++++++++++++++++++++++++++++++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index ba9be374f892..82c788841f1f 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -21,6 +21,13 @@
#define SPMI_VIB_DRV_LEVEL_MASK GENMASK(4, 0)
#define SPMI_VIB_DRV_SHIFT 0
+#define SPMI_VIB_GEN2_DRV_REG 0x40
+#define SPMI_VIB_GEN2_DRV_MASK GENMASK(7, 0)
+#define SPMI_VIB_GEN2_DRV_SHIFT 0
+#define SPMI_VIB_GEN2_DRV2_REG 0x41
+#define SPMI_VIB_GEN2_DRV2_MASK GENMASK(3, 0)
+#define SPMI_VIB_GEN2_DRV2_SHIFT 8
+
#define SPMI_VIB_EN_REG 0x46
#define SPMI_VIB_EN_BIT BIT(7)
@@ -33,12 +40,14 @@
enum vib_hw_type {
SSBI_VIB,
SPMI_VIB,
+ SPMI_VIB_GEN2
};
struct pm8xxx_vib_data {
enum vib_hw_type hw_type;
unsigned int enable_addr;
unsigned int drv_addr;
+ unsigned int drv2_addr;
};
static const struct pm8xxx_vib_data ssbi_vib_data = {
@@ -52,6 +61,13 @@ static const struct pm8xxx_vib_data spmi_vib_data = {
.drv_addr = SPMI_VIB_DRV_REG,
};
+static const struct pm8xxx_vib_data spmi_vib_gen2_data = {
+ .hw_type = SPMI_VIB_GEN2,
+ .enable_addr = SPMI_VIB_EN_REG,
+ .drv_addr = SPMI_VIB_GEN2_DRV_REG,
+ .drv2_addr = SPMI_VIB_GEN2_DRV2_REG,
+};
+
/**
* struct pm8xxx_vib - structure to hold vibrator data
* @vib_input_dev: input device supporting force feedback
@@ -96,9 +112,12 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
mask = SPMI_VIB_DRV_LEVEL_MASK;
shift = SPMI_VIB_DRV_SHIFT;
break;
+ case SPMI_VIB_GEN2:
+ mask = SPMI_VIB_GEN2_DRV_MASK;
+ shift = SPMI_VIB_GEN2_DRV_SHIFT;
+ break;
default:
return -EINVAL;
-
}
if (on)
@@ -112,6 +131,19 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
vib->reg_vib_drv = val;
+ if (vib->data->hw_type == SPMI_VIB_GEN2) {
+ mask = SPMI_VIB_GEN2_DRV2_MASK;
+ shift = SPMI_VIB_GEN2_DRV2_SHIFT;
+ if (on)
+ val = (vib->level >> shift) & mask;
+ else
+ val = 0;
+ rc = regmap_update_bits(vib->regmap,
+ vib->reg_base + vib->data->drv2_addr, mask, val);
+ if (rc < 0)
+ return rc;
+ }
+
if (vib->data->hw_type == SSBI_VIB)
return 0;
@@ -136,10 +168,13 @@ static void pm8xxx_work_handler(struct work_struct *work)
vib->active = true;
vib->level = ((VIB_MAX_LEVELS * vib->speed) / MAX_FF_SPEED) +
VIB_MIN_LEVEL_mV;
- vib->level /= 100;
+ if (vib->data->hw_type != SPMI_VIB_GEN2)
+ vib->level /= 100;
} else {
vib->active = false;
- vib->level = VIB_MIN_LEVEL_mV / 100;
+ vib->level = VIB_MIN_LEVEL_mV;
+ if (vib->data->hw_type != SPMI_VIB_GEN2)
+ vib->level /= 100;
}
pm8xxx_vib_set(vib, vib->active);
@@ -274,6 +309,7 @@ static const struct of_device_id pm8xxx_vib_id_table[] = {
{ .compatible = "qcom,pm8058-vib", .data = &ssbi_vib_data },
{ .compatible = "qcom,pm8921-vib", .data = &ssbi_vib_data },
{ .compatible = "qcom,pm8916-vib", .data = &spmi_vib_data },
+ { .compatible = "qcom,pmi632-vib", .data = &spmi_vib_gen2_data },
{ }
};
MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
--
2.25.1
^ permalink raw reply related
* [PATCH v7 2/3] dt-bindings: input: qcom,pm8xxx-vib: add new SPMI vibrator module
From: Fenglin Wu via B4 Relay @ 2023-09-27 6:05 UTC (permalink / raw)
To: linux-arm-msm, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski
Cc: linux-input, linux-kernel, devicetree, quic_collinsd,
quic_subbaram, quic_kamalw, jestar, Luca Weiss,
Krzysztof Kozlowski, Fenglin Wu
In-Reply-To: <20230927-pm8xxx-vibrator-v7-0-b5d8c92ce818@quicinc.com>
From: Fenglin Wu <quic_fenglinw@quicinc.com>
Add compatible strings to support vibrator module inside PMI632,
PMI7250B, PM7325B, PM7550BA.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Fenglin Wu <quic_fenglinw@quicinc.com>
---
.../devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
index c8832cd0d7da..2025d6a5423e 100644
--- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
+++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.yaml
@@ -11,10 +11,18 @@ maintainers:
properties:
compatible:
- enum:
- - qcom,pm8058-vib
- - qcom,pm8916-vib
- - qcom,pm8921-vib
+ oneOf:
+ - enum:
+ - qcom,pm8058-vib
+ - qcom,pm8916-vib
+ - qcom,pm8921-vib
+ - qcom,pmi632-vib
+ - items:
+ - enum:
+ - qcom,pm7250b-vib
+ - qcom,pm7325b-vib
+ - qcom,pm7550ba-vib
+ - const: qcom,pmi632-vib
reg:
maxItems: 1
--
2.25.1
^ permalink raw reply related
* [PATCH v7 0/3] Add support for vibrator in multiple PMICs
From: Fenglin Wu via B4 Relay @ 2023-09-27 6:05 UTC (permalink / raw)
To: linux-arm-msm, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski
Cc: linux-input, linux-kernel, devicetree, quic_collinsd,
quic_subbaram, quic_kamalw, jestar, Luca Weiss, Fenglin Wu,
Krzysztof Kozlowski
Add SW support for the vibrator module inside PMI632, PM7250B, PM7325B, PM7550BA.
It is very similar to the vibrator module inside PM8916 which is supported in
pm8xxx-vib driver but just the drive amplitude is controlled with 2 registers,
and the register base offset in each PMIC is different.
Changes in v7;
1. Fix a typo: SSBL_VIB_DRV_REG --> SSBI_VIB_DRV_REG
2. Move the hw_type switch case in pm8xxx_vib_set() to the refactoring
change.
Changes in v6:
1. Add "qcom,pmi632-vib" as a standalone compatible string.
Changes in v5:
1. Drop "qcom,spmi-vib-gen2" generic compatible string as requested
and use device specific compatible strings only.
Changes in v4:
1. Update to use the combination of the HW type and register offset
as the constant match data, the register base address defined in
'reg' property will be added when accessing SPMI registers using
regmap APIs.
2. Remove 'qcom,spmi-vib-gen1' generic compatible string.
Changes in v3:
1. Refactor the driver to support different type of the vibrators with
better flexibility by introducing the HW type with corresponding
register fields definitions.
2. Add 'qcom,spmi-vib-gen1' and 'qcom,spmi-vib-gen2' compatible
strings, and add PMI632, PM7250B, PM7325B, PM7550BA as compatbile as
spmi-vib-gen2.
Changes in v2:
Remove the "pm7550ba-vib" compatible string as it's compatible with pm7325b.
Fenglin Wu (3):
input: pm8xxx-vib: refactor to easily support new SPMI vibrator
dt-bindings: input: qcom,pm8xxx-vib: add new SPMI vibrator module
input: pm8xxx-vibrator: add new SPMI vibrator support
.../bindings/input/qcom,pm8xxx-vib.yaml | 16 +-
drivers/input/misc/pm8xxx-vibrator.c | 171 ++++++++++++------
2 files changed, 132 insertions(+), 55 deletions(-)
--
2.25.1
---
Fenglin Wu (3):
input: pm8xxx-vib: refactor to easily support new SPMI vibrator
dt-bindings: input: qcom,pm8xxx-vib: add new SPMI vibrator module
input: pm8xxx-vibrator: add new SPMI vibrator support
.../devicetree/bindings/input/qcom,pm8xxx-vib.yaml | 16 +-
drivers/input/misc/pm8xxx-vibrator.c | 170 ++++++++++++++-------
2 files changed, 131 insertions(+), 55 deletions(-)
---
base-commit: 650cda2ce25f08e8fae391b3ba6be27e7296c6a5
change-id: 20230925-pm8xxx-vibrator-62df3df46a6c
Best regards,
--
Fenglin Wu <quic_fenglinw@quicinc.com>
^ permalink raw reply
* Re: [RFC PATCH] of: device: Support 2nd sources of probeable but undiscoverable devices
From: Jeff LaBundy @ 2023-09-27 2:36 UTC (permalink / raw)
To: Doug Anderson
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, devicetree,
Benjamin Tissoires, Chen-Yu Tsai, linux-input, Jiri Kosina,
Hsin-Yi Wang, linux-gpio, linus.walleij, Dmitry Torokhov,
Johan Hovold, andriy.shevchenko, broonie, frowand.list, gregkh,
hdegoede, james.clark, james, keescook, linux-kernel, rafael,
tglx
In-Reply-To: <CAD=FV=UgFzT0TW2WEV0Wmk05EXUad2EYhN2DcckAxE_Lw5gV1Q@mail.gmail.com>
Hi Doug,
On Fri, Sep 22, 2023 at 05:11:10PM -0700, Doug Anderson wrote:
> Hi,
>
> On Fri, Sep 22, 2023 at 12:08 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > > > This seems like overkill to me. Do we really need groups and a mutex
> > > > for each group? Worst case is what? 2-3 groups of 2-3 devices?
> > > > Instead, what about extending "status" with another value
> > > > ("fail-needs-probe"? (fail-xxx is a documented value)). Currently, the
> > > > kernel would just ignore nodes with that status. Then we can process
> > > > those nodes separately 1-by-1.
> > >
> > > My worry here is that this has the potential to impact boot speed in a
> > > non-trivial way. While trackpads and touchscreens _are_ probable,
> > > their probe routines are often quite slow. This is even mentioned in
> > > Dmitry's initial patches adding async probe to the kernel. See commit
> > > 765230b5f084 ("driver-core: add asynchronous probing support for
> > > drivers") where he specifically brings up input devices as examples.
Ideally, all but one driver in a group should bail out of probe quickly if
the device is not populated. If not, I would consider that to be a bug or at
least room for improvement in that driver.
The reason input devices can take a while to probe is because they may be
loading FW over I2C or performing some sort of calibration procedure; only
one driver in the group should get that far.
> >
> > Perhaps then this should be solved in userspace where it can learn
> > which device is actually present and save that information for
> > subsequent boots.
>
> Yeah, the thought occurred to me as well. I think there are a few
> problems, though:
>
> a) Userspace can't itself probe these devices effectively. While
> userspace could turn on GPIOs manually and query the i2c bus manually,
> it can't (I believe) turn on regulators nor can it turn on clocks, if
> they are needed. About the best userspace could do would be to blindly
> try binding an existing kernel driver, and in that case why did we
> need userspace involved anyway?
>
> b) While deferring to userspace can work for solutions like ChromeOS
> or Android where it's easy to ensure the userspace bits are there,
> it's less appealing as a general solution. I think in Johan's case
> he's taking a laptop that initially ran Windows and then is trying to
> run a generic Linux distro on it. For anyone in a similar situation,
> they'd either need to pick a Linux distro that has the magic userspace
> bits that are needed or they need to know that, on their laptop, they
> need to manually install some software. While requiring special
> userspace might make sense if you've got a special peripheral, like an
> LTE modem, it makes less sense to need special userspace just to get
> the right devices bound...
I recommend against spilling the solution into user space; it's simply not
practical for many downstream use-cases where platform engineers can tightly
control their own bootloader and kernel, but have limited maintainership of
user space which is likely to be shared by many other products.
>
>
> > > It wouldn't be absurd to have a system that has multiple sources for
> > > both the trackpad and the touchscreen. If we have to probe each of
> > > these one at a time then it could be slow. It would be quicker to be
> > > able to probe the trackpads (one at a time) at the same time we're
> > > probing the touchscreens (one at a time). Using the "fail-needs-probe"
> > > doesn't provide information needed to know which devices conflict with
> > > each other.
> >
> > I would guess most of the time that's pretty evident. They are going
> > to be on the same bus/link. If unrelated devices are on the same bus,
> > then that's going to get serialized anyways (if bus accesses are what
> > make things slow).
Agreed with Rob here; in the case of a touchscreen, we're almost always
dealing with a module that connects to the main board by way of a flex
connector. Rarely do the bus and GPIO assignments change across alternates.
> >
> > We could add information on the class of device. touchscreen and
> > touchpad aliases or something.
>
> Ah, I see. So something like "fail-needs-probe-<class>". The
> touchscreens could have "fail-needs-probe-touchscreen" and the
> trackpads could have "fail-needs-probe-trackpad" ? That could work. In
> theory that could fall back to the same solution of grabbing a mutex
> based on the group ID...
>
> Also: if having the mutex in the "struct device" is seen as a bad
> idea, it would also be easy to remove. __driver_probe_device() could
> just make a call like "of_device_probe_start()" at the beginning that
> locks the mutex and then "of_device_probe_end()" that unlocks it. Both
> of those calls could easily lookup the mutex in a list, which would
> get rid of the need to store it in the "struct device".
>
>
> > > That would lead me to suggest this:
> > >
> > > &i2c_bus {
> > > trackpad-prober {
> > > compatible = "mt8173-elm-hana-trackpad-prober";
> > >
> > > tp1: trackpad@10 {
> > > compatible = "hid-over-i2c";
> > > reg = <0x10>;
> > > ...
> > > post-power-on-delay-ms = <200>;
> > > };
> > > tp2: trackpad@20 {
> > > compatible = "hid-over-i2c";
> > > reg = <0x20>;
> > > ...
> > > post-power-on-delay-ms = <200>;
> > > };
> > > };
> > > };
> > >
> > > ...but I suspect that would be insta-NAKed because it's creating a
> > > completely virtual device ("mt8173-elm-hana-trackpad-prober") in the
> > > device tree. I don't know if there's something that's functionally
> > > similar that would be OK?
This solution seems a bit confusing to me, and would require more edits
to the dts each time a second source is added. It also means one would
have to write a small platform driver for each group of devices, correct?
I like the idea of a new "status" string; just my $.02.
> >
> > Why do you need the intermediate node other than a convenient way to
> > instantiate a driver? You just need a flag in each node which needs
> > this special handling. Again, "status" could work well here since it
> > keeps the normal probe from happening. But I'm not saying you can't
> > have some board specific code. Sometimes you just need code to deal
> > with this stuff. Don't try to parameterize everything to DT
> > properties.
>
> I think I'd have an easier time understanding if I knew where you
> envisioned the board-specific code living. Do you have an example of
> board specific code running at boot time in the kernel on DT systems?
>
>
> > Note that the above only works with "generic" compatibles with
> > "generic" power sequencing properties (I won't repeat my dislike
> > again).
>
> I don't think so? I was imagining that we'd have some board specific
> code that ran that knew all the possible combinations of devices,
> could probe them, and then could instantiate the correct driver.
>
> Imagine that instead of the hated "hid-over-i2c" compatible we were
> using two other devices. Imagine that a given board could have a
> "elan,ekth6915" and a "goodix,gt7375p". Both of these devices have
> specific timing requirements on how to sequence their supplies and
> reset GPIOs. For Elan we power on the supplies, wait at least 1 ms,
> deassert reset, wait at least 300 ms, and then can talk i2c. For
> Goodix we power on the supply, wait at least 10 ms, deassert reset,
> wait at least 180 ms, and then can talk i2c. If we had a
> board-specific probing driver then it would power on the supplies,
> wait at least 10 ms (the max of the two), deassert reset, wait at
> least 300 ms (the max of the two), and then see which device talked.
> Then it would instantiate whichever of the two drivers. This could be
> done for any two devices that EEs have determined have "compatible"
> probing sequences.
>
> Ideally in the above situation we'd be able to avoid turning the
> device off and on again between the board-specific probe code and the
> normal driver. That optimization might need special code per-driver
> but it feels doable by passing some sort of hint to the child driver
> when it's instantiated.
>
>
> > If only the driver knows how to handle the device, then you
> > still just have to have the driver probe. If you *only* wanted to
> > solve the above case, I'd just make "hid-over-i2c" take a 2nd (and
> > 3rd) I2C address in reg and have those as fallbacks.
>
> Yeah, it did occur to me that having "hid-over-i2c" take more than one
> register (and I guess more than one "hid-descr-addr") would work in my
> earlier example and this might actually be a good solution for Johan.
> I'm hoping for a better generic solution, though.
>
>
> > You could always make the driver probe smarter where if your supply
> > was already powered on, then don't delay. Then something else could
> > ensure that the supply is enabled. I'm not sure if regulators have the
> > same issue as clocks where the clock might be on from the bootloader,
> > then a failed probe which gets then puts the clock turns it off.
>
> I'm not sure it's that simple. Even if the supply didn't turn off by
> itself in some cases, we wouldn't know how long the supply was on.
>
> -Doug
Thanks for charting this path; I'm really excited to see a solution to
this common problem.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH 1/2] input: generalize the Imagis touchscreen driver
From: Jeff LaBundy @ 2023-09-27 1:48 UTC (permalink / raw)
To: Karel Balej
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Markuss Broks, linux-input, devicetree, linux-kernel,
Duje Mihanović, ~postmarketos/upstreaming
In-Reply-To: <20230926173531.18715-2-balejk@matfyz.cz>
Hi Karel,
On Tue, Sep 26, 2023 at 07:35:23PM +0200, Karel Balej wrote:
> This driver should work with other Imagis ICs of the IST30**C series.
> Make that more apparent.
>
> Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
> ...gis,ist3038c.yaml => imagis,ist30xxc.yaml} | 2 +-
> MAINTAINERS | 2 +-
> drivers/input/touchscreen/Kconfig | 4 +-
> drivers/input/touchscreen/imagis.c | 86 +++++++++++--------
> 4 files changed, 52 insertions(+), 42 deletions(-)
> rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (99%)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> similarity index 99%
> rename from Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> rename to Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> index 0d6b033fd5fb..09bf3a6acc5e 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> %YAML 1.2
> ---
> -$id: http://devicetree.org/schemas/input/touchscreen/imagis,ist3038c.yaml#
> +$id: http://devicetree.org/schemas/input/touchscreen/imagis,ist30xxc.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> title: Imagis IST30XXC family touchscreen controller
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b19995690904..b23e76418d94 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10209,7 +10209,7 @@ F: drivers/usb/atm/ueagle-atm.c
> IMAGIS TOUCHSCREEN DRIVER
> M: Markuss Broks <markuss.broks@gmail.com>
> S: Maintained
> -F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> +F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> F: drivers/input/touchscreen/imagis.c
>
> IMGTEC ASCII LCD DRIVER
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index e3e2324547b9..45503aa2653e 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -665,10 +665,10 @@ config TOUCHSCREEN_NOVATEK_NVT_TS
> module will be called novatek-nvt-ts.
>
> config TOUCHSCREEN_IMAGIS
> - tristate "Imagis touchscreen support"
> + tristate "Imagis IST30XXC touchscreen support"
> depends on I2C
> help
> - Say Y here if you have an Imagis IST30xxC touchscreen.
> + Say Y here if you have an Imagis IST30XXC touchscreen.
> If unsure, say N.
>
> To compile this driver as a module, choose M here: the
> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
> index 07111ca24455..4456f1b4d527 100644
> --- a/drivers/input/touchscreen/imagis.c
> +++ b/drivers/input/touchscreen/imagis.c
> @@ -11,25 +11,26 @@
> #include <linux/property.h>
> #include <linux/regulator/consumer.h>
>
> -#define IST3038C_HIB_ACCESS (0x800B << 16)
> -#define IST3038C_DIRECT_ACCESS BIT(31)
> -#define IST3038C_REG_CHIPID 0x40001000
> -#define IST3038C_REG_HIB_BASE 0x30000100
> -#define IST3038C_REG_TOUCH_STATUS (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS)
> -#define IST3038C_REG_TOUCH_COORD (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x8)
> -#define IST3038C_REG_INTR_MESSAGE (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x4)
> +#define IST30XXC_HIB_ACCESS (0x800B << 16)
> +#define IST30XXC_DIRECT_ACCESS BIT(31)
> +#define IST30XXC_REG_CHIPID 0x40001000
> +#define IST30XXC_REG_HIB_BASE 0x30000100
> +#define IST30XXC_REG_TOUCH_STATUS (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS)
> +#define IST30XXC_REG_TOUCH_COORD (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x8)
> +#define IST30XXC_REG_INTR_MESSAGE (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x4)
> +#define IST30XXC_CHIP_ON_DELAY_MS 60
> +#define IST30XXC_I2C_RETRY_COUNT 3
> +#define IST30XXC_MAX_FINGER_NUM 10
> +#define IST30XXC_X_MASK GENMASK(23, 12)
> +#define IST30XXC_X_SHIFT 12
> +#define IST30XXC_Y_MASK GENMASK(11, 0)
> +#define IST30XXC_AREA_MASK GENMASK(27, 24)
> +#define IST30XXC_AREA_SHIFT 24
> +#define IST30XXC_FINGER_COUNT_MASK GENMASK(15, 12)
> +#define IST30XXC_FINGER_COUNT_SHIFT 12
> +#define IST30XXC_FINGER_STATUS_MASK GENMASK(9, 0)
> +
> #define IST3038C_WHOAMI 0x38c
> -#define IST3038C_CHIP_ON_DELAY_MS 60
> -#define IST3038C_I2C_RETRY_COUNT 3
> -#define IST3038C_MAX_FINGER_NUM 10
> -#define IST3038C_X_MASK GENMASK(23, 12)
> -#define IST3038C_X_SHIFT 12
> -#define IST3038C_Y_MASK GENMASK(11, 0)
> -#define IST3038C_AREA_MASK GENMASK(27, 24)
> -#define IST3038C_AREA_SHIFT 24
> -#define IST3038C_FINGER_COUNT_MASK GENMASK(15, 12)
> -#define IST3038C_FINGER_COUNT_SHIFT 12
> -#define IST3038C_FINGER_STATUS_MASK GENMASK(9, 0)
>
There is no need to change all of the namespacing from IST3038C to IST30XXC;
this is purely cosmetic and adds noise to the actual patch.
It is perfectly acceptable for the driver to call itself IST3038C throughout
while remaining compatible with other devices (e.g. IST3032C), in fact this
flexibility is the intent of the compatible string in the first place.
[...]
>
> -MODULE_DESCRIPTION("Imagis IST3038C Touchscreen Driver");
> +MODULE_DESCRIPTION("Imagis IST30XXC Touchscreen Driver");
> MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
> MODULE_LICENSE("GPL");
> --
> 2.42.0
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Jeff LaBundy @ 2023-09-27 2:59 UTC (permalink / raw)
To: Stephan Gerhold
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Jonathan Albrieux
In-Reply-To: <ZQcx7oQyL6RM06Jt@gerhold.net>
Hi Stephan,
On Sun, Sep 17, 2023 at 07:05:50PM +0200, Stephan Gerhold wrote:
> Hi Jeff,
>
> Thanks a lot for your detailed review!
Thank you for the productive discussion.
>
> On Sat, Sep 16, 2023 at 03:47:55PM -0500, Jeff LaBundy wrote:
> > On Wed, Sep 13, 2023 at 03:25:30PM +0200, Stephan Gerhold wrote:
> > > From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > >
> > > Add a simple driver for the Himax HX852x(ES) touch panel controller,
> > > with support for multi-touch and capacitive touch keys.
> > >
> > > The driver is somewhat based on sample code from Himax. However, that
> > > code was so extremely confusing that we spent a significant amount of
> > > time just trying to understand the packet format and register commands.
> > > In this driver they are described with clean structs and defines rather
> > > than lots of magic numbers and offset calculations.
> > >
> > > Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > > Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
> > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > ---
> > > MAINTAINERS | 7 +
> > > drivers/input/touchscreen/Kconfig | 10 +
> > > drivers/input/touchscreen/Makefile | 1 +
> > > drivers/input/touchscreen/himax_hx852x.c | 491 +++++++++++++++++++++++++++++++
> > > 4 files changed, 509 insertions(+)
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 90f13281d297..c551c60b0598 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -9331,6 +9331,13 @@ S: Maintained
> > > F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> > > F: drivers/input/touchscreen/himax_hx83112b.c
> > >
> > > +HIMAX HX852X TOUCHSCREEN DRIVER
> > > +M: Stephan Gerhold <stephan@gerhold.net>
> > > +L: linux-input@vger.kernel.org
> > > +S: Maintained
> > > +F: Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
> > > +F: drivers/input/touchscreen/himax_hx852x.c
> > > +
> > > HIPPI
> > > M: Jes Sorensen <jes@trained-monkey.org>
> > > L: linux-hippi@sunsite.dk
> > > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > > index e3e2324547b9..8e5667ae5dab 100644
> > > --- a/drivers/input/touchscreen/Kconfig
> > > +++ b/drivers/input/touchscreen/Kconfig
> > > @@ -427,6 +427,16 @@ config TOUCHSCREEN_HIDEEP
> > > To compile this driver as a module, choose M here : the
> > > module will be called hideep_ts.
> > >
> > > +config TOUCHSCREEN_HIMAX_HX852X
> > > + tristate "Himax HX852x(ES) touchscreen"
> > > + depends on I2C
> > > + help
> > > + Say Y here if you have a Himax HX852x(ES) touchscreen.
> > > + If unsure, say N.
> > > +
> > > + To compile this driver as a module, choose M here: the module
> > > + will be called himax_hx852x.
> > > +
> > > config TOUCHSCREEN_HYCON_HY46XX
> > > tristate "Hycon hy46xx touchscreen support"
> > > depends on I2C
> > > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > > index 62bd24f3ac8e..f42a87faa86c 100644
> > > --- a/drivers/input/touchscreen/Makefile
> > > +++ b/drivers/input/touchscreen/Makefile
> > > @@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> > > obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> > > obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> > > obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> > > +obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX852X) += himax_hx852x.o
> > > obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> > > obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> > > obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
> > > diff --git a/drivers/input/touchscreen/himax_hx852x.c b/drivers/input/touchscreen/himax_hx852x.c
> > > new file mode 100644
> > > index 000000000000..31616dcfc5ab
> > > --- /dev/null
> > > +++ b/drivers/input/touchscreen/himax_hx852x.c
> > > @@ -0,0 +1,491 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Himax HX852x(ES) Touchscreen Driver
> > > + * Copyright (c) 2020-2023 Stephan Gerhold <stephan@gerhold.net>
> > > + * Copyright (c) 2020 Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > > + *
> > > + * Based on the Himax Android Driver Sample Code Ver 0.3 for HMX852xES chipset:
> > > + * Copyright (c) 2014 Himax Corporation.
> > > + */
> > > +
> > > +#include <asm/unaligned.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/input.h>
> > > +#include <linux/input/mt.h>
> > > +#include <linux/input/touchscreen.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> >
> > Please explicitly #include of_device.h.
> >
>
> Ack, thanks!
>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +#define HX852X_COORD_SIZE(fingers) ((fingers) * sizeof(struct hx852x_coord))
> > > +#define HX852X_WIDTH_SIZE(fingers) ALIGN(fingers, 4)
> > > +#define HX852X_BUF_SIZE(fingers) (HX852X_COORD_SIZE(fingers) + \
> > > + HX852X_WIDTH_SIZE(fingers) + \
> > > + sizeof(struct hx852x_touch_info))
> > > +
> > > +#define HX852X_MAX_FINGERS 12
> >
> > Well, that's a new one :)
> >
>
> FWIW: I'm not sure if any controller exists that actually supports 12,
> but the bits are layed out in a way that it would be possible. :-)
>
> > > +#define HX852X_MAX_KEY_COUNT 4
> > > +#define HX852X_MAX_BUF_SIZE HX852X_BUF_SIZE(HX852X_MAX_FINGERS)
> > > +
> > > +#define HX852X_TS_SLEEP_IN 0x80
> > > +#define HX852X_TS_SLEEP_OUT 0x81
> > > +#define HX852X_TS_SENSE_OFF 0x82
> > > +#define HX852X_TS_SENSE_ON 0x83
> > > +#define HX852X_READ_ONE_EVENT 0x85
> > > +#define HX852X_READ_ALL_EVENTS 0x86
> > > +#define HX852X_READ_LATEST_EVENT 0x87
> > > +#define HX852X_CLEAR_EVENT_STACK 0x88
> > > +
> > > +#define HX852X_REG_SRAM_SWITCH 0x8c
> > > +#define HX852X_REG_SRAM_ADDR 0x8b
> > > +#define HX852X_REG_FLASH_RPLACE 0x5a
> > > +
> > > +#define HX852X_SRAM_SWITCH_TEST_MODE 0x14
> > > +#define HX852X_SRAM_ADDR_CONFIG 0x7000
> > > +
> > > +struct hx852x {
> > > + struct i2c_client *client;
> > > + struct input_dev *input_dev;
> > > + struct touchscreen_properties props;
> > > +
> > > + struct gpio_desc *reset_gpiod;
> > > + struct regulator_bulk_data supplies[2];
> > > +
> > > + unsigned int max_fingers;
> > > + unsigned int keycount;
> > > + u32 keycodes[HX852X_MAX_KEY_COUNT];
> >
> > Nit: might as well make keycodes 'unsigned int' for consistency; I also do not
> > feel the newlines are necessary.
> >
>
> I don't mind having the newlines but also don't mind removing them,
> will drop them in v2!
>
> > > +};
> > > +
> > > +struct hx852x_config {
> > > + u8 rx_num;
> > > + u8 tx_num;
> > > + u8 max_pt;
> > > + u8 padding1[3];
> > > + __be16 x_res;
> > > + __be16 y_res;
> > > + u8 padding2[2];
> > > +} __packed __aligned(4);
> >
> > What is the reason to include trailing padding in this packed struct? Does the
> > controller require the entire register length to be read for some reason?
> >
>
> Given your question I guess "padding" might be the wrong word here.
> Basically the driver we based this on reads this config in a
> semi-obfuscated way like this:
>
> char data[12];
> i2c_himax_read(..., data, 12, ...);
> rx_num = data[0];
> /* ... */
> x_res = data[6]*256 + data[7];
> /* ... */
>
> data[10] and data[11] are not used in the vendor code, so we don't know
> what is encoded there, if there is something encoded there, if only on
> some models or only on some firmware versions.
>
> I would prefer to keep the read/write operations similar to the vendor
> driver. Who knows if there are peculiar firmware versions that would
> fail if the read size is not correct. And maybe there is actually
> interesting data in there?
>
> Maybe "unknown" would be more clear than "padding"?
If it is unknown whether a specific number of bytes must be read from the
controller for it to remain in a valid state, then I think it is fine to
keep your existing implementation, as well as the original name 'padding'.
>
> > > +
> > > +struct hx852x_coord {
> > > + __be16 x;
> > > + __be16 y;
> > > +} __packed __aligned(4);
> > > +
> > > +struct hx852x_touch_info {
> > > + u8 finger_num;
> > > + __le16 finger_pressed;
> >
> > It's interesting that most registers/packets use big endian (e.g. x_res) while
> > only this uses little endian. Is that expected?
> >
>
> Yes, it's really like that. If you look closely the __le16 is also the
> only 16-bit number that isn't aligned properly. I guess for the
> "protocol designers" this fields was maybe not a 16-bit number but two
> separate fields. No idea what they were thinking.
ACK.
>
> > > + u8 padding;
> >
> > Same question with regard to trailing padding.
> >
>
> I think the same answer as above applies here. Additionally here, the
> packet format seems to be intentionally 4-byte/32-bit aligned (see
> comment in hx852x_handle_events()) so I would say it makes sense to also
> read an aligned size from the controller.
>
> > > +} __packed __aligned(4);
> > > +
> > > +static int hx852x_i2c_read(struct hx852x *hx, u8 cmd, void *data, u16 len)
> > > +{
> > > + struct i2c_client *client = hx->client;
> > > + int ret;
> > > +
> > > + struct i2c_msg msg[] = {
> > > + {
> > > + .addr = client->addr,
> > > + .flags = 0,
> > > + .len = 1,
> > > + .buf = &cmd,
> > > + },
> > > + {
> > > + .addr = client->addr,
> > > + .flags = I2C_M_RD,
> > > + .len = len,
> > > + .buf = data,
> > > + }
> > > + };
> > > +
> > > + ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> > > + if (ret != ARRAY_SIZE(msg)) {
> > > + dev_err(&client->dev, "failed to read %#x: %d\n", cmd, ret);
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> >
> > This function does not appear to be doing anything unique (e.g. retry loop to
> > deal with special HW condition, etc.), so I do not see any reason to open-code
> > a standard write-then-read operation.
> >
> > Can i2c_smbus API simply replace this function,
>
> As far as I know the SMBus standard and API is limited to reading a
> maximum of 32 bytes (I2C_SMBUS_BLOCK_MAX). With >= 6 fingers the touch
> packet sizes will exceed that.
>
> > or better yet, can this driver
> > simply use regmap? In fact, that is what the other mainline Himax driver uses.
>
> Regmap could maybe work, but I think it does not really fit. The I2C
> interface here does not provide a consistent register map. Problem is,
> for regmap_config we would need to define "reg_bits" and "val_bits".
> This does not really exist here: The commands are usually sent without
> any arguments, sometimes with a single byte (HX852X_REG_SRAM_SWITCH) and
> sometimes with a word (HX852X_REG_SRAM_ADDR). There isn't a specific
> register set with values here but more like random "commands".
>
> Since SMBus doesn't allow reading more than 32 bytes and regmap does not
> fit I think this custom but fairly standard routine is necessary. :/
That makes sense; thank you for the follow-up.
>
> >
> > > +
> > > +static int hx852x_power_on(struct hx852x *hx)
> > > +{
> > > + struct device *dev = &hx->client->dev;
> > > + int error;
> > > +
> > > + error = regulator_bulk_enable(ARRAY_SIZE(hx->supplies), hx->supplies);
> > > + if (error < 0) {
> >
> > Nit: if (error) as you have done elsewhere.
> >
>
> Thanks, will fix this.
>
> > > + dev_err(dev, "failed to enable regulators: %d\n", error);
> > > + return error;
> > > + }
> > > +
> > > + gpiod_set_value_cansleep(hx->reset_gpiod, 1);
> > > + msleep(20);
> > > + gpiod_set_value_cansleep(hx->reset_gpiod, 0);
> > > + msleep(20);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int hx852x_start(struct hx852x *hx)
> > > +{
> > > + struct device *dev = &hx->client->dev;
> > > + int error;
> > > +
> > > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_OUT);
> > > + if (error) {
> > > + dev_err(dev, "failed to send TS_SLEEP_OUT: %d\n", error);
> > > + return error;
> > > + }
> > > + msleep(30);
> > > +
> > > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_ON);
> > > + if (error) {
> > > + dev_err(dev, "failed to send TS_SENSE_ON: %d\n", error);
> > > + return error;
> > > + }
> > > + msleep(20);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void hx852x_stop(struct hx852x *hx)
> > > +{
> > > + struct device *dev = &hx->client->dev;
> > > + int error;
> > > +
> > > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_OFF);
> > > + if (error)
> > > + dev_err(dev, "failed to send TS_SENSE_OFF: %d\n", error);
> >
> > Granted the function is of void type, should we not still return if there
> > is an error?
> >
> > Actually, I would still keep this function as an int for future re-use, even
> > though hx852x_input_close() simply ignores the value. This way, the return
> > value can be propagated to the return value of hx852x_suspend() and elsewhere.
> >
> > > +
> > > + msleep(20);
> > > +
> > > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_IN);
> > > + if (error)
> > > + dev_err(dev, "failed to send TS_SLEEP_IN: %d\n", error);
> >
> > Same here; no need to sleep following a register write that seemingly did
> > not happen.
> >
> > > +
> > > + msleep(30);
> > > +}
> > > +
> > > +static void hx852x_power_off(struct hx852x *hx)
> > > +{
> > > + struct device *dev = &hx->client->dev;
> > > + int error;
> > > +
> > > + error = regulator_bulk_disable(ARRAY_SIZE(hx->supplies), hx->supplies);
> > > + if (error)
> > > + dev_err(dev, "failed to disable regulators: %d\n", error);
> > > +}
> >
> > Same comment with regard to function type; it's nice for internal helpers
> > to be of type int, even if the core callback using it is void.
> >
> > > +
> > > +static int hx852x_read_config(struct hx852x *hx)
> > > +{
> > > + struct device *dev = &hx->client->dev;
> > > + struct hx852x_config conf = {0};
> >
> > No need to initialize.
> >
> > > + int x_res, y_res;
> > > + int error;
> > > +
> > > + error = hx852x_power_on(hx);
> > > + if (error)
> > > + return error;
> > > +
> > > + /* Sensing must be turned on briefly to load the config */
> > > + error = hx852x_start(hx);
> > > + if (error)
> > > + goto power_off;
> > > +
> > > + hx852x_stop(hx);
> >
> > See my earlier comment about promoting this function's type to int.
> >
> > > +
> > > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> > > + HX852X_SRAM_SWITCH_TEST_MODE);
> > > + if (error)
> > > + goto power_off;
> > > +
> > > + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> > > + HX852X_SRAM_ADDR_CONFIG);
> > > + if (error)
> > > + goto exit_test_mode;
> > > +
> > > + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> > > + if (error)
> > > + goto exit_test_mode;
> > > +
> > > + x_res = be16_to_cpu(conf.x_res);
> > > + y_res = be16_to_cpu(conf.y_res);
> > > + hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> > > + dev_dbg(dev, "x res: %d, y res: %d, max fingers: %d\n",
> > > + x_res, y_res, hx->max_fingers);
> > > +
> > > + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> > > + dev_err(dev, "max supported fingers: %d, found: %d\n",
> > > + HX852X_MAX_FINGERS, hx->max_fingers);
> > > + error = -EINVAL;
> > > + goto exit_test_mode;
> > > + }
> > > +
> > > + if (x_res && y_res) {
> > > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> > > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> > > + }
> > > +
> > > +exit_test_mode:
> > > + i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> >
> > Nit: it would still be nice to preserve as many return values as possible, perhaps
> > as follows:
> >
> > +exit_test_mode:
> > error = i2c_smbus_write_byte_data(...) ? : error;
> >
> > > +power_off:
> > > + hx852x_power_off(hx);
> > > + return error;
> >
> > Similarly, with hx852x_power_off() being promoted to int as suggested above,
> > this could be:
> >
> > return hx852x_power_off(...) ? : error;
> >
> > There are other idiomatic ways to do the same thing based on your preference.
> > Another (perhaps more clear) option would be to move some of these test mode
> > functions into a helper, which would also avoid some goto statements.
> >
>
> Thanks for your suggestions regarding the error handling / return codes.
> For v2 I will play with the different options you gave and look which
> one feels best. :-)
>
> > > +}
> > > +
> > > +static int hx852x_handle_events(struct hx852x *hx)
> > > +{
> > > + /*
> > > + * The event packets have variable size, depending on the amount of
> > > + * supported fingers (hx->max_fingers). They are laid out as follows:
> > > + * - struct hx852x_coord[hx->max_fingers]: Coordinates for each finger
> > > + * - u8[ALIGN(hx->max_fingers, 4)]: Touch width for each finger
> > > + * with padding for 32-bit alignment
> > > + * - struct hx852x_touch_info
> > > + *
> > > + * Load everything into a 32-bit aligned buffer so the coordinates
> > > + * can be assigned directly, without using get_unaligned_*().
> > > + */
> > > + u8 buf[HX852X_MAX_BUF_SIZE] __aligned(4);
> > > + struct hx852x_coord *coord = (struct hx852x_coord *)buf;
> > > + u8 *width = &buf[HX852X_COORD_SIZE(hx->max_fingers)];
> > > + struct hx852x_touch_info *info = (struct hx852x_touch_info *)
> > > + &width[HX852X_WIDTH_SIZE(hx->max_fingers)];
> > > + unsigned long finger_pressed, key_pressed;
> >
> > It seems these only need to be u16.
> >
>
> Right. However, unsigned long is necessary for using it with
> for_each_set_bit(), which wants a pointer to an unsigned long.
> I can change it for key_pressed though.
Thanks for the clarification; the existing implementation seems fine then.
>
> > > + unsigned int i, x, y, w;
> > > + int error;
> > > +
> > > + error = hx852x_i2c_read(hx, HX852X_READ_ALL_EVENTS, buf,
> > > + HX852X_BUF_SIZE(hx->max_fingers));
> > > + if (error)
> > > + return error;
> > > +
> > > + finger_pressed = get_unaligned_le16(&info->finger_pressed);
> > > + key_pressed = finger_pressed >> HX852X_MAX_FINGERS;
> > > +
> > > + /* All bits are set when no touch is detected */
> > > + if (info->finger_num == 0xff || !(info->finger_num & 0x0f))
> > > + finger_pressed = 0;
> > > + if (key_pressed == 0xf)
> > > + key_pressed = 0;
> > > +
> > > + for_each_set_bit(i, &finger_pressed, hx->max_fingers) {
> > > + x = be16_to_cpu(coord[i].x);
> > > + y = be16_to_cpu(coord[i].y);
> > > + w = width[i];
> > > +
> > > + input_mt_slot(hx->input_dev, i);
> > > + input_mt_report_slot_state(hx->input_dev, MT_TOOL_FINGER, 1);
> > > + touchscreen_report_pos(hx->input_dev, &hx->props, x, y, true);
> > > + input_report_abs(hx->input_dev, ABS_MT_TOUCH_MAJOR, w);
> > > + }
> > > + input_mt_sync_frame(hx->input_dev);
> > > +
> > > + for (i = 0; i < hx->keycount; i++)
> > > + input_report_key(hx->input_dev, hx->keycodes[i], key_pressed & BIT(i));
> > > +
> > > + input_sync(hx->input_dev);
> > > + return 0;
> > > +}
> > > +
> > > +static irqreturn_t hx852x_interrupt(int irq, void *ptr)
> > > +{
> > > + struct hx852x *hx = ptr;
> > > + int error;
> > > +
> > > + error = hx852x_handle_events(hx);
> > > + if (error) {
> > > + dev_err(&hx->client->dev, "failed to handle events: %d\n", error);
> > > + return IRQ_NONE;
> > > + }
> > > +
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int hx852x_input_open(struct input_dev *dev)
> > > +{
> > > + struct hx852x *hx = input_get_drvdata(dev);
> > > + int error;
> > > +
> > > + error = hx852x_power_on(hx);
> > > + if (error)
> > > + return error;
> > > +
> > > + error = hx852x_start(hx);
> > > + if (error) {
> > > + hx852x_power_off(hx);
> > > + return error;
> > > + }
> > > +
> > > + enable_irq(hx->client->irq);
> > > + return 0;
> > > +}
> > > +
> > > +static void hx852x_input_close(struct input_dev *dev)
> > > +{
> > > + struct hx852x *hx = input_get_drvdata(dev);
> > > +
> > > + hx852x_stop(hx);
> > > + disable_irq(hx->client->irq);
> > > + hx852x_power_off(hx);
> > > +}
> > > +
> > > +static int hx852x_parse_properties(struct hx852x *hx)
> > > +{
> > > + struct device *dev = &hx->client->dev;
> > > + int error;
> > > +
> > > + hx->keycount = device_property_count_u32(dev, "linux,keycodes");
> > > + if (hx->keycount <= 0) {
> > > + hx->keycount = 0;
> > > + return 0;
> > > + }
> > > +
> > > + if (hx->keycount > HX852X_MAX_KEY_COUNT) {
> > > + dev_err(dev, "max supported keys: %d, found: %d\n",
> >
> > Nit: these should both be printed as %u.
> >
>
> Right, thanks!
>
> > > + HX852X_MAX_KEY_COUNT, hx->keycount);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + error = device_property_read_u32_array(dev, "linux,keycodes",
> > > + hx->keycodes, hx->keycount);
> > > + if (error)
> > > + dev_err(dev, "failed to read linux,keycodes: %d\n", error);
> > > +
> > > + return error;
> > > +}
> > > +
> > > +static int hx852x_probe(struct i2c_client *client)
> > > +{
> > > + struct device *dev = &client->dev;
> > > + struct hx852x *hx;
> > > + int error, i;
> > > +
> > > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> > > + I2C_FUNC_SMBUS_WRITE_BYTE |
> > > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> > > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> > > + dev_err(dev, "not all i2c functionality supported\n");
> > > + return -ENXIO;
> > > + }
> > > +
> > > + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> > > + if (!hx)
> > > + return -ENOMEM;
> > > +
> > > + hx->client = client;
> > > + hx->input_dev = devm_input_allocate_device(dev);
> > > + if (!hx->input_dev)
> > > + return -ENOMEM;
> > > +
> > > + hx->input_dev->name = "Himax HX852x";
> > > + hx->input_dev->id.bustype = BUS_I2C;
> > > + hx->input_dev->open = hx852x_input_open;
> > > + hx->input_dev->close = hx852x_input_close;
> > > +
> > > + i2c_set_clientdata(client, hx);
> > > + input_set_drvdata(hx->input_dev, hx);
> > > +
> > > + hx->supplies[0].supply = "vcca";
> > > + hx->supplies[1].supply = "vccd";
> > > + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> > > + if (error < 0)
> > > + return dev_err_probe(dev, error, "failed to get regulators");
> > > +
> > > + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > > + if (IS_ERR(hx->reset_gpiod))
> > > + return dev_err_probe(dev, error, "failed to get reset gpio");
> >
> > Can the reset GPIO be optional?
> >
>
> I'm afraid I have no idea if the controller needs this or not. Would it
> be better to keep it required until someone confirms otherwise or have
> it optional for the other way around?
If you have a datasheet handy, or your hardware provides a means for you to
test and confirm whether reset can be left out, I would make the reset GPIO
optional. Often times, these controllers are part of a module and reset may
be tied high locally as opposed to adding another signal to a flex cable.
If you have no way to confirm, I would keep it as required for now; it is not
too cumbersome to be changed later if the need arises on different hardware.
>
> Thanks!
> Stephan
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen
From: Jeff LaBundy @ 2023-09-27 1:40 UTC (permalink / raw)
To: Karel Balej
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Markuss Broks, linux-input, devicetree, linux-kernel,
Duje Mihanović, ~postmarketos/upstreaming
In-Reply-To: <20230926173531.18715-3-balejk@matfyz.cz>
Hi Karel,
On Tue, Sep 26, 2023 at 07:35:24PM +0200, Karel Balej wrote:
> The downstream driver sets the regulator voltage to 3.1 V. Without this,
> the touchscreen generates random touches even after it is no longer
> being touched. It is unknown whether the same problem appears with other
> chips of the IST30**C series.
>
> Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
> .../bindings/input/touchscreen/imagis,ist30xxc.yaml | 1 +
> drivers/input/touchscreen/imagis.c | 13 +++++++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> index 09bf3a6acc5e..d6f75bbfaec3 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> @@ -18,6 +18,7 @@ properties:
>
> compatible:
> enum:
> + - imagis,ist3032c
> - imagis,ist3038c
>
> reg:
> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
> index 4456f1b4d527..df9a8fbf2c5f 100644
> --- a/drivers/input/touchscreen/imagis.c
> +++ b/drivers/input/touchscreen/imagis.c
> @@ -30,6 +30,7 @@
> #define IST30XXC_FINGER_COUNT_SHIFT 12
> #define IST30XXC_FINGER_STATUS_MASK GENMASK(9, 0)
>
> +#define IST3032C_WHOAMI 0x32c
> #define IST3038C_WHOAMI 0x38c
>
> struct imagis_ts {
> @@ -295,6 +296,16 @@ static int imagis_probe(struct i2c_client *i2c)
> return -EINVAL;
> }
>
> + if (chip_id == IST3032C_WHOAMI) {
> + /*
> + * if the regulator voltage is not set like this, the touchscreen
> + * generates random touches without user interaction
> + */
> + error = regulator_set_voltage(ts->supplies[0].consumer, 3100000, 3100000);
> + if (error)
> + dev_warn(dev, "failed to set regulator voltage\n");
> + }
> +
Opinions may vary, but mine is that this kind of hard-coded board-level policy
does not belong in the driver. Surely the supply need not be equal to exactly
3.1 V and this is merely an upper or lower limit? Assuming so, what if the board
designer opts to share this supply with another consumer that requires a specific
voltage not equal to 3.1 V, but still within the safe range of IST3032C?
To me, this restriction belongs in dts, specifically within the regulator child
node referenced by the client which bears the new 'ist3032c' compatible string.
Maybe a better solution is to simply note this presumed silicon erratum in the
description of the vdd supply in the binding which, as Conor states, must not be
clubbed with driver patches.
> error = devm_request_threaded_irq(dev, i2c->irq,
> NULL, imagis_interrupt,
> IRQF_ONESHOT | IRQF_NO_AUTOEN,
> @@ -348,6 +359,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume);
>
> #ifdef CONFIG_OF
> static const struct of_device_id imagis_of_match[] = {
> + { .compatible = "imagis,ist3032c", .data = (void *)IST3032C_WHOAMI, },
> { .compatible = "imagis,ist3038c", .data = (void *)IST3038C_WHOAMI, },
> { },
> };
> @@ -355,6 +367,7 @@ MODULE_DEVICE_TABLE(of, imagis_of_match);
> #endif
>
> static const struct i2c_device_id imagis_ts_i2c_id[] = {
> + { "ist3032c", IST3032C_WHOAMI, },
> { "ist3038c", IST3038C_WHOAMI, },
> { },
> };
> --
> 2.42.0
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: input: touchscreen: goodix: clarify irq-gpios misleading text
From: Jeff LaBundy @ 2023-09-27 1:56 UTC (permalink / raw)
To: Luca Ceresoli
Cc: devicetree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, linux-kernel, Thomas Petazzoni
In-Reply-To: <20230925032208.11698-1-luca.ceresoli@bootlin.com>
Hi Luca,
On Mon, Sep 25, 2023 at 05:22:08AM +0200, Luca Ceresoli wrote:
> The irq-gpios description misleading, apparently saying that driving the
> IRQ GPIO resets the device, which is even more puzzling as there is a reset
> GPIO as well.
>
> In reality the IRQ pin can be driven during the reset sequence to configure
> the client address, as it becomes clear after checking both the datasheet
> and the driver code. Improve the text to clarify that.
>
> Also rephrase to remove reference to the driver, which is not appropriate
> in the bindings.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
>
> ---
>
> Changed in v2:
> - reworded to clarify even further
> ---
> .../devicetree/bindings/input/touchscreen/goodix.yaml | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> index 3d016b87c8df..2a2d86cfd104 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> @@ -37,8 +37,9 @@ properties:
> maxItems: 1
>
> irq-gpios:
> - description: GPIO pin used for IRQ. The driver uses the interrupt gpio pin
> - as output to reset the device.
> + description: GPIO pin used for IRQ input. Additionally, this line is
> + sampled by the device on reset deassertion to select the I2C client
> + address, thus it can be driven by the host during the reset sequence.
> maxItems: 1
>
> reset-gpios:
> --
> 2.34.1
>
Thanks for considering my feedback; the messaging is clear now.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH 1/2] input: generalize the Imagis touchscreen driver
From: Conor Dooley @ 2023-09-26 22:17 UTC (permalink / raw)
To: Karel Balej
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Markuss Broks, linux-input, devicetree, linux-kernel,
Duje Mihanović, ~postmarketos/upstreaming
In-Reply-To: <20230926173531.18715-2-balejk@matfyz.cz>
[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]
On Tue, Sep 26, 2023 at 07:35:23PM +0200, Karel Balej wrote:
> This driver should work with other Imagis ICs of the IST30**C series.
> Make that more apparent.
>
> Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
> ...gis,ist3038c.yaml => imagis,ist30xxc.yaml} | 2 +-
> MAINTAINERS | 2 +-
> drivers/input/touchscreen/Kconfig | 4 +-
> drivers/input/touchscreen/imagis.c | 86 +++++++++++--------
> 4 files changed, 52 insertions(+), 42 deletions(-)
> rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (99%)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
> similarity index 99%
> rename from Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> rename to Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
This rename is pointless.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH 0/2] input: Imagis: add support for the IST3032C touchscreen
From: Conor Dooley @ 2023-09-26 22:16 UTC (permalink / raw)
To: Karel Balej
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Markuss Broks, linux-input, devicetree, linux-kernel,
Duje Mihanović, ~postmarketos/upstreaming
In-Reply-To: <20230926173531.18715-1-balejk@matfyz.cz>
[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]
On Tue, Sep 26, 2023 at 07:35:22PM +0200, Karel Balej wrote:
> This patch series extends the Imagis driver to support the IST3032C
> touchscreen, which is used for instance with the samsung,coreprimevelte
> smartphone, with which this was tested. To use it with this model
> however, the regulator driver needs to be ported first. Support for this
> smartphone is not yet in-tree, upstreaming is ongoing at [1].
>
> [1] https://lore.kernel.org/all/20230812-pxa1908-lkml-v5-0-a5d51937ee34@skole.hr/
For both patches, changes to dt-bindings need to be in their own
patches & not bundled with drivers.
>
> Karel Balej (2):
> input: generalize the Imagis touchscreen driver
> input: Imagis: add support for the IST3032C touchscreen
>
> ...gis,ist3038c.yaml => imagis,ist30xxc.yaml} | 3 +-
> MAINTAINERS | 2 +-
> drivers/input/touchscreen/Kconfig | 4 +-
> drivers/input/touchscreen/imagis.c | 99 ++++++++++++-------
> 4 files changed, 66 insertions(+), 42 deletions(-)
> rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (97%)
>
> --
> 2.42.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* [PATCH 1/2] input: generalize the Imagis touchscreen driver
From: Karel Balej @ 2023-09-26 17:35 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Markuss Broks, linux-input, devicetree, linux-kernel,
Duje Mihanović, ~postmarketos/upstreaming
Cc: Karel Balej
In-Reply-To: <20230926173531.18715-1-balejk@matfyz.cz>
This driver should work with other Imagis ICs of the IST30**C series.
Make that more apparent.
Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
...gis,ist3038c.yaml => imagis,ist30xxc.yaml} | 2 +-
MAINTAINERS | 2 +-
drivers/input/touchscreen/Kconfig | 4 +-
drivers/input/touchscreen/imagis.c | 86 +++++++++++--------
4 files changed, 52 insertions(+), 42 deletions(-)
rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (99%)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
similarity index 99%
rename from Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
rename to Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
index 0d6b033fd5fb..09bf3a6acc5e 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
%YAML 1.2
---
-$id: http://devicetree.org/schemas/input/touchscreen/imagis,ist3038c.yaml#
+$id: http://devicetree.org/schemas/input/touchscreen/imagis,ist30xxc.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
title: Imagis IST30XXC family touchscreen controller
diff --git a/MAINTAINERS b/MAINTAINERS
index b19995690904..b23e76418d94 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10209,7 +10209,7 @@ F: drivers/usb/atm/ueagle-atm.c
IMAGIS TOUCHSCREEN DRIVER
M: Markuss Broks <markuss.broks@gmail.com>
S: Maintained
-F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
+F: Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
F: drivers/input/touchscreen/imagis.c
IMGTEC ASCII LCD DRIVER
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..45503aa2653e 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -665,10 +665,10 @@ config TOUCHSCREEN_NOVATEK_NVT_TS
module will be called novatek-nvt-ts.
config TOUCHSCREEN_IMAGIS
- tristate "Imagis touchscreen support"
+ tristate "Imagis IST30XXC touchscreen support"
depends on I2C
help
- Say Y here if you have an Imagis IST30xxC touchscreen.
+ Say Y here if you have an Imagis IST30XXC touchscreen.
If unsure, say N.
To compile this driver as a module, choose M here: the
diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
index 07111ca24455..4456f1b4d527 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -11,25 +11,26 @@
#include <linux/property.h>
#include <linux/regulator/consumer.h>
-#define IST3038C_HIB_ACCESS (0x800B << 16)
-#define IST3038C_DIRECT_ACCESS BIT(31)
-#define IST3038C_REG_CHIPID 0x40001000
-#define IST3038C_REG_HIB_BASE 0x30000100
-#define IST3038C_REG_TOUCH_STATUS (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS)
-#define IST3038C_REG_TOUCH_COORD (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x8)
-#define IST3038C_REG_INTR_MESSAGE (IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x4)
+#define IST30XXC_HIB_ACCESS (0x800B << 16)
+#define IST30XXC_DIRECT_ACCESS BIT(31)
+#define IST30XXC_REG_CHIPID 0x40001000
+#define IST30XXC_REG_HIB_BASE 0x30000100
+#define IST30XXC_REG_TOUCH_STATUS (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS)
+#define IST30XXC_REG_TOUCH_COORD (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x8)
+#define IST30XXC_REG_INTR_MESSAGE (IST30XXC_REG_HIB_BASE | IST30XXC_HIB_ACCESS | 0x4)
+#define IST30XXC_CHIP_ON_DELAY_MS 60
+#define IST30XXC_I2C_RETRY_COUNT 3
+#define IST30XXC_MAX_FINGER_NUM 10
+#define IST30XXC_X_MASK GENMASK(23, 12)
+#define IST30XXC_X_SHIFT 12
+#define IST30XXC_Y_MASK GENMASK(11, 0)
+#define IST30XXC_AREA_MASK GENMASK(27, 24)
+#define IST30XXC_AREA_SHIFT 24
+#define IST30XXC_FINGER_COUNT_MASK GENMASK(15, 12)
+#define IST30XXC_FINGER_COUNT_SHIFT 12
+#define IST30XXC_FINGER_STATUS_MASK GENMASK(9, 0)
+
#define IST3038C_WHOAMI 0x38c
-#define IST3038C_CHIP_ON_DELAY_MS 60
-#define IST3038C_I2C_RETRY_COUNT 3
-#define IST3038C_MAX_FINGER_NUM 10
-#define IST3038C_X_MASK GENMASK(23, 12)
-#define IST3038C_X_SHIFT 12
-#define IST3038C_Y_MASK GENMASK(11, 0)
-#define IST3038C_AREA_MASK GENMASK(27, 24)
-#define IST3038C_AREA_SHIFT 24
-#define IST3038C_FINGER_COUNT_MASK GENMASK(15, 12)
-#define IST3038C_FINGER_COUNT_SHIFT 12
-#define IST3038C_FINGER_STATUS_MASK GENMASK(9, 0)
struct imagis_ts {
struct i2c_client *client;
@@ -57,7 +58,7 @@ static int imagis_i2c_read_reg(struct imagis_ts *ts,
},
};
int ret, error;
- int retry = IST3038C_I2C_RETRY_COUNT;
+ int retry = IST30XXC_I2C_RETRY_COUNT;
/* Retry in case the controller fails to respond */
do {
@@ -84,7 +85,7 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
int i;
int error;
- error = imagis_i2c_read_reg(ts, IST3038C_REG_INTR_MESSAGE,
+ error = imagis_i2c_read_reg(ts, IST30XXC_REG_INTR_MESSAGE,
&intr_message);
if (error) {
dev_err(&ts->client->dev,
@@ -92,20 +93,20 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
goto out;
}
- finger_count = (intr_message & IST3038C_FINGER_COUNT_MASK) >>
- IST3038C_FINGER_COUNT_SHIFT;
- if (finger_count > IST3038C_MAX_FINGER_NUM) {
+ finger_count = (intr_message & IST30XXC_FINGER_COUNT_MASK) >>
+ IST30XXC_FINGER_COUNT_SHIFT;
+ if (finger_count > IST30XXC_MAX_FINGER_NUM) {
dev_err(&ts->client->dev,
"finger count %d is more than maximum supported\n",
finger_count);
goto out;
}
- finger_pressed = intr_message & IST3038C_FINGER_STATUS_MASK;
+ finger_pressed = intr_message & IST30XXC_FINGER_STATUS_MASK;
for (i = 0; i < finger_count; i++) {
error = imagis_i2c_read_reg(ts,
- IST3038C_REG_TOUCH_COORD + (i * 4),
+ IST30XXC_REG_TOUCH_COORD + (i * 4),
&finger_status);
if (error) {
dev_err(&ts->client->dev,
@@ -118,12 +119,12 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER,
finger_pressed & BIT(i));
touchscreen_report_pos(ts->input_dev, &ts->prop,
- (finger_status & IST3038C_X_MASK) >>
- IST3038C_X_SHIFT,
- finger_status & IST3038C_Y_MASK, 1);
+ (finger_status & IST30XXC_X_MASK) >>
+ IST30XXC_X_SHIFT,
+ finger_status & IST30XXC_Y_MASK, 1);
input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR,
- (finger_status & IST3038C_AREA_MASK) >>
- IST3038C_AREA_SHIFT);
+ (finger_status & IST30XXC_AREA_MASK) >>
+ IST30XXC_AREA_SHIFT);
}
input_mt_sync_frame(ts->input_dev);
@@ -148,7 +149,7 @@ static int imagis_power_on(struct imagis_ts *ts)
if (error)
return error;
- msleep(IST3038C_CHIP_ON_DELAY_MS);
+ msleep(IST30XXC_CHIP_ON_DELAY_MS);
return 0;
}
@@ -220,7 +221,7 @@ static int imagis_init_input_dev(struct imagis_ts *ts)
}
error = input_mt_init_slots(input_dev,
- IST3038C_MAX_FINGER_NUM,
+ IST30XXC_MAX_FINGER_NUM,
INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
if (error) {
dev_err(&ts->client->dev,
@@ -253,7 +254,7 @@ static int imagis_probe(struct i2c_client *i2c)
{
struct device *dev = &i2c->dev;
struct imagis_ts *ts;
- int chip_id, error;
+ int chip_id, dt_chip_id, error;
ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
if (!ts)
@@ -261,6 +262,8 @@ static int imagis_probe(struct i2c_client *i2c)
ts->client = i2c;
+ dt_chip_id = (int)(uintptr_t)device_get_match_data(&i2c->dev);
+
error = imagis_init_regulators(ts);
if (error) {
dev_err(dev, "regulator init error: %d\n", error);
@@ -280,15 +283,15 @@ static int imagis_probe(struct i2c_client *i2c)
}
error = imagis_i2c_read_reg(ts,
- IST3038C_REG_CHIPID | IST3038C_DIRECT_ACCESS,
+ IST30XXC_REG_CHIPID | IST30XXC_DIRECT_ACCESS,
&chip_id);
if (error) {
dev_err(dev, "chip ID read failure: %d\n", error);
return error;
}
- if (chip_id != IST3038C_WHOAMI) {
- dev_err(dev, "unknown chip ID: 0x%x\n", chip_id);
+ if (chip_id != dt_chip_id) {
+ dev_err(dev, "unknown or misconfigured chip ID: 0x%x\n", chip_id);
return -EINVAL;
}
@@ -345,12 +348,18 @@ static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume);
#ifdef CONFIG_OF
static const struct of_device_id imagis_of_match[] = {
- { .compatible = "imagis,ist3038c", },
+ { .compatible = "imagis,ist3038c", .data = (void *)IST3038C_WHOAMI, },
{ },
};
MODULE_DEVICE_TABLE(of, imagis_of_match);
#endif
+static const struct i2c_device_id imagis_ts_i2c_id[] = {
+ { "ist3038c", IST3038C_WHOAMI, },
+ { },
+};
+MODULE_DEVICE_TABLE(i2c, imagis_ts_i2c_id);
+
static struct i2c_driver imagis_ts_driver = {
.driver = {
.name = "imagis-touchscreen",
@@ -358,10 +367,11 @@ static struct i2c_driver imagis_ts_driver = {
.of_match_table = of_match_ptr(imagis_of_match),
},
.probe = imagis_probe,
+ .id_table = imagis_ts_i2c_id,
};
module_i2c_driver(imagis_ts_driver);
-MODULE_DESCRIPTION("Imagis IST3038C Touchscreen Driver");
+MODULE_DESCRIPTION("Imagis IST30XXC Touchscreen Driver");
MODULE_AUTHOR("Markuss Broks <markuss.broks@gmail.com>");
MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related
* [PATCH 2/2] input: Imagis: add support for the IST3032C touchscreen
From: Karel Balej @ 2023-09-26 17:35 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Markuss Broks, linux-input, devicetree, linux-kernel,
Duje Mihanović, ~postmarketos/upstreaming
Cc: Karel Balej
In-Reply-To: <20230926173531.18715-1-balejk@matfyz.cz>
The downstream driver sets the regulator voltage to 3.1 V. Without this,
the touchscreen generates random touches even after it is no longer
being touched. It is unknown whether the same problem appears with other
chips of the IST30**C series.
Co-developed-by: Duje Mihanović <duje.mihanovic@skole.hr>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
.../bindings/input/touchscreen/imagis,ist30xxc.yaml | 1 +
drivers/input/touchscreen/imagis.c | 13 +++++++++++++
2 files changed, 14 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
index 09bf3a6acc5e..d6f75bbfaec3 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist30xxc.yaml
@@ -18,6 +18,7 @@ properties:
compatible:
enum:
+ - imagis,ist3032c
- imagis,ist3038c
reg:
diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
index 4456f1b4d527..df9a8fbf2c5f 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -30,6 +30,7 @@
#define IST30XXC_FINGER_COUNT_SHIFT 12
#define IST30XXC_FINGER_STATUS_MASK GENMASK(9, 0)
+#define IST3032C_WHOAMI 0x32c
#define IST3038C_WHOAMI 0x38c
struct imagis_ts {
@@ -295,6 +296,16 @@ static int imagis_probe(struct i2c_client *i2c)
return -EINVAL;
}
+ if (chip_id == IST3032C_WHOAMI) {
+ /*
+ * if the regulator voltage is not set like this, the touchscreen
+ * generates random touches without user interaction
+ */
+ error = regulator_set_voltage(ts->supplies[0].consumer, 3100000, 3100000);
+ if (error)
+ dev_warn(dev, "failed to set regulator voltage\n");
+ }
+
error = devm_request_threaded_irq(dev, i2c->irq,
NULL, imagis_interrupt,
IRQF_ONESHOT | IRQF_NO_AUTOEN,
@@ -348,6 +359,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume);
#ifdef CONFIG_OF
static const struct of_device_id imagis_of_match[] = {
+ { .compatible = "imagis,ist3032c", .data = (void *)IST3032C_WHOAMI, },
{ .compatible = "imagis,ist3038c", .data = (void *)IST3038C_WHOAMI, },
{ },
};
@@ -355,6 +367,7 @@ MODULE_DEVICE_TABLE(of, imagis_of_match);
#endif
static const struct i2c_device_id imagis_ts_i2c_id[] = {
+ { "ist3032c", IST3032C_WHOAMI, },
{ "ist3038c", IST3038C_WHOAMI, },
{ },
};
--
2.42.0
^ permalink raw reply related
* [PATCH 0/2] input: Imagis: add support for the IST3032C touchscreen
From: Karel Balej @ 2023-09-26 17:35 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Markuss Broks, linux-input, devicetree, linux-kernel,
Duje Mihanović, ~postmarketos/upstreaming
Cc: Karel Balej
This patch series extends the Imagis driver to support the IST3032C
touchscreen, which is used for instance with the samsung,coreprimevelte
smartphone, with which this was tested. To use it with this model
however, the regulator driver needs to be ported first. Support for this
smartphone is not yet in-tree, upstreaming is ongoing at [1].
[1] https://lore.kernel.org/all/20230812-pxa1908-lkml-v5-0-a5d51937ee34@skole.hr/
Karel Balej (2):
input: generalize the Imagis touchscreen driver
input: Imagis: add support for the IST3032C touchscreen
...gis,ist3038c.yaml => imagis,ist30xxc.yaml} | 3 +-
MAINTAINERS | 2 +-
drivers/input/touchscreen/Kconfig | 4 +-
drivers/input/touchscreen/imagis.c | 99 ++++++++++++-------
4 files changed, 66 insertions(+), 42 deletions(-)
rename Documentation/devicetree/bindings/input/touchscreen/{imagis,ist3038c.yaml => imagis,ist30xxc.yaml} (97%)
--
2.42.0
^ permalink raw reply
* Re: [PATCH 06/15] platform/x86/amd/pmf: Add support to get inputs from other subsystems
From: Ilpo Järvinen @ 2023-09-26 17:08 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: hdegoede, markgross, basavaraj.natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-7-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> PMF driver sends changing inputs from each subystem to TA for evaluating
> the conditions in the policy binary.
>
> Add initial support of plumbing in the PMF driver for Smart PC to get
> information from other subsystems in the kernel.
>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/Makefile | 2 +-
> drivers/platform/x86/amd/pmf/pmf.h | 18 ++++
> drivers/platform/x86/amd/pmf/spc.c | 118 ++++++++++++++++++++++++++
> drivers/platform/x86/amd/pmf/tee-if.c | 3 +
> 4 files changed, 140 insertions(+), 1 deletion(-)
> create mode 100644 drivers/platform/x86/amd/pmf/spc.c
>
> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
> index d2746ee7369f..6b26e48ce8ad 100644
> --- a/drivers/platform/x86/amd/pmf/Makefile
> +++ b/drivers/platform/x86/amd/pmf/Makefile
> @@ -7,4 +7,4 @@
> obj-$(CONFIG_AMD_PMF) += amd-pmf.o
> amd-pmf-objs := core.o acpi.o sps.o \
> auto-mode.o cnqf.o \
> - tee-if.o
> + tee-if.o spc.o
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 81acf2a37366..e64b4d285624 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -146,6 +146,21 @@ struct smu_pmf_metrics {
> u16 infra_gfx_maxfreq; /* in MHz */
> u16 skin_temp; /* in centi-Celsius */
> u16 device_state;
> + u16 curtemp; /* in centi-Celsius */
> + u16 filter_alpha_value;
> + u16 avg_gfx_clkfrequency;
> + u16 avg_fclk_frequency;
> + u16 avg_gfx_activity;
> + u16 avg_socclk_frequency;
> + u16 avg_vclk_frequency;
> + u16 avg_vcn_activity;
> + u16 avg_dram_reads;
> + u16 avg_dram_writes;
> + u16 avg_socket_power;
> + u16 avg_core_power[2];
> + u16 avg_core_c0residency[16];
> + u16 spare1;
> + u32 metrics_counter;
> } __packed;
>
> enum amd_stt_skin_temp {
> @@ -592,4 +607,7 @@ extern const struct attribute_group cnqf_feature_attribute_group;
> int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
> int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
> +
> +/* Smart PC - TA interfaces */
> +void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in);
> #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> new file mode 100644
> index 000000000000..08159cd5f853
> --- /dev/null
> +++ b/drivers/platform/x86/amd/pmf/spc.c
> @@ -0,0 +1,118 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Platform Management Framework Driver - Smart PC Capabilities
> + *
> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + * Patil Rajesh Reddy <Patil.Reddy@amd.com>
> + */
> +
> +#include <acpi/button.h>
> +#include <linux/power_supply.h>
> +#include "pmf.h"
> +
> +static void amd_pmf_get_smu_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +{
> + u16 max, avg = 0;
> + int i;
> +
> + memset(dev->buf, 0, sizeof(dev->m_table));
> + amd_pmf_send_cmd(dev, SET_TRANSFER_TABLE, 0, 7, NULL);
> + memcpy(&dev->m_table, dev->buf, sizeof(dev->m_table));
> +
> + in->ev_info.socket_power = dev->m_table.apu_power + dev->m_table.dgpu_power;
> + in->ev_info.skin_temperature = dev->m_table.skin_temp;
> +
> + /* get the avg C0 residency of all the cores */
> + for (i = 0; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++)
> + avg += dev->m_table.avg_core_c0residency[i];
Is this safe from overflow?
> +
> + /* get the max C0 residency of all the cores */
> + max = dev->m_table.avg_core_c0residency[0];
> + for (i = 1; i < ARRAY_SIZE(dev->m_table.avg_core_c0residency); i++) {
> + if (dev->m_table.avg_core_c0residency[i] > max)
> + max = dev->m_table.avg_core_c0residency[i];
> + }
> +
> + in->ev_info.avg_c0residency = avg / ARRAY_SIZE(dev->m_table.avg_core_c0residency);
> + in->ev_info.max_c0residency = max;
> + in->ev_info.gfx_busy = dev->m_table.avg_gfx_activity;
> +}
> +
> +static const char * const pmf_battery_supply_name[] = {
> + "BATT",
> + "BAT0",
> +};
> +
> +static int get_battery_prop(enum power_supply_property prop)
> +{
> + union power_supply_propval value;
> + struct power_supply *psy;
> + int i, ret = -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(pmf_battery_supply_name); i++) {
> + psy = power_supply_get_by_name(pmf_battery_supply_name[i]);
> + if (!psy)
> + continue;
> +
> + ret = power_supply_get_property(psy, prop, &value);
> + if (ret) {
> + power_supply_put(psy);
> + return ret;
> + }
> + }
> +
> + return value.intval;
> +}
> +
> +static int amd_pmf_get_battery_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +{
> + int val;
> +
> + val = get_battery_prop(POWER_SUPPLY_PROP_PRESENT);
> + if (val != 1)
> + return -EINVAL;
> +
> + in->ev_info.bat_percentage = get_battery_prop(POWER_SUPPLY_PROP_CAPACITY);
> + /* all values in mWh metrics */
> + in->ev_info.bat_design = get_battery_prop(POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN) / 1000;
> + in->ev_info.full_charge_capacity = get_battery_prop(POWER_SUPPLY_PROP_ENERGY_FULL) / 1000;
> + in->ev_info.drain_rate = get_battery_prop(POWER_SUPPLY_PROP_POWER_NOW) / 1000;
You don't need literal, use the defines provided in linux/units.h.
> +
> + return 0;
> +}
> +
> +static int amd_pmf_get_slider_info(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +{
> + int val;
> +
> + switch (dev->current_profile) {
> + case PLATFORM_PROFILE_PERFORMANCE:
> + val = TA_BEST_PERFORMANCE;
> + break;
> + case PLATFORM_PROFILE_BALANCED:
> + val = TA_BETTER_PERFORMANCE;
> + break;
> + case PLATFORM_PROFILE_LOW_POWER:
> + val = TA_BEST_BATTERY;
> + break;
> + default:
> + dev_err(dev->dev, "Unknown Platform Profile.\n");
> + return -EOPNOTSUPP;
> + }
> + in->ev_info.power_slider = val;
> +
> + return 0;
> +}
> +
> +void amd_pmf_populate_ta_inputs(struct amd_pmf_dev *dev, struct ta_pmf_enact_table *in)
> +{
> + /* TA side lid open is 1 and close is 0, hence the ! here */
> + in->ev_info.lid_state = !acpi_lid_open();
> + in->ev_info.power_source = amd_pmf_get_power_source();
> + amd_pmf_get_smu_info(dev, in);
> + amd_pmf_get_battery_info(dev, in);
> + amd_pmf_get_slider_info(dev, in);
> +}
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index a8b05e746efd..eb25d5ce3a9a 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -113,6 +113,7 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> {
> struct ta_pmf_shared_memory *ta_sm = NULL;
> struct ta_pmf_enact_result *out = NULL;
> + struct ta_pmf_enact_table *in = NULL;
> struct tee_param param[MAX_TEE_PARAM];
> struct tee_ioctl_invoke_arg arg;
> int ret = 0;
> @@ -123,11 +124,13 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> memset(dev->shbuf, 0, dev->policy_sz);
> ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
> out = &ta_sm->pmf_output.policy_apply_table;
> + in = &ta_sm->pmf_input.enact_table;
>
> memset(ta_sm, 0, sizeof(struct ta_pmf_shared_memory));
> ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER__ENACT_POLICIES;
> ta_sm->if_version = PMF_TA_IF_VERSION__MAJOR;
>
> + amd_pmf_populate_ta_inputs(dev, in);
> amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER__ENACT_POLICIES, &arg, param);
>
> ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
>
--
i.
^ permalink raw reply
* Re: [PATCH 04/15] platform/x86/amd/pmf: Add support for PMF Policy Binary
From: Ilpo Järvinen @ 2023-09-26 17:05 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: hdegoede, markgross, basavaraj.natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-5-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> PMF Policy binary is a encrypted and signed binary that will be part
> of the BIOS. PMF driver via the ACPI interface checks the existence
> of Smart PC bit. If the advertised bit is found, PMF driver walks
> the acpi namespace to find out the policy binary size and the address
> which has to be passed to the TA during the TA init sequence.
>
> The policy binary is comprised of inputs (or the events) and outputs
> (or the actions). With the PMF ecosystem, OEMs generate the policy
> binary (or could be multiple binaries) that contains a supported set
> of inputs and outputs which could be specifically carved out for each
> usage segment (or for each user also) that could influence the system
> behavior either by enriching the user experience or/and boost/throttle
> power limits.
>
> Once the TA init command succeeds, the PMF driver sends the changing
> events in the current environment to the TA for a constant sampling
> frequency time (the event here could be a lid close or open) and
> if the policy binary has corresponding action built within it, the
> TA sends the action for it in the subsequent enact command.
>
> If the inputs sent to the TA has no output defined in the policy
> binary generated by OEMs, there will be no action to be performed
> by the PMF driver.
>
> Example policies:
>
> 1) if slider is performance ; set the SPL to 40W
> Here PMF driver registers with the platform profile interface and
> when the slider position is changed, PMF driver lets the TA know
> about this. TA sends back an action to update the Sustained
> Power Limit (SPL). PMF driver updates this limit via the PMFW mailbox.
>
> 2) if user_away ; then lock the system
> Here PMF driver hooks to the AMD SFH driver to know the user presence
> and send the inputs to TA and if the condition is met, the TA sends
> the action of locking the system. PMF driver generates a uevent and
> based on the udev rule in the userland the system gets locked with
> systemctl.
>
> The intent here is to provide the OEM's to make a policy to lock the
> system when the user is away ; but the userland can make a choice to
> ignore it.
>
> and so on.
>
> The OEMs will have an utility to create numerous such policies and
> the policies shall be reviewed by AMD before signing and encrypting
> them. Policies are shared between operating systems to have seemless user
> experience.
>
> Since all this action has to happen via the "amdtee" driver, currently
> there is no caller for it in the kernel which can load the amdtee driver.
> Without amdtee driver loading onto the system the "tee" calls shall fail
> from the PMF driver. Hence an explicit "request_module" has been added
> to address this.
>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/Kconfig | 1 +
> drivers/platform/x86/amd/pmf/acpi.c | 37 +++++++
> drivers/platform/x86/amd/pmf/core.c | 12 +++
> drivers/platform/x86/amd/pmf/pmf.h | 132 ++++++++++++++++++++++++
> drivers/platform/x86/amd/pmf/tee-if.c | 141 +++++++++++++++++++++++++-
> 5 files changed, 321 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index 3064bc8ea167..437b78c6d1c5 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -9,6 +9,7 @@ config AMD_PMF
> depends on POWER_SUPPLY
> depends on AMD_NB
> select ACPI_PLATFORM_PROFILE
> + depends on AMDTEE
> help
> This driver provides support for the AMD Platform Management Framework.
> The goal is to enhance end user experience by making AMD PCs smarter,
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index 3fc5e4547d9f..d0512af2cd42 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -286,6 +286,43 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
> return 0;
> }
>
> +static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
> +{
> + struct amd_pmf_dev *dev = data;
> +
> + switch (res->type) {
> + case ACPI_RESOURCE_TYPE_ADDRESS64:
> + dev->policy_addr = res->data.address64.address.minimum;
> + dev->policy_sz = res->data.address64.address.address_length;
> + break;
> + case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> + dev->policy_addr = res->data.fixed_memory32.address;
> + dev->policy_sz = res->data.fixed_memory32.address_length;
> + break;
> + }
> +
> + if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
> + pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
> + return AE_ERROR;
> + }
> +
> + return AE_OK;
> +}
> +
> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
> +{
> + acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> + acpi_status status;
> +
> + status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
> + if (ACPI_FAILURE(status)) {
> + dev_err(pmf_dev->dev, "acpi_walk_resources failed\n");
> + return status;
> + }
> +
> + return 0;
> +}
> +
> void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
> {
> acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 5fb03ed614ff..6f36c43e081e 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -376,6 +376,18 @@ static int amd_pmf_probe(struct platform_device *pdev)
> return -ENOMEM;
>
> dev->dev = &pdev->dev;
> + err = apmf_check_smart_pc(dev);
> + if (!err) {
> + /* in order for Smart PC solution to work it has a hard dependency
> + * on the amdtee driver to be loaded first even before the PMF driver
> + * loads. PMF ASL has a _CRS method that advertises the existence
> + * of Smart PC bit. If this information is present, use this to
> + * explicitly probe the amdtee driver, so that "tee" plumbing is done
> + * before the PMF Smart PC init happens.
> + */
> + if (request_module("amdtee"))
> + pr_err("Failed to load amdtee. PMF Smart PC not enabled!\n");
> + }
>
> rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
> if (!rdev || !pci_match_id(pmf_pci_ids, rdev)) {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index ea15ce547d24..81acf2a37366 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -13,6 +13,8 @@
>
> #include <linux/acpi.h>
> #include <linux/platform_profile.h>
> +#define POLICY_BUF_MAX_SZ 0x4b000
> +#define POLICY_SIGN_COOKIE 0x31535024
>
> /* APMF Functions */
> #define APMF_FUNC_VERIFY_INTERFACE 0
> @@ -59,8 +61,20 @@
> #define ARG_NONE 0
> #define AVG_SAMPLE_SIZE 3
>
> +/* Policy Actions */
> +#define PMF_POLICY_SPL 2
> +#define PMF_POLICY_SPPT 3
> +#define PMF_POLICY_FPPT 4
> +#define PMF_POLICY_SPPT_APU_ONLY 5
> +#define PMF_POLICY_STT_MIN 6
> +#define PMF_POLICY_STT_SKINTEMP_APU 7
> +#define PMF_POLICY_STT_SKINTEMP_HS2 8
> +
> /* TA macros */
> #define PMF_TA_IF_VERSION__MAJOR 1
> +#define TA_PMF_ACTION_MAX 32
> +#define TA_PMF_UNDO_MAX 8
> +#define MAX_OPERATION_PARAMS 4
>
> /* AMD PMF BIOS interfaces */
> struct apmf_verify_interface {
> @@ -183,11 +197,16 @@ struct amd_pmf_dev {
> bool cnqf_supported;
> struct notifier_block pwr_src_notifier;
> /* Smart PC solution builder */
> + unsigned char *policy_buf;
> + u32 policy_sz;
> struct tee_context *tee_ctx;
> struct tee_shm *fw_shm_pool;
> u32 session_id;
> void *shbuf;
> struct delayed_work pb_work;
> + struct pmf_action_table *prev_data;
> + u64 policy_addr;
> + void *policy_base;
> bool smart_pc_enabled;
> };
>
> @@ -399,17 +418,129 @@ struct apmf_dyn_slider_output {
> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
> } __packed;
>
> +/* Smart PC - TA internals */
> +enum ta_slider {
> + TA_BEST_BATTERY, /* Best Battery */
> + TA_BETTER_BATTERY, /* Better Battery */
> + TA_BETTER_PERFORMANCE, /* Better Performance */
> + TA_BEST_PERFORMANCE, /* Best Performance */
> + TA_MAX,
> +};
> +
> /* cmd ids for TA communication */
> enum ta_pmf_command {
> TA_PMF_COMMAND_POLICY_BUILDER__INITIALIZE,
> TA_PMF_COMMAND_POLICY_BUILDER__ENACT_POLICIES
> };
>
> +enum ta_pmf_error_type {
> + TA_PMF_TYPE__SUCCESS,
> + TA_PMF_ERROR_TYPE__GENERIC,
> + TA_PMF_ERROR_TYPE__CRYPTO,
> + TA_PMF_ERROR_TYPE__CRYPTO_VALIDATE,
> + TA_PMF_ERROR_TYPE__CRYPTO_VERIFY_OEM,
> + TA_PMF_ERROR_TYPE__POLICY_BUILDER,
> + TA_PMF_ERROR_TYPE__PB_CONVERT,
> + TA_PMF_ERROR_TYPE__PB_SETUP,
> + TA_PMF_ERROR_TYPE__PB_ENACT,
> + TA_PMF_ERROR_TYPE__ASD_GET_DEVICE_INFO,
> + TA_PMF_ERROR_TYPE__ASD_GET_DEVICE_PCIE_INFO,
> + TA_PMF_ERROR_TYPE__SYS_DRV_FW_VALIDATION,
> + TA_PMF_ERROR_TYPE__MAX,
> +};
> +
> +struct pmf_action_table {
> + unsigned long spl; /* in mW */
> + unsigned long sppt; /* in mW */
> + unsigned long sppt_apuonly; /* in mW */
> + unsigned long fppt; /* in mW */
> + unsigned long stt_minlimit; /* in mW */
> + unsigned long stt_skintemp_apu; /* in C */
> + unsigned long stt_skintemp_hs2; /* in C */
> +};
> +
> +/* Input conditions */
> +struct ta_pmf_condition_info {
> + u32 power_source;
> + u32 bat_percentage;
> + u32 power_slider;
> + u32 lid_state;
> + bool user_present;
> + u32 rsvd1[2];
> + u32 monitor_count;
> + u32 rsvd2[2];
> + u32 bat_design;
> + u32 full_charge_capacity;
> + int drain_rate;
> + bool user_engaged;
> + u32 device_state;
> + u32 socket_power;
> + u32 skin_temperature;
> + u32 rsvd3[5];
> + u32 ambient_light;
> + u32 length;
> + u32 avg_c0residency;
> + u32 max_c0residency;
> + u32 s0i3_entry;
> + u32 gfx_busy;
> + u32 rsvd4[7];
> + bool camera_state;
> + u32 workload_type;
> + u32 display_type;
> + u32 display_state;
> + u32 rsvd5[150];
> +};
> +
> +struct ta_pmf_load_policy_table {
> + u32 table_size;
> + u8 table[POLICY_BUF_MAX_SZ];
> +};
> +
> +/* TA initialization params */
> +struct ta_pmf_init_table {
> + u32 frequency; /* SMU sampling frequency */
> + bool validate;
> + bool sku_check;
> + bool metadata_macrocheck;
> + struct ta_pmf_load_policy_table policies_table;
> +};
> +
> +/* Everything the TA needs to Enact Policies */
> +struct ta_pmf_enact_table {
> + struct ta_pmf_condition_info ev_info;
> + u32 name;
> +};
> +
> +struct ta_pmf_action {
> + u32 action_index;
> + u32 value;
> +};
> +
> +/* output actions from TA */
> +struct ta_pmf_enact_result {
> + u32 actions_count;
> + struct ta_pmf_action actions_list[TA_PMF_ACTION_MAX];
> + u32 undo_count;
> + struct ta_pmf_action undo_list[TA_PMF_UNDO_MAX];
> +};
> +
> +union ta_pmf_input {
> + struct ta_pmf_enact_table enact_table;
> + struct ta_pmf_init_table init_table;
> +};
> +
> +union ta_pmf_output {
> + struct ta_pmf_enact_result policy_apply_table;
> + u32 rsvd[906];
> +};
> +
> struct ta_pmf_shared_memory {
> int command_id;
> int resp_id;
> u32 pmf_result;
> u32 if_version;
> + union ta_pmf_output pmf_output;
> + union ta_pmf_input pmf_input;
> };
>
> /* Core Layer */
> @@ -460,4 +591,5 @@ extern const struct attribute_group cnqf_feature_attribute_group;
> /* Smart PC builder Layer*/
> int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
> #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 1fce04beacb3..a8b05e746efd 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -42,9 +42,77 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
> param[0].u.memref.shm_offs = 0;
> }
>
> +static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
> +{
> + u32 val;
> + int idx;
> +
> + for (idx = 0; idx < out->actions_count; idx++) {
> + val = out->actions_list[idx].value;
> + switch (out->actions_list[idx].action_index) {
> + case PMF_POLICY_SPL:
> + if (dev->prev_data->spl != val) {
> + amd_pmf_send_cmd(dev, SET_SPL, false, val, NULL);
> + dev_dbg(dev->dev, "update SPL : %d\n", val);
> + dev->prev_data->spl = val;
> + }
> + break;
> +
> + case PMF_POLICY_SPPT:
> + if (dev->prev_data->sppt != val) {
> + amd_pmf_send_cmd(dev, SET_SPPT, false, val, NULL);
> + dev_dbg(dev->dev, "update SPPT : %d\n", val);
> + dev->prev_data->sppt = val;
> + }
> + break;
> +
> + case PMF_POLICY_FPPT:
> + if (dev->prev_data->fppt != val) {
> + amd_pmf_send_cmd(dev, SET_FPPT, false, val, NULL);
> + dev_dbg(dev->dev, "update FPPT : %d\n", val);
> + dev->prev_data->fppt = val;
> + }
> + break;
> +
> + case PMF_POLICY_SPPT_APU_ONLY:
> + if (dev->prev_data->sppt_apuonly != val) {
> + amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, val, NULL);
> + dev_dbg(dev->dev, "update SPPT_APU_ONLY : %d\n", val);
> + dev->prev_data->sppt_apuonly = val;
> + }
> + break;
> +
> + case PMF_POLICY_STT_MIN:
> + if (dev->prev_data->stt_minlimit != val) {
> + amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false, val, NULL);
> + dev_dbg(dev->dev, "update STT_MIN : %d\n", val);
> + dev->prev_data->stt_minlimit = val;
> + }
> + break;
> +
> + case PMF_POLICY_STT_SKINTEMP_APU:
> + if (dev->prev_data->stt_skintemp_apu != val) {
> + amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, val, NULL);
> + dev_dbg(dev->dev, "update STT_SKINTEMP_APU : %d\n", val);
> + dev->prev_data->stt_skintemp_apu = val;
> + }
> + break;
> +
> + case PMF_POLICY_STT_SKINTEMP_HS2:
> + if (dev->prev_data->stt_skintemp_hs2 != val) {
> + amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, val, NULL);
> + dev_dbg(dev->dev, "update STT_SKINTEMP_HS2 : %d\n", val);
> + dev->prev_data->stt_skintemp_hs2 = val;
> + }
> + break;
> + }
> + }
> +}
> +
> static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> {
> struct ta_pmf_shared_memory *ta_sm = NULL;
> + struct ta_pmf_enact_result *out = NULL;
> struct tee_param param[MAX_TEE_PARAM];
> struct tee_ioctl_invoke_arg arg;
> int ret = 0;
> @@ -52,7 +120,10 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> if (!dev->tee_ctx)
> return -ENODEV;
>
> + memset(dev->shbuf, 0, dev->policy_sz);
> ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
> + out = &ta_sm->pmf_output.policy_apply_table;
> +
> memset(ta_sm, 0, sizeof(struct ta_pmf_shared_memory));
> ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER__ENACT_POLICIES;
> ta_sm->if_version = PMF_TA_IF_VERSION__MAJOR;
> @@ -65,6 +136,12 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> return -EINVAL;
> }
>
> + if (ta_sm->pmf_result == TA_PMF_TYPE__SUCCESS && out->actions_count) {
> + dev_dbg(dev->dev, "action count:%d result:%x\n", out->actions_count,
> + ta_sm->pmf_result);
> + amd_pmf_apply_policies(dev, out);
> + }
> +
> return 0;
> }
>
> @@ -72,6 +149,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
> {
> struct ta_pmf_shared_memory *ta_sm = NULL;
> struct tee_param param[MAX_TEE_PARAM];
> + struct ta_pmf_init_table *in = NULL;
> struct tee_ioctl_invoke_arg arg;
> int ret = 0;
>
> @@ -80,10 +158,20 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
> return -ENODEV;
> }
>
> + dev_dbg(dev->dev, "Policy Binary size: %d bytes\n", dev->policy_sz);
> + memset(dev->shbuf, 0, dev->policy_sz);
> ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
> + in = &ta_sm->pmf_input.init_table;
> +
> ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER__INITIALIZE;
> ta_sm->if_version = PMF_TA_IF_VERSION__MAJOR;
> + in->metadata_macrocheck = false;
> + in->sku_check = false;
> + in->validate = true;
> + in->frequency = pb_actions_ms;
> + in->policies_table.table_size = dev->policy_sz;
>
> + memcpy(in->policies_table.table, dev->policy_buf, dev->policy_sz);
> amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER__INITIALIZE, &arg, param);
>
> ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
> @@ -103,6 +191,47 @@ static void amd_pmf_invoke_cmd(struct work_struct *work)
> schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms));
> }
>
> +static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
> +{
> + u32 cookie, length;
> + int res;
> +
> + cookie = readl(dev->policy_buf + 0x10);
> + length = readl(dev->policy_buf + 0x14);
Name the offsets with defines please.
> +
> + if (cookie != POLICY_SIGN_COOKIE || !length)
> + return -EINVAL;
> +
> + /* update the actual length */
> + dev->policy_sz = length + 512;
> + res = amd_pmf_invoke_cmd_init(dev);
> + if (res == TA_PMF_TYPE__SUCCESS) {
> + /* now its safe to announce that smart pc is enabled */
> + dev->smart_pc_enabled = 1;
> + schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms * 3));
> + } else {
> + dev_err(dev->dev, "%s ta invoke_cmd_init failed err: %x\n", __func__, res);
> + return res;
> + }
> +
> + return 0;
> +}
> +
> +static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
> +{
> + dev->policy_buf = kzalloc(dev->policy_sz, GFP_KERNEL);
> + if (!dev->policy_buf)
> + return -ENOMEM;
> +
> + dev->policy_base = ioremap(dev->policy_addr, dev->policy_sz);
> + if (!dev->policy_base)
> + return -ENOMEM;
Memleak?
> + memcpy(dev->policy_buf, dev->policy_base, dev->policy_sz);
> +
> + return amd_pmf_start_policy_engine(dev);
> +}
> +
> static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
> {
> return ver->impl_id == TEE_IMPL_ID_AMDTEE;
> @@ -149,7 +278,7 @@ static int amd_pmf_tee_init(struct amd_pmf_dev *dev)
> goto out_ctx;
> }
>
> - size = sizeof(struct ta_pmf_shared_memory);
> + size = sizeof(struct ta_pmf_shared_memory) + dev->policy_sz;
> dev->fw_shm_pool = tee_shm_alloc_kernel_buf(dev->tee_ctx, size);
> if (IS_ERR(dev->fw_shm_pool)) {
> dev_err(dev->dev, "%s: tee_shm_alloc_kernel_buf failed\n", __func__);
> @@ -191,11 +320,19 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> return ret;
>
> INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
> - return 0;
> + amd_pmf_set_dram_addr(dev);
> + amd_pmf_get_bios_buffer(dev);
> + dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
> + if (!dev->prev_data)
> + return -ENOMEM;
> +
> + return dev->smart_pc_enabled;
> }
>
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
> {
> + kfree(dev->prev_data);
> + kfree(dev->policy_buf);
> cancel_delayed_work_sync(&dev->pb_work);
> amd_pmf_tee_deinit(dev);
> }
>
--
i.
^ permalink raw reply
* Re: [PATCH 05/15] platform/x86/amd/pmf: change debugfs init sequence
From: Ilpo Järvinen @ 2023-09-26 16:53 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: hdegoede, markgross, basavaraj.natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-6-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> amd_pmf_dbgfs_register() needs to be called before amd_pmf_init_features().
Please answer to why? question too here.
> Hence change the sequence.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 6f36c43e081e..dbfe7c1d6fc4 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -427,9 +427,9 @@ static int amd_pmf_probe(struct platform_device *pdev)
>
> apmf_acpi_init(dev);
> platform_set_drvdata(pdev, dev);
> + amd_pmf_dbgfs_register(dev);
> amd_pmf_init_features(dev);
> apmf_install_handler(dev);
> - amd_pmf_dbgfs_register(dev);
>
> dev_info(dev->dev, "registered PMF device successfully\n");
>
>
--
i.
^ permalink raw reply
* Re: [PATCH 03/15] platform/x86/amd/pmf: Change signature of amd_pmf_set_dram_addr
From: Ilpo Järvinen @ 2023-09-26 16:52 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: hdegoede, markgross, basavaraj.natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-4-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
Add () to the function name in the shortlog. "Change signature" is quite
vague, perhaps you could come up something more descriptive.
> Make amd_pmf_set_dram_addr() as non-static so that same function
> can be used across files.
This says nothing about the move of allocation.
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/core.c | 14 ++++++++------
> drivers/platform/x86/amd/pmf/pmf.h | 1 +
> 2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 68f1389dda3e..5fb03ed614ff 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -251,26 +251,28 @@ static const struct pci_device_id pmf_pci_ids[] = {
> { }
> };
>
> -static void amd_pmf_set_dram_addr(struct amd_pmf_dev *dev)
> +int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev)
> {
> u64 phys_addr;
> u32 hi, low;
>
> + /* Get Metrics Table Address */
> + dev->buf = kzalloc(sizeof(dev->m_table), GFP_KERNEL);
> + if (!dev->buf)
> + return -ENOMEM;
> +
> phys_addr = virt_to_phys(dev->buf);
> hi = phys_addr >> 32;
> low = phys_addr & GENMASK(31, 0);
>
> amd_pmf_send_cmd(dev, SET_DRAM_ADDR_HIGH, 0, hi, NULL);
> amd_pmf_send_cmd(dev, SET_DRAM_ADDR_LOW, 0, low, NULL);
> +
> + return 0;
> }
>
> int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev)
> {
> - /* Get Metrics Table Address */
> - dev->buf = kzalloc(sizeof(dev->m_table), GFP_KERNEL);
> - if (!dev->buf)
> - return -ENOMEM;
> -
> INIT_DELAYED_WORK(&dev->work_buffer, amd_pmf_get_metrics);
>
> amd_pmf_set_dram_addr(dev);
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index a9333ff6c0a7..ea15ce547d24 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -421,6 +421,7 @@ int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev);
> int amd_pmf_get_power_source(void);
> int apmf_install_handler(struct amd_pmf_dev *pmf_dev);
> int apmf_os_power_slider_update(struct amd_pmf_dev *dev, u8 flag);
> +int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev);
>
> /* SPS Layer */
> int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf);
>
Why are not amd_pmf_set_dram_addr() callers made to handle/pass on
errors???
--
i.
^ permalink raw reply
* Re: [PATCH 02/15] platform/x86/amd/pmf: Add support PMF-TA interaction
From: Ilpo Järvinen @ 2023-09-26 16:48 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: hdegoede, markgross, basavaraj.natikar, jikos, benjamin.tissoires,
alexander.deucher, christian.koenig, Xinhui.Pan, airlied, daniel,
Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-3-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> PMF TA (Trusted Application) loads via the TEE environment into the
> AMD ASP.
>
> PMF-TA supports two commands:
> 1) Init: Initialize the TA with the PMF Smart PC policy binary and
> start the policy engine. A policy is a combination of inputs and
> outputs, where;
> - the inputs are the changing dynamics of the system like the user
> behaviour, system heuristics etc.
> - the outputs, which are the actions to be set on the system which
> lead to better power management and enhanced user experience.
>
> PMF driver acts as a central manager in this case to supply the
> inputs required to the TA (either by getting the information from
> the other kernel subsystems or from userland)
>
> 2) Enact: Enact the output actions from the TA. The action could be
> applying a new thermal limit to boost/throttle the power limits or
> change system behavior.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/pmf.h | 10 +++
> drivers/platform/x86/amd/pmf/tee-if.c | 97 ++++++++++++++++++++++++++-
> 2 files changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 02460c2a31ea..a9333ff6c0a7 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -59,6 +59,9 @@
> #define ARG_NONE 0
> #define AVG_SAMPLE_SIZE 3
>
> +/* TA macros */
> +#define PMF_TA_IF_VERSION__MAJOR 1
I suppose double _ is not intentional?
> +
> /* AMD PMF BIOS interfaces */
> struct apmf_verify_interface {
> u16 size;
> @@ -184,6 +187,7 @@ struct amd_pmf_dev {
> struct tee_shm *fw_shm_pool;
> u32 session_id;
> void *shbuf;
> + struct delayed_work pb_work;
> bool smart_pc_enabled;
> };
>
> @@ -395,6 +399,12 @@ struct apmf_dyn_slider_output {
> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
> } __packed;
>
> +/* cmd ids for TA communication */
> +enum ta_pmf_command {
> + TA_PMF_COMMAND_POLICY_BUILDER__INITIALIZE,
> + TA_PMF_COMMAND_POLICY_BUILDER__ENACT_POLICIES
Add comma to the second line too.
Did you mean to have double _?
> +};
> +
> struct ta_pmf_shared_memory {
> int command_id;
> int resp_id;
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index b48340edbf44..1fce04beacb3 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -13,9 +13,96 @@
> #include "pmf.h"
>
> #define MAX_TEE_PARAM 4
> +
> +/* Policy binary actions sampling frequency (in ms) */
> +static int pb_actions_ms = 1000;
> +#ifdef CONFIG_AMD_PMF_DEBUG
> +module_param(pb_actions_ms, int, 0644);
> +MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency (default = 1000ms)");
> +#endif
> +
> static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
>
> +static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
> + struct tee_ioctl_invoke_arg *arg,
> + struct tee_param *param)
> +{
> + memset(arg, 0, sizeof(*arg));
> + memset(param, 0, MAX_TEE_PARAM * sizeof(*param));
> +
> + arg->func = cmd;
> + arg->session = dev->session_id;
> + arg->num_params = MAX_TEE_PARAM;
> +
> + /* Fill invoke cmd params */
> + param[0].u.memref.size = sizeof(struct ta_pmf_shared_memory);
> + param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;
> + param[0].u.memref.shm = dev->fw_shm_pool;
> + param[0].u.memref.shm_offs = 0;
> +}
> +
> +static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
> +{
> + struct ta_pmf_shared_memory *ta_sm = NULL;
> + struct tee_param param[MAX_TEE_PARAM];
> + struct tee_ioctl_invoke_arg arg;
> + int ret = 0;
> +
> + if (!dev->tee_ctx)
> + return -ENODEV;
> +
> + ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
> + memset(ta_sm, 0, sizeof(struct ta_pmf_shared_memory));
> + ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER__ENACT_POLICIES;
> + ta_sm->if_version = PMF_TA_IF_VERSION__MAJOR;
> +
> + amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER__ENACT_POLICIES, &arg, param);
> +
> + ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
> + if (ret < 0 || arg.ret != 0) {
> + dev_err(dev->dev, "%s failed TEE err: %x, ret:%x\n", __func__, arg.ret, ret);
No __func__s please.
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
> +{
> + struct ta_pmf_shared_memory *ta_sm = NULL;
> + struct tee_param param[MAX_TEE_PARAM];
> + struct tee_ioctl_invoke_arg arg;
> + int ret = 0;
> +
> + if (!dev->tee_ctx) {
> + dev_err(dev->dev, "%s tee_ctx no context\n", __func__);
> + return -ENODEV;
> + }
> +
> + ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
> + ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER__INITIALIZE;
> + ta_sm->if_version = PMF_TA_IF_VERSION__MAJOR;
> +
> + amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER__INITIALIZE, &arg, param);
> +
> + ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
> + if (ret < 0 || arg.ret != 0) {
> + dev_err(dev->dev, "%s failed TEE err: %x, ret:%x\n", __func__, arg.ret, ret);
> + return -EINVAL;
> + }
> +
> + return ta_sm->pmf_result;
> +}
> +
> +static void amd_pmf_invoke_cmd(struct work_struct *work)
> +{
> + struct amd_pmf_dev *dev = container_of(work, struct amd_pmf_dev, pb_work.work);
> +
> + amd_pmf_invoke_cmd_enact(dev);
> + schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms));
> +}
> +
> static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
> {
> return ver->impl_id == TEE_IMPL_ID_AMDTEE;
> @@ -97,10 +184,18 @@ static void amd_pmf_tee_deinit(struct amd_pmf_dev *dev)
>
> int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> {
> - return amd_pmf_tee_init(dev);
> + int ret;
> +
> + ret = amd_pmf_tee_init(dev);
> + if (ret)
> + return ret;
> +
> + INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
> + return 0;
> }
>
> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
> {
> + cancel_delayed_work_sync(&dev->pb_work);
> amd_pmf_tee_deinit(dev);
> }
>
--
i.
^ permalink raw reply
* Re: [PATCH 01/15] platform/x86/amd/pmf: Add PMF TEE interface
From: Ilpo Järvinen @ 2023-09-26 16:42 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <20230922175056.244940-2-Shyam-sundar.S-k@amd.com>
On Fri, 22 Sep 2023, Shyam Sundar S K wrote:
> AMD PMF driver loads the PMF TA (Trusted Application) into the AMD
> ASP's (AMD Security Processor) TEE (Trusted Execution Environment).
>
> PMF Trusted Application is a secured firmware placed under
> /lib/firmware/amdtee gets loaded only when the TEE environment is
> initialized. Add the initial code path to build these pipes.
>
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
> drivers/platform/x86/amd/pmf/Makefile | 3 +-
> drivers/platform/x86/amd/pmf/core.c | 11 ++-
> drivers/platform/x86/amd/pmf/pmf.h | 16 ++++
> drivers/platform/x86/amd/pmf/tee-if.c | 106 ++++++++++++++++++++++++++
> 4 files changed, 132 insertions(+), 4 deletions(-)
> create mode 100644 drivers/platform/x86/amd/pmf/tee-if.c
>
> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
> index fdededf54392..d2746ee7369f 100644
> --- a/drivers/platform/x86/amd/pmf/Makefile
> +++ b/drivers/platform/x86/amd/pmf/Makefile
> @@ -6,4 +6,5 @@
>
> obj-$(CONFIG_AMD_PMF) += amd-pmf.o
> amd-pmf-objs := core.o acpi.o sps.o \
> - auto-mode.o cnqf.o
> + auto-mode.o cnqf.o \
> + tee-if.o
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 78ed3ee22555..68f1389dda3e 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -309,8 +309,11 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev)
> dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n");
> }
>
> - /* Enable Auto Mode */
> - if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> + if (amd_pmf_init_smart_pc(dev)) {
> + /* Enable Smart PC Solution builder */
> + dev_dbg(dev->dev, "Smart PC Solution Enabled\n");
> + } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> + /* Enable Auto Mode */
> amd_pmf_init_auto_mode(dev);
> dev_dbg(dev->dev, "Auto Mode Init done\n");
> } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
> @@ -330,7 +333,9 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev)
> amd_pmf_deinit_sps(dev);
> }
>
> - if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> + if (dev->smart_pc_enabled) {
> + amd_pmf_deinit_smart_pc(dev);
> + } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> amd_pmf_deinit_auto_mode(dev);
> } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
> is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index deba88e6e4c8..02460c2a31ea 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -179,6 +179,12 @@ struct amd_pmf_dev {
> bool cnqf_enabled;
> bool cnqf_supported;
> struct notifier_block pwr_src_notifier;
> + /* Smart PC solution builder */
> + struct tee_context *tee_ctx;
> + struct tee_shm *fw_shm_pool;
> + u32 session_id;
> + void *shbuf;
> + bool smart_pc_enabled;
> };
>
> struct apmf_sps_prop_granular {
> @@ -389,6 +395,13 @@ struct apmf_dyn_slider_output {
> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
> } __packed;
>
> +struct ta_pmf_shared_memory {
> + int command_id;
> + int resp_id;
> + u32 pmf_result;
> + u32 if_version;
> +};
> +
> /* Core Layer */
> int apmf_acpi_init(struct amd_pmf_dev *pmf_dev);
> void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev);
> @@ -433,4 +446,7 @@ void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev);
> int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
> extern const struct attribute_group cnqf_feature_attribute_group;
>
> +/* Smart PC builder Layer*/
> +int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
> +void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
> #endif /* PMF_H */
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> new file mode 100644
> index 000000000000..b48340edbf44
> --- /dev/null
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Platform Management Framework Driver - TEE Interface
> + *
> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
> + * All Rights Reserved.
> + *
> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> + */
> +
> +#include <linux/tee_drv.h>
> +#include <linux/uuid.h>
> +#include "pmf.h"
> +
> +#define MAX_TEE_PARAM 4
> +static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> + 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
> +
> +static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
> +{
> + return ver->impl_id == TEE_IMPL_ID_AMDTEE;
> +}
> +
> +static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id)
> +{
> + struct tee_ioctl_open_session_arg sess_arg = {};
> + int rc;
> +
> + export_uuid(sess_arg.uuid, &amd_pmf_ta_uuid);
> + sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> + sess_arg.num_params = 0;
> +
> + rc = tee_client_open_session(ctx, &sess_arg, NULL);
> + if (rc < 0 || sess_arg.ret != 0) {
> + pr_err("%s: tee_client_open_session failed err:%#x, ret:%#x\n",
> + __func__, sess_arg.ret, rc);
Don't print function names but meaningful messages (no __func__ nor
manually added function names). I didn't make the others, please take care
of them all, thank you.
--
i.
> + rc = -EINVAL;
> + } else {
> + *id = sess_arg.session;
> + }
> +
> + return rc;
> +}
> +
> +static int amd_pmf_tee_init(struct amd_pmf_dev *dev)
> +{
> + int ret;
> + u32 size;
> +
> + /* Open context with TEE driver */
> + dev->tee_ctx = tee_client_open_context(NULL, amd_pmf_amdtee_ta_match, NULL, NULL);
> + if (IS_ERR(dev->tee_ctx)) {
> + dev_err(dev->dev, "%s: tee_client_open_context failed\n", __func__);
> + return PTR_ERR(dev->tee_ctx);
> + }
> +
> + /* Open session with PMF Trusted App */
> + ret = amd_pmf_ta_open_session(dev->tee_ctx, &dev->session_id);
> + if (ret) {
> + dev_err(dev->dev, "%s: amd_pmf_ta_open_session failed(%d)\n", __func__, ret);
> + ret = -EINVAL;
> + goto out_ctx;
> + }
> +
> + size = sizeof(struct ta_pmf_shared_memory);
> + dev->fw_shm_pool = tee_shm_alloc_kernel_buf(dev->tee_ctx, size);
> + if (IS_ERR(dev->fw_shm_pool)) {
> + dev_err(dev->dev, "%s: tee_shm_alloc_kernel_buf failed\n", __func__);
> + ret = PTR_ERR(dev->fw_shm_pool);
> + goto out_sess;
> + }
> +
> + dev->shbuf = tee_shm_get_va(dev->fw_shm_pool, 0);
> + dev_dbg(dev->dev, "TEE init done\n");
> +
> + return 0;
> +
> +out_sess:
> + tee_client_close_session(dev->tee_ctx, dev->session_id);
> +out_ctx:
> + tee_client_close_context(dev->tee_ctx);
> +
> + return ret;
> +}
> +
> +static void amd_pmf_tee_deinit(struct amd_pmf_dev *dev)
> +{
> + /* Free the shared memory pool */
> + tee_shm_free(dev->fw_shm_pool);
> +
> + /* close the existing session with PMF TA*/
> + tee_client_close_session(dev->tee_ctx, dev->session_id);
> +
> + /* close the context with TEE driver */
> + tee_client_close_context(dev->tee_ctx);
> +}
> +
> +int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
> +{
> + return amd_pmf_tee_init(dev);
> +}
> +
> +void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
> +{
> + amd_pmf_tee_deinit(dev);
> +}
>
^ permalink raw reply
* [PATCH] dt-bindings: input: syna,rmi4: Make "additionalProperties: true" explicit
From: Rob Herring @ 2023-09-26 14:42 UTC (permalink / raw)
To: Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley,
Jason A. Donenfeld, Matthias Schiffer, Vincent Huang
Cc: linux-input, devicetree, linux-kernel
Make it explicit that the not yet documented child nodes have additional
properties and the child node schema is not complete.
Signed-off-by: Rob Herring <robh@kernel.org>
---
Documentation/devicetree/bindings/input/syna,rmi4.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/syna,rmi4.yaml b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
index 4d4e1a8e36be..b522c8d3ce0d 100644
--- a/Documentation/devicetree/bindings/input/syna,rmi4.yaml
+++ b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
@@ -164,6 +164,8 @@ patternProperties:
"^rmi4-f[0-9a-f]+@[0-9a-f]+$":
type: object
+ additionalProperties: true
+
description:
Other functions, not documented yet.
--
2.40.1
^ permalink raw reply related
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