* Re: [RESEND PATCH v6 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu @ 2023-10-09 4:01 UTC (permalink / raw)
To: Dmitry Torokhov
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: <ZRhKAWYBLcBZHc73@google.com>
On 10/1/2023 12:17 AM, Dmitry Torokhov wrote:
> 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.
>
Actually, the main motivation for adding 'hw_type' is to avoid reading
'reg_base' from DT for SSBI_VIB. It can also help to simplify the
'pm8xxx_vib_data' structure and make following code logic more
straightforward and easier to understand(check hw_type instead of
checking specific constant reg/mask value), it has been used in
following places:
1) Avoid reading 'reg_base' from DT for SSBI_VIB.
2) Only do manual-mode-mask-write for SSBI_VIB. Previously, it was
achieved by giving a valid 'drv_en_manual_mask' value only for SSBI_VIB,
having hw_type make it more straightforward.
3) Not writing VIB_EN register for SSBI_VIB. A similar strategy was
used previously, only write VIB_EN register when 'enable_mask' is valid,
checking hw_type make it more straightforward.
4) To achieve different drive step size for SPMI_VIB （100mV per step)
and SPMI_VIB_GEN2 (1mV per step).
5) Do different VIB_DRV mask and shift assignment for SPMI_VIB and
SPMI_VIB_GEN2
6) Only write VIB_DRV2 for SPMI_VIB_GEN2.
> Thanks.
>
^ permalink raw reply
* Re: [PATCH v2 01/16] platform/x86/amd/pmf: Add PMF TEE interface
From: Shyam Sundar S K @ 2023-10-09 4:58 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <584aebd-5c89-d085-275-bb79f93429f@linux.intel.com>
On 10/4/2023 4:20 PM, Ilpo Järvinen wrote:
> On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
>
>> AMD PMF driver loads the PMF TA (Trusted Application) into the AMD
>> ASP's (AMD Security Processor) TEE (Trusted Execution Environment).
>>
>> PMF Trusted Application is a secured firmware placed under
>> /lib/firmware/amdtee gets loaded only when the TEE environment is
>> initialized. Add the initial code path to build these pipes.
>>
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>> drivers/platform/x86/amd/pmf/Makefile | 3 +-
>> drivers/platform/x86/amd/pmf/core.c | 11 ++-
>> drivers/platform/x86/amd/pmf/pmf.h | 16 ++++
>> drivers/platform/x86/amd/pmf/tee-if.c | 112 ++++++++++++++++++++++++++
>> 4 files changed, 138 insertions(+), 4 deletions(-)
>> create mode 100644 drivers/platform/x86/amd/pmf/tee-if.c
>>
>> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
>> index fdededf54392..d2746ee7369f 100644
>> --- a/drivers/platform/x86/amd/pmf/Makefile
>> +++ b/drivers/platform/x86/amd/pmf/Makefile
>> @@ -6,4 +6,5 @@
>>
>> obj-$(CONFIG_AMD_PMF) += amd-pmf.o
>> amd-pmf-objs := core.o acpi.o sps.o \
>> - auto-mode.o cnqf.o
>> + auto-mode.o cnqf.o \
>> + tee-if.o
>> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
>> index 78ed3ee22555..68f1389dda3e 100644
>> --- a/drivers/platform/x86/amd/pmf/core.c
>> +++ b/drivers/platform/x86/amd/pmf/core.c
>> @@ -309,8 +309,11 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev)
>> dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n");
>> }
>>
>> - /* Enable Auto Mode */
>> - if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
>> + if (amd_pmf_init_smart_pc(dev)) {
>> + /* Enable Smart PC Solution builder */
>> + dev_dbg(dev->dev, "Smart PC Solution Enabled\n");
>> + } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
>> + /* Enable Auto Mode */
>
> I'm pretty certain neither of these two comments add any information to
> what's readily visible from the code itself so they can be dropped.
>
>> amd_pmf_init_auto_mode(dev);
>> dev_dbg(dev->dev, "Auto Mode Init done\n");
>> } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
>> @@ -330,7 +333,9 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev)
>> amd_pmf_deinit_sps(dev);
>> }
>>
>> - if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
>> + if (dev->smart_pc_enabled) {
>> + amd_pmf_deinit_smart_pc(dev);
>> + } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
>> amd_pmf_deinit_auto_mode(dev);
>> } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
>> is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index deba88e6e4c8..02460c2a31ea 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -179,6 +179,12 @@ struct amd_pmf_dev {
>> bool cnqf_enabled;
>> bool cnqf_supported;
>> struct notifier_block pwr_src_notifier;
>> + /* Smart PC solution builder */
>> + struct tee_context *tee_ctx;
>> + struct tee_shm *fw_shm_pool;
>> + u32 session_id;
>> + void *shbuf;
>> + bool smart_pc_enabled;
>> };
>>
>> struct apmf_sps_prop_granular {
>> @@ -389,6 +395,13 @@ struct apmf_dyn_slider_output {
>> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
>> } __packed;
>>
>> +struct ta_pmf_shared_memory {
>> + int command_id;
>> + int resp_id;
>> + u32 pmf_result;
>> + u32 if_version;
>> +};
>> +
>> /* Core Layer */
>> int apmf_acpi_init(struct amd_pmf_dev *pmf_dev);
>> void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev);
>> @@ -433,4 +446,7 @@ void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev);
>> int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
>> extern const struct attribute_group cnqf_feature_attribute_group;
>>
>> +/* Smart PC builder Layer*/
>
> Missing space.
>
>> +int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
>> +void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
>> #endif /* PMF_H */
>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>> new file mode 100644
>> index 000000000000..4db80ca59a11
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>> @@ -0,0 +1,112 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD Platform Management Framework Driver - TEE Interface
>> + *
>> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
>> + * All Rights Reserved.
>> + *
>> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> + */
>> +
>> +#include <linux/tee_drv.h>
>> +#include <linux/uuid.h>
>> +#include "pmf.h"
>> +
>> +#define MAX_TEE_PARAM 4
>> +static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
>> + 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
>> +
>> +static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
>> +{
>> + return ver->impl_id == TEE_IMPL_ID_AMDTEE;
>> +}
>> +
>> +static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id)
>> +{
>> + struct tee_ioctl_open_session_arg sess_arg = {};
>> + int rc;
>> +
>> + export_uuid(sess_arg.uuid, &amd_pmf_ta_uuid);
>> + sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
>> + sess_arg.num_params = 0;
>> +
>> + rc = tee_client_open_session(ctx, &sess_arg, NULL);
>> + if (rc < 0 || sess_arg.ret != 0) {
>> + pr_err("Failed to open TEE session err:%#x, ret:%#x\n", sess_arg.ret, rc);
>
> Print normal -Exx error codes as %d, not %x (rc). I don't know what would
> be best to do with sess_arg.ret, TEEC_ERROR_* look like errnos (negative
> values) manually converted into u32.
in drivers/tee/amdtee/amdtee_private.h, all the TEEC_* are hex. So
sess_arg.ret can remain %x? rc I have changed to %d.
Rest all I will address in v3.
Thanks,
Shyam
>
>> + rc = -EINVAL;
>
> If rc < 0, I think you should just pass the error code on.
>
>> + } else {
>> + *id = sess_arg.session;
>> + }
>> +
>> + return rc;
>> +}
>> +
>> +static int amd_pmf_tee_init(struct amd_pmf_dev *dev)
>> +{
>> + int ret;
>> + u32 size;
>> +
>> + /* Open context with TEE driver */
>
> Too obvious comment to stay, it's what the code already says on the next
> line so there's little point to repeat something this obvious in the
> comments.
>
>> + dev->tee_ctx = tee_client_open_context(NULL, amd_pmf_amdtee_ta_match, NULL, NULL);
>> + if (IS_ERR(dev->tee_ctx)) {
>> + dev_err(dev->dev, "Failed to open TEE context\n");
>> + return PTR_ERR(dev->tee_ctx);
>> + }
>> +
>> + /* Open session with PMF Trusted App */
>
> Remove this one too.
>
>> + ret = amd_pmf_ta_open_session(dev->tee_ctx, &dev->session_id);
>> + if (ret) {
>> + dev_err(dev->dev, "Failed to open TA session (%d)\n", ret);
>> + ret = -EINVAL;
>> + goto out_ctx;
>> + }
>> +
>> + size = sizeof(struct ta_pmf_shared_memory);
>> + dev->fw_shm_pool = tee_shm_alloc_kernel_buf(dev->tee_ctx, size);
>> + if (IS_ERR(dev->fw_shm_pool)) {
>> + dev_err(dev->dev, "Failed to alloc TEE shared memory\n");
>> + ret = PTR_ERR(dev->fw_shm_pool);
>> + goto out_sess;
>> + }
>> +
>> + dev->shbuf = tee_shm_get_va(dev->fw_shm_pool, 0);
>> + if (IS_ERR(dev->shbuf)) {
>> + dev_err(dev->dev, "Failed to get TEE virtual address\n");
>> + ret = PTR_ERR(dev->shbuf);
>> + goto out_shm;
>> + }
>> + dev_dbg(dev->dev, "TEE init done\n");
>> +
>> + return 0;
>> +
>> +out_shm:
>> + tee_shm_free(dev->fw_shm_pool);
>> +out_sess:
>> + tee_client_close_session(dev->tee_ctx, dev->session_id);
>> +out_ctx:
>> + tee_client_close_context(dev->tee_ctx);
>> +
>> + return ret;
>> +}
>> +
>> +static void amd_pmf_tee_deinit(struct amd_pmf_dev *dev)
>> +{
>> + /* Free the shared memory pool */
>> + tee_shm_free(dev->fw_shm_pool);
>> +
>> + /* close the existing session with PMF TA*/
>
> Missing space.
>
>> + tee_client_close_session(dev->tee_ctx, dev->session_id);
>> +
>> + /* close the context with TEE driver */
>> + tee_client_close_context(dev->tee_ctx);
>> +}
>> +
>> +int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>> +{
>> + return amd_pmf_tee_init(dev);
>> +}
>> +
>> +void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev)
>> +{
>> + amd_pmf_tee_deinit(dev);
>> +}
>>
>
^ permalink raw reply
* Re: [PATCH v2 04/16] platform/x86/amd/pmf: Add support for PMF Policy Binary
From: Shyam Sundar S K @ 2023-10-09 5:15 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <d05080a8-1d9f-f36c-6569-f81a94258f7a@linux.intel.com>
On 10/4/2023 5:30 PM, Ilpo Järvinen wrote:
> On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
>
>> PMF Policy binary is a encrypted and signed binary that will be part
>> of the BIOS. PMF driver via the ACPI interface checks the existence
>> of Smart PC bit. If the advertised bit is found, PMF driver walks
>> the acpi namespace to find out the policy binary size and the address
>> which has to be passed to the TA during the TA init sequence.
>>
>> The policy binary is comprised of inputs (or the events) and outputs
>> (or the actions). With the PMF ecosystem, OEMs generate the policy
>> binary (or could be multiple binaries) that contains a supported set
>> of inputs and outputs which could be specifically carved out for each
>> usage segment (or for each user also) that could influence the system
>> behavior either by enriching the user experience or/and boost/throttle
>> power limits.
>>
>> Once the TA init command succeeds, the PMF driver sends the changing
>> events in the current environment to the TA for a constant sampling
>> frequency time (the event here could be a lid close or open) and
>> if the policy binary has corresponding action built within it, the
>> TA sends the action for it in the subsequent enact command.
>>
>> If the inputs sent to the TA has no output defined in the policy
>> binary generated by OEMs, there will be no action to be performed
>> by the PMF driver.
>>
>> Example policies:
>>
>> 1) if slider is performance ; set the SPL to 40W
>> Here PMF driver registers with the platform profile interface and
>> when the slider position is changed, PMF driver lets the TA know
>> about this. TA sends back an action to update the Sustained
>> Power Limit (SPL). PMF driver updates this limit via the PMFW mailbox.
>>
>> 2) if user_away ; then lock the system
>> Here PMF driver hooks to the AMD SFH driver to know the user presence
>> and send the inputs to TA and if the condition is met, the TA sends
>> the action of locking the system. PMF driver generates a uevent and
>> based on the udev rule in the userland the system gets locked with
>> systemctl.
>>
>> The intent here is to provide the OEM's to make a policy to lock the
>> system when the user is away ; but the userland can make a choice to
>> ignore it.
>>
>> and so on.
>>
>> The OEMs will have an utility to create numerous such policies and
>> the policies shall be reviewed by AMD before signing and encrypting
>> them. Policies are shared between operating systems to have seemless user
>> experience.
>>
>> Since all this action has to happen via the "amdtee" driver, currently
>> there is no caller for it in the kernel which can load the amdtee driver.
>> Without amdtee driver loading onto the system the "tee" calls shall fail
>> from the PMF driver. Hence an explicit "request_module" has been added
>> to address this.
>>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>> drivers/platform/x86/amd/pmf/Kconfig | 1 +
>> drivers/platform/x86/amd/pmf/acpi.c | 37 +++++++
>> drivers/platform/x86/amd/pmf/core.c | 12 +++
>> drivers/platform/x86/amd/pmf/pmf.h | 135 ++++++++++++++++++++++++
>> drivers/platform/x86/amd/pmf/tee-if.c | 141 +++++++++++++++++++++++++-
>> 5 files changed, 324 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
>> index 3064bc8ea167..437b78c6d1c5 100644
>> --- a/drivers/platform/x86/amd/pmf/Kconfig
>> +++ b/drivers/platform/x86/amd/pmf/Kconfig
>> @@ -9,6 +9,7 @@ config AMD_PMF
>> depends on POWER_SUPPLY
>> depends on AMD_NB
>> select ACPI_PLATFORM_PROFILE
>> + depends on AMDTEE
>> help
>> This driver provides support for the AMD Platform Management Framework.
>> The goal is to enhance end user experience by making AMD PCs smarter,
>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>> index 3fc5e4547d9f..d0512af2cd42 100644
>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>> @@ -286,6 +286,43 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
>> return 0;
>> }
>>
>> +static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
>> +{
>> + struct amd_pmf_dev *dev = data;
>> +
>> + switch (res->type) {
>> + case ACPI_RESOURCE_TYPE_ADDRESS64:
>> + dev->policy_addr = res->data.address64.address.minimum;
>> + dev->policy_sz = res->data.address64.address.address_length;
>> + break;
>> + case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
>> + dev->policy_addr = res->data.fixed_memory32.address;
>> + dev->policy_sz = res->data.fixed_memory32.address_length;
>> + break;
>> + }
>> +
>> + if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
>> + pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
>> + return AE_ERROR;
>> + }
>> +
>> + return AE_OK;
>> +}
>> +
>> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>> +{
>> + acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>> + acpi_status status;
>> +
>> + status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
>> + if (ACPI_FAILURE(status)) {
>> + dev_err(pmf_dev->dev, "acpi_walk_resources failed\n");
>> + return status;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
>> {
>> acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
>> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
>> index 678dce4fea08..787f25511191 100644
>> --- a/drivers/platform/x86/amd/pmf/core.c
>> +++ b/drivers/platform/x86/amd/pmf/core.c
>> @@ -384,6 +384,18 @@ static int amd_pmf_probe(struct platform_device *pdev)
>> return -ENOMEM;
>>
>> dev->dev = &pdev->dev;
>> + err = apmf_check_smart_pc(dev);
>> + if (!err) {
>> + /* in order for Smart PC solution to work it has a hard dependency
>> + * on the amdtee driver to be loaded first even before the PMF driver
>> + * loads. PMF ASL has a _CRS method that advertises the existence
>> + * of Smart PC bit. If this information is present, use this to
>> + * explicitly probe the amdtee driver, so that "tee" plumbing is done
>> + * before the PMF Smart PC init happens.
>> + */
>
> But please follow no-text on /* line formatting for multiline comments.
> Also start with a capital letter.
>
>
>> + if (request_module("amdtee"))
>
> Are you aware that this won't give you very strong guarantees about
> anything if request_module()'s function comments is to be believed?
>
> If that's all what you're after, MODULE_SOFTDEP("pre: amdtee"); is
> probably enough (and I unfortunately don't know the answer how to do it if
> you want something stronger than that when you don't directly depend on
> the symbols of the other module).
MODULE_SOFTDEP("pre: amdtee"); did not help.
There is no consumer loading the 'amdtee' driver today in the kernel.
Even now with this change, the pmf driver calls the TEE subsystem APIs
that will eventually land in amdtee code.
So the call flow would be:
pmf driver-> tee subsystem -> amdtee driver -> ASP
IMO, in order to make this link work request_module() would be
required. Is that OK to retain request_module() in v3?
>
>> + pr_err("Failed to load amdtee. PMF Smart PC not enabled!\n");
>> + }
>>
>> rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
>> if (!rdev || !pci_match_id(pmf_pci_ids, rdev)) {
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index 3930b8ed8333..6f4b6f4ecee4 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -14,6 +14,11 @@
>> #include <linux/acpi.h>
>> #include <linux/platform_profile.h>
>>
>> +#define POLICY_BUF_MAX_SZ 0x4b000
>> +#define POLICY_SIGN_COOKIE 0x31535024
>> +#define POLICY_COOKIE_OFFSET 0x10
>> +#define POLICY_COOKIE_LEN 0x14
>> +
>> /* APMF Functions */
>> #define APMF_FUNC_VERIFY_INTERFACE 0
>> #define APMF_FUNC_GET_SYS_PARAMS 1
>> @@ -59,8 +64,20 @@
>> #define ARG_NONE 0
>> #define AVG_SAMPLE_SIZE 3
>>
>> +/* Policy Actions */
>> +#define PMF_POLICY_SPL 2
>> +#define PMF_POLICY_SPPT 3
>> +#define PMF_POLICY_FPPT 4
>> +#define PMF_POLICY_SPPT_APU_ONLY 5
>> +#define PMF_POLICY_STT_MIN 6
>> +#define PMF_POLICY_STT_SKINTEMP_APU 7
>> +#define PMF_POLICY_STT_SKINTEMP_HS2 8
>> +
>> /* TA macros */
>> #define PMF_TA_IF_VERSION_MAJOR 1
>> +#define TA_PMF_ACTION_MAX 32
>> +#define TA_PMF_UNDO_MAX 8
>> +#define MAX_OPERATION_PARAMS 4
>>
>> /* AMD PMF BIOS interfaces */
>> struct apmf_verify_interface {
>> @@ -183,11 +200,16 @@ struct amd_pmf_dev {
>> bool cnqf_supported;
>> struct notifier_block pwr_src_notifier;
>> /* Smart PC solution builder */
>> + unsigned char *policy_buf;
>> + u32 policy_sz;
>> struct tee_context *tee_ctx;
>> struct tee_shm *fw_shm_pool;
>> u32 session_id;
>> void *shbuf;
>> struct delayed_work pb_work;
>> + struct pmf_action_table *prev_data;
>> + u64 policy_addr;
>> + void *policy_base;
>> bool smart_pc_enabled;
>> };
>>
>> @@ -399,17 +421,129 @@ struct apmf_dyn_slider_output {
>> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
>> } __packed;
>>
>> +/* Smart PC - TA internals */
>> +enum ta_slider {
>> + TA_BEST_BATTERY, /* Best Battery */
>> + TA_BETTER_BATTERY, /* Better Battery */
>> + TA_BETTER_PERFORMANCE, /* Better Performance */
>> + TA_BEST_PERFORMANCE, /* Best Performance */
>
> Align comments.
>
>> + TA_MAX,
>> +};
>> +
>> /* cmd ids for TA communication */
>> enum ta_pmf_command {
>> TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE,
>> TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES,
>> };
>>
>> +enum ta_pmf_error_type {
>> + TA_PMF_TYPE_SUCCESS,
>> + TA_PMF_ERROR_TYPE_GENERIC,
>> + TA_PMF_ERROR_TYPE_CRYPTO,
>> + TA_PMF_ERROR_TYPE_CRYPTO_VALIDATE,
>> + TA_PMF_ERROR_TYPE_CRYPTO_VERIFY_OEM,
>> + TA_PMF_ERROR_TYPE_POLICY_BUILDER,
>> + TA_PMF_ERROR_TYPE_PB_CONVERT,
>> + TA_PMF_ERROR_TYPE_PB_SETUP,
>> + TA_PMF_ERROR_TYPE_PB_ENACT,
>> + TA_PMF_ERROR_TYPE_ASD_GET_DEVICE_INFO,
>> + TA_PMF_ERROR_TYPE_ASD_GET_DEVICE_PCIE_INFO,
>> + TA_PMF_ERROR_TYPE_SYS_DRV_FW_VALIDATION,
>> + TA_PMF_ERROR_TYPE_MAX,
>> +};
>> +
>> +struct pmf_action_table {
>> + unsigned long spl; /* in mW */
>> + unsigned long sppt; /* in mW */
>> + unsigned long sppt_apuonly; /* in mW */
>> + unsigned long fppt; /* in mW */
>> + unsigned long stt_minlimit; /* in mW */
>> + unsigned long stt_skintemp_apu; /* in C */
>> + unsigned long stt_skintemp_hs2; /* in C */
>
> Ditto.
>
>> +};
>> +
>> +/* Input conditions */
>> +struct ta_pmf_condition_info {
>> + u32 power_source;
>> + u32 bat_percentage;
>> + u32 power_slider;
>> + u32 lid_state;
>> + bool user_present;
>> + u32 rsvd1[2];
>> + u32 monitor_count;
>> + u32 rsvd2[2];
>> + u32 bat_design;
>> + u32 full_charge_capacity;
>> + int drain_rate;
>> + bool user_engaged;
>> + u32 device_state;
>> + u32 socket_power;
>> + u32 skin_temperature;
>> + u32 rsvd3[5];
>> + u32 ambient_light;
>> + u32 length;
>> + u32 avg_c0residency;
>> + u32 max_c0residency;
>> + u32 s0i3_entry;
>> + u32 gfx_busy;
>> + u32 rsvd4[7];
>> + bool camera_state;
>> + u32 workload_type;
>> + u32 display_type;
>> + u32 display_state;
>> + u32 rsvd5[150];
>> +};
>> +
>> +struct ta_pmf_load_policy_table {
>> + u32 table_size;
>> + u8 table[POLICY_BUF_MAX_SZ];
>> +};
>> +
>> +/* TA initialization params */
>> +struct ta_pmf_init_table {
>> + u32 frequency; /* SMU sampling frequency */
>> + bool validate;
>> + bool sku_check;
>> + bool metadata_macrocheck;
>> + struct ta_pmf_load_policy_table policies_table;
>> +};
>> +
>> +/* Everything the TA needs to Enact Policies */
>> +struct ta_pmf_enact_table {
>> + struct ta_pmf_condition_info ev_info;
>> + u32 name;
>> +};
>> +
>> +struct ta_pmf_action {
>> + u32 action_index;
>> + u32 value;
>> +};
>> +
>> +/* output actions from TA */
>> +struct ta_pmf_enact_result {
>> + u32 actions_count;
>> + struct ta_pmf_action actions_list[TA_PMF_ACTION_MAX];
>> + u32 undo_count;
>> + struct ta_pmf_action undo_list[TA_PMF_UNDO_MAX];
>> +};
>> +
>> +union ta_pmf_input {
>> + struct ta_pmf_enact_table enact_table;
>> + struct ta_pmf_init_table init_table;
>> +};
>> +
>> +union ta_pmf_output {
>> + struct ta_pmf_enact_result policy_apply_table;
>> + u32 rsvd[906];
>
> This is some size (SZ_1K?) - sizeof(ta_pmf_enact_result)? I don't know if
> compiler would like such a construct though in the array declaration. If
> the compiler isn't complaining it would be the most informative way to
> state the size but if it's not happy, a comment might be useful.
This is a reserved space for future use. And that's the same way
defined in the FW as well. If I play with the sizes, the FW (PMF TA)
starts to misbehave and does not provide outputs all the time.
Like you mentioned, are you Ok if I just put a comment as "reserved
space for future use"?
Rest all remarks, I have addressed.
Thanks,
Shyam
>
>> +};
>> +
>> struct ta_pmf_shared_memory {
>> int command_id;
>> int resp_id;
>> u32 pmf_result;
>> u32 if_version;
>> + union ta_pmf_output pmf_output;
>> + union ta_pmf_input pmf_input;
>> };
>>
>> /* Core Layer */
>> @@ -460,4 +594,5 @@ extern const struct attribute_group cnqf_feature_attribute_group;
>> /* Smart PC builder Layer*/
>> int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
>> void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
>> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev);
>> #endif /* PMF_H */
>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>> index 1b3985cd7c08..15aa6e6e1050 100644
>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>> @@ -42,9 +42,77 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
>> param[0].u.memref.shm_offs = 0;
>> }
>>
>> +static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
>> +{
>> + unsigned long val;
>> + int idx;
>> +
>> + for (idx = 0; idx < out->actions_count; idx++) {
>> + val = out->actions_list[idx].value;
>> + switch (out->actions_list[idx].action_index) {
>> + case PMF_POLICY_SPL:
>> + if (dev->prev_data->spl != val) {
>> + amd_pmf_send_cmd(dev, SET_SPL, false, val, NULL);
>> + dev_dbg(dev->dev, "update SPL : %lu\n", val);
>> + dev->prev_data->spl = val;
>
> Well, I'd have expected you to go u32 (and %u) here as isn't
> out->actions_list[idx].value u32? And amd_pmf_send_cmd also takes u32.
> So unsigned long looks quite inconsistent and wasteful.
>
>> + }
>> + break;
>> +
>> + case PMF_POLICY_SPPT:
>> + if (dev->prev_data->sppt != val) {
>> + amd_pmf_send_cmd(dev, SET_SPPT, false, val, NULL);
>> + dev_dbg(dev->dev, "update SPPT : %lu\n", val);
>> + dev->prev_data->sppt = val;
>> + }
>> + break;
>> +
>> + case PMF_POLICY_FPPT:
>> + if (dev->prev_data->fppt != val) {
>> + amd_pmf_send_cmd(dev, SET_FPPT, false, val, NULL);
>> + dev_dbg(dev->dev, "update FPPT : %lu\n", val);
>> + dev->prev_data->fppt = val;
>> + }
>> + break;
>> +
>> + case PMF_POLICY_SPPT_APU_ONLY:
>> + if (dev->prev_data->sppt_apuonly != val) {
>> + amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, val, NULL);
>> + dev_dbg(dev->dev, "update SPPT_APU_ONLY : %lu\n", val);
>> + dev->prev_data->sppt_apuonly = val;
>> + }
>> + break;
>> +
>> + case PMF_POLICY_STT_MIN:
>> + if (dev->prev_data->stt_minlimit != val) {
>> + amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false, val, NULL);
>> + dev_dbg(dev->dev, "update STT_MIN : %lu\n", val);
>> + dev->prev_data->stt_minlimit = val;
>> + }
>> + break;
>> +
>> + case PMF_POLICY_STT_SKINTEMP_APU:
>> + if (dev->prev_data->stt_skintemp_apu != val) {
>> + amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, val, NULL);
>> + dev_dbg(dev->dev, "update STT_SKINTEMP_APU : %lu\n", val);
>> + dev->prev_data->stt_skintemp_apu = val;
>> + }
>> + break;
>> +
>> + case PMF_POLICY_STT_SKINTEMP_HS2:
>> + if (dev->prev_data->stt_skintemp_hs2 != val) {
>> + amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, val, NULL);
>> + dev_dbg(dev->dev, "update STT_SKINTEMP_HS2 : %lu\n", val);
>> + dev->prev_data->stt_skintemp_hs2 = val;
>> + }
>> + break;
>> + }
>> + }
>> +}
>> +
>> static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
>> {
>> struct ta_pmf_shared_memory *ta_sm = NULL;
>> + struct ta_pmf_enact_result *out = NULL;
>> struct tee_param param[MAX_TEE_PARAM];
>> struct tee_ioctl_invoke_arg arg;
>> int ret = 0;
>> @@ -52,7 +120,10 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
>> if (!dev->tee_ctx)
>> return -ENODEV;
>>
>> + memset(dev->shbuf, 0, dev->policy_sz);
>> ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
>> + out = &ta_sm->pmf_output.policy_apply_table;
>> +
>> memset(ta_sm, 0, sizeof(struct ta_pmf_shared_memory));
>> ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER_ENACT_POLICIES;
>> ta_sm->if_version = PMF_TA_IF_VERSION_MAJOR;
>> @@ -65,6 +136,12 @@ static int amd_pmf_invoke_cmd_enact(struct amd_pmf_dev *dev)
>> return -EINVAL;
>> }
>>
>> + if (ta_sm->pmf_result == TA_PMF_TYPE_SUCCESS && out->actions_count) {
>> + dev_dbg(dev->dev, "action count:%u result:%x\n", out->actions_count,
>> + ta_sm->pmf_result);
>> + amd_pmf_apply_policies(dev, out);
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -72,6 +149,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
>> {
>> struct ta_pmf_shared_memory *ta_sm = NULL;
>> struct tee_param param[MAX_TEE_PARAM];
>> + struct ta_pmf_init_table *in = NULL;
>> struct tee_ioctl_invoke_arg arg;
>> int ret = 0;
>>
>> @@ -80,10 +158,20 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
>> return -ENODEV;
>> }
>>
>> + dev_dbg(dev->dev, "Policy Binary size: %u bytes\n", dev->policy_sz);
>> + memset(dev->shbuf, 0, dev->policy_sz);
>> ta_sm = (struct ta_pmf_shared_memory *)dev->shbuf;
>> + in = &ta_sm->pmf_input.init_table;
>> +
>> ta_sm->command_id = TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE;
>> ta_sm->if_version = PMF_TA_IF_VERSION_MAJOR;
>> + in->metadata_macrocheck = false;
>> + in->sku_check = false;
>> + in->validate = true;
>> + in->frequency = pb_actions_ms;
>> + in->policies_table.table_size = dev->policy_sz;
>>
>> + memcpy(in->policies_table.table, dev->policy_buf, dev->policy_sz);
>> amd_pmf_prepare_args(dev, TA_PMF_COMMAND_POLICY_BUILDER_INITIALIZE, &arg, param);
>>
>> ret = tee_client_invoke_func(dev->tee_ctx, &arg, param);
>> @@ -103,6 +191,47 @@ static void amd_pmf_invoke_cmd(struct work_struct *work)
>> schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms));
>> }
>>
>> +static int amd_pmf_start_policy_engine(struct amd_pmf_dev *dev)
>> +{
>> + u32 cookie, length;
>> + int res;
>> +
>> + cookie = readl(dev->policy_buf + POLICY_COOKIE_OFFSET);
>> + length = readl(dev->policy_buf + POLICY_COOKIE_LEN);
>> +
>> + if (cookie != POLICY_SIGN_COOKIE || !length)
>> + return -EINVAL;
>> +
>> + /* update the actual length */
>> + dev->policy_sz = length + 512;
>> + res = amd_pmf_invoke_cmd_init(dev);
>> + if (res == TA_PMF_TYPE_SUCCESS) {
>> + /* now its safe to announce that smart pc is enabled */
>> + dev->smart_pc_enabled = 1;
>> + schedule_delayed_work(&dev->pb_work, msecs_to_jiffies(pb_actions_ms * 3));
>
> Why * 3? Explain in comment if you feel it's necessary as it's not
> obvious.
>
>
^ permalink raw reply
* Re: [PATCH v2 06/16] platform/x86/amd/pmf: Add support to get inputs from other subsystems
From: Shyam Sundar S K @ 2023-10-09 5:20 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <2eb2b3e5-4849-10ec-1c1b-66d2f0ba561@linux.intel.com>
On 10/4/2023 5:44 PM, Ilpo Järvinen wrote:
> On Sat, 30 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 | 119 ++++++++++++++++++++++++++
>> drivers/platform/x86/amd/pmf/tee-if.c | 3 +
>> 4 files changed, 141 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 6f4b6f4ecee4..60b11455dadf 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -149,6 +149,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 {
>> @@ -595,4 +610,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..3113bde051d9
>> --- /dev/null
>> +++ b/drivers/platform/x86/amd/pmf/spc.c
>> @@ -0,0 +1,119 @@
>> +// 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 <linux/units.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];
>> +
>> + /* 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];
>> + }
>
> My comments weren't either answered adequately or changes made here.
> Please check the v1 comments. I hope it's not because you feel hurry to
> get the next version out...
>
> I'm still unsure if the u16 thing can overflow because I don't know what's
> the max value for avg_core_c0residency[i].
the highest value for avg_core_c0residency[i] is merely a small number
and hence I retained the avg variable as u16. Not sure if there a
'real' case where it can overflow.
Sorry, I missed to merge both into a single for loop. I will address
this in v3.
Thanks,
Shyam
>
>> +
>> + 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) / MILLI;
>> + in->ev_info.full_charge_capacity = get_battery_prop(POWER_SUPPLY_PROP_ENERGY_FULL) / MILLI;
>> + in->ev_info.drain_rate = get_battery_prop(POWER_SUPPLY_PROP_POWER_NOW) / MILLI;
>
> There are defines specifically for watts so you should use them rather
> than generic MILLI.
>
>
^ permalink raw reply
* Re: [PATCH v2 08/16] platform/x86/amd/pmf: Add support to update system state
From: Shyam Sundar S K @ 2023-10-09 5:44 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel,
kernel test robot
In-Reply-To: <b827b663-871-5437-247-1c6c502d596d@linux.intel.com>
On 10/4/2023 5:53 PM, Ilpo Järvinen wrote:
> On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
>
>> PMF driver based on the output actions from the TA can request to update
>> the system states like entering s0i3, lock screen etc. by generating
>> an uevent. Based on the udev rules set in the userspace the event id
>> matching the uevent shall get updated accordingly using the systemctl.
>>
>> Sample udev rules under Documentation/admin-guide/pmf.rst.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202309260515.5XbR6N0g-lkp@intel.com/
>
> Please don't put lkp tags for patches that are still under development
> (even if the email you get misleadingly instructs you to). Only use them
> when you fix code that's already in tree based on LKP's report.
Agreed.
>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>> Documentation/admin-guide/index.rst | 1 +
>> Documentation/admin-guide/pmf.rst | 25 ++++++++++++++++
>> drivers/platform/x86/amd/pmf/pmf.h | 9 ++++++
>> drivers/platform/x86/amd/pmf/tee-if.c | 41 ++++++++++++++++++++++++++-
>> 4 files changed, 75 insertions(+), 1 deletion(-)
>> create mode 100644 Documentation/admin-guide/pmf.rst
>>
>> diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
>> index 43ea35613dfc..fb40a1f6f79e 100644
>> --- a/Documentation/admin-guide/index.rst
>> +++ b/Documentation/admin-guide/index.rst
>> @@ -119,6 +119,7 @@ configure specific aspects of kernel behavior to your liking.
>> parport
>> perf-security
>> pm/index
>> + pmf
>> pnp
>> rapidio
>> ras
>> diff --git a/Documentation/admin-guide/pmf.rst b/Documentation/admin-guide/pmf.rst
>> new file mode 100644
>> index 000000000000..90072add511e
>> --- /dev/null
>> +++ b/Documentation/admin-guide/pmf.rst
>> @@ -0,0 +1,25 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +Set udev rules for PMF Smart PC Builder
>> +---------------------------------------
>> +
>> +AMD PMF(Platform Management Framework) Smart PC Solution builder has to set the system states
>> +like S0i3, Screen lock, hibernate etc, based on the output actions provided by the PMF
>> +TA (Trusted Application).
>> +
>> +In order for this to work the PMF driver generates a uevent for userspace to react to. Below are
>> +sample udev rules that can facilitate this experience when a machine has PMF Smart PC solution builder
>> +enabled.
>> +
>> +Please add the following line(s) to
>> +``/etc/udev/rules.d/99-local.rules``::
>> +
>> + DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="1", RUN+="/usr/bin/systemctl suspend"
>> + DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="2", RUN+="/usr/bin/systemctl hibernate"
>> + DRIVERS=="amd-pmf", ACTION=="change", ENV{EVENT_ID}=="3", RUN+="/bin/loginctl lock-sessions"
>> +
>> +EVENT_ID values:
>> +1= Put the system to S0i3/S2Idle
>> +2= Put the system to hibernate
>> +3= Lock the screen
>> +
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index d5e410c41e81..34778192432e 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -73,6 +73,7 @@
>> #define PMF_POLICY_STT_MIN 6
>> #define PMF_POLICY_STT_SKINTEMP_APU 7
>> #define PMF_POLICY_STT_SKINTEMP_HS2 8
>> +#define PMF_POLICY_SYSTEM_STATE 9
>> #define PMF_POLICY_P3T 38
>>
>> /* TA macros */
>> @@ -439,6 +440,13 @@ struct apmf_dyn_slider_output {
>> } __packed;
>>
>> /* Smart PC - TA internals */
>> +enum system_state {
>> + SYSTEM_STATE__S0i3 = 1,
>> + SYSTEM_STATE__S4,
>> + SYSTEM_STATE__SCREEN_LOCK,
>> + SYSTEM_STATE__MAX
>> +};
>> +
>> enum ta_slider {
>> TA_BEST_BATTERY, /* Best Battery */
>> TA_BETTER_BATTERY, /* Better Battery */
>> @@ -470,6 +478,7 @@ enum ta_pmf_error_type {
>> };
>>
>> struct pmf_action_table {
>> + enum system_state system_state;
>> unsigned long spl; /* in mW */
>> unsigned long sppt; /* in mW */
>> unsigned long sppt_apuonly; /* in mW */
>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>> index 315e3d2eacdf..961011530c1b 100644
>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>> @@ -24,6 +24,20 @@ MODULE_PARM_DESC(pb_actions_ms, "Policy binary actions sampling frequency (defau
>> static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
>> 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
>>
>> +static const char *amd_pmf_uevent_as_str(unsigned int state)
>> +{
>> + switch (state) {
>> + case SYSTEM_STATE__S0i3:
>> + return "S0i3";
>> + case SYSTEM_STATE__S4:
>> + return "S4";
>> + case SYSTEM_STATE__SCREEN_LOCK:
>> + return "SCREEN_LOCK";
>> + default:
>> + return "Unknown Smart PC event";
>> + }
>> +}
>> +
>> static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
>> struct tee_ioctl_invoke_arg *arg,
>> struct tee_param *param)
>> @@ -42,9 +56,23 @@ static void amd_pmf_prepare_args(struct amd_pmf_dev *dev, int cmd,
>> param[0].u.memref.shm_offs = 0;
>> }
>>
>> +static int amd_pmf_update_uevents(struct amd_pmf_dev *dev, u16 event)
>> +{
>> + char *envp[2] = {};
>> +
>> + envp[0] = kasprintf(GFP_KERNEL, "EVENT_ID=%d", event);
>> + if (!envp[0])
>> + return -EINVAL;
>> +
>> + kobject_uevent_env(&dev->dev->kobj, KOBJ_CHANGE, envp);
>> +
>> + kfree(envp[0]);
>> + return 0;
>> +}
>> +
>> static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_result *out)
>> {
>> - unsigned long val;
>> + unsigned long val, event = 0;
>> int idx;
>>
>> for (idx = 0; idx < out->actions_count; idx++) {
>> @@ -113,6 +141,17 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>> dev->prev_data->p3t_limit = val;
>> }
>> break;
>> +
>> + case PMF_POLICY_SYSTEM_STATE:
>> + event = val + 1;
>> + if (dev->prev_data->system_state != event) {
>> + amd_pmf_update_uevents(dev, event);
>> + dev_dbg(dev->dev, "update SYSTEM_STATE : %s\n",
>> + amd_pmf_uevent_as_str(event));
>> + /* reset the previous recorded state to zero */
>> + dev->prev_data->system_state = 0;
>
> No, a comment won't help you here. As is, system_state is constant 0 so
> it's entirely unnecessary to keep that value at all. In fact, the comment
> is wrong because there never was "previously recorder state" in
> ->system_state to begin with since it's either initialized to zero on
> alloc or reset to zero by this line.
>
> So what are you trying to achieve here with this ->system_state variable?
Sorry I misunderstood your previous remark. This was a test code which
was supposed to be cleaned up before sending.
I will fix this in v3.
Thanks,
Shyam
>
^ permalink raw reply
* Re: [PATCH v2 12/16] platform/x86/amd/pmf: Add PMF-AMDGPU get interface
From: Shyam Sundar S K @ 2023-10-09 5:56 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <e7b33961-23bb-cb8-2941-ced3f0cf2620@linux.intel.com>
On 10/4/2023 6:19 PM, Ilpo Järvinen wrote:
> On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
>
>> In order to provide GPU inputs to TA for the Smart PC solution to work, we
>> need to have interface between the PMF driver and the AMDGPU driver.
>>
>> Add the initial code path for get interface from AMDGPU.
>>
>> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>
>> @@ -355,6 +356,21 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
>> return amd_pmf_start_policy_engine(dev);
>> }
>>
>> +static int amd_pmf_get_gpu_handle(struct pci_dev *pdev, void *data)
>> +{
>> + struct amd_pmf_dev *dev = data;
>> +
>> + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->devfn == 0) {
>> + /* get the amdgpu handle from the pci root after walking through the pci bus */
>
> I can see from the code that you assign to amdgpu handle so this comment
> added no information.
>
> It doesn't really answer at all why you're doing this second step. Based
> on the give parameters to pci_get_device(), it looks as if you're asking
> for the same device you already have in pdev to be searched to you.
Not sure if I understand you remark completely.
amd_pmf_get_gpu_handle() is a callback function for pci_walk_bus
(which is done below).
What I am trying to do here is to get the PCI handle for the GPU
device by walking the PCI bus.
I think the 'pdev' here refers to the pci root, using that root we
walk the entire tree and only stop walking when we find a handle to
GPU device.
Do you want me to change the "pdev" parameter to be renamed as "root" ?
Am I missing something?
Thanks,
Shyam
>
>> + dev->gfx_data.gpu_dev = pci_get_device(pdev->vendor, pdev->device, NULL);
>> + if (dev->gfx_data.gpu_dev) {
>> + pci_dev_put(pdev);
>> + return 1; /* stop walking */
>> + }
>> + }
>> + return 0; /* continue walking */
>> +}
>> +
>> static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
>> {
>> return ver->impl_id == TEE_IMPL_ID_AMDTEE;
>> @@ -451,6 +467,15 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>> INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
>> amd_pmf_set_dram_addr(dev);
>> amd_pmf_get_bios_buffer(dev);
>> +
>> + /* get amdgpu handle */
>> + pci_walk_bus(dev->root->bus, amd_pmf_get_gpu_handle, dev);
>> + if (!dev->gfx_data.gpu_dev)
>> + dev_err(dev->dev, "GPU handle not found!\n");
>> +
>> + if (!amd_pmf_gpu_init(&dev->gfx_data))
>> + dev->gfx_data.gpu_dev_en = true;
>> +
>
>
^ permalink raw reply
* [PATCH v2 0/2] HID: uclogic: Fix two bugs in uclogic
From: Jinjie Ruan @ 2023-10-09 6:42 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input, linux-kernel,
jose.exposito89
Cc: ruanjinjie
When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch
kernel and then there are a user-memory-access bug and a work->entry
not empty bug. This patchset fix these issues.
Changes in v2:
- Use kunit_kzalloc() instead of kzalloc().
- Add KUNIT_ASSERT_NOT_ERR_OR_NULL() for kunit_kzalloc().
- Add Reviewed-by.
Jinjie Ruan (2):
HID: uclogic: Fix user-memory-access bug in
uclogic_params_ugee_v2_init_event_hooks()
HID: uclogic: Fix a work->entry not empty bug in __queue_work()
drivers/hid/hid-uclogic-core-test.c | 7 +++++++
drivers/hid/hid-uclogic-params-test.c | 16 +++++++++++++++-
2 files changed, 22 insertions(+), 1 deletion(-)
--
2.34.1
^ permalink raw reply
* [PATCH v2 1/2] HID: uclogic: Fix user-memory-access bug in uclogic_params_ugee_v2_init_event_hooks()
From: Jinjie Ruan @ 2023-10-09 6:42 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input, linux-kernel,
jose.exposito89
Cc: ruanjinjie
In-Reply-To: <20231009064245.3573397-1-ruanjinjie@huawei.com>
When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch kernel and
then the below user-memory-access bug occurs.
In hid_test_uclogic_params_cleanup_event_hooks(),it call
uclogic_params_ugee_v2_init_event_hooks() with the first arg=NULL, so
when it calls uclogic_params_ugee_v2_has_battery(), the hid_get_drvdata()
will access hdev->dev with hdev=NULL, which will cause below
user-memory-access.
So add a fake_device with quirks member and call hid_set_drvdata()
to assign hdev->dev->driver_data which avoids the null-ptr-def bug
for drvdata->quirks in uclogic_params_ugee_v2_has_battery(). After applying
this patch, the below user-memory-access bug never occurs.
general protection fault, probably for non-canonical address 0xdffffc0000000329: 0000 [#1] PREEMPT SMP KASAN
KASAN: probably user-memory-access in range [0x0000000000001948-0x000000000000194f]
CPU: 5 PID: 2189 Comm: kunit_try_catch Tainted: G B W N 6.6.0-rc2+ #30
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
RIP: 0010:uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
Code: f3 f3 65 48 8b 14 25 28 00 00 00 48 89 54 24 60 31 d2 48 89 fa c7 44 24 30 00 00 00 00 48 c7 44 24 28 02 f8 02 01 48 c1 ea 03 <80> 3c 02 00 0f 85 2c 04 00 00 48 8b 9d 48 19 00 00 48 b8 00 00 00
RSP: 0000:ffff88810679fc88 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: 0000000000000004 RCX: 0000000000000000
RDX: 0000000000000329 RSI: ffff88810679fd88 RDI: 0000000000001948
RBP: 0000000000000000 R08: 0000000000000000 R09: ffffed1020f639f0
R10: ffff888107b1cf87 R11: 0000000000000400 R12: 1ffff11020cf3f92
R13: ffff88810679fd88 R14: ffff888100b97b08 R15: ffff8881030bb080
FS: 0000000000000000(0000) GS:ffff888119e80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000005286001 CR4: 0000000000770ee0
DR0: ffffffff8fdd6cf4 DR1: ffffffff8fdd6cf5 DR2: ffffffff8fdd6cf6
DR3: ffffffff8fdd6cf7 DR6: 00000000fffe0ff0 DR7: 0000000000000600
PKRU: 55555554
Call Trace:
<TASK>
? die_addr+0x3d/0xa0
? exc_general_protection+0x144/0x220
? asm_exc_general_protection+0x22/0x30
? uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
? sched_clock_cpu+0x69/0x550
? uclogic_parse_ugee_v2_desc_gen_params+0x70/0x70
? load_balance+0x2950/0x2950
? rcu_trc_cmpxchg_need_qs+0x67/0xa0
hid_test_uclogic_params_cleanup_event_hooks+0x9e/0x1a0
? uclogic_params_ugee_v2_init_event_hooks+0x600/0x600
? __switch_to+0x5cf/0xe60
? migrate_enable+0x260/0x260
? __kthread_parkme+0x83/0x150
? kunit_try_run_case_cleanup+0xe0/0xe0
kunit_generic_run_threadfn_adapter+0x4a/0x90
? kunit_try_catch_throw+0x80/0x80
kthread+0x2b5/0x380
? kthread_complete_and_exit+0x20/0x20
ret_from_fork+0x2d/0x70
? kthread_complete_and_exit+0x20/0x20
ret_from_fork_asm+0x11/0x20
</TASK>
Modules linked in:
Dumping ftrace buffer:
(ftrace buffer empty)
---[ end trace 0000000000000000 ]---
RIP: 0010:uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
Code: f3 f3 65 48 8b 14 25 28 00 00 00 48 89 54 24 60 31 d2 48 89 fa c7 44 24 30 00 00 00 00 48 c7 44 24 28 02 f8 02 01 48 c1 ea 03 <80> 3c 02 00 0f 85 2c 04 00 00 48 8b 9d 48 19 00 00 48 b8 00 00 00
RSP: 0000:ffff88810679fc88 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: 0000000000000004 RCX: 0000000000000000
RDX: 0000000000000329 RSI: ffff88810679fd88 RDI: 0000000000001948
RBP: 0000000000000000 R08: 0000000000000000 R09: ffffed1020f639f0
R10: ffff888107b1cf87 R11: 0000000000000400 R12: 1ffff11020cf3f92
R13: ffff88810679fd88 R14: ffff888100b97b08 R15: ffff8881030bb080
FS: 0000000000000000(0000) GS:ffff888119e80000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000005286001 CR4: 0000000000770ee0
DR0: ffffffff8fdd6cf4 DR1: ffffffff8fdd6cf5 DR2: ffffffff8fdd6cf6
DR3: ffffffff8fdd6cf7 DR6: 00000000fffe0ff0 DR7: 0000000000000600
PKRU: 55555554
Kernel panic - not syncing: Fatal exception
Dumping ftrace buffer:
(ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 1 seconds..
Fixes: a251d6576d2a ("HID: uclogic: Handle wireless device reconnection")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
---
v2:
- Use kunit_kzalloc() instead of kzalloc().
- Add KUNIT_ASSERT_NOT_ERR_OR_NULL() for kunit_kzalloc().
- Add Reviewed-by.
---
drivers/hid/hid-uclogic-params-test.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-uclogic-params-test.c b/drivers/hid/hid-uclogic-params-test.c
index 678f50cbb160..a30121419a29 100644
--- a/drivers/hid/hid-uclogic-params-test.c
+++ b/drivers/hid/hid-uclogic-params-test.c
@@ -174,12 +174,26 @@ static void hid_test_uclogic_parse_ugee_v2_desc(struct kunit *test)
KUNIT_EXPECT_EQ(test, params->frame_type, frame_type);
}
+struct fake_device {
+ unsigned long quirks;
+};
+
static void hid_test_uclogic_params_cleanup_event_hooks(struct kunit *test)
{
int res, n;
+ struct hid_device *hdev;
+ struct fake_device *fake_dev;
struct uclogic_params p = {0, };
- res = uclogic_params_ugee_v2_init_event_hooks(NULL, &p);
+ hdev = kunit_kzalloc(test, sizeof(struct hid_device), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, hdev);
+
+ fake_dev = kunit_kzalloc(test, sizeof(struct fake_device), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, fake_dev);
+
+ hid_set_drvdata(hdev, fake_dev);
+
+ res = uclogic_params_ugee_v2_init_event_hooks(hdev, &p);
KUNIT_ASSERT_EQ(test, res, 0);
/* Check that the function can be called repeatedly */
--
2.34.1
^ permalink raw reply related
* [PATCH v2 2/2] HID: uclogic: Fix a work->entry not empty bug in __queue_work()
From: Jinjie Ruan @ 2023-10-09 6:42 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input, linux-kernel,
jose.exposito89
Cc: ruanjinjie
In-Reply-To: <20231009064245.3573397-1-ruanjinjie@huawei.com>
When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch
kernel and then the below work->entry not empty bug occurs.
In hid_test_uclogic_exec_event_hook_test(), the filter->work is not
initialized to be added to p.event_hooks->list, and then the
schedule_work() in uclogic_exec_event_hook() will call __queue_work(),
which check whether the work->entry is empty and cause the below
warning call trace.
So call INIT_WORK() with a fake work to solve the issue. After applying
this patch, the below work->entry not empty bug never occurs.
WARNING: CPU: 0 PID: 2177 at kernel/workqueue.c:1787 __queue_work.part.0+0x780/0xad0
Modules linked in:
CPU: 0 PID: 2177 Comm: kunit_try_catch Tainted: G B W N 6.6.0-rc2+ #30
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
RIP: 0010:__queue_work.part.0+0x780/0xad0
Code: 44 24 20 0f b6 00 84 c0 74 08 3c 03 0f 8e 52 03 00 00 f6 83 00 01 00 00 02 74 6f 4c 89 ef e8 c7 d8 f1 02 f3 90 e9 e5 f8 ff ff <0f> 0b e9 63 fc ff ff 89 e9 49 8d 57 68 4c 89 e6 4c 89 ff 83 c9 02
RSP: 0000:ffff888102bb7ce8 EFLAGS: 00010086
RAX: 0000000000000000 RBX: ffff888106b8e460 RCX: ffffffff84141cc7
RDX: 1ffff11020d71c8c RSI: 0000000000000004 RDI: ffff8881001d0118
RBP: dffffc0000000000 R08: 0000000000000001 R09: ffffed1020576f92
R10: 0000000000000003 R11: ffff888102bb7980 R12: ffff888106b8e458
R13: ffff888119c38800 R14: 0000000000000000 R15: ffff8881001d0100
FS: 0000000000000000(0000) GS:ffff888119c00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff888119506000 CR3: 0000000005286001 CR4: 0000000000770ef0
DR0: ffffffff8fdd6ce0 DR1: ffffffff8fdd6ce1 DR2: ffffffff8fdd6ce3
DR3: ffffffff8fdd6ce5 DR6: 00000000fffe0ff0 DR7: 0000000000000600
PKRU: 55555554
Call Trace:
<TASK>
? __warn+0xc9/0x260
? __queue_work.part.0+0x780/0xad0
? report_bug+0x345/0x400
? handle_bug+0x3c/0x70
? exc_invalid_op+0x14/0x40
? asm_exc_invalid_op+0x16/0x20
? _raw_spin_lock+0x87/0xe0
? __queue_work.part.0+0x780/0xad0
? __queue_work.part.0+0x249/0xad0
queue_work_on+0x48/0x50
uclogic_exec_event_hook.isra.0+0xf7/0x160
hid_test_uclogic_exec_event_hook_test+0x2f1/0x5d0
? try_to_wake_up+0x151/0x13e0
? uclogic_exec_event_hook.isra.0+0x160/0x160
? _raw_spin_lock_irqsave+0x8d/0xe0
? __sched_text_end+0xa/0xa
? __sched_text_end+0xa/0xa
? migrate_enable+0x260/0x260
? kunit_try_run_case_cleanup+0xe0/0xe0
kunit_generic_run_threadfn_adapter+0x4a/0x90
? kunit_try_catch_throw+0x80/0x80
kthread+0x2b5/0x380
? kthread_complete_and_exit+0x20/0x20
ret_from_fork+0x2d/0x70
? kthread_complete_and_exit+0x20/0x20
ret_from_fork_asm+0x11/0x20
</TASK>
Fixes: a251d6576d2a ("HID: uclogic: Handle wireless device reconnection")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
---
v2:
- Add Reviewed-by.
---
drivers/hid/hid-uclogic-core-test.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/hid/hid-uclogic-core-test.c b/drivers/hid/hid-uclogic-core-test.c
index 2bb916226a38..cb274cde3ad2 100644
--- a/drivers/hid/hid-uclogic-core-test.c
+++ b/drivers/hid/hid-uclogic-core-test.c
@@ -56,6 +56,11 @@ static struct uclogic_raw_event_hook_test test_events[] = {
},
};
+static void fake_work(struct work_struct *work)
+{
+
+}
+
static void hid_test_uclogic_exec_event_hook_test(struct kunit *test)
{
struct uclogic_params p = {0, };
@@ -77,6 +82,8 @@ static void hid_test_uclogic_exec_event_hook_test(struct kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filter->event);
memcpy(filter->event, &hook_events[n].event[0], filter->size);
+ INIT_WORK(&filter->work, fake_work);
+
list_add_tail(&filter->list, &p.event_hooks->list);
}
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 00/14] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Benjamin Tissoires @ 2023-10-09 8:13 UTC (permalink / raw)
To: Hans de Goede
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input
In-Reply-To: <98bc1918-653e-b298-392c-c525d069ea31@redhat.com>
Hi Hans,
On Oct 08 2023, Hans de Goede wrote:
> Hi,
>
> On 10/8/23 11:54, Hans de Goede wrote:
> > Hi Benjamin,
> >
> > Here is a v2 of my series to fix issues with hidpp_connect_event() running
> > while restarting IO, which now also fixes the issues you mentioned with
> > potentially missing connect events.
Great, thanks a lot for this hard work.
> >
> > This series is best explained by a brief sketch of how probe()
> > looks at the end of the series (1):
TBH, I couldn't parse the following yesterday evening, but after looking
at all patches one by one I can now get it :)
> >
> > Prep work:
> >
> > 1. All code depending on a device being in connected state is moved to
> > the hidpp_connect_event() workqueue item
> >
> > 2. hidpp_connect_event() now checks the connected state itself by
> > checking that hidpp_root_get_protocol_version() succeeds, instead
> > of relying on possibly stale (racy) data in struct hidpp_device
> >
> > With this in place the new probe() sequence looks like this:
> >
> > 1. enable_connect_event flag starts at 0, this filters out / ignores any
> > connect-events in hidpp_raw_hidpp_event() avoiding
> > hidpp_connect_event() getting queued before the IO restart
> >
> > 2. IO is started with connect-mask = 0
> > this avoids hid-input and hidraw connecting while probe() is setting
> > hdev->name and hdev->uniq
> >
> > 3. Name and serialnr are retrieved and stored in hdev
> >
> > 4. IO is fully restarted (including hw_open + io_start, not just hw_start)
> > with the actual connect-mask.
> >
> > 5. enable_connect_event atomic_t is set to 1 to enable processing of
> > connect events.
> >
> > 6. hidpp_connect_event() is queued + flushed to query the connected state
> > and do the connect work if the device is connected.
> >
> > 7. probe() now ends with:
> >
> > /*
> > * This relies on logi_dj_ll_close() being a no-op so that
> > * DJ connection events will still be received.
> > */
> > hid_hw_close(hdev);
> >
> > Since on restarting IO it now is fully restarted so the hid_hw_open()
> > there needs to be balanced.
> >
> > This series now obviously is no longer 6.6 material, instead I hope we
> > can get this rework (and IMHO nice cleanup) into 6.7 .
Yeah, not 6.6 anymore, but should be doable for 6.7.
> >
> > Regards,
> >
> > Hans
>
> I forgot to add info on the list of devices I tested this on:
>
> Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0)
> Logitech M720 Triathlon (bluetooth, HID++ 4.5)
> Logitech K400 Pro (unifying, HID++ 4.1)
> Logitech K270 (eQUAD nano Lite, HID++ 2.0)
> Logitech M185 (eQUAD nano Lite, HID++ 4.5)
> Logitech Keyboard LX501 (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0)
> Logitech 27Mhz mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0)
We should probably add this list to the commit messages.
I'll need to test also myself on some problematic devices that have a
special case (WTP, USB wired, BLE).
Anyway, I'll have to test everything, but this looks like it's in a
better shape than previously.
However, the thing I am afraid is that commit 498ba2069035 ("HID:
logitech-hidpp: Don't restart communication if not necessary") was
fixing devices that did not like the hid_hw_stop/start. I can't find the
bug numbers however... So with your series, we might breaking those
once again.
How about we do the following (in pseudo code):
probe():
hidpp_connect_and_start(connect_mask = 0)
// retrieve name and serial
hid_connect(connect_mask) // with connect_mask ensuring we don't
// create inputs if HIDPP_QUIRK_DELAYED_INIT
// is set, instead of stop/start
hid_hw_close(hdev); // to balance hidpp_connect_and_start()
I think the above should even remove the need for the
enable_connect_event atomic_t given that now we are not restarting the
devices at all.
>
> Regards,
>
> Hans
>
>
>
> > 1) For reviewing you may also want to apply the entire series and look
> > at the end result to help you understand why various intermediate steps
> > are taken.
> >
> >
> > Hans de Goede (14):
> > HID: logitech-hidpp: Revert "Don't restart communication if not
> > necessary"
> > HID: logitech-hidpp: Move hidpp_overwrite_name() to before connect
> > check
> > HID: logitech-hidpp: Add hidpp_non_unifying_init() helper
> > HID: logitech-hidpp: Remove connected check for non-unifying devices
> > HID: logitech-hidpp: Move get_wireless_feature_index() check to
> > hidpp_connect_event()
> > HID: logitech-hidpp: Remove wtp_get_config() call from probe()
> > HID: logitech-hidpp: Remove connected check for g920_get_config() call
> > HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
> > HID: logitech-hidpp: Move the connected check to after restarting IO
> > HID: logitech-hidpp: Move g920_get_config() to just before
> > hidpp_ff_init()
> > HID: logitech-hidpp: Remove unused connected param from *_connect()
> > HID: logitech-hidpp: Fix connect event race
> > HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe()
> > restarts IO
> > HID: logitech-hidpp: Drop delayed_work_cb()
> >
> > drivers/hid/hid-logitech-hidpp.c | 211 +++++++++++++------------------
> > 1 file changed, 91 insertions(+), 120 deletions(-)
> >
I like when the total number of deletions is higher than the additions
:)
Cheers,
Benjamin
>
^ permalink raw reply
* [PATCH] input: bbnsm_pwrkey: Fix key press missed issue after suspend
From: Jacky Bai @ 2023-10-09 8:34 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: linux-input, linux-imx
When wakeup from a on/off key press event, need to report
this input event directly to make sure no press event is
missed when resume from suspend.
Fixes: 40e40fdfec3f ("Input: bbnsm_pwrkey - add bbnsm power key support")
Signed-off-by: Jacky Bai <ping.bai@nxp.com>
---
drivers/input/misc/nxp-bbnsm-pwrkey.c | 38 ++++++++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/input/misc/nxp-bbnsm-pwrkey.c b/drivers/input/misc/nxp-bbnsm-pwrkey.c
index 1d99206dd3a8..55d4fd115887 100644
--- a/drivers/input/misc/nxp-bbnsm-pwrkey.c
+++ b/drivers/input/misc/nxp-bbnsm-pwrkey.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0+
//
-// Copyright 2022 NXP.
+// Copyright 2022-2023 NXP.
#include <linux/device.h>
#include <linux/err.h>
@@ -38,6 +38,7 @@ struct bbnsm_pwrkey {
int irq;
int keycode;
int keystate; /* 1:pressed */
+ bool suspended;
struct timer_list check_timer;
struct input_dev *input;
};
@@ -70,6 +71,7 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id)
{
struct platform_device *pdev = dev_id;
struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev);
+ struct input_dev *input = bbnsm->input;
u32 event;
regmap_read(bbnsm->regmap, BBNSM_EVENTS, &event);
@@ -81,6 +83,16 @@ static irqreturn_t bbnsm_pwrkey_interrupt(int irq, void *dev_id)
mod_timer(&bbnsm->check_timer,
jiffies + msecs_to_jiffies(DEBOUNCE_TIME));
+ /*
+ * Directly report key event after resume to make no key press
+ * event is missed.
+ */
+ if (bbnsm->suspended) {
+ bbnsm->keystate = 1;
+ input_event(input, EV_KEY, bbnsm->keycode, 1);
+ input_sync(input);
+ }
+
/* clear PWR OFF */
regmap_write(bbnsm->regmap, BBNSM_EVENTS, BBNSM_BTN_OFF);
@@ -173,6 +185,29 @@ static int bbnsm_pwrkey_probe(struct platform_device *pdev)
return 0;
}
+static int __maybe_unused bbnsm_pwrkey_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev);
+
+ bbnsm->suspended = true;
+
+ return 0;
+}
+
+static int __maybe_unused bbnsm_pwrkey_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct bbnsm_pwrkey *bbnsm = platform_get_drvdata(pdev);
+
+ bbnsm->suspended = false;
+
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(bbnsm_pwrkey_pm_ops, bbnsm_pwrkey_suspend,
+ bbnsm_pwrkey_resume);
+
static const struct of_device_id bbnsm_pwrkey_ids[] = {
{ .compatible = "nxp,imx93-bbnsm-pwrkey" },
{ /* sentinel */ }
@@ -182,6 +217,7 @@ MODULE_DEVICE_TABLE(of, bbnsm_pwrkey_ids);
static struct platform_driver bbnsm_pwrkey_driver = {
.driver = {
.name = "bbnsm_pwrkey",
+ .pm = &bbnsm_pwrkey_pm_ops,
.of_match_table = bbnsm_pwrkey_ids,
},
.probe = bbnsm_pwrkey_probe,
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 06/16] platform/x86/amd/pmf: Add support to get inputs from other subsystems
From: Ilpo Järvinen @ 2023-10-09 10:26 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <61840843-8cb6-4353-a92c-befc46960fad@amd.com>
[-- Attachment #1: Type: text/plain, Size: 2925 bytes --]
On Mon, 9 Oct 2023, Shyam Sundar S K wrote:
> On 10/4/2023 5:44 PM, Ilpo Järvinen wrote:
> > On Sat, 30 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>
> >> diff --git a/drivers/platform/x86/amd/pmf/spc.c b/drivers/platform/x86/amd/pmf/spc.c
> >> new file mode 100644
> >> index 000000000000..3113bde051d9
> >> --- /dev/null
> >> +++ b/drivers/platform/x86/amd/pmf/spc.c
> >> @@ -0,0 +1,119 @@
> >> +// 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 <linux/units.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];
> >> +
> >> + /* 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];
> >> + }
> >
> > My comments weren't either answered adequately or changes made here.
> > Please check the v1 comments. I hope it's not because you feel hurry to
> > get the next version out...
> >
> > I'm still unsure if the u16 thing can overflow because I don't know what's
> > the max value for avg_core_c0residency[i].
>
> the highest value for avg_core_c0residency[i] is merely a small number
> and hence I retained the avg variable as u16. Not sure if there a
> 'real' case where it can overflow.
Okay, if you think it's fine, no problem with it then (not that there's
a big advantage having it as u16 instead of e.g. unsigned int).
> Sorry, I missed to merge both into a single for loop. I will address
> this in v3.
Thanks.
--
i.
^ permalink raw reply
* Re: [PATCH v2 01/16] platform/x86/amd/pmf: Add PMF TEE interface
From: Ilpo Järvinen @ 2023-10-09 10:29 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <01eef1b0-8695-4284-8a4a-973826f2c3cc@amd.com>
[-- Attachment #1: Type: text/plain, Size: 6339 bytes --]
On Mon, 9 Oct 2023, Shyam Sundar S K wrote:
>
>
> On 10/4/2023 4:20 PM, Ilpo Järvinen wrote:
> > On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> >
> >> AMD PMF driver loads the PMF TA (Trusted Application) into the AMD
> >> ASP's (AMD Security Processor) TEE (Trusted Execution Environment).
> >>
> >> PMF Trusted Application is a secured firmware placed under
> >> /lib/firmware/amdtee gets loaded only when the TEE environment is
> >> initialized. Add the initial code path to build these pipes.
> >>
> >> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> >> drivers/platform/x86/amd/pmf/Makefile | 3 +-
> >> drivers/platform/x86/amd/pmf/core.c | 11 ++-
> >> drivers/platform/x86/amd/pmf/pmf.h | 16 ++++
> >> drivers/platform/x86/amd/pmf/tee-if.c | 112 ++++++++++++++++++++++++++
> >> 4 files changed, 138 insertions(+), 4 deletions(-)
> >> create mode 100644 drivers/platform/x86/amd/pmf/tee-if.c
> >>
> >> diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile
> >> index fdededf54392..d2746ee7369f 100644
> >> --- a/drivers/platform/x86/amd/pmf/Makefile
> >> +++ b/drivers/platform/x86/amd/pmf/Makefile
> >> @@ -6,4 +6,5 @@
> >>
> >> obj-$(CONFIG_AMD_PMF) += amd-pmf.o
> >> amd-pmf-objs := core.o acpi.o sps.o \
> >> - auto-mode.o cnqf.o
> >> + auto-mode.o cnqf.o \
> >> + tee-if.o
> >> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> >> index 78ed3ee22555..68f1389dda3e 100644
> >> --- a/drivers/platform/x86/amd/pmf/core.c
> >> +++ b/drivers/platform/x86/amd/pmf/core.c
> >> @@ -309,8 +309,11 @@ static void amd_pmf_init_features(struct amd_pmf_dev *dev)
> >> dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n");
> >> }
> >>
> >> - /* Enable Auto Mode */
> >> - if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> >> + if (amd_pmf_init_smart_pc(dev)) {
> >> + /* Enable Smart PC Solution builder */
> >> + dev_dbg(dev->dev, "Smart PC Solution Enabled\n");
> >> + } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> >> + /* Enable Auto Mode */
> >
> > I'm pretty certain neither of these two comments add any information to
> > what's readily visible from the code itself so they can be dropped.
> >
> >> amd_pmf_init_auto_mode(dev);
> >> dev_dbg(dev->dev, "Auto Mode Init done\n");
> >> } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
> >> @@ -330,7 +333,9 @@ static void amd_pmf_deinit_features(struct amd_pmf_dev *dev)
> >> amd_pmf_deinit_sps(dev);
> >> }
> >>
> >> - if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> >> + if (dev->smart_pc_enabled) {
> >> + amd_pmf_deinit_smart_pc(dev);
> >> + } else if (is_apmf_func_supported(dev, APMF_FUNC_AUTO_MODE)) {
> >> amd_pmf_deinit_auto_mode(dev);
> >> } else if (is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC) ||
> >> is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_DC)) {
> >> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> >> index deba88e6e4c8..02460c2a31ea 100644
> >> --- a/drivers/platform/x86/amd/pmf/pmf.h
> >> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> >> @@ -179,6 +179,12 @@ struct amd_pmf_dev {
> >> bool cnqf_enabled;
> >> bool cnqf_supported;
> >> struct notifier_block pwr_src_notifier;
> >> + /* Smart PC solution builder */
> >> + struct tee_context *tee_ctx;
> >> + struct tee_shm *fw_shm_pool;
> >> + u32 session_id;
> >> + void *shbuf;
> >> + bool smart_pc_enabled;
> >> };
> >>
> >> struct apmf_sps_prop_granular {
> >> @@ -389,6 +395,13 @@ struct apmf_dyn_slider_output {
> >> struct apmf_cnqf_power_set ps[APMF_CNQF_MAX];
> >> } __packed;
> >>
> >> +struct ta_pmf_shared_memory {
> >> + int command_id;
> >> + int resp_id;
> >> + u32 pmf_result;
> >> + u32 if_version;
> >> +};
> >> +
> >> /* Core Layer */
> >> int apmf_acpi_init(struct amd_pmf_dev *pmf_dev);
> >> void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev);
> >> @@ -433,4 +446,7 @@ void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev);
> >> int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_lapsed_ms);
> >> extern const struct attribute_group cnqf_feature_attribute_group;
> >>
> >> +/* Smart PC builder Layer*/
> >
> > Missing space.
> >
> >> +int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev);
> >> +void amd_pmf_deinit_smart_pc(struct amd_pmf_dev *dev);
> >> #endif /* PMF_H */
> >> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> >> new file mode 100644
> >> index 000000000000..4db80ca59a11
> >> --- /dev/null
> >> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> >> @@ -0,0 +1,112 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * AMD Platform Management Framework Driver - TEE Interface
> >> + *
> >> + * Copyright (c) 2023, Advanced Micro Devices, Inc.
> >> + * All Rights Reserved.
> >> + *
> >> + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> + */
> >> +
> >> +#include <linux/tee_drv.h>
> >> +#include <linux/uuid.h>
> >> +#include "pmf.h"
> >> +
> >> +#define MAX_TEE_PARAM 4
> >> +static const uuid_t amd_pmf_ta_uuid = UUID_INIT(0x6fd93b77, 0x3fb8, 0x524d,
> >> + 0xb1, 0x2d, 0xc5, 0x29, 0xb1, 0x3d, 0x85, 0x43);
> >> +
> >> +static int amd_pmf_amdtee_ta_match(struct tee_ioctl_version_data *ver, const void *data)
> >> +{
> >> + return ver->impl_id == TEE_IMPL_ID_AMDTEE;
> >> +}
> >> +
> >> +static int amd_pmf_ta_open_session(struct tee_context *ctx, u32 *id)
> >> +{
> >> + struct tee_ioctl_open_session_arg sess_arg = {};
> >> + int rc;
> >> +
> >> + export_uuid(sess_arg.uuid, &amd_pmf_ta_uuid);
> >> + sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
> >> + sess_arg.num_params = 0;
> >> +
> >> + rc = tee_client_open_session(ctx, &sess_arg, NULL);
> >> + if (rc < 0 || sess_arg.ret != 0) {
> >> + pr_err("Failed to open TEE session err:%#x, ret:%#x\n", sess_arg.ret, rc);
> >
> > Print normal -Exx error codes as %d, not %x (rc). I don't know what would
> > be best to do with sess_arg.ret, TEEC_ERROR_* look like errnos (negative
> > values) manually converted into u32.
>
> in drivers/tee/amdtee/amdtee_private.h, all the TEEC_* are hex. So
> sess_arg.ret can remain %x?
Sure, you can leave it to %x.
--
i.
^ permalink raw reply
* Re: [PATCH v2 04/16] platform/x86/amd/pmf: Add support for PMF Policy Binary
From: Ilpo Järvinen @ 2023-10-09 10:49 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <6a0a2b8c-b942-4029-bf3c-4f20c4492795@amd.com>
[-- Attachment #1: Type: text/plain, Size: 6145 bytes --]
On Mon, 9 Oct 2023, Shyam Sundar S K wrote:
> On 10/4/2023 5:30 PM, Ilpo Järvinen wrote:
> > On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> >
> >> PMF Policy binary is a encrypted and signed binary that will be part
> >> of the BIOS. PMF driver via the ACPI interface checks the existence
> >> of Smart PC bit. If the advertised bit is found, PMF driver walks
> >> the acpi namespace to find out the policy binary size and the address
> >> which has to be passed to the TA during the TA init sequence.
> >>
> >> The policy binary is comprised of inputs (or the events) and outputs
> >> (or the actions). With the PMF ecosystem, OEMs generate the policy
> >> binary (or could be multiple binaries) that contains a supported set
> >> of inputs and outputs which could be specifically carved out for each
> >> usage segment (or for each user also) that could influence the system
> >> behavior either by enriching the user experience or/and boost/throttle
> >> power limits.
> >>
> >> Once the TA init command succeeds, the PMF driver sends the changing
> >> events in the current environment to the TA for a constant sampling
> >> frequency time (the event here could be a lid close or open) and
> >> if the policy binary has corresponding action built within it, the
> >> TA sends the action for it in the subsequent enact command.
> >>
> >> If the inputs sent to the TA has no output defined in the policy
> >> binary generated by OEMs, there will be no action to be performed
> >> by the PMF driver.
> >>
> >> Example policies:
> >>
> >> 1) if slider is performance ; set the SPL to 40W
> >> Here PMF driver registers with the platform profile interface and
> >> when the slider position is changed, PMF driver lets the TA know
> >> about this. TA sends back an action to update the Sustained
> >> Power Limit (SPL). PMF driver updates this limit via the PMFW mailbox.
> >>
> >> 2) if user_away ; then lock the system
> >> Here PMF driver hooks to the AMD SFH driver to know the user presence
> >> and send the inputs to TA and if the condition is met, the TA sends
> >> the action of locking the system. PMF driver generates a uevent and
> >> based on the udev rule in the userland the system gets locked with
> >> systemctl.
> >>
> >> The intent here is to provide the OEM's to make a policy to lock the
> >> system when the user is away ; but the userland can make a choice to
> >> ignore it.
> >>
> >> and so on.
> >>
> >> The OEMs will have an utility to create numerous such policies and
> >> the policies shall be reviewed by AMD before signing and encrypting
> >> them. Policies are shared between operating systems to have seemless user
> >> experience.
> >>
> >> Since all this action has to happen via the "amdtee" driver, currently
> >> there is no caller for it in the kernel which can load the amdtee driver.
> >> Without amdtee driver loading onto the system the "tee" calls shall fail
> >> from the PMF driver. Hence an explicit "request_module" has been added
> >> to address this.
> >>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> >> index 678dce4fea08..787f25511191 100644
> >> --- a/drivers/platform/x86/amd/pmf/core.c
> >> +++ b/drivers/platform/x86/amd/pmf/core.c
> >> @@ -384,6 +384,18 @@ static int amd_pmf_probe(struct platform_device *pdev)
> >> return -ENOMEM;
> >>
> >> dev->dev = &pdev->dev;
> >> + err = apmf_check_smart_pc(dev);
> >> + if (!err) {
> >> + /* in order for Smart PC solution to work it has a hard dependency
> >> + * on the amdtee driver to be loaded first even before the PMF driver
> >> + * loads. PMF ASL has a _CRS method that advertises the existence
> >> + * of Smart PC bit. If this information is present, use this to
> >> + * explicitly probe the amdtee driver, so that "tee" plumbing is done
> >> + * before the PMF Smart PC init happens.
> >> + */
> >
> > But please follow no-text on /* line formatting for multiline comments.
> > Also start with a capital letter.
> >
> >
> >> + if (request_module("amdtee"))
> >
> > Are you aware that this won't give you very strong guarantees about
> > anything if request_module()'s function comments is to be believed?
> >
> > If that's all what you're after, MODULE_SOFTDEP("pre: amdtee"); is
> > probably enough (and I unfortunately don't know the answer how to do it if
> > you want something stronger than that when you don't directly depend on
> > the symbols of the other module).
>
> MODULE_SOFTDEP("pre: amdtee"); did not help.
So how was this module loaded then? I suppose if the user does insmod, the
softdep wouldn't be honored but modprobe should load the dependencies
first.
> There is no consumer loading the 'amdtee' driver today in the kernel.
> Even now with this change, the pmf driver calls the TEE subsystem APIs
> that will eventually land in amdtee code.
>
> So the call flow would be:
> pmf driver-> tee subsystem -> amdtee driver -> ASP
Right, and that indirect route is why it won't be made as hard
dependency.
> IMO, in order to make this link work request_module() would be
> required. Is that OK to retain request_module() in v3?
Fine.
> >> + struct ta_pmf_enact_result policy_apply_table;
> >> + u32 rsvd[906];
> >
> > This is some size (SZ_1K?) - sizeof(ta_pmf_enact_result)? I don't know if
> > compiler would like such a construct though in the array declaration. If
> > the compiler isn't complaining it would be the most informative way to
> > state the size but if it's not happy, a comment might be useful.
>
> This is a reserved space for future use. And that's the same way
> defined in the FW as well. If I play with the sizes, the FW (PMF TA)
> starts to misbehave and does not provide outputs all the time.
>
> Like you mentioned, are you Ok if I just put a comment as "reserved
> space for future use"?
You can put the comment if you want but I understood that even without
the comment. I was just interested in how that magic number 906 was
derived and if it could have been better defined.
I think you can leave the line as is.
--
i.
^ permalink raw reply
* Re: [PATCH v2 12/16] platform/x86/amd/pmf: Add PMF-AMDGPU get interface
From: Ilpo Järvinen @ 2023-10-09 11:02 UTC (permalink / raw)
To: Shyam Sundar S K
Cc: Hans de Goede, markgross, basavaraj.natikar, jikos,
benjamin.tissoires, alexander.deucher, christian.koenig,
Xinhui.Pan, airlied, daniel, Patil.Reddy, mario.limonciello,
platform-driver-x86, linux-input, amd-gfx, dri-devel
In-Reply-To: <c5834d5e-af71-4d96-88ae-c2acd5f6604b@amd.com>
[-- Attachment #1: Type: text/plain, Size: 2615 bytes --]
On Mon, 9 Oct 2023, Shyam Sundar S K wrote:
> On 10/4/2023 6:19 PM, Ilpo Järvinen wrote:
> > On Sat, 30 Sep 2023, Shyam Sundar S K wrote:
> >
> >> In order to provide GPU inputs to TA for the Smart PC solution to work, we
> >> need to have interface between the PMF driver and the AMDGPU driver.
> >>
> >> Add the initial code path for get interface from AMDGPU.
> >>
> >> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
> >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >
> >> @@ -355,6 +356,21 @@ static int amd_pmf_get_bios_buffer(struct amd_pmf_dev *dev)
> >> return amd_pmf_start_policy_engine(dev);
> >> }
> >>
> >> +static int amd_pmf_get_gpu_handle(struct pci_dev *pdev, void *data)
> >> +{
> >> + struct amd_pmf_dev *dev = data;
> >> +
> >> + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->devfn == 0) {
> >> + /* get the amdgpu handle from the pci root after walking through the pci bus */
> >
> > I can see from the code that you assign to amdgpu handle so this comment
> > added no information.
> >
> > It doesn't really answer at all why you're doing this second step. Based
> > on the give parameters to pci_get_device(), it looks as if you're asking
> > for the same device you already have in pdev to be searched to you.
>
> Not sure if I understand you remark completely.
>
> amd_pmf_get_gpu_handle() is a callback function for pci_walk_bus
> (which is done below).
>
> What I am trying to do here is to get the PCI handle for the GPU
> device by walking the PCI bus.
>
> I think the 'pdev' here refers to the pci root, using that root we
> walk the entire tree and only stop walking when we find a handle to
> GPU device.
Not exactly what happens, in amd_pmf_get_gpu_handle() pdev changes on each
call so I don't know why you stated it is refering to the "pci root".
> Do you want me to change the "pdev" parameter to be renamed as "root" ?
No, please don't do that, it would be misleading.
> Am I missing something?
I meant that at some point of the walk through the PCI devices, you have
a PCI device pdev with ->vendor PCI_VENDOR_ID_AMD when that if condition
above matched. Please explain why you need to do another lookup with
pci_get_device() at that point (with the same ->vendor and ->device as
shown below)?
> >> + dev->gfx_data.gpu_dev = pci_get_device(pdev->vendor, pdev->device, NULL);
> >> + if (dev->gfx_data.gpu_dev) {
> >> + pci_dev_put(pdev);
> >> + return 1; /* stop walking */
> >> + }
> >> + }
> >> + return 0; /* continue walking */
--
i.
^ permalink raw reply
* [PATCH] Input: cyapa - add missing input core locking to suspend/resume functions
From: Marek Szyprowski @ 2023-10-09 12:10 UTC (permalink / raw)
To: linux-input, linux-kernel
Cc: Marek Szyprowski, Dmitry Torokhov, Andrzej Pietrasiewicz
In-Reply-To: <CGME20231009121026eucas1p19ed2a6a88fa6b899ef9b915a73ad87b5@eucas1p1.samsung.com>
Grab input->mutex during suspend/resume functions like it is done in
other input drivers. This fixes the following warning during system
suspend/resume cycle on Samsung Exynos5250-based Snow Chromebook:
------------[ cut here ]------------
WARNING: CPU: 1 PID: 1680 at drivers/input/input.c:2291 input_device_enabled+0x68/0x6c
Modules linked in: ...
CPU: 1 PID: 1680 Comm: kworker/u4:12 Tainted: G W 6.6.0-rc5-next-20231009 #14109
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_unbound async_run_entry_fn
unwind_backtrace from show_stack+0x10/0x14
show_stack from dump_stack_lvl+0x58/0x70
dump_stack_lvl from __warn+0x1a8/0x1cc
__warn from warn_slowpath_fmt+0x18c/0x1b4
warn_slowpath_fmt from input_device_enabled+0x68/0x6c
input_device_enabled from cyapa_gen3_set_power_mode+0x13c/0x1dc
cyapa_gen3_set_power_mode from cyapa_reinitialize+0x10c/0x15c
cyapa_reinitialize from cyapa_resume+0x48/0x98
cyapa_resume from dpm_run_callback+0x90/0x298
dpm_run_callback from device_resume+0xb4/0x258
device_resume from async_resume+0x20/0x64
async_resume from async_run_entry_fn+0x40/0x15c
async_run_entry_fn from process_scheduled_works+0xbc/0x6a8
process_scheduled_works from worker_thread+0x188/0x454
worker_thread from kthread+0x108/0x140
kthread from ret_from_fork+0x14/0x28
Exception stack(0xf1625fb0 to 0xf1625ff8)
...
---[ end trace 0000000000000000 ]---
...
------------[ cut here ]------------
WARNING: CPU: 1 PID: 1680 at drivers/input/input.c:2291 input_device_enabled+0x68/0x6c
Modules linked in: ...
CPU: 1 PID: 1680 Comm: kworker/u4:12 Tainted: G W 6.6.0-rc5-next-20231009 #14109
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_unbound async_run_entry_fn
unwind_backtrace from show_stack+0x10/0x14
show_stack from dump_stack_lvl+0x58/0x70
dump_stack_lvl from __warn+0x1a8/0x1cc
__warn from warn_slowpath_fmt+0x18c/0x1b4
warn_slowpath_fmt from input_device_enabled+0x68/0x6c
input_device_enabled from cyapa_gen3_set_power_mode+0x13c/0x1dc
cyapa_gen3_set_power_mode from cyapa_reinitialize+0x10c/0x15c
cyapa_reinitialize from cyapa_resume+0x48/0x98
cyapa_resume from dpm_run_callback+0x90/0x298
dpm_run_callback from device_resume+0xb4/0x258
device_resume from async_resume+0x20/0x64
async_resume from async_run_entry_fn+0x40/0x15c
async_run_entry_fn from process_scheduled_works+0xbc/0x6a8
process_scheduled_works from worker_thread+0x188/0x454
worker_thread from kthread+0x108/0x140
kthread from ret_from_fork+0x14/0x28
Exception stack(0xf1625fb0 to 0xf1625ff8)
...
---[ end trace 0000000000000000 ]---
Fixes: d69f0a43c677 ("Input: use input_device_enabled()")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/input/mouse/cyapa.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
index a84098448f5b..cf23f95b5f11 100644
--- a/drivers/input/mouse/cyapa.c
+++ b/drivers/input/mouse/cyapa.c
@@ -1347,10 +1347,16 @@ static int cyapa_suspend(struct device *dev)
u8 power_mode;
int error;
- error = mutex_lock_interruptible(&cyapa->state_sync_lock);
+ error = mutex_lock_interruptible(&cyapa->input->mutex);
if (error)
return error;
+ error = mutex_lock_interruptible(&cyapa->state_sync_lock);
+ if (error) {
+ mutex_unlock(&cyapa->input->mutex);
+ return error;
+ }
+
/*
* Runtime PM is enable only when device is in operational mode and
* users in use, so need check it before disable it to
@@ -1385,6 +1391,8 @@ static int cyapa_suspend(struct device *dev)
cyapa->irq_wake = (enable_irq_wake(client->irq) == 0);
mutex_unlock(&cyapa->state_sync_lock);
+ mutex_unlock(&cyapa->input->mutex);
+
return 0;
}
@@ -1394,6 +1402,7 @@ static int cyapa_resume(struct device *dev)
struct cyapa *cyapa = i2c_get_clientdata(client);
int error;
+ mutex_lock(&cyapa->input->mutex);
mutex_lock(&cyapa->state_sync_lock);
if (device_may_wakeup(dev) && cyapa->irq_wake) {
@@ -1412,6 +1421,7 @@ static int cyapa_resume(struct device *dev)
enable_irq(client->irq);
mutex_unlock(&cyapa->state_sync_lock);
+ mutex_unlock(&cyapa->input->mutex);
return 0;
}
--
2.34.1
^ permalink raw reply related
* [PATCH 6.5 071/163] HID: nvidia-shield: add LEDS_CLASS dependency
From: Greg Kroah-Hartman @ 2023-10-09 13:00 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, Randy Dunlap, Rahul Rameshbabu,
Jiri Kosina, Benjamin Tissoires, linux-input, Sasha Levin
In-Reply-To: <20231009130124.021290599@linuxfoundation.org>
6.5-stable review patch. If anyone has any objections, please let me know.
------------------
From: Randy Dunlap <rdunlap@infradead.org>
[ Upstream commit 058574879853260a22bbec1f94221dfc5149d85c ]
The hid-nvidia-shield driver uses functions that are built
only when LEDS_CLASS is set, so make the driver depend on that
symbol to prevent build errors.
riscv32-linux-ld: drivers/hid/hid-nvidia-shield.o: in function `.L11':
hid-nvidia-shield.c:(.text+0x192): undefined reference to `led_classdev_unregister'
riscv32-linux-ld: drivers/hid/hid-nvidia-shield.o: in function `.L113':
hid-nvidia-shield.c:(.text+0xfa4): undefined reference to `led_classdev_register_ext'
Fixes: 09308562d4af ("HID: nvidia-shield: Initial driver implementation with Thunderstrike support")
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Cc: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: linux-input@vger.kernel.org
Reviewed-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index e11c1c8036769..dc456c86e9569 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -792,6 +792,7 @@ config HID_NVIDIA_SHIELD
tristate "NVIDIA SHIELD devices"
depends on USB_HID
depends on BT_HIDP
+ depends on LEDS_CLASS
help
Support for NVIDIA SHIELD accessories.
--
2.40.1
^ permalink raw reply related
* [PATCH v2 0/3] Input: Add TouchNetix aXiom touchscreen driver
From: Kamel Bouhara @ 2023-10-09 13:44 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg
Cc: linux-input, linux-kernel, devicetree, mark.satterthwaite,
pedro.torruella, bartp, hannah.rossiter, Thomas Petazzoni,
Gregory Clement, bsp-development.geo, Kamel Bouhara
Add a new driver for the TouchNetix's aXiom family of
touchscreen controller. This driver only support i2c
and can be later adapted for SPI and USB support.
---
Changes in v2:
- Add device tree binding documentation
- Move core functions in axiom_i2c as we only care about i2c support now
- Use static function when required
- Use syntax dev_err_probe()
- Add an hardware based reset
Kamel Bouhara (3):
dt-bindings: vendor-prefixes: Add TouchNetix AS
dt-bindings: input: Add bindings for TouchNetix axiom touchscreen
Input: Add TouchNetix aXiom i2c touchscreen driver
.../touchscreen/touchnetix,axiom_ax54a.yaml | 52 ++
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 6 +
drivers/input/touchscreen/Kconfig | 13 +
drivers/input/touchscreen/Makefile | 1 +
.../input/touchscreen/touchnetix_axiom_i2c.c | 751 ++++++++++++++++++
6 files changed, 825 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom_ax54a.yaml
create mode 100644 drivers/input/touchscreen/touchnetix_axiom_i2c.c
--
2.25.1
^ permalink raw reply
* [PATCH v2 1/3] dt-bindings: vendor-prefixes: Add TouchNetix AS
From: Kamel Bouhara @ 2023-10-09 13:44 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg
Cc: linux-input, linux-kernel, devicetree, mark.satterthwaite,
pedro.torruella, bartp, hannah.rossiter, Thomas Petazzoni,
Gregory Clement, bsp-development.geo, Kamel Bouhara
In-Reply-To: <20231009134435.36311-1-kamel.bouhara@bootlin.com>
Add vendor prefix for TouchNetix AS (https://www.touchnetix.com/products/).
Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 573578db9509..78581001a79c 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1400,6 +1400,8 @@ patternProperties:
description: Toradex AG
"^toshiba,.*":
description: Toshiba Corporation
+ "^touchnetix,.*":
+ description: TouchNetix AS
"^toumaz,.*":
description: Toumaz
"^tpk,.*":
--
2.25.1
^ permalink raw reply related
* [PATCH v2 2/3] dt-bindings: input: Add bindings for TouchNetix axiom touchscreen
From: Kamel Bouhara @ 2023-10-09 13:44 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg
Cc: linux-input, linux-kernel, devicetree, mark.satterthwaite,
pedro.torruella, bartp, hannah.rossiter, Thomas Petazzoni,
Gregory Clement, bsp-development.geo, Kamel Bouhara
In-Reply-To: <20231009134435.36311-1-kamel.bouhara@bootlin.com>
Add the TouchNetix axiom I2C touchscreen device tree bindings
documentation.
Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
.../touchscreen/touchnetix,axiom-ax54a.yaml | 51 +++++++++++++++++++
MAINTAINERS | 6 +++
2 files changed, 57 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml
diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml
new file mode 100644
index 000000000000..41201d7112a6
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/touchnetix,axiom-ax54a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TouchNetix Axiom series touchscreen controller
+
+maintainers:
+ - Kamel Bouhara <kamel.bouhara@bootlin.com>
+
+properties:
+ compatible:
+ const: touchnetix,axiom-ax54a
+
+ reg:
+ const: 0x66
+
+ interrupts:
+ maxItems: 1
+
+ irq-gpios:
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ axiom@66 {
+ compatible = "touchnetix,axiom-ax54a";
+ reg = <0x66>;
+ interrupt-parent = <&gpio2>;
+ interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+ irq-gpios = <&gpio2 0 GPIO_ACTIVE_LOW>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
+ };
+ };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 389fe9e38884..12ae8bc6b8cf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21410,6 +21410,12 @@ S: Maintained
F: Documentation/ABI/testing/sysfs-class-firmware-attributes
F: drivers/platform/x86/think-lmi.?
+TOUCHNETIX AXIOM I2C TOUCHSCREEN DRIVER
+M: Kamel Bouhara <kamel.bouhara@bootlin.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml
+
THUNDERBOLT DMA TRAFFIC TEST DRIVER
M: Isaac Hazan <isaac.hazan@intel.com>
L: linux-usb@vger.kernel.org
--
2.25.1
^ permalink raw reply related
* [PATCH v2 3/3] Input: Add TouchNetix aXiom i2c touchscreen driver
From: Kamel Bouhara @ 2023-10-09 13:44 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg
Cc: linux-input, linux-kernel, devicetree, mark.satterthwaite,
pedro.torruella, bartp, hannah.rossiter, Thomas Petazzoni,
Gregory Clement, bsp-development.geo, Kamel Bouhara
In-Reply-To: <20231009134435.36311-1-kamel.bouhara@bootlin.com>
Add a new driver for the TouchNetix's aXiom family of
touchscreen controllers. This driver only supports i2c
and can be later adapted for SPI and USB support.
Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
MAINTAINERS | 1 +
drivers/input/touchscreen/Kconfig | 13 +
drivers/input/touchscreen/Makefile | 1 +
.../input/touchscreen/touchnetix_axiom_i2c.c | 747 ++++++++++++++++++
4 files changed, 762 insertions(+)
create mode 100644 drivers/input/touchscreen/touchnetix_axiom_i2c.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 12ae8bc6b8cf..2d1e0b025e89 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21415,6 +21415,7 @@ M: Kamel Bouhara <kamel.bouhara@bootlin.com>
L: linux-input@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml
+F: drivers/input/touchscreen/touchnetix_axiom_i2c.c
THUNDERBOLT DMA TRAFFIC TEST DRIVER
M: Isaac Hazan <isaac.hazan@intel.com>
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..58665ccbe077 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -803,6 +803,19 @@ config TOUCHSCREEN_MIGOR
To compile this driver as a module, choose M here: the
module will be called migor_ts.
+config TOUCHSCREEN_TOUCHNETIX_AXIOM_I2C
+ tristate "TouchNetix AXIOM based touchscreen controllers"
+ depends on I2C
+ depends on GPIOLIB || COMPILE_TEST
+ help
+ Say Y here if you have a axiom touchscreen connected to
+ your system.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called axiom_i2c.
+
config TOUCHSCREEN_TOUCHRIGHT
tristate "Touchright serial touchscreen"
select SERIO
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 62bd24f3ac8e..23b6fb8864b0 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_TOUCHSCREEN_SUR40) += sur40.o
obj-$(CONFIG_TOUCHSCREEN_SURFACE3_SPI) += surface3_spi.o
obj-$(CONFIG_TOUCHSCREEN_TI_AM335X_TSC) += ti_am335x_tsc.o
obj-$(CONFIG_TOUCHSCREEN_TOUCHIT213) += touchit213.o
+obj-$(CONFIG_TOUCHSCREEN_TOUCHNETIX_AXIOM_I2C) += touchnetix_axiom_i2c.o
obj-$(CONFIG_TOUCHSCREEN_TOUCHRIGHT) += touchright.o
obj-$(CONFIG_TOUCHSCREEN_TOUCHWIN) += touchwin.o
obj-$(CONFIG_TOUCHSCREEN_TS4800) += ts4800-ts.o
diff --git a/drivers/input/touchscreen/touchnetix_axiom_i2c.c b/drivers/input/touchscreen/touchnetix_axiom_i2c.c
new file mode 100644
index 000000000000..d1683440ad91
--- /dev/null
+++ b/drivers/input/touchscreen/touchnetix_axiom_i2c.c
@@ -0,0 +1,747 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TouchNetix aXiom Touchscreen Driver
+ *
+ * Copyright (C) 2020-2023 TouchNetix Ltd.
+ *
+ * Author(s): Bart Prescott <bartp@baasheep.co.uk>
+ * Pedro Torruella <pedro.torruella@touchnetix.com>
+ * Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
+ * Hannah Rossiter <hannah.rossiter@touchnetix.com>
+ * Kamel Bouhara <kamel.bouhara@bootlin.com>
+ *
+ */
+
+#include <linux/crc16.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+
+/*
+ * Runtime TCP mode: device is executing normal code and is
+ * accessible via the Touch Controller Mode
+ */
+#define BOOT_TCP 0
+/*
+ * Bootloader BLP mode: device is executing bootloader and is
+ * accessible via the Boot Loader Protocol.
+ */
+#define BOOT_BLP 1
+#define AXIOM_PROX_LEVEL -128
+/*
+ * Register group u31 has 2 pages for usage table entries.
+ * (2 * AXIOM_COMMS_PAGE_SIZE) / AXIOM_U31_BYTES_PER_USAGE = 85
+ */
+#define AXIOM_U31_MAX_USAGES 85
+#define AXIOM_U31_BYTES_PER_USAGE 6
+#define AXIOM_U31_PAGE0_LENGTH 0x0C
+#define AXIOM_U31_BOOTMODE_MASK BIT(7)
+#define AXIOM_U31_FW_INFO_VARIANT_MASK GENMASK(6, 0)
+#define AXIOM_U31_FW_INFO_STATUS_MASK BIT(7)
+
+#define AXIOM_U41_MAX_TARGETS 10
+
+#define AXIOM_U46_AUX_CHANNELS 4
+#define AXIOM_U46_AUX_MASK GENMASK(11, 0)
+
+#define AXIOM_COMMS_MAX_USAGE_PAGES 3
+#define AXIOM_COMMS_PAGE_SIZE 256
+#define AXIOM_COMMS_OVERFLOW_MASK BIT(7)
+#define AXIOM_COMMS_REPORT_LEN_MASK GENMASK(7, 0)
+
+#define AXIOM_REBASELINE_CMD 0x03
+
+#define AXIOM_REPORT_USAGE_ID 0x34
+#define AXIOM_DEVINFO_USAGE_ID 0x31
+#define AXIOM_USAGE_2HB_REPORT_ID 0x01
+#define AXIOM_REBASELINE_USAGE_ID 0x02
+#define AXIOM_USAGE_2AUX_REPORT_ID 0x46
+#define AXIOM_USAGE_2DCTS_REPORT_ID 0x41
+
+#define AXIOM_PAGE_MASK GENMASK(15, 8)
+#define AXIOM_PAGE_OFFSET_MASK GENMASK(7, 0)
+
+struct axiom_devinfo {
+ char bootloader_fw_major;
+ char bootloader_fw_minor;
+ char bootmode;
+ u16 device_id;
+ char fw_major;
+ char fw_minor;
+ char fw_info_extra;
+ char tcp_revision;
+ u16 jedec_id;
+ char num_usages;
+ char silicon_revision;
+};
+
+/*
+ * Describes parameters of a specific usage, essenstially a single element of
+ * the "Usage Table"
+ */
+struct usage_entry {
+ char id;
+ char is_report;
+ char start_page;
+ char num_pages;
+};
+
+/*
+ * Holds state of a touch or target when detected prior a touch (eg.
+ * hover or proximity events).
+ */
+enum axiom_target_state {
+ TARGET_STATE_NOT_PRESENT = 0,
+ TARGET_STATE_PROX = 1,
+ TARGET_STATE_HOVER = 2,
+ TARGET_STATE_TOUCHING = 3,
+ TARGET_STATE_MIN = TARGET_STATE_NOT_PRESENT,
+ TARGET_STATE_MAX = TARGET_STATE_TOUCHING,
+};
+
+struct u41_target {
+ enum axiom_target_state state;
+ u16 x;
+ u16 y;
+ s8 z;
+ bool insert;
+ bool touch;
+};
+
+struct axiom_target_report {
+ u8 index;
+ u8 present;
+ u16 x;
+ u16 y;
+ s8 z;
+};
+
+struct axiom_cmd_header {
+ u16 target_address;
+ u16 length:15;
+ u16 read:1;
+};
+
+struct axiom_data {
+ struct axiom_devinfo devinfo;
+ struct device *dev;
+ struct gpio_desc *reset_gpio;
+ struct gpio_desc *irq_gpio;
+ struct i2c_client *client;
+ struct input_dev *input_dev;
+ u32 max_report_len;
+ u32 report_overflow_counter;
+ u32 report_counter;
+ char rx_buf[AXIOM_COMMS_MAX_USAGE_PAGES * AXIOM_COMMS_PAGE_SIZE];
+ struct u41_target targets[AXIOM_U41_MAX_TARGETS];
+ struct usage_entry usage_table[AXIOM_U31_MAX_USAGES];
+ bool usage_table_populated;
+};
+
+/*
+ * aXiom devices are typically configured to report
+ * touches at a rate of 100Hz (10ms). For systems
+ * that require polling for reports, 100ms seems like
+ * an acceptable polling rate.
+ * When reports are polled, it will be expected to
+ * occasionally observe the overflow bit being set
+ * in the reports. This indicates that reports are not
+ * being read fast enough.
+ */
+#define POLL_INTERVAL_DEFAULT_MS 100
+
+/**
+ * Translate usage/page/offset triplet into physical address.
+ *
+ * @usage: groups of registers
+ * @page: page to which the usage belongs to offset
+ * @offset: offset of the usage
+ * @return: target address on success, 0xFFFF on error
+ */
+static u16
+usage_to_target_address(struct axiom_data *ts, char usage, char page,
+ char offset)
+{
+ struct axiom_devinfo *device_info;
+ struct usage_entry *usage_table;
+ u32 i;
+
+ device_info = &ts->devinfo;
+ usage_table = ts->usage_table;
+
+ /* At the moment the convention is that u31 is always at physical address 0x0 */
+ if (!ts->usage_table_populated) {
+ if (usage == AXIOM_DEVINFO_USAGE_ID)
+ return ((page << 8) + offset);
+ else
+ return 0xffff;
+ }
+
+ for (i = 0; i < device_info->num_usages; i++) {
+ if (usage_table[i].id != usage)
+ continue;
+
+ if (page >= usage_table[i].num_pages) {
+ dev_err(ts->dev, "Invalid usage table! usage: %u, page: %u, offset: %u\n",
+ usage, page, offset);
+ return 0xffff;
+ }
+ }
+
+ return ((usage_table[i].start_page + page) << 8) + offset;
+}
+
+static int
+axiom_i2c_read(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
+{
+ struct axiom_data *ts = i2c_get_clientdata(client);
+ struct axiom_cmd_header cmd_header;
+ struct i2c_msg msg[2];
+ int ret;
+
+ cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
+ cmd_header.length = cpu_to_le16(len);
+ cmd_header.read = cpu_to_le16(1);
+
+ msg[0].addr = client->addr;
+ msg[0].flags = 0;
+ msg[0].len = sizeof(cmd_header);
+ msg[0].buf = (u8 *)&cmd_header;
+
+ msg[1].addr = client->addr;
+ msg[1].flags = I2C_M_RD;
+ msg[1].len = len;
+ msg[1].buf = (char *)buf;
+
+ ret = i2c_transfer(client->adapter, msg, 2);
+ if (ret != ARRAY_SIZE(msg)) {
+ dev_err(&client->dev,
+ "Failed reading usage %#x page %#x, error=%d\n",
+ usage, page, ret);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int
+axiom_i2c_write(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
+{
+ struct axiom_data *ts = i2c_get_clientdata(client);
+ struct axiom_cmd_header cmd_header;
+ struct i2c_msg msg[2];
+ int ret;
+
+ cmd_header.target_address = cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
+ cmd_header.length = cpu_to_le16(len);
+ cmd_header.read = 0;
+
+ msg[0].addr = client->addr;
+ msg[0].flags = 0;
+ msg[0].len = sizeof(cmd_header);
+ msg[0].buf = (u8 *)&cmd_header;
+
+ msg[1].addr = client->addr;
+ msg[1].flags = 0;
+ msg[1].len = len;
+ msg[1].buf = (char *)buf;
+
+ ret = i2c_transfer(client->adapter, msg, 2);
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "Failed to write usage %#x page %#x, error=%d\n", usage,
+ page, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+/*
+ * Decodes and populates the local Usage Table.
+ * Given a buffer of data read from page 1 onwards of u31 from an aXiom
+ * device.
+ */
+static u32 axiom_populate_usage_table(struct axiom_data *ts, char *rx_data)
+{
+ u32 usage_id = 0;
+ u32 max_report_len = 0;
+ struct axiom_devinfo *device_info;
+ struct usage_entry *usage_table;
+
+ device_info = &ts->devinfo;
+ usage_table = ts->usage_table;
+
+ for (usage_id = 0; usage_id < device_info->num_usages; usage_id++) {
+ u16 offset = (usage_id * AXIOM_U31_BYTES_PER_USAGE);
+ char id = rx_data[offset + 0];
+ char start_page = rx_data[offset + 1];
+ char num_pages = rx_data[offset + 2];
+ u32 max_offset = ((rx_data[offset + 3] & AXIOM_PAGE_OFFSET_MASK) + 1) * 2;
+
+ if (!num_pages)
+ usage_table[usage_id].is_report = true;
+
+ /* Store the entry into the usage table */
+ usage_table[usage_id].id = id;
+ usage_table[usage_id].start_page = start_page;
+ usage_table[usage_id].num_pages = num_pages;
+
+ dev_dbg(ts->dev, "Usage %2u Info: %*ph\n", usage_id,
+ AXIOM_U31_BYTES_PER_USAGE,
+ &rx_data[offset]);
+
+ /* Identify the max report length the module will receive */
+ if (usage_table[usage_id].is_report && max_offset > max_report_len)
+ max_report_len = max_offset;
+ }
+ ts->usage_table_populated = true;
+
+ return max_report_len;
+}
+
+/* Retrieve, store and print the axiom device information */
+static int axiom_discover(struct axiom_data *ts)
+{
+ struct axiom_devinfo *devinfo = &ts->devinfo;
+ struct device *dev = ts->dev;
+ char *rx_data = ts->rx_buf;
+ int ret;
+
+ /*
+ * Fetch the first page of usage u31 to get the
+ * device information and the number of usages
+ */
+ ret = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 0, rx_data,
+ AXIOM_U31_PAGE0_LENGTH);
+ if (ret)
+ return ret;
+
+ devinfo->bootmode = (rx_data[0] & AXIOM_U31_BOOTMODE_MASK);
+ devinfo->device_id = ((rx_data[1] & AXIOM_PAGE_OFFSET_MASK) << 8) | rx_data[0];
+ devinfo->fw_minor = rx_data[2];
+ devinfo->fw_major = rx_data[3];
+ devinfo->fw_info_extra = rx_data[4];
+ devinfo->bootloader_fw_minor = rx_data[6];
+ devinfo->bootloader_fw_major = rx_data[7];
+ devinfo->jedec_id = (rx_data[8]) | (rx_data[9] << 8);
+ devinfo->num_usages = rx_data[10];
+ devinfo->silicon_revision = rx_data[11];
+
+ dev_dbg(dev, " Boot Mode: %s\n", ts->devinfo.bootmode ? "BLP" : "TCP");
+ dev_dbg(dev, " Device ID : %04x\n", ts->devinfo.device_id);
+ dev_dbg(dev, " Firmware Rev : %02x.%02x\n", ts->devinfo.fw_major,
+ ts->devinfo.fw_minor);
+ dev_dbg(dev, " Bootloader Rev : %02x.%02x\n",
+ ts->devinfo.bootloader_fw_major,
+ ts->devinfo.bootloader_fw_minor);
+ dev_dbg(dev, " FW Extra Info : %04x\n", ts->devinfo.fw_info_extra);
+ dev_dbg(dev, " Silicon : %02x\n", ts->devinfo.jedec_id);
+ dev_dbg(dev, " Num Usages : %04x\n", ts->devinfo.num_usages);
+
+ /* Read the second page of usage u31 to get the usage table */
+ ret = axiom_i2c_read(ts->client, AXIOM_DEVINFO_USAGE_ID, 1, rx_data,
+ (AXIOM_U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
+ if (ret)
+ return ret;
+
+ ts->max_report_len = axiom_populate_usage_table(ts, rx_data);
+ dev_dbg(dev, "Max Report Length: %u\n", ts->max_report_len);
+
+ return 0;
+}
+
+/*
+ * Support function to axiom_process_u41_report.
+ * Generates input-subsystem events for every target.
+ * After calling this function the caller shall issue
+ * a Sync to the input sub-system.
+ */
+static bool
+axiom_process_u41_report_target(struct axiom_data *ts,
+ struct axiom_target_report *target)
+{
+ struct input_dev *input_dev = ts->input_dev;
+ enum axiom_target_state current_state;
+ struct u41_target *target_prev_state;
+ struct device *dev = ts->dev;
+ bool update = false;
+ int slot;
+
+ /* Verify the target index */
+ if (target->index >= AXIOM_U41_MAX_TARGETS) {
+ dev_dbg(dev, "Invalid target index! %u\n", target->index);
+ return false;
+ }
+
+ target_prev_state = &ts->targets[target->index];
+
+ current_state = TARGET_STATE_NOT_PRESENT;
+
+ if (target->present) {
+ if (target->z >= 0)
+ current_state = TARGET_STATE_TOUCHING;
+ else if (target->z > AXIOM_PROX_LEVEL && target->z < 0)
+ current_state = TARGET_STATE_HOVER;
+ else if (target->z AXIOM_PROX_LEVEL)
+ current_state = TARGET_STATE_PROX;
+ }
+
+ if (target_prev_state->state == current_state &&
+ target_prev_state->x == target->x &&
+ target_prev_state->y == target->y &&
+ target_prev_state->z == target->z) {
+ return false;
+ }
+
+ slot = target->index;
+
+ dev_dbg(dev, "U41 Target T%u, slot:%u present:%u, x:%u, y:%u, z:%d\n",
+ target->index, slot, target->present,
+ target->x, target->y, target->z);
+
+ switch (current_state) {
+ case TARGET_STATE_NOT_PRESENT:
+ case TARGET_STATE_PROX:
+ if (target_prev_state->insert)
+ break;
+ update = true;
+ target_prev_state->insert = false;
+ input_mt_slot(input_dev, slot);
+
+ if (!slot)
+ input_report_key(input_dev, BTN_LEFT, 0);
+
+ input_mt_report_slot_inactive(input_dev);
+ /*
+ * make sure the previous coordinates are
+ * all off screen when the finger comes back
+ */
+ target->x = 65535;
+ target->y = 65535;
+ target->z = AXIOM_PROX_LEVEL;
+ break;
+ case TARGET_STATE_HOVER:
+ case TARGET_STATE_TOUCHING:
+ target_prev_state->insert = true;
+ update = true;
+ input_mt_slot(input_dev, slot);
+ input_report_abs(input_dev, ABS_MT_TRACKING_ID, slot);
+ input_report_abs(input_dev, ABS_MT_POSITION_X, target->x);
+ input_report_abs(input_dev, ABS_X, target->x);
+ input_report_abs(input_dev, ABS_MT_POSITION_Y, target->y);
+ input_report_abs(input_dev, ABS_Y, target->y);
+
+ if (current_state == TARGET_STATE_TOUCHING) {
+ input_report_abs(input_dev, ABS_MT_DISTANCE, 0);
+ input_report_abs(input_dev, ABS_DISTANCE, 0);
+ input_report_abs(input_dev, ABS_MT_PRESSURE, target->z);
+ input_report_abs(input_dev, ABS_PRESSURE, target->z);
+ } else {
+ input_report_abs(input_dev, ABS_MT_DISTANCE, -target->z);
+ input_report_abs(input_dev, ABS_DISTANCE, -target->z);
+ input_report_abs(input_dev, ABS_MT_PRESSURE, 0);
+ input_report_abs(input_dev, ABS_PRESSURE, 0);
+ }
+
+ if (!slot)
+ input_report_key(input_dev, BTN_LEFT, (current_state ==
+ TARGET_STATE_TOUCHING));
+ break;
+ }
+
+ target_prev_state->state = current_state;
+ target_prev_state->x = target->x;
+ target_prev_state->y = target->y;
+ target_prev_state->z = target->z;
+
+ if (update)
+ input_mt_sync_frame(input_dev);
+
+ return update;
+}
+
+/*
+ * Take a raw buffer with u41 report data and decode it.
+ * Also generate input events if needed.
+ * rx_buf: ptr to a byte array [0]: Usage number [1]: Status LSB [2]: Status MSB
+ */
+static void axiom_process_u41_report(struct axiom_data *ts, char *rx_buf)
+{
+ struct input_dev *input_dev = ts->input_dev;
+ struct axiom_target_report target;
+ bool update_done = false;
+ u16 target_status;
+ u32 i;
+
+ target_status = ((rx_buf[1]) | (rx_buf[2] << 8));
+
+ for (i = 0; i < AXIOM_U41_MAX_TARGETS; i++) {
+ char target_step = rx_buf[(i * 4)];
+
+ target.index = i;
+ target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
+ target.x = ((target_step + 3) | ((target_step + 4) << 8));
+ target.y = ((target_step + 5) | ((target_step + 6) << 8));
+ target.z = (s8)(rx_buf[i + 43]);
+ update_done |= axiom_process_u41_report_target(ts, &target);
+ }
+
+ if (update_done)
+ input_sync(input_dev);
+}
+
+static void axiom_process_u46_report(struct axiom_data *ts, char *rx_buf)
+{
+ struct input_dev *input_dev = ts->input_dev;
+ u32 event_value;
+ u16 aux_value;
+ u32 i = 0;
+
+ for (i = 0; i < AXIOM_U46_AUX_CHANNELS; i++) {
+ char target_step = rx_buf[(i * 2)];
+
+ aux_value = (((target_step + 2) << 8) | (target_step + 1)) & AXIOM_U46_AUX_MASK;
+ event_value = (i << 16) | (aux_value);
+ input_event(input_dev, EV_MSC, MSC_RAW, event_value);
+ }
+
+ input_mt_sync(input_dev);
+ input_sync(input_dev);
+}
+
+/*
+ * Validates the crc and demultiplexes the axiom reports to the appropriate
+ * report handler
+ */
+void axiom_handle_events(struct axiom_data *ts)
+{
+ char *report_data = ts->rx_buf;
+ struct device *dev = ts->dev;
+ char usage = report_data[1];
+ u16 crc_report;
+ u16 crc_calc;
+ char len;
+
+ axiom_i2c_read(ts->client, AXIOM_REPORT_USAGE_ID, 0, report_data, ts->max_report_len);
+
+ if ((report_data[0] & AXIOM_COMMS_OVERFLOW_MASK) != 0)
+ ts->report_overflow_counter++;
+
+ len = (report_data[0] & AXIOM_COMMS_REPORT_LEN_MASK) * 2;
+ if (!len) {
+ dev_err(dev, "Zero length report discarded.\n");
+ return;
+ }
+
+ dev_dbg(dev, "Payload Data %*ph\n", len, report_data);
+
+ /* Validate the report CRC */
+ crc_report = (report_data[len - 1] << 8) | (report_data[len - 2]);
+ /* Length is in 16 bit words and remove the size of the CRC16 itself */
+ crc_calc = crc16(0, report_data, (len - 2));
+
+ if (crc_calc != crc_report) {
+ dev_err(dev,
+ "CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
+ crc_report, crc_calc);
+ return;
+ }
+
+ switch (usage) {
+ case AXIOM_USAGE_2DCTS_REPORT_ID:
+ axiom_process_u41_report(ts, &report_data[1]);
+ break;
+
+ case AXIOM_USAGE_2AUX_REPORT_ID:
+ /* This is an aux report (force) */
+ axiom_process_u46_report(ts, &report_data[1]);
+ break;
+
+ case AXIOM_USAGE_2HB_REPORT_ID:
+ /* This is a heartbeat report */
+ break;
+ }
+
+ ts->report_counter++;
+}
+
+static void axiom_i2c_poll(struct input_dev *input_dev)
+{
+ struct axiom_data *ts = input_get_drvdata(input_dev);
+
+ axiom_handle_events(ts);
+}
+
+static irqreturn_t axiom_irq(int irq, void *dev_id)
+{
+ struct axiom_data *ts = dev_id;
+
+ axiom_handle_events(ts);
+
+ return IRQ_HANDLED;
+}
+
+static void axiom_reset(struct gpio_desc *reset_gpio)
+{
+ gpiod_set_value_cansleep(reset_gpio, 1);
+ usleep_range(1000, 2000);
+ gpiod_set_value_cansleep(reset_gpio, 0);
+ msleep(100);
+}
+
+/* Rebaseline the touchscreen, effectively zero-ing it */
+static int axiom_rebaseline(struct axiom_data *ts)
+{
+ char buffer[8] = {};
+
+ buffer[0] = AXIOM_REBASELINE_CMD;
+
+ return axiom_i2c_write(ts->client, AXIOM_REPORT_USAGE_ID, 0, buffer, sizeof(buffer));
+}
+
+static int axiom_i2c_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct input_dev *input_dev;
+ struct axiom_data *ts;
+ int ret;
+ int target;
+
+ ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+ if (!ts)
+ return -ENOMEM;
+
+ ts->client = client;
+ i2c_set_clientdata(client, ts);
+ ts->dev = dev;
+
+ ts->irq_gpio = devm_gpiod_get_optional(dev, "irq", GPIOD_IN);
+ if (IS_ERR(ts->irq_gpio))
+ return dev_err_probe(dev, PTR_ERR(ts->irq_gpio), "failed to get irq GPIO");
+
+ ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(ts->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(ts->reset_gpio), "failed to get reset GPIO\n");
+
+ axiom_reset(ts->reset_gpio);
+
+ if (ts->irq_gpio) {
+ ret = devm_request_threaded_irq(dev, client->irq, NULL,
+ axiom_irq, 0, dev_name(dev), ts);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to request threaded IRQ\n");
+ }
+
+ ret = axiom_discover(ts);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed touchscreen discover\n");
+
+ ret = axiom_rebaseline(ts);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed touchscreen re-baselining\n");
+
+ input_dev = devm_input_allocate_device(ts->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ input_dev->name = "TouchNetix aXiom Touchscreen";
+ input_dev->phys = "input/axiom_ts";
+
+ /* Single Touch */
+ input_set_abs_params(input_dev, ABS_X, 0, 65535, 0, 0);
+ input_set_abs_params(input_dev, ABS_Y, 0, 65535, 0, 0);
+
+ /* Multi Touch */
+ /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
+ input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
+ /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
+ input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
+
+ /* Registers the axiom device as a touchscreen instead of as a mouse pointer */
+ input_mt_init_slots(input_dev, AXIOM_U41_MAX_TARGETS, INPUT_MT_DIRECT);
+
+ input_set_capability(input_dev, EV_KEY, BTN_LEFT);
+
+ /* Enables the raw data for up to 4 force channels to be sent to the input subsystem */
+ set_bit(EV_REL, input_dev->evbit);
+ set_bit(EV_MSC, input_dev->evbit);
+ /* Declare that we support "RAW" Miscellaneous events */
+ set_bit(MSC_RAW, input_dev->mscbit);
+
+ if (!ts->irq_gpio) {
+ ret = input_setup_polling(input_dev, axiom_i2c_poll);
+ if (ret)
+ return dev_err_probe(ts->dev, ret, "Unable to set up polling mode\n");
+ input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
+ }
+
+ ts->input_dev = input_dev;
+ input_set_drvdata(ts->input_dev, ts);
+
+ /* Ensure that all reports are initialised to not be present. */
+ for (target = 0; target < AXIOM_U41_MAX_TARGETS; target++)
+ ts->targets[target].state = TARGET_STATE_NOT_PRESENT;
+
+ ret = input_register_device(input_dev);
+
+ if (ret)
+ return dev_err_probe(ts->dev, ret,
+ "Could not register with Input Sub-system.\n");
+
+ return 0;
+}
+
+static void axiom_i2c_remove(struct i2c_client *client)
+{
+ struct axiom_data *ts = i2c_get_clientdata(client);
+
+ input_unregister_device(ts->input_dev);
+}
+
+static const struct i2c_device_id axiom_i2c_id_table[] = {
+ { "axiom-ax54a" },
+ {},
+};
+
+MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
+
+static const struct of_device_id axiom_i2c_of_match[] = {
+ { .compatible = "touchnetix,axiom-ax54a", },
+ {}
+};
+
+MODULE_DEVICE_TABLE(of, axiom_i2c_of_match);
+
+static struct i2c_driver axiom_i2c_driver = {
+ .driver = {
+ .name = "axiom",
+ .of_match_table = axiom_i2c_of_match,
+ },
+ .id_table = axiom_i2c_id_table,
+ .probe = axiom_i2c_probe,
+ .remove = axiom_i2c_remove,
+};
+
+module_i2c_driver(axiom_i2c_driver);
+
+MODULE_AUTHOR("Bart Prescott <bartp@baasheep.co.uk>");
+MODULE_AUTHOR("Pedro Torruella <pedro.torruella@touchnetix.com>");
+MODULE_AUTHOR("Mark Satterthwaite <mark.satterthwaite@touchnetix.com>");
+MODULE_AUTHOR("Hannah Rossiter <hannah.rossiter@touchnetix.com>");
+MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
+MODULE_DESCRIPTION("TouchNetix aXiom touchscreen I2C bus driver");
+MODULE_LICENSE("GPL");
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v2 00/14] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Hans de Goede @ 2023-10-09 15:00 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input
In-Reply-To: <up2e4vgb24rb25cwnkhhrswusous2wyo376has23k2dakfdmgk@eb76ysbnz3yu>
Hi Benjamin,
On 10/9/23 10:13, Benjamin Tissoires wrote:
>
> Hi Hans,
>
> On Oct 08 2023, Hans de Goede wrote:
>> Hi,
>>
>> On 10/8/23 11:54, Hans de Goede wrote:
>>> Hi Benjamin,
>>>
>>> Here is a v2 of my series to fix issues with hidpp_connect_event() running
>>> while restarting IO, which now also fixes the issues you mentioned with
>>> potentially missing connect events.
>
> Great, thanks a lot for this hard work.
You're welcome.
>>> This series is best explained by a brief sketch of how probe()
>>> looks at the end of the series (1):
>
> TBH, I couldn't parse the following yesterday evening, but after looking
> at all patches one by one I can now get it :)
I'm glad you get it now :)
>
>>>
>>> Prep work:
>>>
>>> 1. All code depending on a device being in connected state is moved to
>>> the hidpp_connect_event() workqueue item
>>>
>>> 2. hidpp_connect_event() now checks the connected state itself by
>>> checking that hidpp_root_get_protocol_version() succeeds, instead
>>> of relying on possibly stale (racy) data in struct hidpp_device
>>>
>>> With this in place the new probe() sequence looks like this:
>>>
>>> 1. enable_connect_event flag starts at 0, this filters out / ignores any
>>> connect-events in hidpp_raw_hidpp_event() avoiding
>>> hidpp_connect_event() getting queued before the IO restart
>>>
>>> 2. IO is started with connect-mask = 0
>>> this avoids hid-input and hidraw connecting while probe() is setting
>>> hdev->name and hdev->uniq
>>>
>>> 3. Name and serialnr are retrieved and stored in hdev
>>>
>>> 4. IO is fully restarted (including hw_open + io_start, not just hw_start)
>>> with the actual connect-mask.
>>>
>>> 5. enable_connect_event atomic_t is set to 1 to enable processing of
>>> connect events.
>>>
>>> 6. hidpp_connect_event() is queued + flushed to query the connected state
>>> and do the connect work if the device is connected.
>>>
>>> 7. probe() now ends with:
>>>
>>> /*
>>> * This relies on logi_dj_ll_close() being a no-op so that
>>> * DJ connection events will still be received.
>>> */
>>> hid_hw_close(hdev);
>>>
>>> Since on restarting IO it now is fully restarted so the hid_hw_open()
>>> there needs to be balanced.
>>>
>>> This series now obviously is no longer 6.6 material, instead I hope we
>>> can get this rework (and IMHO nice cleanup) into 6.7 .
>
> Yeah, not 6.6 anymore, but should be doable for 6.7.
>
>>>
>>> Regards,
>>>
>>> Hans
>>
>> I forgot to add info on the list of devices I tested this on:
>>
>> Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0)
>> Logitech M720 Triathlon (bluetooth, HID++ 4.5)
>> Logitech K400 Pro (unifying, HID++ 4.1)
>> Logitech K270 (eQUAD nano Lite, HID++ 2.0)
>> Logitech M185 (eQUAD nano Lite, HID++ 4.5)
>> Logitech Keyboard LX501 (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0)
>> Logitech 27Mhz mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0)
>
> We should probably add this list to the commit messages.
> I'll need to test also myself on some problematic devices that have a
> special case (WTP, USB wired, BLE).
So you want to add this to all 14 commit messages ?
> Anyway, I'll have to test everything, but this looks like it's in a
> better shape than previously.
>
> However, the thing I am afraid is that commit 498ba2069035 ("HID:
> logitech-hidpp: Don't restart communication if not necessary") was
> fixing devices that did not like the hid_hw_stop/start. I can't find the
> bug numbers however... So with your series, we might breaking those
> once again.
>
> How about we do the following (in pseudo code):
> probe():
> hidpp_connect_and_start(connect_mask = 0)
> // retrieve name and serial
> hid_connect(connect_mask) // with connect_mask ensuring we don't
> // create inputs if HIDPP_QUIRK_DELAYED_INIT
> // is set, instead of stop/start
> hid_hw_close(hdev); // to balance hidpp_connect_and_start()
>
> I think the above should even remove the need for the
> enable_connect_event atomic_t given that now we are not restarting the
> devices at all.
Interesting yes that looks good, any idea why this was not done
in commit 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable")
right away ?
Let me rework the series to use that tomorrow. This will probably also
allow dropping a bunch of the patches.
Regards,
Hans
>
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>> 1) For reviewing you may also want to apply the entire series and look
>>> at the end result to help you understand why various intermediate steps
>>> are taken.
>>>
>>>
>>> Hans de Goede (14):
>>> HID: logitech-hidpp: Revert "Don't restart communication if not
>>> necessary"
>>> HID: logitech-hidpp: Move hidpp_overwrite_name() to before connect
>>> check
>>> HID: logitech-hidpp: Add hidpp_non_unifying_init() helper
>>> HID: logitech-hidpp: Remove connected check for non-unifying devices
>>> HID: logitech-hidpp: Move get_wireless_feature_index() check to
>>> hidpp_connect_event()
>>> HID: logitech-hidpp: Remove wtp_get_config() call from probe()
>>> HID: logitech-hidpp: Remove connected check for g920_get_config() call
>>> HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
>>> HID: logitech-hidpp: Move the connected check to after restarting IO
>>> HID: logitech-hidpp: Move g920_get_config() to just before
>>> hidpp_ff_init()
>>> HID: logitech-hidpp: Remove unused connected param from *_connect()
>>> HID: logitech-hidpp: Fix connect event race
>>> HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe()
>>> restarts IO
>>> HID: logitech-hidpp: Drop delayed_work_cb()
>>>
>>> drivers/hid/hid-logitech-hidpp.c | 211 +++++++++++++------------------
>>> 1 file changed, 91 insertions(+), 120 deletions(-)
>>>
>
> I like when the total number of deletions is higher than the additions
> :)
>
> Cheers,
> Benjamin
>
>>
>
^ permalink raw reply
* Re: [PATCH v2 1/3] dt-bindings: vendor-prefixes: Add TouchNetix AS
From: Krzysztof Kozlowski @ 2023-10-09 15:03 UTC (permalink / raw)
To: Kamel Bouhara, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Henrik Rydberg
Cc: linux-input, linux-kernel, devicetree, mark.satterthwaite,
pedro.torruella, bartp, hannah.rossiter, Thomas Petazzoni,
Gregory Clement, bsp-development.geo
In-Reply-To: <20231009134435.36311-2-kamel.bouhara@bootlin.com>
On 09/10/2023 15:44, Kamel Bouhara wrote:
> Add vendor prefix for TouchNetix AS (https://www.touchnetix.com/products/).
>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 2/3] dt-bindings: input: Add bindings for TouchNetix axiom touchscreen
From: Krzysztof Kozlowski @ 2023-10-09 15:05 UTC (permalink / raw)
To: Kamel Bouhara, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Henrik Rydberg
Cc: linux-input, linux-kernel, devicetree, mark.satterthwaite,
pedro.torruella, bartp, hannah.rossiter, Thomas Petazzoni,
Gregory Clement, bsp-development.geo
In-Reply-To: <20231009134435.36311-3-kamel.bouhara@bootlin.com>
On 09/10/2023 15:44, Kamel Bouhara wrote:
> Add the TouchNetix axiom I2C touchscreen device tree bindings
> documentation.
A nit, subject: drop second/last, redundant "bindings for". The
"dt-bindings" prefix is already stating that these are bindings.
>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
> .../touchscreen/touchnetix,axiom-ax54a.yaml | 51 +++++++++++++++++++
> MAINTAINERS | 6 +++
> 2 files changed, 57 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml b/Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml
> new file mode 100644
> index 000000000000..41201d7112a6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/touchnetix,axiom-ax54a.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/touchnetix,axiom-ax54a.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TouchNetix Axiom series touchscreen controller
> +
> +maintainers:
> + - Kamel Bouhara <kamel.bouhara@bootlin.com>
> +
> +properties:
> + compatible:
> + const: touchnetix,axiom-ax54a
> +
> + reg:
> + const: 0x66
> +
> + interrupts:
> + maxItems: 1
> +
> + irq-gpios:
> + maxItems: 1
Why these are GPIOs? Interrupts are usually just interrupts... You need
to clearly describe this.
> +
> + reset-gpios:
> + maxItems: 1
> +
> +additionalProperties: false
This goes after required: block.
> +
> +required:
> + - compatible
> + - reg
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + axiom@66 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "touchnetix,axiom-ax54a";
> + reg = <0x66>;
> + interrupt-parent = <&gpio2>;
> + interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
> + irq-gpios = <&gpio2 0 GPIO_ACTIVE_LOW>;
Eh? This looks really wrong.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad
From: Conor Dooley @ 2023-10-09 16:21 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shuah Khan,
linux-kernel-mentees
In-Reply-To: <20231008185709.2448423-1-anshulusr@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]
On Mon, Oct 09, 2023 at 12:27:06AM +0530, Anshul Dalal wrote:
> A simple driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> For testing on a RPi Zero 2W, I utilized the following devicetree overlay:
>
> /dts-v1/;
> /plugin/;
> / {
> compatible = "brcm,bcm2835";
> fragment@0 {
> target = <&i2c1>;
> __overlay__ {
> #address-cells = <1>;
> #size-cells = <0>;
> joystick@50 {
> compatible = "adafruit,seesaw-gamepad";
> reg = <0x50>;
> };
> };
> };
> };
>
> I used the above overlay as reference for writing this binding. Though the
> gamepad also has an interrupt pin that needs to be enabled explicitly (not
> currently implemented in driver). The pin triggers a rising edge when a
> button is pressed or joystick is moved which can be detected on a GPIO
> of the Microcontroller.
>
> I wasn't sure how to represent that functionality in the binding so I have
> left it out for now.
>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
>
> Changes for v3:
> - Updated id field to reflect updated file name from previous version
> - Added `reg` property
>
> Changes for v2:
> - Renamed file to `adafruit,seesaw-gamepad.yaml`
> - Removed quotes for `$id` and `$schema`
> - Removed "Bindings for" from the description
> - Changed node name to the generic name "joystick"
> - Changed compatible to 'adafruit,seesaw-gamepad' instead of 'adafruit,seesaw_gamepad'
>
> .../input/adafruit,seesaw-gamepad.yaml | 54 +++++++++++++++++++
> 1 file changed, 54 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
> new file mode 100644
> index 000000000000..610c99594439
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
> +$id: http://devicetree.org/schemas/input/adafruit-seesaw.yaml#
You're at version 3 now, but do not seem to have figured out how to test
the bindings?
https://docs.kernel.org/devicetree/bindings/writing-schema.html#running-checks
Thanks,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox