* Re: [PATCH 06/15] platform/x86/amd/pmf: Add support to get inputs from other subsystems
From: Shyam Sundar S K @ 2023-09-30 8:40 UTC (permalink / raw)
To: Ilpo Järvinen
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: <be35f637-23c7-64c2-65bf-5b1783801d16@linux.intel.com>
On 9/26/2023 10:38 PM, Ilpo Järvinen wrote:
> 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?
Yes I think. Can you elaborate a bit more please if there a overflow
and I am missing it?
Thanks,
Shyam
>
>> +
>> + /* 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);
>>
>
^ permalink raw reply
* Re: [RFC PATCH 2/2] hid: lenovo: move type checks to lenovo_features_set_cptkbd()
From: Martin Kepplinger @ 2023-09-30 9:26 UTC (permalink / raw)
To: Jamie Lentin; +Cc: jikos, benjamin.tissoires, linux-kernel, linux-input
In-Reply-To: <6adc3e66402f38258eae3a044db9ee11@lentin.co.uk>
Am Donnerstag, dem 28.09.2023 um 22:06 +0100 schrieb Jamie Lentin:
> On 2023-09-27 12:20, Martin Kepplinger wrote:
> > Am Mittwoch, dem 27.09.2023 um 09:19 +0100 schrieb Jamie Lentin:
> > > 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);
> >
> > ok. ignore my change (this whole patch) and look at your addition
> > here,
> > don't you already make sure only the mouse-part gets the messages?
> > you
> > just write switch()case instead of if(); what do you think is
> > missing
> > here?
>
> Correct, this switch statement() that you're removing in this patch
> already does exactly this, so replacing it with the
> if()-and-return-early block would result in equivalent code (ignoring
> the Trackpoint keyboard II). That suggestion wasn't the most helpful
> of
> mine, sorry!
>
> The reason I originally used a switch here is for symmetry with
> lenovo_probe(), lenovo_remove(), etc. It might some day be useful to
> add
> something like:
>
> case USB_DEVICE_ID_LENOVO_X1_TAB:
> lenovo_reset_resume_tp10ubkbd(hdev);
> break;
>
> ...to the switch. For completeness, lenovo_reset_resume() should
> probably call a separate lenovo_reset_resume_cptkbd() that does the
> work. For just 3 lines of code it didn't seem worth it at the time
> though.
>
> Cheers,
ok your original patch seems to basically be a valid first fix. Should
I send it on your behalf (with you as author) or do you want to send it
yourself? Let's get this fixed :)
thanks,
martin
^ permalink raw reply
* Re: [RFC PATCH 2/2] hid: lenovo: move type checks to lenovo_features_set_cptkbd()
From: Jamie Lentin @ 2023-09-30 9:58 UTC (permalink / raw)
To: Martin Kepplinger; +Cc: jikos, benjamin.tissoires, linux-kernel, linux-input
In-Reply-To: <2d1e21cc9677a5cfe828decd8cbd5e930237e76d.camel@posteo.de>
On 2023-09-30 10:26, Martin Kepplinger wrote:
> Am Donnerstag, dem 28.09.2023 um 22:06 +0100 schrieb Jamie Lentin:
>> On 2023-09-27 12:20, Martin Kepplinger wrote:
>> > Am Mittwoch, dem 27.09.2023 um 09:19 +0100 schrieb Jamie Lentin:
>> > > 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);
>> >
>> > ok. ignore my change (this whole patch) and look at your addition
>> > here,
>> > don't you already make sure only the mouse-part gets the messages?
>> > you
>> > just write switch()case instead of if(); what do you think is
>> > missing
>> > here?
>>
>> Correct, this switch statement() that you're removing in this patch
>> already does exactly this, so replacing it with the
>> if()-and-return-early block would result in equivalent code (ignoring
>> the Trackpoint keyboard II). That suggestion wasn't the most helpful
>> of
>> mine, sorry!
>>
>> The reason I originally used a switch here is for symmetry with
>> lenovo_probe(), lenovo_remove(), etc. It might some day be useful to
>> add
>> something like:
>>
>> case USB_DEVICE_ID_LENOVO_X1_TAB:
>> lenovo_reset_resume_tp10ubkbd(hdev);
>> break;
>>
>> ...to the switch. For completeness, lenovo_reset_resume() should
>> probably call a separate lenovo_reset_resume_cptkbd() that does the
>> work. For just 3 lines of code it didn't seem worth it at the time
>> though.
>>
>> Cheers,
>
> ok your original patch seems to basically be a valid first fix. Should
> I send it on your behalf (with you as author) or do you want to send it
> yourself? Let's get this fixed :)
If it's working for you and you don't mind, feel free send it on my
behalf. I don't have the hardware to test the patch currently.
Thanks!
>
> thanks,
> martin
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-09-30 14:27 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Jonathan Albrieux
In-Reply-To: <ZROaqRiWa6ReVH/D@nixie71>
Hi Jeff,
On Tue, Sep 26, 2023 at 09:59:53PM -0500, Jeff LaBundy wrote:
> On Sun, Sep 17, 2023 at 07:05:50PM +0200, Stephan Gerhold wrote:
> > 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>
> [...]
> > > > +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.
>
I don't have a datasheet unfortunately. :(
However, I tried to simulate this case on my board by keeping the reset
GPIO permanently de-asserted (i.e. high because of active-low). The
results are not entirely conclusive: The controller seems to respond to
commands and the initial configuration is read correctly. However, it
does not report any touch events. As soon as I add the temporary
assertion of the reset signal back it works fine again.
I suspect toggling the reset signal might be required to make the
controller come properly out of reset. I'll keep it required to be sure.
Thanks,
Stephan
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-09-30 15:28 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Jonathan Albrieux
In-Reply-To: <ZQYUe46/rj8jqNvg@nixie71>
Hi Jeff,
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/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.
>
In v2 I have added linux/of.h and linux/mod_devicetable.h, since I'm
actually using definitions from these two only. Seems like including
of_device.h is discouraged nowadays, see commit dbce1a7d5dce ("Input:
Explicitly include correct DT includes").
> > [...]
> > +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};
> > + 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.
>
I played with this for a bit. A problem of the "? : error" approach is
that it hides the original error in case the new calls error again.
Let's assume
error = hx852x_start(hx);
if (error)
goto power_off;
fails with error = -ENXIO. We jump to power_off:
power_off:
return hx852x_power_off(hx) ? : error;
Let's say for whatever reason hx852x_power_off() fails too but returns
-EINVAL. Then the final return value will be -EINVAL, while with the
current approach in this patch it would return the original cause
(-ENXIO). I think that's more clear.
I also played with moving code to a separate function to avoid the
gotos, but I feel like that makes the fairly focused logic of this
function (reading the configuration by temporarily entering the test
mode) just more confusing.
To still fix the error handling I ended up with duplicating the
"success" code path and the "error" code path (it's just two function
calls), i.e.:
error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
if (error)
goto err_power_off;
return hx852x_power_off(hx);
err_test_mode:
i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
err_power_off:
hx852x_power_off(hx);
return error;
I hope that's fine too. A bit ugly maybe but in this case I would prefer
having the main code path (reading the configuration) clearly readable.
Let me know if you have a better suggestion for these (I'll send v2 in a
bit so that you can see the full diff there).
Thanks!
Stephan
^ permalink raw reply
* [PATCH v2 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-09-30 15:32 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg,
linux-input, devicetree, linux-kernel, Jeff LaBundy,
Christophe JAILLET, Jonathan Albrieux, Stephan Gerhold
In-Reply-To: <20230930-hx852x-v2-0-c5821947b225@gerhold.net>
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 | 499 +++++++++++++++++++++++++++++++
4 files changed, 517 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..98a55be7891d
--- /dev/null
+++ b/drivers/input/touchscreen/himax_hx852x.c
@@ -0,0 +1,499 @@
+// 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/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#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
+#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;
+ unsigned int keycodes[HX852X_MAX_KEY_COUNT];
+};
+
+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);
+
+struct hx852x_coord {
+ __be16 x;
+ __be16 y;
+} __packed __aligned(4);
+
+struct hx852x_touch_info {
+ u8 finger_num;
+ __le16 finger_pressed;
+ u8 padding;
+} __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;
+}
+
+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) {
+ 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 int 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);
+ return error;
+ }
+ 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);
+ return error;
+ }
+ msleep(30);
+
+ return 0;
+}
+
+static int 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);
+ return error;
+}
+
+static int hx852x_read_config(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ struct hx852x_config conf;
+ 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 err_power_off;
+
+ error = hx852x_stop(hx);
+ if (error)
+ goto err_power_off;
+
+ error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
+ HX852X_SRAM_SWITCH_TEST_MODE);
+ if (error)
+ goto err_power_off;
+
+ error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
+ HX852X_SRAM_ADDR_CONFIG);
+ if (error)
+ goto err_test_mode;
+
+ error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
+ if (error)
+ goto err_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: %u, y res: %u, max fingers: %u\n",
+ x_res, y_res, hx->max_fingers);
+
+ if (hx->max_fingers > HX852X_MAX_FINGERS) {
+ dev_err(dev, "max supported fingers: %u, found: %u\n",
+ HX852X_MAX_FINGERS, hx->max_fingers);
+ error = -EINVAL;
+ goto err_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);
+ }
+
+ error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
+ if (error)
+ goto err_power_off;
+
+ return hx852x_power_off(hx);
+
+err_test_mode:
+ i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
+err_power_off:
+ hx852x_power_off(hx);
+ return error;
+}
+
+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;
+ 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_ratelimited(&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: %u, found: %u\n",
+ 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 required 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)
+ return dev_err_probe(dev, error, "failed to get regulators\n");
+
+ hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(hx->reset_gpiod))
+ return dev_err_probe(dev, PTR_ERR(hx->reset_gpiod),
+ "failed to get reset gpio\n");
+
+ error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
+ IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
+ if (error)
+ return dev_err_probe(dev, error, "failed to request irq %d", client->irq);
+
+ error = hx852x_read_config(hx);
+ if (error)
+ return error;
+
+ input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
+ input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
+ input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+
+ touchscreen_parse_properties(hx->input_dev, true, &hx->props);
+ error = hx852x_parse_properties(hx);
+ if (error)
+ return error;
+
+ hx->input_dev->keycode = hx->keycodes;
+ hx->input_dev->keycodemax = hx->keycount;
+ hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
+ for (i = 0; i < hx->keycount; i++)
+ input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
+
+ error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (error)
+ return dev_err_probe(dev, error, "failed to init MT slots\n");
+
+ error = input_register_device(hx->input_dev);
+ if (error)
+ return dev_err_probe(dev, error, "failed to register input device\n");
+
+ return 0;
+}
+
+static int hx852x_suspend(struct device *dev)
+{
+ struct hx852x *hx = dev_get_drvdata(dev);
+
+ mutex_lock(&hx->input_dev->mutex);
+ if (input_device_enabled(hx->input_dev))
+ hx852x_stop(hx);
+ mutex_unlock(&hx->input_dev->mutex);
+
+ return 0;
+}
+
+static int hx852x_resume(struct device *dev)
+{
+ struct hx852x *hx = dev_get_drvdata(dev);
+ int error = 0;
+
+ mutex_lock(&hx->input_dev->mutex);
+ if (input_device_enabled(hx->input_dev))
+ error = hx852x_start(hx);
+ mutex_unlock(&hx->input_dev->mutex);
+
+ return error;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(hx852x_pm_ops, hx852x_suspend, hx852x_resume);
+
+#ifdef CONFIG_OF
+static const struct of_device_id hx852x_of_match[] = {
+ { .compatible = "himax,hx852es" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, hx852x_of_match);
+#endif
+
+static struct i2c_driver hx852x_driver = {
+ .probe = hx852x_probe,
+ .driver = {
+ .name = "himax_hx852x",
+ .pm = pm_sleep_ptr(&hx852x_pm_ops),
+ .of_match_table = of_match_ptr(hx852x_of_match),
+ },
+};
+module_i2c_driver(hx852x_driver);
+
+MODULE_DESCRIPTION("Himax HX852x(ES) Touchscreen Driver");
+MODULE_AUTHOR("Jonathan Albrieux <jonathan.albrieux@gmail.com>");
+MODULE_AUTHOR("Stephan Gerhold <stephan@gerhold.net>");
+MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related
* [PATCH v2 0/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-09-30 15:31 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg,
linux-input, devicetree, linux-kernel, Jeff LaBundy,
Christophe JAILLET, Jonathan Albrieux, Stephan Gerhold,
Krzysztof Kozlowski
Add DT schema and driver for the Himax HX852x(ES) touch panel
controller, with support for multi-touch and capacitive touch keys.
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Changes in v2:
- dt-bindings: Swap required:/additionalProperties: (Krzysztof)
- Use dev_err_ratelimited() for error in IRQ thread (Christophe)
- Use dev_err_probe() consistently (Christophe)
- Improve error handling of hx852x_power_off()/hx852x_stop() (Jeff)
- Add linux/of.h and linux/mod_devicetable.h include (Jeff)
- Fix %d -> %u in some format strings (Jeff)
- Fix other small comments from Jeff
- Link to v1: https://lore.kernel.org/r/20230913-hx852x-v1-0-9c1ebff536eb@gerhold.net
---
Jonathan Albrieux (1):
Input: add Himax HX852x(ES) touchscreen driver
Stephan Gerhold (1):
dt-bindings: input: touchscreen: document Himax HX852x(ES)
.../bindings/input/touchscreen/himax,hx852es.yaml | 81 ++++
MAINTAINERS | 7 +
drivers/input/touchscreen/Kconfig | 10 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/himax_hx852x.c | 499 +++++++++++++++++++++
5 files changed, 598 insertions(+)
---
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
change-id: 20230816-hx852x-3490d2773322
Best regards,
--
Stephan Gerhold <stephan@gerhold.net>
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: input: touchscreen: document Himax HX852x(ES)
From: Stephan Gerhold @ 2023-09-30 15:32 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg,
linux-input, devicetree, linux-kernel, Jeff LaBundy,
Christophe JAILLET, Jonathan Albrieux, Stephan Gerhold,
Krzysztof Kozlowski
In-Reply-To: <20230930-hx852x-v2-0-c5821947b225@gerhold.net>
Himax HX852x(ES) is a touch panel controller with optional support
for capacitive touch keys.
Unfortunately, the model naming is quite unclear and confusing. There
seems to be a distinction between models (e.g. HX8526) and the "series"
suffix (e.g. -A, -B, -C, -D, -E, -ES). But this doesn't seem to be
applied very consistently because e.g. HX8527-E(44) actually seems to
belong to the -ES series.
The compatible consists of the actual part number followed by the
"series" as fallback compatible. Typically only the latter will be
interesting for drivers as there is no relevant difference on the
driver side.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
.../bindings/input/touchscreen/himax,hx852es.yaml | 81 ++++++++++++++++++++++
1 file changed, 81 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
new file mode 100644
index 000000000000..40a60880111d
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/himax,hx852es.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Himax HX852x(ES) touch panel controller
+
+maintainers:
+ - Stephan Gerhold <stephan@gerhold.net>
+
+allOf:
+ - $ref: touchscreen.yaml#
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - himax,hx8525e
+ - himax,hx8526e
+ - himax,hx8527e
+ - const: himax,hx852es
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+ description: Touch Screen Interrupt (TSIX), active low
+
+ reset-gpios:
+ maxItems: 1
+ description: External Reset (XRES), active low
+
+ vcca-supply:
+ description: Analog power supply (VCCA)
+
+ vccd-supply:
+ description: Digital power supply (VCCD)
+
+ touchscreen-inverted-x: true
+ touchscreen-inverted-y: true
+ touchscreen-size-x: true
+ touchscreen-size-y: true
+ touchscreen-swapped-x-y: true
+
+ linux,keycodes:
+ minItems: 1
+ maxItems: 4
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - reset-gpios
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/input/input.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@48 {
+ compatible = "himax,hx8527e", "himax,hx852es";
+ reg = <0x48>;
+ interrupt-parent = <&tlmm>;
+ interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&tlmm 12 GPIO_ACTIVE_LOW>;
+ vcca-supply = <®_ts_vcca>;
+ vccd-supply = <&pm8916_l6>;
+ linux,keycodes = <KEY_BACK KEY_HOMEPAGE KEY_APPSELECT>;
+ };
+ };
+
+...
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 00/52] input: Convert to platform remove callback returning void
From: Dmitry Torokhov @ 2023-09-30 15:37 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Lee Jones, Linus Walleij, Alexandre Torgue, Pavel Machek,
Guenter Roeck, Liang He, linux-stm32, Daniel Lezcano,
chrome-platform, Arnd Bergmann, Samuel Holland, Andrey Moiseev,
Michal Simek, Ruan Jinjie, Yangtao Li, Jernej Skrabec,
joewu (吳仲振), Miloslav Trmac, Robert Jarzmik,
Chen-Yu Tsai, Andy Gross, linux-input, Jeff LaBundy, linux-sunxi,
linux-arm-msm, Maxime Coquelin, Michael Hennerich, Rob Herring,
ye xingchen, Kalle Valo, Steven Rostedt (Google), Hans de Goede,
Siarhei Volkau, Christophe JAILLET, Jonathan Cameron,
Andy Shevchenko, Benson Leung, linux-arm-kernel, Paolo Abeni,
Support Opensource, Chen Jun, Greg Kroah-Hartman,
Mattijs Korpershoek, Rafael J. Wysocki, Konrad Dybcio,
Krzysztof Kozlowski, kernel, patches, Dmitry Baryshkov,
Jonathan Corbet, Bjorn Andersson
In-Reply-To: <20230924155057.e4k4ruv5iggbt6q6@pengutronix.de>
Hi Uwe,
Sorry for the spotty responses.
On Sun, Sep 24, 2023 at 05:50:57PM +0200, Uwe Kleine-König wrote:
> Hello Dmitry,
>
> On Sat, Sep 23, 2023 at 07:48:21PM -0700, Dmitry Torokhov wrote:
> > On Wed, Sep 20, 2023 at 02:57:37PM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > >
> > > this series converts all platform drivers below drivers/input to use
> > > remove_new. The motivation is to get rid of an integer return code
> > > that is (mostly) ignored by the platform driver core and error prone on
> > > the driver side.
> > >
> > > See commit 5c5a7680e67b ("platform: Provide a remove callback that
> > > returns no value") for an extended explanation and the eventual goal.
> > >
> > > There are no interdependencies between the patches. As there are still
> > > quite a few drivers to convert, I'm happy about every patch that makes
> > > it in. So even if there is a merge conflict with one patch until you
> > > apply or a subject prefix is suboptimal, please apply the remainder of
> > > this series anyhow.
> >
> > Applied the lot (fixing the i8042-sparcio patch subject), thank you!
>
> Thanks. In the meantime I found out why my process failed here: I only
> fixed *.c, but this driver struct is defined in a header file
> i8042-sparcio.h.
>
> This file is only included by drivers/input/serio/i8042.h which in turn
> is only included by drivers/input/serio/i8042.c. So there is only one
> instance created, but I'd call that unusual anyhow.
Right, i8042 is essentially a singleton, and what you see here is an
attempt to bolt OF onto a legacy driver that is largely predates the
current driver model. I wanted to clean it up, but it is still widely
used and I am hesitant to disturb it too much.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: synaptics-rmi4 - replace deprecated strncpy
From: Dmitry Torokhov @ 2023-09-30 16:06 UTC (permalink / raw)
To: Justin Stitt; +Cc: linux-input, linux-kernel, linux-hardening
In-Reply-To: <20230921-strncpy-drivers-input-rmi4-rmi_f34-c-v1-1-4aef2e84b8d2@google.com>
On Thu, Sep 21, 2023 at 09:58:11AM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1]
>
> Let's use memcpy() as the bounds have already been checked and this
> decays into a simple byte copy from one buffer to another removing any
> ambiguity that strncpy has.
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] input: generalize the Imagis touchscreen driver
From: Markuss Broks @ 2023-09-30 16:10 UTC (permalink / raw)
To: Karel Balej, Karel Balej
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-input, devicetree, linux-kernel, Duje Mihanović,
~postmarketos/upstreaming, Jeff LaBundy, linmengbo0689
In-Reply-To: <CVVJR34G5A55.2LYQW8Z5PEEDA@gimli.ms.mff.cuni.cz>
Hi Karel,
On 9/29/23 19:37, Karel Balej wrote:
> Hello, Markuss,
>
>> If you don't mind the extra hassle, I'm all in for my generalization
>> thing going together with your series.
>>
>> Alternatively, I can resend it myself, but I believe it would be better
>> if they go in bulk since they need to be applied together.
> great. Do you wish to make any changes to your original series? If not,
> please let me know and I will use the v2 [1] as it is.
I believe that version should be fine.
>
>>>> As for the voltage set, I believe this does not belong in a kernel
>>>> driver. You should set it in device-tree with `regulator-max-microvolt`
>>>> and `regulator-min-microvolt`.
>>> Please see my response to Jeff regarding this. I will be happy to hear
>>> your thoughts on what I propose.
>> Actually, the regulator values belong to the device-tree, because the
>> device-tree for the board is what describes the board's regulators, and
>> since you know what components are installed on that specific board you
>> can know which regulator values are supposed to be set for it.
>>
>> [...]
>>
>> The actual min/max values for regulators or its voltage table is
>> provided by the regulator driver itself, so there's not much point in
>> specifying those again in the device-tree.
> I see. I think the reason why I thought what I wrote before is that
> downstream the regulator DTS lives separately from the board as a .dtsi
> file which made me think that it can be used universally. So if I
> understand correctly now, the hardware specifications of the regulator,
> such as the minimal and maximal voltage should be part of the driver,
> while the DT should contain requirements for the given use of the
> regulator (with a specific board) - is this correct?
That is correct.
>
>> This manual voltage setting can cause conflicts with other drivers for
>> example. Also some device can use a variable wide voltage range, and
>> some only specific narrow one, and e.g. the driver with wide range
>> would set it to voltage that isn't suitable for the narrow range
>> device, so it's much better to just specify it manually than have it
>> resolved.
> I would expect that in the case you describe, the kernel would set the
> voltage to a value which would satisfy both the ranges. I don't know
> what would happen if that was not possible (i. e. there was no
> intersection in the two requested voltage ranges), though. Or does a
> call to regulator_set_voltage set the voltage immediately taking notice
> only of the hardware contraints? I think I am having trouble
> understanding what this quote from the regulator_set_voltage
> documentation means:
>
>> If the regulator is shared between several devices then the lowest
>> request voltage that meets the system constraints will be used.
> But actually, it probably doesn't make sense that the kernel would try
> to resolve a range suitable for all calls to this function as, I assume,
> a single driver could call it multiple times with disjoint voltage
> intervals.
Well, I assume kernel has some sort of behaviour for that stuff, but I
believe setting the voltage by the device manually is discouraged in
general. It should be possible, though.
>
> Thank you for your patience.
>
> Kind regards,
> K. B.
- Markuss
^ permalink raw reply
* Re: [RESEND PATCH v6 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Dmitry Torokhov @ 2023-09-30 16:17 UTC (permalink / raw)
To: Fenglin Wu
Cc: Dmitry Baryshkov, linux-arm-msm, linux-kernel,
krzysztof.kozlowski+dt, robh+dt, agross, andersson, Konrad Dybcio,
linux-input, quic_collinsd, quic_subbaram, quic_kamalw, jestar,
Luca Weiss
In-Reply-To: <12887370-0ada-359b-8a4f-18a28495c69a@quicinc.com>
On Mon, Sep 25, 2023 at 10:54:45AM +0800, Fenglin Wu wrote:
>
>
> On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
> > > +
> > > + 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;
> > > + case SPMI_VIB_GEN2:
> > > + mask = SPMI_VIB_GEN2_DRV_MASK;
> > > + shift = SPMI_VIB_GEN2_DRV_SHIFT;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > Could you please move the switch to the previous patch? Then it would
> > be more obvious that you are just adding the SPMI_VIB_GEN2 here.
> >
> > Other than that LGTM.
>
> Sure, I can move the switch to the previous refactoring patch.
Actually, the idea of having a const "reg" or "chip", etc. structure is
to avoid this kind of runtime checks based on hardware type and instead
use common computation. I believe you need to move mask and shift into
the chip-specific structure and avoid defining hw_type.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: xpad - add PXN V900 support
From: Dmitry Torokhov @ 2023-09-30 16:29 UTC (permalink / raw)
To: Matthias Berndt
Cc: Vicki Pfau, Ismael Ferreras Morezuelas, Lyude Paul,
Matthias Benkmann, John Butler, Pierre-Loup A. Griffais,
Jonathan Frederick, linux-input, linux-kernel
In-Reply-To: <2305012.ElGaqSPkdT@fedora>
Hi Matthias,
On Fri, Sep 29, 2023 at 07:45:52PM +0200, Matthias Berndt wrote:
> Hi everybody,
>
> I recently sent this patch to the linux-input list where it was ignored, so
> now I'm sending it again to every email address that get_maintainer.pl gives
> me in the hope that it'll somehow get merged.
> This is a trivial patch that enables support for the PXN V900 steering wheel
> in the xpad driver. It's just a matter of adding the relevant USB vendorId/
> productId to the list of supported IDs. I've tried it and
I need your "Signed-off-by: ..." in order to merge this patch. Please
resend following guidelines in Documentation/process/submitting-patches.rst
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] Input: axp20x-pek - avoid needless newline removal
From: Dmitry Torokhov @ 2023-09-30 16:34 UTC (permalink / raw)
To: Justin Stitt
Cc: Chen-Yu Tsai, linux-input, linux-kernel, linux-hardening,
Kees Cook
In-Reply-To: <20230925-strncpy-drivers-input-misc-axp20x-pek-c-v2-1-ff7abe8498d6@google.com>
On Mon, Sep 25, 2023 at 04:31:05AM +0000, Justin Stitt wrote:
> This code is doing more work than it needs to.
>
> Before handing off `val_str` to `kstrtouint()` we are eagerly removing
> any trailing newline which requires copying `buf`, validating it's
> length and checking/replacing any potential newlines.
>
> kstrtouint() handles this implicitly:
> kstrtouint ->
> kstrotoull -> (documentation)
> | /**
> | * kstrtoull - convert a string to an unsigned long long
> | * @s: The start of the string. The string must be null-terminated, and may also
> | * include a single newline before its terminating null. The first character
> | ...
>
> Let's remove the redundant functionality and let kstrtouint handle it.
>
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: Annotate struct evdev_client with __counted_by
From: Dmitry Torokhov @ 2023-09-30 16:36 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Nathan Chancellor, Nick Desaulniers, Tom Rix,
linux-kernel, llvm, linux-hardening
In-Reply-To: <20230922175027.work.563-kees@kernel.org>
On Fri, Sep 22, 2023 at 10:50:28AM -0700, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
>
> As found with Coccinelle[1], add __counted_by for struct evdev_client.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: Annotate struct input_leds with __counted_by
From: Dmitry Torokhov @ 2023-09-30 16:37 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Nathan Chancellor, Nick Desaulniers, Tom Rix,
linux-kernel, llvm, linux-hardening
In-Reply-To: <20230922175031.work.467-kees@kernel.org>
On Fri, Sep 22, 2023 at 10:50:32AM -0700, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
>
> As found with Coccinelle[1], add __counted_by for struct input_leds.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: mt: Annotate struct input_mt with __counted_by
From: Dmitry Torokhov @ 2023-09-30 16:38 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Nathan Chancellor, Nick Desaulniers, Tom Rix,
linux-kernel, llvm, linux-hardening
In-Reply-To: <20230922175036.work.762-kees@kernel.org>
On Fri, Sep 22, 2023 at 10:50:37AM -0700, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
>
> As found with Coccinelle[1], add __counted_by for struct input_mt.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* [PATCH 0/7] Input: synaptics-rmi4: add quirks for third party touchscreen controllers
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly, Jason A. Donenfeld,
Matthias Schiffer, Rob Herring, Krzysztof Kozlowski, Conor Dooley
With the growing popularity of running upstream Linux on mobile devices,
we're beginning to run into more and more edgecases. The OnePlus 6 is a
fairly well supported 2018 era smartphone, selling over a million units
in it's first 22 days. With this level of popularity, it's almost
inevitable that we get third party replacement displays, and as a
result, replacement touchscreen controllers.
The OnePlus 6 shipped with an extremely usecase specific touchscreen
driver, it implemented only the bare minimum parts of the highly generic
rmi4 protocol, instead hardcoding most of the register addresses.
As a result, the third party touchscreen controllers that are often
found in replacement screens, implement only the registers that the
downstream driver reads from. They additionally have other restrictions
such as heavy penalties on unaligned reads.
This series attempts to implement the necessary workaround to support
some of these chips with the rmi4 driver. Although it's worth noting
that at the time of writing there are other unofficial controllers in
the wild that don't work even with these patches.
We have been shipping these patches in postmarketOS for the last several
months, and they are known to not cause any regressions on the OnePlus
6/6T (with the official Synaptics controller), however I don't own any
other rmi4 hardware to further validate this.
These patches were contributed by a community developer who has given
permission for me to submit them on their behalf.
---
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Vincent Huang <vincent.huang@tw.synaptics.com>
Cc: methanal <baclofen@tuta.io>
Cc: linux-input@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: phone-devel@vger.kernel.org
Cc: ~postmarketos/upstreaming@lists.sr.ht
---
Caleb Connolly (2):
dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc
Input: synaptics-rmi4 - handle duplicate/unknown PDT entries
methanal (5):
Input: synaptics-rmi4 - f12: use hardcoded values for aftermarket touch ICs
Input: synaptics-rmi4 - f55: handle zero electrode count
Input: synaptics-rmi4 - don't do unaligned reads in IRQ context
Input: synaptics-rmi4 - read product ID on aftermarket touch ICs
Input: synaptics-rmi4 - support fallback values for PDT descriptor bytes
.../devicetree/bindings/input/syna,rmi4.yaml | 10 ++
drivers/input/rmi4/rmi_driver.c | 134 ++++++++++++++++++---
drivers/input/rmi4/rmi_driver.h | 8 ++
drivers/input/rmi4/rmi_f01.c | 14 +++
drivers/input/rmi4/rmi_f12.c | 117 ++++++++++++++----
drivers/input/rmi4/rmi_f55.c | 5 +
include/linux/rmi.h | 3 +
7 files changed, 247 insertions(+), 44 deletions(-)
---
base-commit: b0d95ff7653ef6ace66a24d6c09147d0731825ce
// Caleb (they/them)
^ permalink raw reply
* [PATCH 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly, Jason A. Donenfeld,
Matthias Schiffer, Rob Herring, Krzysztof Kozlowski, Conor Dooley
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
This new property allows devices to specify some register values which
are missing on units with third party replacement displays. These
displays use unofficial touch ICs which only implement a subset of the
RMI4 specification.
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
To: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
To: Rob Herring <robh+dt@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
To: Conor Dooley <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
Documentation/devicetree/bindings/input/syna,rmi4.yaml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/syna,rmi4.yaml b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
index 4d4e1a8e36be..bd6beb67ac21 100644
--- a/Documentation/devicetree/bindings/input/syna,rmi4.yaml
+++ b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
@@ -49,6 +49,16 @@ properties:
description:
Delay to wait after powering on the device.
+ syna,pdt-fallback-desc:
+ $ref: /schemas/types.yaml#/definitions/uint8-array
+ description:
+ An array of pairs of function number and version. These are used
+ on some devices with replacement displays that use unofficial touch
+ controllers. These controllers do report the properties of their PDT
+ entries, but leave the function_number and function_version registers
+ blank. These values should match exactly the 5th and 4th bytes of each
+ PDT entry from the original display's touch controller.
+
vdd-supply: true
vio-supply: true
--
2.42.0
^ permalink raw reply related
* [PATCH 2/7] Input: synaptics-rmi4 - handle duplicate/unknown PDT entries
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
Some third party rmi4-compatible ICs don't expose their PDT entries
very well. Add a few checks to skip duplicate entries as well as entries
for unsupported functions.
This is required to support some phones with third party displays.
Validated on a stock OnePlus 6T (original parts):
manufacturer: Synaptics, product: S3706B, fw id: 2852315
Co-developed-by: methanal <baclofen@tuta.io>
Signed-off-by: methanal <baclofen@tuta.io>
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
drivers/input/rmi4/rmi_driver.c | 47 +++++++++++++++++++++++++++++++++++------
drivers/input/rmi4/rmi_driver.h | 6 ++++++
2 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 258d5fe3d395..cd3e4e77ab9b 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -493,12 +493,44 @@ static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt,
fd->function_version = pdt->function_version;
}
+static bool rmi_pdt_entry_is_valid(struct rmi_device *rmi_dev,
+ struct pdt_scan_state *state, u8 fn)
+{
+ int i;
+
+ switch (fn) {
+ case 0x01:
+ case 0x03:
+ case 0x11:
+ case 0x12:
+ case 0x30:
+ case 0x34:
+ case 0x3a:
+ case 0x54:
+ case 0x55:
+ break;
+
+ default:
+ rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev,
+ "PDT has unknown function number %#02x\n", fn);
+ return false;
+ }
+
+ for (i = 0; i < state->pdt_count; i++) {
+ if (state->pdts[i] == fn)
+ return false;
+ }
+
+ state->pdts[state->pdt_count++] = fn;
+ return true;
+}
+
#define RMI_SCAN_CONTINUE 0
#define RMI_SCAN_DONE 1
static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
int page,
- int *empty_pages,
+ struct pdt_scan_state *state,
void *ctx,
int (*callback)(struct rmi_device *rmi_dev,
void *ctx,
@@ -521,6 +553,9 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
if (RMI4_END_OF_PDT(pdt_entry.function_number))
break;
+ if (!rmi_pdt_entry_is_valid(rmi_dev, state, pdt_entry.function_number))
+ continue;
+
retval = callback(rmi_dev, ctx, &pdt_entry);
if (retval != RMI_SCAN_CONTINUE)
return retval;
@@ -531,11 +566,11 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
* or more is found, stop scanning.
*/
if (addr == pdt_start)
- ++*empty_pages;
+ ++state->empty_pages;
else
- *empty_pages = 0;
+ state->empty_pages = 0;
- return (data->bootloader_mode || *empty_pages >= 2) ?
+ return (data->bootloader_mode || state->empty_pages >= 2) ?
RMI_SCAN_DONE : RMI_SCAN_CONTINUE;
}
@@ -544,11 +579,11 @@ int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
void *ctx, const struct pdt_entry *entry))
{
int page;
- int empty_pages = 0;
+ struct pdt_scan_state state = {0, 0, {0}};
int retval = RMI_SCAN_DONE;
for (page = 0; page <= RMI4_MAX_PAGE; page++) {
- retval = rmi_scan_pdt_page(rmi_dev, page, &empty_pages,
+ retval = rmi_scan_pdt_page(rmi_dev, page, &state,
ctx, callback);
if (retval != RMI_SCAN_CONTINUE)
break;
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index 1c6c6086c0e5..e1a5412f2f8f 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -46,6 +46,12 @@ struct pdt_entry {
u8 function_number;
};
+struct pdt_scan_state {
+ u8 empty_pages;
+ u8 pdt_count;
+ u8 pdts[254];
+};
+
#define RMI_REG_DESC_PRESENSE_BITS (32 * BITS_PER_BYTE)
#define RMI_REG_DESC_SUBPACKET_BITS (37 * BITS_PER_BYTE)
--
2.42.0
^ permalink raw reply related
* [PATCH 3/7] Input: synaptics-rmi4 - f12: use hardcoded values for aftermarket touch ICs
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
From: methanal <baclofen@tuta.io>
Some replacement displays include third-party touch ICs which are
devoid of register descriptors. Create a fake data register descriptor
for such ICs and provide hardcoded default values.
It isn't possible to reliably determine if the touch IC is original or
not, so these fallback values are offered as an alternative to the error
path when register descriptors aren't available.
Signed-off-by: methanal <baclofen@tuta.io>
[changes for readability / codeflow, checkpatch fixes]
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
drivers/input/rmi4/rmi_f12.c | 117 +++++++++++++++++++++++++++++++++----------
1 file changed, 91 insertions(+), 26 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
index 7e97944f7616..719ee3b42550 100644
--- a/drivers/input/rmi4/rmi_f12.c
+++ b/drivers/input/rmi4/rmi_f12.c
@@ -195,6 +195,41 @@ static void rmi_f12_process_objects(struct f12_data *f12, u8 *data1, int size)
rmi_2d_sensor_abs_report(sensor, &sensor->objs[i], i);
}
+static void rmi_f12_set_hardcoded_desc(struct rmi_function *fn, struct f12_data *f12)
+{
+ struct rmi_2d_sensor *sensor = &f12->sensor;
+ struct rmi_register_desc_item *reg_desc;
+
+ /* We have no f12->data_reg_desc, so the pkt_size is 0, override it with
+ * a somewhat sensible default (this corresponds to 10 fingers).
+ */
+ sensor->pkt_size = 88;
+
+ /*
+ * There are no register descriptors to get these values from.
+ * We set them to high values to either be overwritten by the clip
+ * properties from devicetree, or to just not get in the way.
+ */
+ sensor->max_x = 65535;
+ sensor->max_y = 65535;
+
+ /*
+ * Create the Data1 register descriptor so that touch events
+ * can work properly.
+ */
+ reg_desc = devm_kcalloc(&fn->dev, 1,
+ sizeof(struct rmi_register_desc_item), GFP_KERNEL);
+ reg_desc->reg = 1;
+ reg_desc->reg_size = 80;
+ reg_desc->num_subpackets = 10;
+
+ f12->data1 = reg_desc;
+ f12->data1_offset = 0;
+ sensor->nbr_fingers = reg_desc->num_subpackets;
+ sensor->report_abs = 1;
+ sensor->attn_size += reg_desc->reg_size;
+}
+
static irqreturn_t rmi_f12_attention(int irq, void *ctx)
{
int retval;
@@ -315,6 +350,40 @@ static int rmi_f12_config(struct rmi_function *fn)
return 0;
}
+static int rmi_f12_sensor_init(struct rmi_function *fn, struct f12_data *f12)
+{
+ struct rmi_2d_sensor *sensor = &f12->sensor;
+
+ sensor->fn = fn;
+ f12->data_addr = fn->fd.data_base_addr;
+
+ /* On quirky devices that don't have a data_reg_desc we hardcode the packet
+ * in rmi_f12_set_hardcoded_desc(). Make sure not to set it to 0 here.
+ */
+ if (!sensor->pkt_size)
+ sensor->pkt_size = rmi_register_desc_calc_size(&f12->data_reg_desc);
+
+ sensor->axis_align =
+ f12->sensor_pdata.axis_align;
+
+ sensor->x_mm = f12->sensor_pdata.x_mm;
+ sensor->y_mm = f12->sensor_pdata.y_mm;
+ sensor->dribble = f12->sensor_pdata.dribble;
+
+ if (sensor->sensor_type == rmi_sensor_default)
+ sensor->sensor_type =
+ f12->sensor_pdata.sensor_type;
+
+ rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s: data packet size: %d\n", __func__,
+ sensor->pkt_size);
+
+ sensor->data_pkt = devm_kzalloc(&fn->dev, sensor->pkt_size, GFP_KERNEL);
+ if (!sensor->data_pkt)
+ return -ENOMEM;
+
+ return 0;
+}
+
static int rmi_f12_probe(struct rmi_function *fn)
{
struct f12_data *f12;
@@ -328,6 +397,7 @@ static int rmi_f12_probe(struct rmi_function *fn)
struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
u16 data_offset = 0;
int mask_size;
+ bool hardcoded_desc_quirk = false;
rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s\n", __func__);
@@ -342,9 +412,9 @@ static int rmi_f12_probe(struct rmi_function *fn)
++query_addr;
if (!(buf & BIT(0))) {
- dev_err(&fn->dev,
- "Behavior of F12 without register descriptors is undefined.\n");
- return -ENODEV;
+ rmi_dbg(RMI_DEBUG_FN, &fn->dev,
+ "No register descriptors defined for F12, using fallback\n");
+ hardcoded_desc_quirk = true;
}
f12 = devm_kzalloc(&fn->dev, sizeof(struct f12_data) + mask_size * 2,
@@ -352,6 +422,8 @@ static int rmi_f12_probe(struct rmi_function *fn)
if (!f12)
return -ENOMEM;
+ dev_set_drvdata(&fn->dev, f12);
+
f12->abs_mask = (unsigned long *)((char *)f12
+ sizeof(struct f12_data));
f12->rel_mask = (unsigned long *)((char *)f12
@@ -370,6 +442,18 @@ static int rmi_f12_probe(struct rmi_function *fn)
f12->sensor_pdata = pdata->sensor_pdata;
}
+ sensor = &f12->sensor;
+
+ if (hardcoded_desc_quirk) {
+ rmi_f12_set_hardcoded_desc(fn, f12);
+
+ ret = rmi_f12_sensor_init(fn, f12);
+ if (ret)
+ return ret;
+
+ goto skip_register_desc;
+ }
+
ret = rmi_read_register_desc(rmi_dev, query_addr,
&f12->query_reg_desc);
if (ret) {
@@ -400,29 +484,9 @@ static int rmi_f12_probe(struct rmi_function *fn)
}
query_addr += 3;
- sensor = &f12->sensor;
- sensor->fn = fn;
- f12->data_addr = fn->fd.data_base_addr;
- sensor->pkt_size = rmi_register_desc_calc_size(&f12->data_reg_desc);
-
- sensor->axis_align =
- f12->sensor_pdata.axis_align;
-
- sensor->x_mm = f12->sensor_pdata.x_mm;
- sensor->y_mm = f12->sensor_pdata.y_mm;
- sensor->dribble = f12->sensor_pdata.dribble;
-
- if (sensor->sensor_type == rmi_sensor_default)
- sensor->sensor_type =
- f12->sensor_pdata.sensor_type;
-
- rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s: data packet size: %d\n", __func__,
- sensor->pkt_size);
- sensor->data_pkt = devm_kzalloc(&fn->dev, sensor->pkt_size, GFP_KERNEL);
- if (!sensor->data_pkt)
- return -ENOMEM;
-
- dev_set_drvdata(&fn->dev, f12);
+ ret = rmi_f12_sensor_init(fn, f12);
+ if (ret)
+ return ret;
ret = rmi_f12_read_sensor_tuning(f12);
if (ret)
@@ -520,6 +584,7 @@ static int rmi_f12_probe(struct rmi_function *fn)
data_offset += item->reg_size;
}
+skip_register_desc:
/* allocate the in-kernel tracking buffers */
sensor->tracking_pos = devm_kcalloc(&fn->dev,
sensor->nbr_fingers, sizeof(struct input_mt_pos),
--
2.42.0
^ permalink raw reply related
* [PATCH 5/7] Input: synaptics-rmi4 - don't do unaligned reads in IRQ context
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
From: methanal <baclofen@tuta.io>
Some replacement displays include third-party touch ICs which incur a
significant penalty (1-2 seconds) when doing certain unaligned reads.
This is enough to break functionality when it happens in the hot path,
so adjust the interrupt handler to not read from an unaligned address.
Signed-off-by: methanal <baclofen@tuta.io>
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
This solution does wind up doing the unaligned reads on the CPU instead,
although I'm not sure how significant of a penalty this is in practise.
---
drivers/input/rmi4/rmi_driver.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index cd3e4e77ab9b..58bf3dfbf81c 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -136,9 +136,14 @@ static int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
return 0;
if (!data->attn_data.data) {
+ /*
+ * Read the device status register as well and ignore it.
+ * Some aftermarket ICs have issues with interrupt requests
+ * otherwise.
+ */
error = rmi_read_block(rmi_dev,
- data->f01_container->fd.data_base_addr + 1,
- data->irq_status, data->num_of_irq_regs);
+ data->f01_container->fd.data_base_addr,
+ data->irq_status - 1, data->num_of_irq_regs + 1);
if (error < 0) {
dev_err(dev, "Failed to read irqs, code=%d\n", error);
return error;
@@ -1083,16 +1088,17 @@ int rmi_probe_interrupts(struct rmi_driver_data *data)
data->num_of_irq_regs = (data->irq_count + 7) / 8;
size = BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long);
- data->irq_memory = devm_kcalloc(dev, size, 4, GFP_KERNEL);
+ data->irq_memory = devm_kzalloc(dev, size * 4 + 1, GFP_KERNEL);
if (!data->irq_memory) {
dev_err(dev, "Failed to allocate memory for irq masks.\n");
return -ENOMEM;
}
- data->irq_status = data->irq_memory + size * 0;
- data->fn_irq_bits = data->irq_memory + size * 1;
- data->current_irq_mask = data->irq_memory + size * 2;
- data->new_irq_mask = data->irq_memory + size * 3;
+ /* The first byte is reserved for the device status register */
+ data->irq_status = data->irq_memory + size * 0 + 1;
+ data->fn_irq_bits = data->irq_memory + size * 1 + 1;
+ data->current_irq_mask = data->irq_memory + size * 2 + 1;
+ data->new_irq_mask = data->irq_memory + size * 3 + 1;
return retval;
}
--
2.42.0
^ permalink raw reply related
* [PATCH 4/7] Input: synaptics-rmi4 - f55: handle zero electrode count
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
From: methanal <baclofen@tuta.io>
Some third party ICs claim to support f55 but report an electrode count
of 0. Catch this and bail out early so that we don't confuse the i2c bus
with 0 sized reads.
Signed-off-by: methanal <baclofen@tuta.io>
[simplify code, adjust wording]
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
drivers/input/rmi4/rmi_f55.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
index 488adaca4dd0..ad2ef14ae9f4 100644
--- a/drivers/input/rmi4/rmi_f55.c
+++ b/drivers/input/rmi4/rmi_f55.c
@@ -52,6 +52,11 @@ static int rmi_f55_detect(struct rmi_function *fn)
f55->num_rx_electrodes = f55->qry[F55_NUM_RX_OFFSET];
f55->num_tx_electrodes = f55->qry[F55_NUM_TX_OFFSET];
+ if (!f55->num_rx_electrodes || !f55->num_tx_electrodes) {
+ rmi_dbg(RMI_DEBUG_FN, &fn->dev,
+ "F55 query returned no electrodes, giving up\n");
+ return 0;
+ }
f55->cfg_num_rx_electrodes = f55->num_rx_electrodes;
f55->cfg_num_tx_electrodes = f55->num_rx_electrodes;
--
2.42.0
^ permalink raw reply related
* [PATCH 6/7] Input: synaptics-rmi4 - read product ID on aftermarket touch ICs
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
From: methanal <baclofen@tuta.io>
Some replacement displays include third-party touch ICs which do not
report the product ID correctly unless we read directly from the
product ID register. Add a check and a fallback read to handle this.
Signed-off-by: methanal <baclofen@tuta.io>
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
drivers/input/rmi4/rmi_f01.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index d7603c50f864..4aee30d2dcde 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -250,6 +250,20 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
}
}
+ /*
+ * Some aftermarket ICs put garbage into the product id field unless
+ * we read directly from the product id register.
+ */
+ if (props->product_id[0] < 0x20) {
+ ret = rmi_read_block(rmi_dev, query_base_addr + 11,
+ props->product_id, RMI_PRODUCT_ID_LENGTH);
+ if (ret) {
+ dev_err(&rmi_dev->dev,
+ "Failed to read product id: %d\n", ret);
+ return ret;
+ }
+ }
+
return 0;
}
--
2.42.0
^ permalink raw reply related
* [PATCH 7/7] Input: synaptics-rmi4 - support fallback values for PDT descriptor bytes
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
From: methanal <baclofen@tuta.io>
Some replacement displays include third-party touch ICs which do not
expose the function number and the interrupt status in its PDT entries.
OnePlus 6 (original touch IC)
rmi4_i2c 12-0020: read 6 bytes at 0x00e3: 0 (2b 22 0d 06 01 01)
OnePlus 6 (aftermarket touch IC)
rmi4_i2c 12-0020: read 6 bytes at 0x00e3: 0 (2c 23 0d 06 00 00)
Introduce a new devicetree property `syna,pdt-desc` which can be used to
provide platform-specific fallback values for users with a replacement
display with an aftermarket touch IC.
Signed-off-by: methanal <baclofen@tuta.io>
[codeflow adjustments, checkpatch fixes, wording]
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
drivers/input/rmi4/rmi_driver.c | 67 ++++++++++++++++++++++++++++++++++++++---
drivers/input/rmi4/rmi_driver.h | 2 ++
include/linux/rmi.h | 3 ++
3 files changed, 67 insertions(+), 5 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 58bf3dfbf81c..5e1e3d5dd800 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -461,9 +461,10 @@ static int rmi_driver_reset_handler(struct rmi_device *rmi_dev)
return 0;
}
-static int rmi_read_pdt_entry(struct rmi_device *rmi_dev,
- struct pdt_entry *entry, u16 pdt_address)
+static int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
+ struct pdt_scan_state *state, u16 pdt_address)
{
+ const struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
u8 buf[RMI_PDT_ENTRY_SIZE];
int error;
@@ -474,6 +475,21 @@ static int rmi_read_pdt_entry(struct rmi_device *rmi_dev,
return error;
}
+ if (pdata->pdt_fallback_size > state->pdt_count * RMI_OF_PDT_DESC_CELLS + 1) {
+ /* Use the description bytes from DT */
+ buf[5] = pdata->pdt_fallback_desc[state->pdt_count * RMI_OF_PDT_DESC_CELLS];
+ buf[4] = pdata->pdt_fallback_desc[state->pdt_count * RMI_OF_PDT_DESC_CELLS + 1];
+
+ error = rmi_read_block(rmi_dev, pdt_address, buf,
+ RMI_PDT_ENTRY_SIZE - 2);
+ if (error) {
+ dev_err(&rmi_dev->dev,
+ "Read PDT entry at %#06x failed, code: %d.\n",
+ pdt_address, error);
+ return error;
+ }
+ }
+
entry->page_start = pdt_address & RMI4_PAGE_MASK;
entry->query_base_addr = buf[0];
entry->command_base_addr = buf[1];
@@ -551,7 +567,7 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
int retval;
for (addr = pdt_start; addr >= pdt_end; addr -= RMI_PDT_ENTRY_SIZE) {
- error = rmi_read_pdt_entry(rmi_dev, &pdt_entry, addr);
+ error = rmi_read_pdt_entry(rmi_dev, &pdt_entry, state, addr);
if (error)
return error;
@@ -1028,9 +1044,11 @@ static int rmi_driver_remove(struct device *dev)
}
#ifdef CONFIG_OF
-static int rmi_driver_of_probe(struct device *dev,
+static int rmi_driver_of_probe(struct rmi_device *rmi_dev,
struct rmi_device_platform_data *pdata)
{
+ struct device *dev = rmi_dev->xport->dev;
+ u8 buf[RMI_PDT_ENTRY_SIZE];
int retval;
retval = rmi_of_property_read_u32(dev, &pdata->reset_delay_ms,
@@ -1038,6 +1056,45 @@ static int rmi_driver_of_probe(struct device *dev,
if (retval)
return retval;
+ /*
+ * In some aftermerket touch ICs, the first PDT entry is empty and
+ * the function number register is 0. If so, the platform
+ * may have provided backup PDT entries in the device tree.
+ */
+
+ retval = rmi_read_block(rmi_dev, PDT_START_SCAN_LOCATION,
+ buf, RMI_PDT_ENTRY_SIZE);
+ if (retval) {
+ dev_err(dev, "Read PDT entry at %#06x failed, code: %d.\n",
+ PDT_START_SCAN_LOCATION, retval);
+ return retval;
+ }
+
+ if (!RMI4_END_OF_PDT(buf[5]))
+ return 0;
+
+ pdata->pdt_fallback_size = of_property_count_u8_elems(dev->of_node,
+ "syna,pdt-fallback-desc");
+
+ /* The rmi4 driver would fail later anyway, so just error out now. */
+ if (pdata->pdt_fallback_size == -EINVAL) {
+ dev_err(dev, "First PDT entry is empty and no backup values provided\n");
+ return -EINVAL;
+ }
+
+ if (pdata->pdt_fallback_size < 0) {
+ dev_err(dev, "syna,ptd-desc property was not specified properly: %d\n",
+ pdata->pdt_fallback_size);
+ return pdata->pdt_fallback_size;
+ }
+
+ pdata->pdt_fallback_desc = devm_kzalloc(dev, pdata->pdt_fallback_size, GFP_KERNEL);
+
+ retval = of_property_read_u8_array(dev->of_node, "syna,pdt-fallback-desc",
+ pdata->pdt_fallback_desc, pdata->pdt_fallback_size);
+ if (retval < 0)
+ return retval;
+
return 0;
}
#else
@@ -1163,7 +1220,7 @@ static int rmi_driver_probe(struct device *dev)
pdata = rmi_get_platform_data(rmi_dev);
if (rmi_dev->xport->dev->of_node) {
- retval = rmi_driver_of_probe(rmi_dev->xport->dev, pdata);
+ retval = rmi_driver_of_probe(rmi_dev, pdata);
if (retval)
return retval;
}
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index e1a5412f2f8f..2531c32d6163 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -31,6 +31,8 @@
#define RMI_PDT_FUNCTION_VERSION_MASK 0x60
#define RMI_PDT_INT_SOURCE_COUNT_MASK 0x07
+#define RMI_OF_PDT_DESC_CELLS 2
+
#define PDT_START_SCAN_LOCATION 0x00e9
#define PDT_END_SCAN_LOCATION 0x0005
#define RMI4_END_OF_PDT(id) ((id) == 0x00 || (id) == 0xff)
diff --git a/include/linux/rmi.h b/include/linux/rmi.h
index ab7eea01ab42..974597960b5e 100644
--- a/include/linux/rmi.h
+++ b/include/linux/rmi.h
@@ -214,6 +214,9 @@ struct rmi_device_platform_data {
int reset_delay_ms;
int irq;
+ u8 *pdt_fallback_desc;
+ int pdt_fallback_size;
+
struct rmi_device_platform_data_spi spi_data;
/* function handler pdata */
--
2.42.0
^ 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