* Re: [PATCH] Input: add driver for TouchNetix aXiom touchscreen
From: Kamel Bouhara @ 2023-09-17 8:04 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: linux-input, mark.satterthwaite, pedro.torruella, bartp,
hannah.rossiter, Thomas Petazzoni, Gregory Clement,
bsp-development.geo
In-Reply-To: <42d9358d-b5ef-f68f-45e0-088cde4361a5@kernel.org>
Le Mon, Sep 11, 2023 at 08:59:21AM +0200, Krzysztof Kozlowski a écrit :
> On 08/09/2023 17:32, Kamel Bouhara wrote:
> > Add a new driver for the TouchNetix's aXiom family of
> > multi-touch controller. This driver only support i2c
> > and can be later adapted for SPI and USB support.
> >
> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > ---
> > .../devicetree/bindings/vendor-prefixes.yaml | 2 +
>
>
> Thank you for your patch. There is something to discuss/improve.
>
> 1. Bindings are separate patches.
>
> I am surprised that you did not send this through your collegagues at
> Bootlin for some internal review. It would spot such easy things to fix.
>
> 2. Please use subject prefixes matching the subsystem. You can get them
> for example with `git log --oneline -- DIRECTORY_OR_FILE` on the
> directory your patch is touching.
>
> 3. Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
>
> 4. Please use scripts/get_maintainers.pl to get a list of necessary
> people and lists to CC. It might happen, that command when run on an
> older kernel, gives you outdated entries. Therefore please be sure you
> base your patches on recent Linux kernel.
>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time, thus I will skip this patch entirely till you follow
> the process allowing the patch to be tested.
>
> Please kindly resend and include all necessary To/Cc entries.
>
> Limited review follows.
Hello Krzysztof,
Sorry for this late answer and thanks for this not so limited review,
much appreciate :) !
>
>
> > MAINTAINERS | 7 +
> > drivers/input/touchscreen/Kconfig | 11 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/axiom_core.c | 382 ++++++++++++++++++
> > drivers/input/touchscreen/axiom_core.h | 140 +++++++
> > drivers/input/touchscreen/axiom_i2c.c | 349 ++++++++++++++++
> > 7 files changed, 892 insertions(+)
> > create mode 100644 drivers/input/touchscreen/axiom_core.c
> > create mode 100644 drivers/input/touchscreen/axiom_core.h
> > create mode 100644 drivers/input/touchscreen/axiom_i2c.c
> >
> > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > index 573578db9509..b0a3ed451f15 100644
> > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > @@ -175,6 +175,8 @@ patternProperties:
> > "^awinic,.*":
> > description: Shanghai Awinic Technology Co., Ltd.
> > "^axentia,.*":
> > + description: TouchNetix
> > + "^axiom,.*":
> > description: Axentia Technologies AB
> > "^axis,.*":
> > description: Axis Communications AB
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 389fe9e38884..43add48257d8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3373,6 +3373,13 @@ W: https://ez.analog.com/linux-software-drivers
> > F: Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
> > F: drivers/hwmon/axi-fan-control.c
> >
> > +AXIOM I2C TOUCHSCREEN DRIVER
> > +M: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > +L: linux-input@vger.kernel.org
> > +S: Maintained
> > +F: drivers/input/touchscreen/axiom_core.c
> > +F: drivers/input/touchscreen/axiom_i2.c
> > +
> > AXXIA I2C CONTROLLER
> > M: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> > L: linux-i2c@vger.kernel.org
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index e3e2324547b9..08a770a0c5e5 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -150,6 +150,17 @@ config TOUCHSCREEN_AUO_PIXCIR
> > To compile this driver as a module, choose M here: the
> > module will be called auo-pixcir-ts.
> >
> > +config TOUCHSCREEN_AXIOM_I2C
> > + tristate "AXIOM based multi-touch panel controllers"
> > + 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_BU21013
> > tristate "BU21013 based touch panel controllers"
> > depends on I2C
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 62bd24f3ac8e..59a3234ddb09 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_TOUCHSCREEN_ADS7846) += ads7846.o
> > obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C) += ar1021_i2c.o
> > obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT) += atmel_mxt_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR) += auo-pixcir-ts.o
> > +obj-$(CONFIG_TOUCHSCREEN_AXIOM_I2C) += axiom_core.o axiom_i2c.o
> > obj-$(CONFIG_TOUCHSCREEN_BU21013) += bu21013_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_BU21029) += bu21029_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_CHIPONE_ICN8318) += chipone_icn8318.o
> > diff --git a/drivers/input/touchscreen/axiom_core.c b/drivers/input/touchscreen/axiom_core.c
> > new file mode 100644
> > index 000000000000..d381afd7fb84
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/axiom_core.c
> > @@ -0,0 +1,382 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * TouchNetix aXiom Touchscreen Driver
> > + *
> > + * Copyright (C) 2020-2023 TouchNetix Ltd.
> > + *
> > + * Author(s): Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> > + * Pedro Torruella <pedro.torruella@touchnetix.com>
> > + * Bart Prescott <bartp@baasheep.co.uk>
> > + * Hannah Rossiter <hannah.rossiter@touchnetix.com>
> > + *
> > + */
> > +
> > +#include <linux/crc16.h>
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +
> > +#include "axiom_core.h"
> > +
> > +/**
> > + * Decodes and populates the local u31 structure.
> > + * Given a buffer of data read from page 0 of u31 in an aXiom device.
> > + */
> > +void axiom_get_dev_info(struct axiom_data *ts, char *info)
> > +{
> > + struct axiom_devinfo *devinfo = &ts->devinfo;
> > +
> > + if (devinfo) {
> > + devinfo->bootloader_mode = ((info[1] & 0x80) != 0) ? 1 : 0;
> > + devinfo->device_id = ((info[1] & 0x7F) << 8) | info[0];
> > + devinfo->fw_minor = info[2];
> > + devinfo->fw_major = info[3];
> > + devinfo->fw_info_extra = (info[4]) | (info[5] << 8);
> > + devinfo->bootloader_fw_ver_minor = info[6];
> > + devinfo->bootloader_fw_ver_major = info[7];
> > + devinfo->jedec_id = (info[8]) | (info[9] << 8);
> > + devinfo->num_usages = info[10];
> > + devinfo->silicon_revision = info[11];
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_get_dev_info);
>
> Nope
I guess this means no need to export the function ?
I agree this is not required currently as we only have support the i2c
variant.
>
> > +
> > +/**
> > + * Decodes and populates the local Usage Table.
> > + * Given a buffer of data read from page 1 onwards of u31 from an aXiom
> > + * device.
> > + */
> > +char axiom_populate_usage_table(struct axiom_data *ts, char *rx_data)
> > +{
> > + u32 usage_id = 0;
> > + char 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 * U31_BYTES_PER_USAGE);
> > + char id = rx_data[offset + 0];
> > + char start_page = rx_data[offset + 1];
> > + char num_pages = rx_data[offset + 2];
> > + char max_offset = ((rx_data[offset + 3] & 0x7F) + 1) * 2;
> > +
> > + /* Store the entry into the usage table */
> > + usage_table[usage_id].id = id;
> > + usage_table[usage_id].is_report = ((num_pages == 0) ? 1 : 0);
> > + usage_table[usage_id].start_page = start_page;
> > + usage_table[usage_id].num_pages = num_pages;
> > +
> > + dev_dbg(ts->pdev, "Usage %2u Info: %*ph\n", usage_id,
> > + 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;
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_populate_usage_table);
> > +
> > +/**
> > + * Translate usage/page/offset triplet into physical address.
> > + *
> > + * @usage: groups of registers
> > + * @page: page to which the usage belongs to offset
> > + * @offset of the usage
> > + */
> > +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;
> > + u16 target_address;
> > + 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 && (usage == DEVINFO_USAGE_ID)) {
> > + target_address = ((page << 8) + offset);
> > + } else if (ts->usage_table_populated) {
> > + for (i = 0; i < device_info->num_usages; i++) {
> > + if (usage_table[i].id == usage) {
> > + if (page < usage_table[i].num_pages) {
> > + target_address =
> > + ((usage_table[i].start_page + page) << 8) + offset;
> > + } else {
> > + target_address = 0xFFFF;
> > + dev_err(ts->pdev,
> > + "Invalid usage table! usage: %u, page: %u, offset: %u\n",
> > + usage, page, offset);
> > + }
> > + break;
> > + }
> > + }
> > + } else {
> > + target_address = 0xFFFF;
> > + dev_err(ts->pdev, "Unpopulated usage table for usage: %u\n",
> > + usage);
> > + }
> > +
> > + dev_dbg(ts->pdev, "target_address is 0x%04x for usage: %u page %u\n",
> > + target_address, usage, page);
> > +
> > + return target_address;
> > +}
> > +EXPORT_SYMBOL_GPL(usage_to_target_address);
>
> Nope, drop.
>
> > +
> > +void axiom_remove(struct axiom_data *ts)
> > +{
> > + if (ts->usage_table)
> > + ts->usage_table_populated = false;
> > +
> > + if (ts->input_dev)
> > + input_unregister_device(ts->input_dev);
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_remove);
>
> Nope, drop.
>
> > +
>
>
> > +
> > +/**
> > + * 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
> > + */
> > +void axiom_process_u41_report(struct axiom_data *ts, char *rx_buf)
> > +{
> > + struct axiom_target_report target;
> > + struct input_dev *input_dev = ts->input_dev;
> > + bool update_done = false;
> > + u16 target_status;
> > + u32 i;
> > +
> > + if (rx_buf[0] != 0x41) {
> > + dev_err(ts->pdev,
> > + "Data in buffer does not have expected u41 format.\n");
> > + return;
> > + }
> > +
> > + target_status = ((rx_buf[1]) | (rx_buf[2] << 8));
> > +
> > + for (i = 0; i < U41_MAX_TARGETS; i++) {
> > + target.index = i;
> > + target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
> > + target.x = (rx_buf[(i * 4) + 3]) | (rx_buf[(i * 4) + 4] << 8);
> > + target.y = (rx_buf[(i * 4) + 5]) | (rx_buf[(i * 4) + 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);
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_process_u41_report);
>
> Nope, drop.
>
> > +
> > +void axiom_process_u46_report(struct axiom_data *ts, char *rx_buf)
> > +{
> > + struct input_dev *input_dev = ts->input_dev;
> > + u16 aux_value;
> > + u32 event_value;
> > + u32 i = 0;
> > +
> > + for (i = 0; i < U46_AUX_CHANNELS; i++) {
> > + aux_value =
> > + ((rx_buf[(i * 2) + 2] << 8) | (rx_buf[(i * 2) + 1])) &
> > + 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);
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_process_u46_report);
>
> Nope, drop.
>
> Clean your driver before sending upstream. :(
>
> > +
> > +/**
> > + * Support function to axiom_process_report.
> > + * It validates the crc and multiplexes the axiom reports to the appropriate
> > + * report handler
> > + */
> > +void axiom_process_report(struct axiom_data *ts, char *report_data)
> > +{
> > + struct device *pdev = ts->pdev;
> > + char usage = report_data[1];
> > + u16 crc_report;
> > + u16 crc_calc;
> > + char len;
> > +
> > + if ((report_data[0] & COMMS_OVERFLOW_MSK) != 0)
> > + ts->report_overflow_counter++;
> > +
> > + len = (report_data[0] & COMMS_REPORT_LEN_MSK) << 1;
> > + if (len == 0) {
> > + dev_err(pdev, "Zero length report discarded.\n");
> > + return;
> > + }
> > +
> > + dev_dbg(pdev, "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(pdev,
> > + "CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
> > + crc_report, crc_calc);
> > + return;
> > + }
> > +
> > + switch (usage) {
> > + case USAGE_2DCTS_REPORT_ID:
> > + axiom_process_u41_report(ts, &report_data[1]);
> > + break;
> > +
> > + case USAGE_2AUX_REPORT_ID:
> > + /* This is an aux report (force) */
> > + axiom_process_u46_report(ts, &report_data[1]);
> > + break;
> > +
> > + case USAGE_2HB_REPORT_ID:
> > + /* This is a heartbeat report */
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > +
> > + ts->report_counter++;
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_process_report);
>
> Nope, drop.
>
> > +
> > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> > +MODULE_DESCRIPTION("aXiom touchscreen core logic");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("axiom");
>
> ???? How can you have multiple drivers with the same alias? Even if your
> Makefile allowed it (but it doesn't!), it would not make sense.
>
>
> > diff --git a/drivers/input/touchscreen/axiom_core.h b/drivers/input/touchscreen/axiom_core.h
> > new file mode 100644
> > index 000000000000..f129d28671ff
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/axiom_core.h
> > @@ -0,0 +1,140 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * TouchNetix aXiom Touchscreen Driver
> > + *
> > + * Copyright (C) 2020-2023 TouchNetix Ltd.
> > + *
> > + * Author(s): Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> > + * Pedro Torruella <pedro.torruella@touchnetix.com>
> > + * Bart Prescott <bartp@baasheep.co.uk>
> > + * Hannah Rossiter <hannah.rossiter@touchnetix.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
>
> OK, so this is some old, vendor crappy code. We do not have it since
> some years!
Yes :).
>
> > + *
>
> ...
>
> > +
> > +static void axiom_i2c_poll(struct input_dev *input_dev)
> > +{
> > + struct axiom_data *ts = input_get_drvdata(input_dev);
> > + char *rx_data = ts->rx_buf;
> > +
> > + axiom_i2c_read(ts->client, REPORT_USAGE_ID, 0, rx_data,
> > + ts->max_report_len);
> > + axiom_process_report(ts, rx_data);
> > +}
> > +
> > +/**
> > + * Retrieve, store and print the axiom device information
> > + */
> > +int axiom_discover(struct axiom_data *ts)
>
> This has to be static.
ok.
>
> > +{
> > + char *rx_data = &ts->rx_buf[0];
> > + struct device *pdev = ts->pdev;
> > + int ret;
> > +
> > + /* First the first page of u31 to get the device information and */
> > + /* the number of usages */
> > + ret =
> > + axiom_i2c_read(ts->client, DEVINFO_USAGE_ID, 0, rx_data,
> > + AX_U31_PAGE0_LENGTH);
> > + if (ret)
> > + return ret;
> > +
> > + axiom_get_dev_info(ts, rx_data);
> > +
> > + dev_dbg(pdev, "Data Decode:\n");
> > + dev_dbg(pdev, " Bootloader Mode: %u\n", ts->devinfo.bootloader_mode);
> > + dev_dbg(pdev, " Device ID : %04x\n", ts->devinfo.device_id);
> > + dev_dbg(pdev, " Firmware Rev : %02x.%02x\n", ts->devinfo.fw_major,
> > + ts->devinfo.fw_minor);
> > + dev_dbg(pdev, " Bootloader Rev : %02x.%02x\n",
> > + ts->devinfo.bootloader_fw_ver_major,
> > + ts->devinfo.bootloader_fw_ver_minor);
> > + dev_dbg(pdev, " FW Extra Info : %04x\n", ts->devinfo.fw_info_extra);
> > + dev_dbg(pdev, " Silicon : %02x\n", ts->devinfo.jedec_id);
> > + dev_dbg(pdev, " Num Usages : %04x\n", ts->devinfo.num_usages);
> > +
> > + /* Read the second page of u31 to get the usage table */
> > + ret = axiom_i2c_read(ts->client, DEVINFO_USAGE_ID, 1, rx_data,
> > + (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(pdev, "Max Report Length: %u\n", ts->max_report_len);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_discover);
>
> Nope.
>
> > +
> > +/**
> > + * Rebaseline the touchscreen, effectively zero-ing it
> > + */
> > +void axiom_rebaseline(struct axiom_data *ts)
>
> Missing static.
>
> > +{
> > + struct device *pdev = ts->pdev;
> > + char buffer[8] = { 0 };
> > + int ret;
> > +
> > + memset(buffer, 0, sizeof(buffer));
> > +
> > + buffer[0] = REBASELINE_CMD;
> > +
> > + ret = axiom_i2c_write(ts->client, 0x02, 0, buffer, sizeof(buffer));
> > + if (ret)
> > + dev_err(pdev, "Rebaseline failed\n");
> > +
> > + dev_info(pdev, "Capture Baseline Requested\n");
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_rebaseline);
>
> Nope.
>
> > +
> > +static irqreturn_t axiom_irq(int irq, void *handle)
> > +{
> > + struct axiom_data *ts = handle;
> > + u8 *rx_data = &ts->rx_buf[0];
> > +
> > + axiom_i2c_read(ts->client, REPORT_USAGE_ID, 0, rx_data, ts->max_report_len);
> > + axiom_process_report(ts, rx_data);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int axiom_i2c_probe(struct i2c_client *client)
> > +{
> > + struct axiom_data *ts;
> > + struct device *pdev = &client->dev;
> > + struct input_dev *input_dev;
> > + int error;
> > + int target;
> > +
> > + ts = devm_kzalloc(pdev, sizeof(*ts), GFP_KERNEL);
> > + if (!ts)
> > + return -ENOMEM;
> > +
> > + ts->irq_gpio = devm_gpiod_get_optional(pdev, "irq", GPIOD_IN);
> > + if (IS_ERR(ts->irq_gpio)) {
> > + error = PTR_ERR(ts->irq_gpio);
> > + dev_err(pdev, "failed to request irq GPIO: %d", error);
>
> Heh... syntax is: return dev_err_probe()
>
> > + return error;
> > + }
> > +
> > + ts->reset_gpio = devm_gpiod_get_optional(pdev, "reset", GPIOD_OUT_HIGH);
>
> You do not use this GPIO at all. Dead code, especially that you keep the
> device in reset. This was absolutely never tested (unless with incorrect
> DTS...).
Actually this has been exclusively tested on a X86 platform with ACPI
overrides however this still need to be fixed indeed.
>
> > + if (IS_ERR(ts->reset_gpio)) {
> > + error = PTR_ERR(ts->reset_gpio);
>
> return dev_err_probe()
>
> > + dev_err(pdev, "failed to request reset GPIO: %d", error);
> > + return error;
> > + }
> > +
> > + if (ts->irq_gpio) {
> > + error =
> > + devm_request_threaded_irq(pdev, client->irq, NULL,
> > + axiom_irq,
> > + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > + "axiom_irq", ts);
> > + if (error != 0) {
> > + dev_err(pdev, "Failed to request IRQ %u (error: %d)\n",
> > + client->irq, error);
>
> return dev_err_probe()
>
> > + return error;
> > + }
> > + }
> > +
> > + ts->client = client;
> > + ts->pdev = pdev;
> > + ts->usage_table_populated = false;
> > +
> > + i2c_set_clientdata(client, ts);
> > +
> > + axiom_discover(ts);
> > + axiom_rebaseline(ts);
> > +
> > + input_dev = devm_input_allocate_device(ts->pdev);
> > + if (!input_dev) {
> > + dev_err(pdev, "Failed to allocate input device\n");
>
> I don't think we print memory allocation failures.
>
> > + 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 touch screen instead of as a mouse pointer */
> > + input_mt_init_slots(input_dev, 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) {
> > + error = input_setup_polling(input_dev, axiom_i2c_poll);
> > + if (error) {
> > + dev_err(ts->pdev, "Unable to set up polling: %d\n",
> > + error);
> > + return error;
> > + }
> > + input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> > + }
> > +
> > + error = input_register_device(input_dev);
> > + if (error != 0) {
> > + dev_err(ts->pdev,
> > + "Could not register with Input Sub-system., error=%d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + ts->input_dev = input_dev;
> > + input_set_drvdata(ts->input_dev, ts);
> > +
> > + dev_info(pdev, "AXIOM: I2C driver registered with Input Sub-System.\n");
>
> Drop silly tracing messages.
>
> > +
> > + /* Delay just a smidge before enabling the IRQ */
> > + udelay(10);
>
> ??? So where is the IRQ being enabled? This is confusing.
This need to move before IRQ is enabled.
>
> > +
> > + /* Ensure that all reports are initialised to not be present. */
> > + for (target = 0; target < U41_MAX_TARGETS; target++)
> > + ts->targets[target].state = TARGET_STATE_NOT_PRESENT;
> > +
> > + return 0;
> > +}
> > +
> > +static void axiom_i2c_remove(struct i2c_client *client)
> > +{
> > + struct axiom_data *ts = i2c_get_clientdata(client);
> > +
> > + axiom_remove(ts);
> > +}
> > +
> > +static const struct i2c_device_id axiom_i2c_id_table[] = {
> > + {"ax54a"},
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> > +
> > +static const struct of_device_id axiom_i2c_dt_ids[] = {
> > + {
> > + .compatible = "axiom,ax54a",
>
> That's not documented. And you would know it if you run basic checks
> before sending patches upstream.
>
I was actually suprised to found not all touchscreen drivers have
a documented binding and I know understand this is bad :) !
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
OK, actually those are the only checkpatch warnings reported:
$ ./scripts/checkpatch.pl 0001-Input-add-driver-for-TouchNetix-aXiom-touchscreen.patch
WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst
WARNING: DT compatible string "axiom,ax54a" appears un-documented -- check ./Documentation/devicetree/bindings/
#954: FILE: drivers/input/touchscreen/axiom_i2c.c:326:
+ .compatible = "axiom,ax54a",
total: 0 errors, 2 warnings, 916 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
>
> > + .data = "axiom",
>
> Why?
Actually not used, more clean up required.
>
> That's not readable. Indent it like in other drivers.
>
> > + },
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, axiom_i2c_dt_ids);
> > +
> > +static struct i2c_driver axiom_i2c_driver = {
> > + .driver = {
> > + .name = "axiom_i2c",
> > + .of_match_table = of_match_ptr(axiom_i2c_dt_ids),
>
> Drop of_match_ptr(), it causes warnings in your code...
Ok.
>
> > + },
> > + .id_table = axiom_i2c_id_table,
> > + .probe = axiom_i2c_probe,
> > + .remove = axiom_i2c_remove,
> > +};
> > +
> > +module_i2c_driver(axiom_i2c_driver);
> > +
> > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> > +MODULE_DESCRIPTION("aXiom touchscreen I2C bus driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("axiom");
>
> Nope, you cannot have such alias and it is not even needed.
Ack, thanks !
>
>
> Best regards,
> Krzysztof
>
--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Christophe JAILLET @ 2023-09-17 6:03 UTC (permalink / raw)
To: stephan
Cc: conor+dt, devicetree, dmitry.torokhov, jeff, jonathan.albrieux,
krzysztof.kozlowski+dt, linux-input, linux-kernel, robh+dt,
rydberg
In-Reply-To: <20230913-hx852x-v1-2-9c1ebff536eb@gerhold.net>
Le 13/09/2023 à 15:25, Stephan Gerhold a écrit :
> From: Jonathan Albrieux <jonathan.albrieux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Add a simple driver for the Himax HX852x(ES) touch panel controller,
> with support for multi-touch and capacitive touch keys.
>
> The driver is somewhat based on sample code from Himax. However, that
> code was so extremely confusing that we spent a significant amount of
> time just trying to understand the packet format and register commands.
> In this driver they are described with clean structs and defines rather
> than lots of magic numbers and offset calculations.
>
> Signed-off-by: Jonathan Albrieux <jonathan.albrieux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Co-developed-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> ---
...
> +static irqreturn_t hx852x_interrupt(int irq, void *ptr)
> +{
> + struct hx852x *hx = ptr;
> + int error;
> +
> + error = hx852x_handle_events(hx);
> + if (error) {
> + dev_err(&hx->client->dev, "failed to handle events: %d\n", error);
Should dev_err_ratelimited() be preferred?
> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}
...
> +static int hx852x_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct hx852x *hx;
> + int error, i;
Nit: err or ret is shorter and maybe more "standard".
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_WRITE_BYTE |
> + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> + dev_err(dev, "not all i2c functionality supported\n");
> + return -ENXIO;
> + }
> +
> + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> + if (!hx)
> + return -ENOMEM;
> +
> + hx->client = client;
> + hx->input_dev = devm_input_allocate_device(dev);
> + if (!hx->input_dev)
> + return -ENOMEM;
> +
> + hx->input_dev->name = "Himax HX852x";
> + hx->input_dev->id.bustype = BUS_I2C;
> + hx->input_dev->open = hx852x_input_open;
> + hx->input_dev->close = hx852x_input_close;
> +
> + i2c_set_clientdata(client, hx);
> + input_set_drvdata(hx->input_dev, hx);
> +
> + hx->supplies[0].supply = "vcca";
> + hx->supplies[1].supply = "vccd";
> + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> + if (error < 0)
> + return dev_err_probe(dev, error, "failed to get regulators");
> +
> + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(hx->reset_gpiod))
> + return dev_err_probe(dev, error, "failed to get reset gpio");
> +
> + error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
> + IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
> + if (error) {
> + dev_err(dev, "failed to request irq %d: %d\n", client->irq, error);
dev_err_probe() could be used to be consistent with above code.
Same for below dev_err() calls.
> + return error;
> + }
> +
> + error = hx852x_read_config(hx);
> + if (error)
> + return error;
> +
> + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
> + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
> + input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(hx->input_dev, true, &hx->props);
> + error = hx852x_parse_properties(hx);
> + if (error)
> + return error;
> +
> + hx->input_dev->keycode = hx->keycodes;
> + hx->input_dev->keycodemax = hx->keycount;
> + hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
> + for (i = 0; i < hx->keycount; i++)
> + input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
> +
> + error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> + if (error) {
> + dev_err(dev, "failed to init MT slots: %d\n", error);
> + return error;
> + }
> +
> + error = input_register_device(hx->input_dev);
> + if (error) {
input_mt_destroy_slots() should be called here, or in an error handling
path below, or via a devm_add_action_or_reset().
It should also be called in a .remove function (unless
devm_add_action_or_reset is prefered)
CJ
> + dev_err(dev, "failed to register input device: %d\n", error);
> + return error;
> + }
> +
> + return 0;
> +}
...
^ permalink raw reply
* [PATCH] Input: powermate - fix use-after-free in powermate_config_complete
From: Javier Carrasco @ 2023-09-16 21:28 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, linux-kernel, Javier Carrasco,
syzbot+0434ac83f907a1dbdd1e
syzbot has found a use-after-free bug [1] in the powermate driver. This
happens when the device is disconnected, which leads to a memory free
from the powermate_device struct.
When an asynchronous control message completes after the kfree and its
callback is invoked, the lock does not exist anymore and hence the bug.
Return immediately if the URB status is -ESHUTDOWN (the actual status
that triggered this bug) or -ENOENT, avoiding any access to potentially
freed memory.
[1] https://syzkaller.appspot.com/bug?extid=0434ac83f907a1dbdd1e
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Reported-by: syzbot+0434ac83f907a1dbdd1e@syzkaller.appspotmail.com
---
drivers/input/misc/powermate.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/input/misc/powermate.c b/drivers/input/misc/powermate.c
index c1c733a9cb89..f61333fea35f 100644
--- a/drivers/input/misc/powermate.c
+++ b/drivers/input/misc/powermate.c
@@ -196,8 +196,11 @@ static void powermate_config_complete(struct urb *urb)
struct powermate_device *pm = urb->context;
unsigned long flags;
- if (urb->status)
+ if (urb->status) {
printk(KERN_ERR "powermate: config urb returned %d\n", urb->status);
+ if (status == -ENOENT || status == -ESHUTDOWN)
+ return;
+ }
spin_lock_irqsave(&pm->lock, flags);
powermate_sync_state(pm);
---
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
change-id: 20230916-topic-powermate_use_after_free-c703c7969c91
Best regards,
--
Javier Carrasco <javier.carrasco.cruz@gmail.com>
^ permalink raw reply related
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Jeff LaBundy @ 2023-09-16 20:47 UTC (permalink / raw)
To: Stephan Gerhold
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Jonathan Albrieux
In-Reply-To: <20230913-hx852x-v1-2-9c1ebff536eb@gerhold.net>
Hi Stephan and Jonathan,
Great work! Just a few minor comments below.
On Wed, Sep 13, 2023 at 03:25:30PM +0200, Stephan Gerhold wrote:
> From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
>
> Add a simple driver for the Himax HX852x(ES) touch panel controller,
> with support for multi-touch and capacitive touch keys.
>
> The driver is somewhat based on sample code from Himax. However, that
> code was so extremely confusing that we spent a significant amount of
> time just trying to understand the packet format and register commands.
> In this driver they are described with clean structs and defines rather
> than lots of magic numbers and offset calculations.
>
> Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> MAINTAINERS | 7 +
> drivers/input/touchscreen/Kconfig | 10 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/himax_hx852x.c | 491 +++++++++++++++++++++++++++++++
> 4 files changed, 509 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 90f13281d297..c551c60b0598 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9331,6 +9331,13 @@ S: Maintained
> F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> F: drivers/input/touchscreen/himax_hx83112b.c
>
> +HIMAX HX852X TOUCHSCREEN DRIVER
> +M: Stephan Gerhold <stephan@gerhold.net>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
> +F: drivers/input/touchscreen/himax_hx852x.c
> +
> HIPPI
> M: Jes Sorensen <jes@trained-monkey.org>
> L: linux-hippi@sunsite.dk
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index e3e2324547b9..8e5667ae5dab 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -427,6 +427,16 @@ config TOUCHSCREEN_HIDEEP
> To compile this driver as a module, choose M here : the
> module will be called hideep_ts.
>
> +config TOUCHSCREEN_HIMAX_HX852X
> + tristate "Himax HX852x(ES) touchscreen"
> + depends on I2C
> + help
> + Say Y here if you have a Himax HX852x(ES) touchscreen.
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called himax_hx852x.
> +
> config TOUCHSCREEN_HYCON_HY46XX
> tristate "Hycon hy46xx touchscreen support"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 62bd24f3ac8e..f42a87faa86c 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> +obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX852X) += himax_hx852x.o
> obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
> diff --git a/drivers/input/touchscreen/himax_hx852x.c b/drivers/input/touchscreen/himax_hx852x.c
> new file mode 100644
> index 000000000000..31616dcfc5ab
> --- /dev/null
> +++ b/drivers/input/touchscreen/himax_hx852x.c
> @@ -0,0 +1,491 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Himax HX852x(ES) Touchscreen Driver
> + * Copyright (c) 2020-2023 Stephan Gerhold <stephan@gerhold.net>
> + * Copyright (c) 2020 Jonathan Albrieux <jonathan.albrieux@gmail.com>
> + *
> + * Based on the Himax Android Driver Sample Code Ver 0.3 for HMX852xES chipset:
> + * Copyright (c) 2014 Himax Corporation.
> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
Please explicitly #include of_device.h.
> +#include <linux/regulator/consumer.h>
> +
> +#define HX852X_COORD_SIZE(fingers) ((fingers) * sizeof(struct hx852x_coord))
> +#define HX852X_WIDTH_SIZE(fingers) ALIGN(fingers, 4)
> +#define HX852X_BUF_SIZE(fingers) (HX852X_COORD_SIZE(fingers) + \
> + HX852X_WIDTH_SIZE(fingers) + \
> + sizeof(struct hx852x_touch_info))
> +
> +#define HX852X_MAX_FINGERS 12
Well, that's a new one :)
> +#define HX852X_MAX_KEY_COUNT 4
> +#define HX852X_MAX_BUF_SIZE HX852X_BUF_SIZE(HX852X_MAX_FINGERS)
> +
> +#define HX852X_TS_SLEEP_IN 0x80
> +#define HX852X_TS_SLEEP_OUT 0x81
> +#define HX852X_TS_SENSE_OFF 0x82
> +#define HX852X_TS_SENSE_ON 0x83
> +#define HX852X_READ_ONE_EVENT 0x85
> +#define HX852X_READ_ALL_EVENTS 0x86
> +#define HX852X_READ_LATEST_EVENT 0x87
> +#define HX852X_CLEAR_EVENT_STACK 0x88
> +
> +#define HX852X_REG_SRAM_SWITCH 0x8c
> +#define HX852X_REG_SRAM_ADDR 0x8b
> +#define HX852X_REG_FLASH_RPLACE 0x5a
> +
> +#define HX852X_SRAM_SWITCH_TEST_MODE 0x14
> +#define HX852X_SRAM_ADDR_CONFIG 0x7000
> +
> +struct hx852x {
> + struct i2c_client *client;
> + struct input_dev *input_dev;
> + struct touchscreen_properties props;
> +
> + struct gpio_desc *reset_gpiod;
> + struct regulator_bulk_data supplies[2];
> +
> + unsigned int max_fingers;
> + unsigned int keycount;
> + u32 keycodes[HX852X_MAX_KEY_COUNT];
Nit: might as well make keycodes 'unsigned int' for consistency; I also do not
feel the newlines are necessary.
> +};
> +
> +struct hx852x_config {
> + u8 rx_num;
> + u8 tx_num;
> + u8 max_pt;
> + u8 padding1[3];
> + __be16 x_res;
> + __be16 y_res;
> + u8 padding2[2];
> +} __packed __aligned(4);
What is the reason to include trailing padding in this packed struct? Does the
controller require the entire register length to be read for some reason?
> +
> +struct hx852x_coord {
> + __be16 x;
> + __be16 y;
> +} __packed __aligned(4);
> +
> +struct hx852x_touch_info {
> + u8 finger_num;
> + __le16 finger_pressed;
It's interesting that most registers/packets use big endian (e.g. x_res) while
only this uses little endian. Is that expected?
> + u8 padding;
Same question with regard to trailing padding.
> +} __packed __aligned(4);
> +
> +static int hx852x_i2c_read(struct hx852x *hx, u8 cmd, void *data, u16 len)
> +{
> + struct i2c_client *client = hx->client;
> + int ret;
> +
> + struct i2c_msg msg[] = {
> + {
> + .addr = client->addr,
> + .flags = 0,
> + .len = 1,
> + .buf = &cmd,
> + },
> + {
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = len,
> + .buf = data,
> + }
> + };
> +
> + ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> + if (ret != ARRAY_SIZE(msg)) {
> + dev_err(&client->dev, "failed to read %#x: %d\n", cmd, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
This function does not appear to be doing anything unique (e.g. retry loop to
deal with special HW condition, etc.), so I do not see any reason to open-code
a standard write-then-read operation.
Can i2c_smbus API simply replace this function, or better yet, can this driver
simply use regmap? In fact, that is what the other mainline Himax driver uses.
> +
> +static int hx852x_power_on(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = regulator_bulk_enable(ARRAY_SIZE(hx->supplies), hx->supplies);
> + if (error < 0) {
Nit: if (error) as you have done elsewhere.
> + dev_err(dev, "failed to enable regulators: %d\n", error);
> + return error;
> + }
> +
> + gpiod_set_value_cansleep(hx->reset_gpiod, 1);
> + msleep(20);
> + gpiod_set_value_cansleep(hx->reset_gpiod, 0);
> + msleep(20);
> +
> + return 0;
> +}
> +
> +static int hx852x_start(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_OUT);
> + if (error) {
> + dev_err(dev, "failed to send TS_SLEEP_OUT: %d\n", error);
> + return error;
> + }
> + msleep(30);
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_ON);
> + if (error) {
> + dev_err(dev, "failed to send TS_SENSE_ON: %d\n", error);
> + return error;
> + }
> + msleep(20);
> +
> + return 0;
> +}
> +
> +static void hx852x_stop(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_OFF);
> + if (error)
> + dev_err(dev, "failed to send TS_SENSE_OFF: %d\n", error);
Granted the function is of void type, should we not still return if there
is an error?
Actually, I would still keep this function as an int for future re-use, even
though hx852x_input_close() simply ignores the value. This way, the return
value can be propagated to the return value of hx852x_suspend() and elsewhere.
> +
> + msleep(20);
> +
> + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_IN);
> + if (error)
> + dev_err(dev, "failed to send TS_SLEEP_IN: %d\n", error);
Same here; no need to sleep following a register write that seemingly did
not happen.
> +
> + msleep(30);
> +}
> +
> +static void hx852x_power_off(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + error = regulator_bulk_disable(ARRAY_SIZE(hx->supplies), hx->supplies);
> + if (error)
> + dev_err(dev, "failed to disable regulators: %d\n", error);
> +}
Same comment with regard to function type; it's nice for internal helpers
to be of type int, even if the core callback using it is void.
> +
> +static int hx852x_read_config(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + struct hx852x_config conf = {0};
No need to initialize.
> + int x_res, y_res;
> + int error;
> +
> + error = hx852x_power_on(hx);
> + if (error)
> + return error;
> +
> + /* Sensing must be turned on briefly to load the config */
> + error = hx852x_start(hx);
> + if (error)
> + goto power_off;
> +
> + hx852x_stop(hx);
See my earlier comment about promoting this function's type to int.
> +
> + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> + HX852X_SRAM_SWITCH_TEST_MODE);
> + if (error)
> + goto power_off;
> +
> + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> + HX852X_SRAM_ADDR_CONFIG);
> + if (error)
> + goto exit_test_mode;
> +
> + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> + if (error)
> + goto exit_test_mode;
> +
> + x_res = be16_to_cpu(conf.x_res);
> + y_res = be16_to_cpu(conf.y_res);
> + hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> + dev_dbg(dev, "x res: %d, y res: %d, max fingers: %d\n",
> + x_res, y_res, hx->max_fingers);
> +
> + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> + dev_err(dev, "max supported fingers: %d, found: %d\n",
> + HX852X_MAX_FINGERS, hx->max_fingers);
> + error = -EINVAL;
> + goto exit_test_mode;
> + }
> +
> + if (x_res && y_res) {
> + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> + }
> +
> +exit_test_mode:
> + i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
Nit: it would still be nice to preserve as many return values as possible, perhaps
as follows:
+exit_test_mode:
error = i2c_smbus_write_byte_data(...) ? : error;
> +power_off:
> + hx852x_power_off(hx);
> + return error;
Similarly, with hx852x_power_off() being promoted to int as suggested above,
this could be:
return hx852x_power_off(...) ? : error;
There are other idiomatic ways to do the same thing based on your preference.
Another (perhaps more clear) option would be to move some of these test mode
functions into a helper, which would also avoid some goto statements.
> +}
> +
> +static int hx852x_handle_events(struct hx852x *hx)
> +{
> + /*
> + * The event packets have variable size, depending on the amount of
> + * supported fingers (hx->max_fingers). They are laid out as follows:
> + * - struct hx852x_coord[hx->max_fingers]: Coordinates for each finger
> + * - u8[ALIGN(hx->max_fingers, 4)]: Touch width for each finger
> + * with padding for 32-bit alignment
> + * - struct hx852x_touch_info
> + *
> + * Load everything into a 32-bit aligned buffer so the coordinates
> + * can be assigned directly, without using get_unaligned_*().
> + */
> + u8 buf[HX852X_MAX_BUF_SIZE] __aligned(4);
> + struct hx852x_coord *coord = (struct hx852x_coord *)buf;
> + u8 *width = &buf[HX852X_COORD_SIZE(hx->max_fingers)];
> + struct hx852x_touch_info *info = (struct hx852x_touch_info *)
> + &width[HX852X_WIDTH_SIZE(hx->max_fingers)];
> + unsigned long finger_pressed, key_pressed;
It seems these only need to be u16.
> + unsigned int i, x, y, w;
> + int error;
> +
> + error = hx852x_i2c_read(hx, HX852X_READ_ALL_EVENTS, buf,
> + HX852X_BUF_SIZE(hx->max_fingers));
> + if (error)
> + return error;
> +
> + finger_pressed = get_unaligned_le16(&info->finger_pressed);
> + key_pressed = finger_pressed >> HX852X_MAX_FINGERS;
> +
> + /* All bits are set when no touch is detected */
> + if (info->finger_num == 0xff || !(info->finger_num & 0x0f))
> + finger_pressed = 0;
> + if (key_pressed == 0xf)
> + key_pressed = 0;
> +
> + for_each_set_bit(i, &finger_pressed, hx->max_fingers) {
> + x = be16_to_cpu(coord[i].x);
> + y = be16_to_cpu(coord[i].y);
> + w = width[i];
> +
> + input_mt_slot(hx->input_dev, i);
> + input_mt_report_slot_state(hx->input_dev, MT_TOOL_FINGER, 1);
> + touchscreen_report_pos(hx->input_dev, &hx->props, x, y, true);
> + input_report_abs(hx->input_dev, ABS_MT_TOUCH_MAJOR, w);
> + }
> + input_mt_sync_frame(hx->input_dev);
> +
> + for (i = 0; i < hx->keycount; i++)
> + input_report_key(hx->input_dev, hx->keycodes[i], key_pressed & BIT(i));
> +
> + input_sync(hx->input_dev);
> + return 0;
> +}
> +
> +static irqreturn_t hx852x_interrupt(int irq, void *ptr)
> +{
> + struct hx852x *hx = ptr;
> + int error;
> +
> + error = hx852x_handle_events(hx);
> + if (error) {
> + dev_err(&hx->client->dev, "failed to handle events: %d\n", error);
> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int hx852x_input_open(struct input_dev *dev)
> +{
> + struct hx852x *hx = input_get_drvdata(dev);
> + int error;
> +
> + error = hx852x_power_on(hx);
> + if (error)
> + return error;
> +
> + error = hx852x_start(hx);
> + if (error) {
> + hx852x_power_off(hx);
> + return error;
> + }
> +
> + enable_irq(hx->client->irq);
> + return 0;
> +}
> +
> +static void hx852x_input_close(struct input_dev *dev)
> +{
> + struct hx852x *hx = input_get_drvdata(dev);
> +
> + hx852x_stop(hx);
> + disable_irq(hx->client->irq);
> + hx852x_power_off(hx);
> +}
> +
> +static int hx852x_parse_properties(struct hx852x *hx)
> +{
> + struct device *dev = &hx->client->dev;
> + int error;
> +
> + hx->keycount = device_property_count_u32(dev, "linux,keycodes");
> + if (hx->keycount <= 0) {
> + hx->keycount = 0;
> + return 0;
> + }
> +
> + if (hx->keycount > HX852X_MAX_KEY_COUNT) {
> + dev_err(dev, "max supported keys: %d, found: %d\n",
Nit: these should both be printed as %u.
> + HX852X_MAX_KEY_COUNT, hx->keycount);
> + return -EINVAL;
> + }
> +
> + error = device_property_read_u32_array(dev, "linux,keycodes",
> + hx->keycodes, hx->keycount);
> + if (error)
> + dev_err(dev, "failed to read linux,keycodes: %d\n", error);
> +
> + return error;
> +}
> +
> +static int hx852x_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct hx852x *hx;
> + int error, i;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_WRITE_BYTE |
> + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> + dev_err(dev, "not all i2c functionality supported\n");
> + return -ENXIO;
> + }
> +
> + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> + if (!hx)
> + return -ENOMEM;
> +
> + hx->client = client;
> + hx->input_dev = devm_input_allocate_device(dev);
> + if (!hx->input_dev)
> + return -ENOMEM;
> +
> + hx->input_dev->name = "Himax HX852x";
> + hx->input_dev->id.bustype = BUS_I2C;
> + hx->input_dev->open = hx852x_input_open;
> + hx->input_dev->close = hx852x_input_close;
> +
> + i2c_set_clientdata(client, hx);
> + input_set_drvdata(hx->input_dev, hx);
> +
> + hx->supplies[0].supply = "vcca";
> + hx->supplies[1].supply = "vccd";
> + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> + if (error < 0)
> + return dev_err_probe(dev, error, "failed to get regulators");
> +
> + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(hx->reset_gpiod))
> + return dev_err_probe(dev, error, "failed to get reset gpio");
Can the reset GPIO be optional?
> +
> + error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
> + IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
> + if (error) {
> + dev_err(dev, "failed to request irq %d: %d\n", client->irq, error);
> + return error;
> + }
> +
> + error = hx852x_read_config(hx);
> + if (error)
> + return error;
> +
> + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
> + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
> + input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(hx->input_dev, true, &hx->props);
> + error = hx852x_parse_properties(hx);
> + if (error)
> + return error;
> +
> + hx->input_dev->keycode = hx->keycodes;
> + hx->input_dev->keycodemax = hx->keycount;
> + hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
> + for (i = 0; i < hx->keycount; i++)
> + input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
> +
> + error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> + if (error) {
> + dev_err(dev, "failed to init MT slots: %d\n", error);
> + return error;
> + }
> +
> + error = input_register_device(hx->input_dev);
> + if (error) {
> + dev_err(dev, "failed to register input device: %d\n", error);
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static int hx852x_suspend(struct device *dev)
> +{
> + struct hx852x *hx = dev_get_drvdata(dev);
> +
> + mutex_lock(&hx->input_dev->mutex);
> + if (input_device_enabled(hx->input_dev))
> + hx852x_stop(hx);
> + mutex_unlock(&hx->input_dev->mutex);
> +
> + return 0;
> +}
> +
> +static int hx852x_resume(struct device *dev)
> +{
> + struct hx852x *hx = dev_get_drvdata(dev);
> + int error = 0;
> +
> + mutex_lock(&hx->input_dev->mutex);
> + if (input_device_enabled(hx->input_dev))
> + error = hx852x_start(hx);
> + mutex_unlock(&hx->input_dev->mutex);
> +
> + return error;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(hx852x_pm_ops, hx852x_suspend, hx852x_resume);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id hx852x_of_match[] = {
> + { .compatible = "himax,hx852es" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, hx852x_of_match);
> +#endif
> +
> +static struct i2c_driver hx852x_driver = {
> + .probe = hx852x_probe,
> + .driver = {
> + .name = "himax_hx852x",
> + .pm = pm_sleep_ptr(&hx852x_pm_ops),
> + .of_match_table = of_match_ptr(hx852x_of_match),
> + },
> +};
> +module_i2c_driver(hx852x_driver);
> +
> +MODULE_DESCRIPTION("Himax HX852x(ES) Touchscreen Driver");
> +MODULE_AUTHOR("Jonathan Albrieux <jonathan.albrieux@gmail.com>");
> +MODULE_AUTHOR("Stephan Gerhold <stephan@gerhold.net>");
> +MODULE_LICENSE("GPL");
>
> --
> 2.42.0
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH v4 0/5] ARM: omap: omap4-embt2ws: 32K clock for WLAN
From: Andreas Kemnade @ 2023-09-16 10:05 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, tony, mturquette, sboyd, andreas, linux-input,
devicetree, linux-kernel, linux-omap, linux-clk
To have WLAN working properly, enable a 32K clock of the TWL6032.
In earlier tests, it was still enabled from a previous boot into
the vendor system.
Changes in V4:
- use dev_err_probe in clk probe()
- R-by
Changes in V3:
- maintainer change in binding doc
- fix references to binding doc
- additionalProperties: false
- remove subdevices also from examples until
subdevices are referenced/added
Changes in V2:
- no separate device node for the clock
- converted toplevel node of TWL
Andreas Kemnade (5):
dt-bindings: mfd: convert twl-family.txt to json-schema
dt-bindings: mfd: ti,twl: Add clock provider properties
mfd: twl-core: Add a clock subdevice for the TWL6032
clk: twl: add clock driver for TWL6032
ARM: dts: omap4-embt2ws: enable 32K clock on WLAN
.../bindings/input/twl4030-pwrbutton.txt | 2 +-
.../devicetree/bindings/mfd/ti,twl.yaml | 67 ++++++
.../devicetree/bindings/mfd/twl-family.txt | 46 ----
.../boot/dts/ti/omap/omap4-epson-embt2ws.dts | 8 +
drivers/clk/Kconfig | 9 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-twl.c | 197 ++++++++++++++++++
drivers/mfd/twl-core.c | 16 ++
8 files changed, 299 insertions(+), 47 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/ti,twl.yaml
delete mode 100644 Documentation/devicetree/bindings/mfd/twl-family.txt
create mode 100644 drivers/clk/clk-twl.c
--
2.39.2
^ permalink raw reply
* [PATCH v4 5/5] ARM: dts: omap4-embt2ws: enable 32K clock on WLAN
From: Andreas Kemnade @ 2023-09-16 10:05 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, tony, mturquette, sboyd, andreas, linux-input,
devicetree, linux-kernel, linux-omap, linux-clk
In-Reply-To: <20230916100515.1650336-1-andreas@kemnade.info>
WLAN did only work if clock was left enabled by the original system,
so make it fully enable the needed resources itself.
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts b/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts
index ee86981b2e448..9d2f2d8639496 100644
--- a/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts
+++ b/arch/arm/boot/dts/ti/omap/omap4-epson-embt2ws.dts
@@ -69,6 +69,12 @@ unknown_supply: unknown-supply {
regulator-name = "unknown";
};
+ wl12xx_pwrseq: wl12xx-pwrseq {
+ compatible = "mmc-pwrseq-simple";
+ clocks = <&twl 1>;
+ clock-names = "ext_clock";
+ };
+
/* regulator for wl12xx on sdio2 */
wl12xx_vmmc: wl12xx-vmmc {
pinctrl-names = "default";
@@ -92,6 +98,7 @@ &i2c1 {
twl: pmic@48 {
compatible = "ti,twl6032";
reg = <0x48>;
+ #clock-cells = <1>;
/* IRQ# = 7 */
interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>; /* IRQ_SYS_1N cascaded to gic */
interrupt-controller;
@@ -316,6 +323,7 @@ &mmc3 {
pinctrl-names = "default";
pinctrl-0 = <&wl12xx_pins>;
vmmc-supply = <&wl12xx_vmmc>;
+ mmc-pwrseq = <&wl12xx_pwrseq>;
interrupts-extended = <&wakeupgen GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH
&omap4_pmx_core 0x12e>;
non-removable;
--
2.39.2
^ permalink raw reply related
* [PATCH v4 4/5] clk: twl: add clock driver for TWL6032
From: Andreas Kemnade @ 2023-09-16 10:05 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, tony, mturquette, sboyd, andreas, linux-input,
devicetree, linux-kernel, linux-omap, linux-clk
In-Reply-To: <20230916100515.1650336-1-andreas@kemnade.info>
The TWL6032 has some clock outputs which are controlled like
fixed-voltage regulators, in some drivers for these chips
found in the wild, just the regulator api is abused for controlling
them, so simply use something similar to the regulator functions.
Due to a lack of hardware available for testing, leave out the
TWL6030-specific part of those functions.
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
drivers/clk/Kconfig | 9 ++
drivers/clk/Makefile | 1 +
drivers/clk/clk-twl.c | 197 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 207 insertions(+)
create mode 100644 drivers/clk/clk-twl.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index c30099866174d..3944f081ebade 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -277,6 +277,15 @@ config COMMON_CLK_S2MPS11
clock. These multi-function devices have two (S2MPS14) or three
(S2MPS11, S5M8767) fixed-rate oscillators, clocked at 32KHz each.
+config CLK_TWL
+ tristate "Clock driver for the TWL PMIC family"
+ depends on TWL4030_CORE
+ help
+ Enable support for controlling the clock resources on TWL family
+ PMICs. These devices have some 32K clock outputs which can be
+ controlled by software. For now, only the TWL6032 clocks are
+ supported.
+
config CLK_TWL6040
tristate "External McPDM functional clock from twl6040"
depends on TWL6040_CORE
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 18969cbd4bb1e..86e46adcb619c 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -72,6 +72,7 @@ obj-$(CONFIG_COMMON_CLK_STM32H7) += clk-stm32h7.o
obj-$(CONFIG_COMMON_CLK_STM32MP157) += clk-stm32mp1.o
obj-$(CONFIG_COMMON_CLK_TPS68470) += clk-tps68470.o
obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o
+obj-$(CONFIG_CLK_TWL) += clk-twl.o
obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
obj-$(CONFIG_COMMON_CLK_RS9_PCIE) += clk-renesas-pcie.o
obj-$(CONFIG_COMMON_CLK_SI521XX) += clk-si521xx.o
diff --git a/drivers/clk/clk-twl.c b/drivers/clk/clk-twl.c
new file mode 100644
index 0000000000000..eab9d3c8ed8ae
--- /dev/null
+++ b/drivers/clk/clk-twl.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Clock driver for twl device.
+ *
+ * inspired by the driver for the Palmas device
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/mfd/twl.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define VREG_STATE 2
+#define TWL6030_CFG_STATE_OFF 0x00
+#define TWL6030_CFG_STATE_ON 0x01
+#define TWL6030_CFG_STATE_MASK 0x03
+
+struct twl_clock_info {
+ struct device *dev;
+ u8 base;
+ struct clk_hw hw;
+};
+
+static inline int
+twlclk_read(struct twl_clock_info *info, unsigned int slave_subgp,
+ unsigned int offset)
+{
+ u8 value;
+ int status;
+
+ status = twl_i2c_read_u8(slave_subgp, &value,
+ info->base + offset);
+ return (status < 0) ? status : value;
+}
+
+static inline int
+twlclk_write(struct twl_clock_info *info, unsigned int slave_subgp,
+ unsigned int offset, u8 value)
+{
+ return twl_i2c_write_u8(slave_subgp, value,
+ info->base + offset);
+}
+
+static inline struct twl_clock_info *to_twl_clks_info(struct clk_hw *hw)
+{
+ return container_of(hw, struct twl_clock_info, hw);
+}
+
+static unsigned long twl_clks_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ return 32768;
+}
+
+static int twl6032_clks_prepare(struct clk_hw *hw)
+{
+ struct twl_clock_info *cinfo = to_twl_clks_info(hw);
+ int ret;
+
+ ret = twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
+ TWL6030_CFG_STATE_ON);
+ if (ret < 0)
+ dev_err(cinfo->dev, "clk prepare failed\n");
+
+ return ret;
+}
+
+static void twl6032_clks_unprepare(struct clk_hw *hw)
+{
+ struct twl_clock_info *cinfo = to_twl_clks_info(hw);
+ int ret;
+
+ ret = twlclk_write(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE,
+ TWL6030_CFG_STATE_OFF);
+ if (ret < 0)
+ dev_err(cinfo->dev, "clk unprepare failed\n");
+}
+
+static int twl6032_clks_is_prepared(struct clk_hw *hw)
+{
+ struct twl_clock_info *cinfo = to_twl_clks_info(hw);
+ int val;
+
+ val = twlclk_read(cinfo, TWL_MODULE_PM_RECEIVER, VREG_STATE);
+ if (val < 0) {
+ dev_err(cinfo->dev, "clk read failed\n");
+ return val;
+ }
+
+ val &= TWL6030_CFG_STATE_MASK;
+
+ return val == TWL6030_CFG_STATE_ON;
+}
+
+static const struct clk_ops twl6032_clks_ops = {
+ .prepare = twl6032_clks_prepare,
+ .unprepare = twl6032_clks_unprepare,
+ .is_prepared = twl6032_clks_is_prepared,
+ .recalc_rate = twl_clks_recalc_rate,
+};
+
+struct twl_clks_data {
+ struct clk_init_data init;
+ u8 base;
+};
+
+static const struct twl_clks_data twl6032_clks[] = {
+ {
+ .init = {
+ .name = "clk32kg",
+ .ops = &twl6032_clks_ops,
+ .flags = CLK_IGNORE_UNUSED,
+ },
+ .base = 0x8C,
+ },
+ {
+ .init = {
+ .name = "clk32kaudio",
+ .ops = &twl6032_clks_ops,
+ .flags = CLK_IGNORE_UNUSED,
+ },
+ .base = 0x8F,
+ },
+ {
+ /* sentinel */
+ }
+};
+
+static int twl_clks_probe(struct platform_device *pdev)
+{
+ struct clk_hw_onecell_data *clk_data;
+ const struct twl_clks_data *hw_data;
+
+ struct twl_clock_info *cinfo;
+ int ret;
+ int i;
+ int count;
+
+ hw_data = twl6032_clks;
+ for (count = 0; hw_data[count].init.name; count++)
+ ;
+
+ clk_data = devm_kzalloc(&pdev->dev,
+ struct_size(clk_data, hws, count),
+ GFP_KERNEL);
+ if (!clk_data)
+ return -ENOMEM;
+
+ clk_data->num = count;
+ cinfo = devm_kcalloc(&pdev->dev, count, sizeof(*cinfo), GFP_KERNEL);
+ if (!cinfo)
+ return -ENOMEM;
+
+ for (i = 0; i < count; i++) {
+ cinfo[i].base = hw_data[i].base;
+ cinfo[i].dev = &pdev->dev;
+ cinfo[i].hw.init = &hw_data[i].init;
+ ret = devm_clk_hw_register(&pdev->dev, &cinfo[i].hw);
+ if (ret) {
+ return dev_err_probe(&pdev->dev, ret,
+ "Fail to register clock %s\n",
+ hw_data[i].init.name);
+ }
+ clk_data->hws[i] = &cinfo[i].hw;
+ }
+
+ ret = devm_of_clk_add_hw_provider(&pdev->dev,
+ of_clk_hw_onecell_get, clk_data);
+ if (ret < 0)
+ return dev_err_probe(&pdev->dev, ret,
+ "Fail to add clock driver\n");
+
+ return 0;
+}
+
+static const struct platform_device_id twl_clks_id[] = {
+ {
+ .name = "twl6032-clk",
+ }, {
+ /* sentinel */
+ }
+};
+MODULE_DEVICE_TABLE(platform, twl_clks_id);
+
+static struct platform_driver twl_clks_driver = {
+ .driver = {
+ .name = "twl-clk",
+ },
+ .probe = twl_clks_probe,
+ .id_table = twl_clks_id,
+};
+
+module_platform_driver(twl_clks_driver);
+
+MODULE_DESCRIPTION("Clock driver for TWL Series Devices");
+MODULE_LICENSE("GPL");
--
2.39.2
^ permalink raw reply related
* [PATCH v4 1/5] dt-bindings: mfd: convert twl-family.txt to json-schema
From: Andreas Kemnade @ 2023-09-16 10:05 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, tony, mturquette, sboyd, andreas, linux-input,
devicetree, linux-kernel, linux-omap, linux-clk
Cc: Conor Dooley
In-Reply-To: <20230916100515.1650336-1-andreas@kemnade.info>
Convert the TWL[46]030 binding to DT schema format. To do it as a step by
step work, do not include / handle nodes for subdevices yet, just convert
things with minimal corrections. There are already some bindings for its
subdevices in the tree.
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
.../bindings/input/twl4030-pwrbutton.txt | 2 +-
.../devicetree/bindings/mfd/ti,twl.yaml | 64 +++++++++++++++++++
.../devicetree/bindings/mfd/twl-family.txt | 46 -------------
3 files changed, 65 insertions(+), 47 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/ti,twl.yaml
delete mode 100644 Documentation/devicetree/bindings/mfd/twl-family.txt
diff --git a/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
index f5021214edecb..6c201a2ba8acf 100644
--- a/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
+++ b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
@@ -1,7 +1,7 @@
Texas Instruments TWL family (twl4030) pwrbutton module
This module is part of the TWL4030. For more details about the whole
-chip see Documentation/devicetree/bindings/mfd/twl-family.txt.
+chip see Documentation/devicetree/bindings/mfd/ti,twl.yaml.
This module provides a simple power button event via an Interrupt.
diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
new file mode 100644
index 0000000000000..f125b254a4b93
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/ti,twl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments TWL family
+
+maintainers:
+ - Andreas Kemnade <andreas@kemnade.info>
+
+description: |
+ The TWLs are Integrated Power Management Chips.
+ Some version might contain much more analog function like
+ USB transceiver or Audio amplifier.
+ These chips are connected to an i2c bus.
+
+properties:
+ compatible:
+ description:
+ TWL4030 for integrated power-management/audio CODEC device used in OMAP3
+ based boards
+ TWL6030/32 for integrated power-management used in OMAP4 based boards
+ enum:
+ - ti,twl4030
+ - ti,twl6030
+ - ti,twl6032
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ interrupt-controller: true
+
+ "#interrupt-cells":
+ const: 1
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - interrupt-controller
+ - "#interrupt-cells"
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ pmic@48 {
+ compatible = "ti,twl6030";
+ reg = <0x48>;
+ interrupts = <39>; /* IRQ_SYS_1N cascaded to gic */
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ interrupt-parent = <&gic>;
+ };
+ };
+
diff --git a/Documentation/devicetree/bindings/mfd/twl-family.txt b/Documentation/devicetree/bindings/mfd/twl-family.txt
deleted file mode 100644
index c2f9302965dea..0000000000000
--- a/Documentation/devicetree/bindings/mfd/twl-family.txt
+++ /dev/null
@@ -1,46 +0,0 @@
-Texas Instruments TWL family
-
-The TWLs are Integrated Power Management Chips.
-Some version might contain much more analog function like
-USB transceiver or Audio amplifier.
-These chips are connected to an i2c bus.
-
-
-Required properties:
-- compatible : Must be "ti,twl4030";
- For Integrated power-management/audio CODEC device used in OMAP3
- based boards
-- compatible : Must be "ti,twl6030";
- For Integrated power-management used in OMAP4 based boards
-- interrupts : This i2c device has an IRQ line connected to the main SoC
-- interrupt-controller : Since the twl support several interrupts internally,
- it is considered as an interrupt controller cascaded to the SoC one.
-- #interrupt-cells = <1>;
-
-Optional node:
-- Child nodes contain in the twl. The twl family is made of several variants
- that support a different number of features.
- The children nodes will thus depend of the capability of the variant.
-
-
-Example:
-/*
- * Integrated Power Management Chip
- * https://www.ti.com/lit/ds/symlink/twl6030.pdf
- */
-twl@48 {
- compatible = "ti,twl6030";
- reg = <0x48>;
- interrupts = <39>; /* IRQ_SYS_1N cascaded to gic */
- interrupt-controller;
- #interrupt-cells = <1>;
- interrupt-parent = <&gic>;
- #address-cells = <1>;
- #size-cells = <0>;
-
- twl_rtc {
- compatible = "ti,twl_rtc";
- interrupts = <11>;
- reg = <0>;
- };
-};
--
2.39.2
^ permalink raw reply related
* [PATCH v4 3/5] mfd: twl-core: Add a clock subdevice for the TWL6032
From: Andreas Kemnade @ 2023-09-16 10:05 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, tony, mturquette, sboyd, andreas, linux-input,
devicetree, linux-kernel, linux-omap, linux-clk
In-Reply-To: <20230916100515.1650336-1-andreas@kemnade.info>
Clock device needs no separate devicetree node, so add it as
a platform device. Other devices in the family also have controllable
clocks, but due to the lack of testing, just add it for the TWL6032
now.
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
drivers/mfd/twl-core.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index ce01a87f8dc39..234500b2e53fc 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -31,6 +31,8 @@
#include <linux/regulator/machine.h>
#include <linux/i2c.h>
+
+#include <linux/mfd/core.h>
#include <linux/mfd/twl.h>
/* Register descriptions for audio */
@@ -690,6 +692,10 @@ static struct of_dev_auxdata twl_auxdata_lookup[] = {
{ /* sentinel */ },
};
+static const struct mfd_cell twl6032_cells[] = {
+ { .name = "twl6032-clk" },
+};
+
/* NOTE: This driver only handles a single twl4030/tps659x0 chip */
static int
twl_probe(struct i2c_client *client)
@@ -836,6 +842,16 @@ twl_probe(struct i2c_client *client)
TWL4030_DCDC_GLOBAL_CFG);
}
+ if (id->driver_data == (TWL6030_CLASS | TWL6032_SUBCLASS)) {
+ status = devm_mfd_add_devices(&client->dev,
+ PLATFORM_DEVID_NONE,
+ twl6032_cells,
+ ARRAY_SIZE(twl6032_cells),
+ NULL, 0, NULL);
+ if (status < 0)
+ goto free;
+ }
+
status = of_platform_populate(node, NULL, twl_auxdata_lookup,
&client->dev);
--
2.39.2
^ permalink raw reply related
* [PATCH v4 2/5] dt-bindings: mfd: ti,twl: Add clock provider properties
From: Andreas Kemnade @ 2023-09-16 10:05 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, tony, mturquette, sboyd, andreas, linux-input,
devicetree, linux-kernel, linux-omap, linux-clk
Cc: Conor Dooley
In-Reply-To: <20230916100515.1650336-1-andreas@kemnade.info>
Since these devices provide clock outputs, add the corresponding
property.
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
Documentation/devicetree/bindings/mfd/ti,twl.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
index f125b254a4b93..c04d57ba22b49 100644
--- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml
+++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
@@ -37,6 +37,9 @@ properties:
"#interrupt-cells":
const: 1
+ "#clock-cells":
+ const: 1
+
additionalProperties: false
required:
--
2.39.2
^ permalink raw reply related
* Re: [PATCH RESEND] Input: xpad - Add HyperX Clutch Gladiate Support
From: HP Dev @ 2023-09-16 1:44 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: maxwell.nguyen, chris.toledanes, carl.ng, linux-input
In-Reply-To: <ZQTEZEdd7xwHz9CN@google.com>
Hi Dmitry,
That should be ok. Thanks again for your support.
^ permalink raw reply
* Re: [PATCH RESEND] Input: xpad - Add HyperX Clutch Gladiate Support
From: Dmitry Torokhov @ 2023-09-15 20:53 UTC (permalink / raw)
To: HP Dev; +Cc: linux-input, Chris Toledanes, Carl Ng, Max Nguyen
In-Reply-To: <20230906231514.4291-1-hphyperxdev@gmail.com>
Hi Max,
On Wed, Sep 06, 2023 at 04:15:15PM -0700, HP Dev wrote:
> Add HyperX controller support to xpad_device and xpad_table.
>
> Suggested-by: Chris Toledanes <chris.toledanes@hp.com>
> Reviewed-by: Carl Ng <carl.ng@hp.com>
> Signed-off-by: Max Nguyen <maxwell.nguyen@hp.com>
Sorry for the delay. It is weird to have "HP Dev" as an author of the
patch, if you not mind I will change this to "Max Nguyen
<maxwell.nguyen@hp.com>" before applying, please let me know.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
From: Rahul Rameshbabu @ 2023-09-15 20:51 UTC (permalink / raw)
To: Christophe JAILLET
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel,
kernel-janitors
In-Reply-To: <d083215e-e9bf-860d-6d04-d919a9b90752@wanadoo.fr>
On Fri, 15 Sep, 2023 22:14:18 +0200 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> Le 15/09/2023 à 20:16, Rahul Rameshbabu a écrit :
>> Hi Christophe,
>> On Sat, 26 Aug, 2023 19:42:16 +0200 Christophe JAILLET
>> <christophe.jaillet@wanadoo.fr> wrote:
>>> This serie fixes some missing clean-up function calls in the error handling of
>>> the probe.
>>>
>>> Patch 1 and 2 fix some similar issues introduced in 2 different commits (hence 2
>>> patches)
>>>
>>> Patch 3 is a proposal to be more future proof.
>>>
>>>
>>> *Note*: I'm not 100% sure that the order of the functions is the best one in
>>> thunderstrike_destroy(), but it is the way it was.
>>>
>>> My personal preference would be to undo things in reverse order they are
>>> allocated, such as:
>>> led_classdev_unregister(&ts->led_dev);
>>> power_supply_unregister(ts->base.battery_dev.psy);
>>> if (ts->haptics_dev)
>>> input_unregister_device(ts->haptics_dev);
>>> ida_free(&thunderstrike_ida, ts->id);
>>> This order was explicitly chnaged by 3ab196f88237, so, as I can't test the
>>> changes on a real harware, I've left it as-is.
>>>
>>> Christophe JAILLET (3):
>>> HID: nvidia-shield: Fix a missing led_classdev_unregister() in the
>>> probe error handling path
>>> HID: nvidia-shield: Fix some missing function calls() in the probe
>>> error handling path
>>> HID: nvidia-shield: Introduce thunderstrike_destroy()
>>>
>>> drivers/hid/hid-nvidia-shield.c | 23 ++++++++++++++++-------
>>> 1 file changed, 16 insertions(+), 7 deletions(-)
>> I was wondering if you have time to address the comments in this
>> submission. If not, I can re-spin the patches with the needed changes in
>> upcoming days.
>
> I can send an update tomorrow, but I'm only working with -next, so should using
> for-6.6/nvidia (as said in your comment in #1/3) be a must have, then it would
> be more convenient for me if you make the changes by yourself.
Luckily, it does not have to be on top of for-6.6/nvidia to add the fix
I mentioned with regards to the led_classdev flag for not trying to
power off the led when unregistering the led_classdev. That should still
merge nicely on top of for-6.6/nvidia. The main reason I mentioned it
was due to the commit living there with regards to the issue involving
unregistering the led_classdev without the mentioned flag.
--
Thanks for the patches,
Rahul Rameshbabu
>
> CJ
>
>> --
>> Thanks,
>> Rahul Rameshbabu
>>
^ permalink raw reply
* Re: [PATCH] HID: uhid: refactor deprecated strncpy
From: Kees Cook @ 2023-09-15 20:48 UTC (permalink / raw)
To: David Rheinsberg
Cc: Justin Stitt, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, linux-hardening, David Herrmann
In-Reply-To: <98d981a1-4e4c-4173-b8eb-09b4245bca60@app.fastmail.com>
On Fri, Sep 15, 2023 at 09:36:23AM +0200, David Rheinsberg wrote:
> Hi
>
> On Fri, Sep 15, 2023, at 7:13 AM, Kees Cook wrote:
> >> - /* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
> >> - len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
> >> - strncpy(hid->name, ev->u.create2.name, len);
> >> - len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
> >> - strncpy(hid->phys, ev->u.create2.phys, len);
> >> - len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
> >> - strncpy(hid->uniq, ev->u.create2.uniq, len);
> >
> > ev->u.create2 is:
> > struct uhid_create2_req {
> > __u8 name[128];
> > __u8 phys[64];
> > __u8 uniq[64];
> > ...
> >
> > hid is:
> > struct hid_device { /* device report descriptor */
> > ...
> > char name[128]; /* Device name */
> > char phys[64]; /* Device physical location */
> > char uniq[64]; /* Device unique identifier (serial #) */
> >
> > So these "min" calls are redundant -- it wants to copy at most 1 less so
> > it can be %NUL terminated. Which is what strscpy() already does. And
> > source and dest are the same size, so we can't over-read source if it
> > weren't terminated (since strscpy won't overread like strlcpy).
>
> I *really* think we should keep the `min` calls. The compiler
> should already optimize them away, as both arguments are compile-time
> constants. There is no inherent reason why source and target are equal in
> size. Yes, it is unlikely to change, but I don't understand why we would
> want to implicitly rely on it, rather than make the compiler verify it for
> us. And `struct hid_device` is very much allowed to change in the future.
>
> As an alternative, you can use BUILD_BUG_ON() and verify both are equal in length.
If we can't depend on ev->u.create2.name/phys/uniq being %NUL-terminated,
we've already done the "min" calculations, and we've already got the
dest zeroed, then I suspect the thing to do is just use memcpy instead
of strncpy (or strscpy).
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
From: Christophe JAILLET @ 2023-09-15 20:14 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel,
kernel-janitors
In-Reply-To: <87msxns3nv.fsf@nvidia.com>
Le 15/09/2023 à 20:16, Rahul Rameshbabu a écrit :
> Hi Christophe,
>
> On Sat, 26 Aug, 2023 19:42:16 +0200 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
>> This serie fixes some missing clean-up function calls in the error handling of
>> the probe.
>>
>> Patch 1 and 2 fix some similar issues introduced in 2 different commits (hence 2
>> patches)
>>
>> Patch 3 is a proposal to be more future proof.
>>
>>
>> *Note*: I'm not 100% sure that the order of the functions is the best one in
>> thunderstrike_destroy(), but it is the way it was.
>>
>> My personal preference would be to undo things in reverse order they are
>> allocated, such as:
>> led_classdev_unregister(&ts->led_dev);
>> power_supply_unregister(ts->base.battery_dev.psy);
>> if (ts->haptics_dev)
>> input_unregister_device(ts->haptics_dev);
>> ida_free(&thunderstrike_ida, ts->id);
>> This order was explicitly chnaged by 3ab196f88237, so, as I can't test the
>> changes on a real harware, I've left it as-is.
>>
>> Christophe JAILLET (3):
>> HID: nvidia-shield: Fix a missing led_classdev_unregister() in the
>> probe error handling path
>> HID: nvidia-shield: Fix some missing function calls() in the probe
>> error handling path
>> HID: nvidia-shield: Introduce thunderstrike_destroy()
>>
>> drivers/hid/hid-nvidia-shield.c | 23 ++++++++++++++++-------
>> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> I was wondering if you have time to address the comments in this
> submission. If not, I can re-spin the patches with the needed changes in
> upcoming days.
I can send an update tomorrow, but I'm only working with -next, so
should using for-6.6/nvidia (as said in your comment in #1/3) be a must
have, then it would be more convenient for me if you make the changes by
yourself.
CJ
>
> --
> Thanks,
>
> Rahul Rameshbabu
>
^ permalink raw reply
* Re: [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
From: Rahul Rameshbabu @ 2023-09-15 18:16 UTC (permalink / raw)
To: Christophe JAILLET
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel,
kernel-janitors
In-Reply-To: <cover.1693070958.git.christophe.jaillet@wanadoo.fr>
Hi Christophe,
On Sat, 26 Aug, 2023 19:42:16 +0200 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> This serie fixes some missing clean-up function calls in the error handling of
> the probe.
>
> Patch 1 and 2 fix some similar issues introduced in 2 different commits (hence 2
> patches)
>
> Patch 3 is a proposal to be more future proof.
>
>
> *Note*: I'm not 100% sure that the order of the functions is the best one in
> thunderstrike_destroy(), but it is the way it was.
>
> My personal preference would be to undo things in reverse order they are
> allocated, such as:
> led_classdev_unregister(&ts->led_dev);
> power_supply_unregister(ts->base.battery_dev.psy);
> if (ts->haptics_dev)
> input_unregister_device(ts->haptics_dev);
> ida_free(&thunderstrike_ida, ts->id);
> This order was explicitly chnaged by 3ab196f88237, so, as I can't test the
> changes on a real harware, I've left it as-is.
>
> Christophe JAILLET (3):
> HID: nvidia-shield: Fix a missing led_classdev_unregister() in the
> probe error handling path
> HID: nvidia-shield: Fix some missing function calls() in the probe
> error handling path
> HID: nvidia-shield: Introduce thunderstrike_destroy()
>
> drivers/hid/hid-nvidia-shield.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
I was wondering if you have time to address the comments in this
submission. If not, I can re-spin the patches with the needed changes in
upcoming days.
--
Thanks,
Rahul Rameshbabu
^ permalink raw reply
* Re: [PATCH v2] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()
From: Bastien Nocera @ 2023-09-15 14:59 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, kernel-janitors
In-Reply-To: <6e0a33e3-1564-419a-9946-b2d0afa0f98d@moroto.mountain>
On Fri, 2023-09-15 at 15:59 +0300, Dan Carpenter wrote:
> The hid_hw_raw_request() function returns negative error codes or the
> number bytes transferred. The problem is that when it returns
> negative
> error codes and we check if "ret < sizeof(arctis_1_battery_request)",
> then the negative values are type promoted from int to high unsigned
> long
> values and treated as success.
>
> This was detected using Smatch:
>
> drivers/hid/hid-steelseries.c:393
> steelseries_headset_arctis_1_fetch_battery()
> warn: error code type promoted to positive: 'ret'
>
> Fixes: a0c76896c3fb ("HID: steelseries: Add support for Arctis 1
> XBox")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Reviewed-by: Bastien Nocera <hadess@hadess.net>
Thanks Dan!
> ---
> v2: Re-word the commit message. Add the Smatch warning. Use a cast
> instead of an explicit check for negatives.
>
> drivers/hid/hid-steelseries.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-
> steelseries.c
> index 43d2cf7153d7..b3edadf42d6d 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -390,7 +390,7 @@ static int
> steelseries_headset_arctis_1_fetch_battery(struct hid_device *hdev)
> ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0],
> write_buf,
> sizeof(arctis_1_battery_request),
> HID_OUTPUT_REPORT,
> HID_REQ_SET_REPORT);
> - if (ret < sizeof(arctis_1_battery_request)) {
> + if (ret < (int)sizeof(arctis_1_battery_request)) {
> hid_err(hdev, "hid_hw_raw_request() failed with
> %d\n", ret);
> ret = -ENODATA;
> }
^ permalink raw reply
* [PATCH v2 2/2] Input: st1232 - add wake up event counting
From: Javier Carrasco @ 2023-09-15 13:47 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Javier Carrasco
In-Reply-To: <20230826-feature-st1232_freeze_wakeup-v2-0-29ae9f747137@wolfvision.net>
This device driver provides wakeup capabilities but the wakeup events
are not reflected in sysfs. Add pm_wakeup_event to the interrupt handler
in order to do so.
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
drivers/input/touchscreen/st1232.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index d0ee90abc293..ad18d3944bf2 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -59,6 +59,7 @@ struct st1232_ts_data {
const struct st_chip_info *chip_info;
int read_buf_len;
u8 *read_buf;
+ bool suspended;
};
static int st1232_ts_read_data(struct st1232_ts_data *ts, u8 reg,
@@ -173,9 +174,13 @@ static int st1232_ts_parse_and_report(struct st1232_ts_data *ts)
static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
{
struct st1232_ts_data *ts = dev_id;
+ struct i2c_client *client = ts->client;
int count;
int error;
+ if (ts->suspended && device_may_wakeup(&client->dev))
+ pm_wakeup_event(&client->dev, 0);
+
error = st1232_ts_read_data(ts, REG_XY_COORDINATES, ts->read_buf_len);
if (error)
goto out;
@@ -345,6 +350,8 @@ static int st1232_ts_suspend(struct device *dev)
struct i2c_client *client = to_i2c_client(dev);
struct st1232_ts_data *ts = i2c_get_clientdata(client);
+ ts->suspended = true;
+
if (!device_may_wakeup(&client->dev)) {
disable_irq(client->irq);
st1232_ts_power(ts, false);
@@ -358,6 +365,8 @@ static int st1232_ts_resume(struct device *dev)
struct i2c_client *client = to_i2c_client(dev);
struct st1232_ts_data *ts = i2c_get_clientdata(client);
+ ts->suspended = false;
+
if (!device_may_wakeup(&client->dev)) {
enable_irq(client->irq);
st1232_ts_power(ts, true);
--
2.39.2
^ permalink raw reply related
* [PATCH v2 1/2] Input: st1232 - remove enable/disable irq if device may wake up
From: Javier Carrasco @ 2023-09-15 13:47 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Javier Carrasco
In-Reply-To: <20230826-feature-st1232_freeze_wakeup-v2-0-29ae9f747137@wolfvision.net>
Disabling the interrupts unconditionally in the suspend callback leads
to a loss of the wakeup capability in suspend to idle (freeze) mode.
In commit 95dc58a9a02f ("Input: st1232 - rely on I2C core to configure
wakeup interrupt") redundancy was removed by letting the I2C core manage
the wake interrupt handling. On the other hand, the irq enabling/disabling
became unconditional.
Revert the general interrupt enabling/disabling to its previous state.
Fixes: 95dc58a9a02f ("Input: st1232 - rely on I2C core to configure wakeup interrupt")
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
drivers/input/touchscreen/st1232.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 6475084aee1b..d0ee90abc293 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -345,10 +345,10 @@ static int st1232_ts_suspend(struct device *dev)
struct i2c_client *client = to_i2c_client(dev);
struct st1232_ts_data *ts = i2c_get_clientdata(client);
- disable_irq(client->irq);
-
- if (!device_may_wakeup(&client->dev))
+ if (!device_may_wakeup(&client->dev)) {
+ disable_irq(client->irq);
st1232_ts_power(ts, false);
+ }
return 0;
}
@@ -358,10 +358,10 @@ static int st1232_ts_resume(struct device *dev)
struct i2c_client *client = to_i2c_client(dev);
struct st1232_ts_data *ts = i2c_get_clientdata(client);
- if (!device_may_wakeup(&client->dev))
+ if (!device_may_wakeup(&client->dev)) {
+ enable_irq(client->irq);
st1232_ts_power(ts, true);
-
- enable_irq(client->irq);
+ }
return 0;
}
--
2.39.2
^ permalink raw reply related
* [PATCH v2 0/2] Input: st1232: wakeup in Suspend to idle
From: Javier Carrasco @ 2023-09-15 13:47 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Javier Carrasco
The st1232 touchscreen provides an interrupt line that can be configured
as a wakeup source, and currently it is possible to use this mechanism in
"Suspend to RAM" and "Hibernate" power states.
Unfortunately, that does not work in "Suspend to idle" (freeze) because
the device driver disables interrupts in its suspend callback
unconditionally.
Partially reverting the patch where this modification was made to
disable interrupts only if the device is not a wakeup source has proved
to solve the issue.
Given that the st1232 device driver does not reflect its wakeup events
in sysfs, this series also adds pm_wakeup_event to the interrupt
handler.
These changes have been successfully tested with an ST1624-N32C and a
Rockchip-based platform.
Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>
---
Changes in v2:
- PATCH 1/2: enable/disable irq if the device is not a wakeup source.
- PATCH 2/2: remove unnecessary mutex locking/unlocking.
- Link to v1: https://lore.kernel.org/r/20230403124707.102986-2-javier.carrasco@wolfvision.net
---
Javier Carrasco (2):
Input: st1232 - remove enable/disable irq if device may wake up
Input: st1232 - add wake up event counting
drivers/input/touchscreen/st1232.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
---
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
change-id: 20230826-feature-st1232_freeze_wakeup-c602968f2f76
Best regards,
--
Javier Carrasco <javier.carrasco@wolfvision.net>
^ permalink raw reply
* [PATCH v2] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()
From: Dan Carpenter @ 2023-09-15 12:59 UTC (permalink / raw)
To: Bastien Nocera
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, kernel-janitors
The hid_hw_raw_request() function returns negative error codes or the
number bytes transferred. The problem is that when it returns negative
error codes and we check if "ret < sizeof(arctis_1_battery_request)",
then the negative values are type promoted from int to high unsigned long
values and treated as success.
This was detected using Smatch:
drivers/hid/hid-steelseries.c:393 steelseries_headset_arctis_1_fetch_battery()
warn: error code type promoted to positive: 'ret'
Fixes: a0c76896c3fb ("HID: steelseries: Add support for Arctis 1 XBox")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
v2: Re-word the commit message. Add the Smatch warning. Use a cast
instead of an explicit check for negatives.
drivers/hid/hid-steelseries.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
index 43d2cf7153d7..b3edadf42d6d 100644
--- a/drivers/hid/hid-steelseries.c
+++ b/drivers/hid/hid-steelseries.c
@@ -390,7 +390,7 @@ static int steelseries_headset_arctis_1_fetch_battery(struct hid_device *hdev)
ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0],
write_buf, sizeof(arctis_1_battery_request),
HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
- if (ret < sizeof(arctis_1_battery_request)) {
+ if (ret < (int)sizeof(arctis_1_battery_request)) {
hid_err(hdev, "hid_hw_raw_request() failed with %d\n", ret);
ret = -ENODATA;
}
--
2.39.2
^ permalink raw reply related
* Re: [PATCH v4 29/42] dt-bindings: input: Add Cirrus EP93xx keypad
From: Krzysztof Kozlowski @ 2023-09-15 11:05 UTC (permalink / raw)
To: nikita.shubin, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Alexander Sverdlin
Cc: linux-input, devicetree, linux-kernel, Arnd Bergmann
In-Reply-To: <20230915-ep93xx-v4-29-a1d779dcec10@maquefel.me>
On 15/09/2023 10:11, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Add YAML bindings for ep93xx SoC keypad.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v4 30/42] input: keypad: ep93xx: add DT support for Cirrus EP93xx
From: Nikita Shubin via B4 Relay @ 2023-09-15 8:11 UTC (permalink / raw)
To: Hartley Sweeten, Alexander Sverdlin, Russell King,
Dmitry Torokhov, Jonathan Cameron, Nikita Shubin, Andy Shevchenko,
Uwe Kleine-König, Sergey Shtylyov, Damien Le Moal
Cc: Linus Walleij, linux-arm-kernel, linux-kernel, linux-input,
Arnd Bergmann, Alexander Sverdlin
In-Reply-To: <20230915-ep93xx-v4-0-a1d779dcec10@maquefel.me>
From: Nikita Shubin <nikita.shubin@maquefel.me>
- drop flags, they were not used anyway
- add OF ID match table
- process "autorepeat", "debounce-delay-ms", prescale from device tree
- drop platform data usage and it's header
- keymap goes from device tree now on
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
arch/arm/mach-ep93xx/core.c | 46 ---------------------
drivers/input/keyboard/ep93xx_keypad.c | 74 ++++++++++------------------------
include/linux/soc/cirrus/ep93xx.h | 4 --
3 files changed, 22 insertions(+), 102 deletions(-)
diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
index c81a2e84821b..c60a9d3632dd 100644
--- a/arch/arm/mach-ep93xx/core.c
+++ b/arch/arm/mach-ep93xx/core.c
@@ -696,52 +696,6 @@ void __init ep93xx_register_keypad(struct ep93xx_keypad_platform_data *data)
platform_device_register(&ep93xx_keypad_device);
}
-int ep93xx_keypad_acquire_gpio(struct platform_device *pdev)
-{
- int err;
- int i;
-
- for (i = 0; i < 8; i++) {
- err = gpio_request(EP93XX_GPIO_LINE_C(i), dev_name(&pdev->dev));
- if (err)
- goto fail_gpio_c;
- err = gpio_request(EP93XX_GPIO_LINE_D(i), dev_name(&pdev->dev));
- if (err)
- goto fail_gpio_d;
- }
-
- /* Enable the keypad controller; GPIO ports C and D used for keypad */
- ep93xx_devcfg_clear_bits(EP93XX_SYSCON_DEVCFG_KEYS |
- EP93XX_SYSCON_DEVCFG_GONK);
-
- return 0;
-
-fail_gpio_d:
- gpio_free(EP93XX_GPIO_LINE_C(i));
-fail_gpio_c:
- for (--i; i >= 0; --i) {
- gpio_free(EP93XX_GPIO_LINE_C(i));
- gpio_free(EP93XX_GPIO_LINE_D(i));
- }
- return err;
-}
-EXPORT_SYMBOL(ep93xx_keypad_acquire_gpio);
-
-void ep93xx_keypad_release_gpio(struct platform_device *pdev)
-{
- int i;
-
- for (i = 0; i < 8; i++) {
- gpio_free(EP93XX_GPIO_LINE_C(i));
- gpio_free(EP93XX_GPIO_LINE_D(i));
- }
-
- /* Disable the keypad controller; GPIO ports C and D used for GPIO */
- ep93xx_devcfg_set_bits(EP93XX_SYSCON_DEVCFG_KEYS |
- EP93XX_SYSCON_DEVCFG_GONK);
-}
-EXPORT_SYMBOL(ep93xx_keypad_release_gpio);
-
/*************************************************************************
* EP93xx I2S audio peripheral handling
*************************************************************************/
diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c
index 55075addcac2..bc302f0fd0b3 100644
--- a/drivers/input/keyboard/ep93xx_keypad.c
+++ b/drivers/input/keyboard/ep93xx_keypad.c
@@ -6,20 +6,13 @@
*
* Based on the pxa27x matrix keypad controller by Rodolfo Giometti.
*
- * NOTE:
- *
- * The 3-key reset is triggered by pressing the 3 keys in
- * Row 0, Columns 2, 4, and 7 at the same time. This action can
- * be disabled by setting the EP93XX_KEYPAD_DISABLE_3_KEY flag.
- *
- * Normal operation for the matrix does not autorepeat the key press.
- * This action can be enabled by setting the EP93XX_KEYPAD_AUTOREPEAT
- * flag.
*/
#include <linux/bits.h>
#include <linux/module.h>
+#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/mod_devicetable.h>
#include <linux/interrupt.h>
#include <linux/clk.h>
#include <linux/io.h>
@@ -27,7 +20,6 @@
#include <linux/input/matrix_keypad.h>
#include <linux/slab.h>
#include <linux/soc/cirrus/ep93xx.h>
-#include <linux/platform_data/keypad-ep93xx.h>
#include <linux/pm_wakeirq.h>
/*
@@ -61,12 +53,16 @@
#define KEY_REG_KEY1_MASK GENMASK(5, 0)
#define KEY_REG_KEY1_SHIFT 0
+#define EP93XX_MATRIX_ROWS (8)
+#define EP93XX_MATRIX_COLS (8)
+
#define EP93XX_MATRIX_SIZE (EP93XX_MATRIX_ROWS * EP93XX_MATRIX_COLS)
struct ep93xx_keypad {
- struct ep93xx_keypad_platform_data *pdata;
struct input_dev *input_dev;
struct clk *clk;
+ unsigned int debounce;
+ uint16_t prescale;
void __iomem *mmio_base;
@@ -133,23 +129,11 @@ static irqreturn_t ep93xx_keypad_irq_handler(int irq, void *dev_id)
static void ep93xx_keypad_config(struct ep93xx_keypad *keypad)
{
- struct ep93xx_keypad_platform_data *pdata = keypad->pdata;
unsigned int val = 0;
- clk_set_rate(keypad->clk, pdata->clk_rate);
-
- if (pdata->flags & EP93XX_KEYPAD_DISABLE_3_KEY)
- val |= KEY_INIT_DIS3KY;
- if (pdata->flags & EP93XX_KEYPAD_DIAG_MODE)
- val |= KEY_INIT_DIAG;
- if (pdata->flags & EP93XX_KEYPAD_BACK_DRIVE)
- val |= KEY_INIT_BACK;
- if (pdata->flags & EP93XX_KEYPAD_TEST_MODE)
- val |= KEY_INIT_T2;
-
- val |= ((pdata->debounce << KEY_INIT_DBNC_SHIFT) & KEY_INIT_DBNC_MASK);
+ val |= ((keypad->debounce << KEY_INIT_DBNC_SHIFT) & KEY_INIT_DBNC_MASK);
- val |= ((pdata->prescale << KEY_INIT_PRSCL_SHIFT) & KEY_INIT_PRSCL_MASK);
+ val |= ((keypad->prescale << KEY_INIT_PRSCL_SHIFT) & KEY_INIT_PRSCL_MASK);
__raw_writel(val, keypad->mmio_base + KEY_INIT);
}
@@ -220,17 +204,10 @@ static int ep93xx_keypad_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(ep93xx_keypad_pm_ops,
ep93xx_keypad_suspend, ep93xx_keypad_resume);
-static void ep93xx_keypad_release_gpio_action(void *_pdev)
-{
- struct platform_device *pdev = _pdev;
-
- ep93xx_keypad_release_gpio(pdev);
-}
-
static int ep93xx_keypad_probe(struct platform_device *pdev)
{
+ struct device *dev = &pdev->dev;
struct ep93xx_keypad *keypad;
- const struct matrix_keymap_data *keymap_data;
struct input_dev *input_dev;
int err;
@@ -238,14 +215,6 @@ static int ep93xx_keypad_probe(struct platform_device *pdev)
if (!keypad)
return -ENOMEM;
- keypad->pdata = dev_get_platdata(&pdev->dev);
- if (!keypad->pdata)
- return -EINVAL;
-
- keymap_data = keypad->pdata->keymap_data;
- if (!keymap_data)
- return -EINVAL;
-
keypad->irq = platform_get_irq(pdev, 0);
if (keypad->irq < 0)
return keypad->irq;
@@ -254,19 +223,13 @@ static int ep93xx_keypad_probe(struct platform_device *pdev)
if (IS_ERR(keypad->mmio_base))
return PTR_ERR(keypad->mmio_base);
- err = ep93xx_keypad_acquire_gpio(pdev);
- if (err)
- return err;
-
- err = devm_add_action_or_reset(&pdev->dev,
- ep93xx_keypad_release_gpio_action, pdev);
- if (err)
- return err;
-
keypad->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(keypad->clk))
return PTR_ERR(keypad->clk);
+ device_property_read_u32(dev, "debounce-delay-ms", &keypad->debounce);
+ device_property_read_u16(dev, "cirrus,prescale", &keypad->prescale);
+
input_dev = devm_input_allocate_device(&pdev->dev);
if (!input_dev)
return -ENOMEM;
@@ -278,13 +241,13 @@ static int ep93xx_keypad_probe(struct platform_device *pdev)
input_dev->open = ep93xx_keypad_open;
input_dev->close = ep93xx_keypad_close;
- err = matrix_keypad_build_keymap(keymap_data, NULL,
+ err = matrix_keypad_build_keymap(NULL, NULL,
EP93XX_MATRIX_ROWS, EP93XX_MATRIX_COLS,
keypad->keycodes, input_dev);
if (err)
return err;
- if (keypad->pdata->flags & EP93XX_KEYPAD_AUTOREPEAT)
+ if (device_property_read_bool(&pdev->dev, "autorepeat"))
__set_bit(EV_REP, input_dev->evbit);
input_set_drvdata(input_dev, keypad);
@@ -315,10 +278,17 @@ static int ep93xx_keypad_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id ep93xx_keypad_of_ids[] = {
+ { .compatible = "cirrus,ep9307-keypad" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ep93xx_keypad_of_ids);
+
static struct platform_driver ep93xx_keypad_driver = {
.driver = {
.name = "ep93xx-keypad",
.pm = pm_sleep_ptr(&ep93xx_keypad_pm_ops),
+ .of_match_table = ep93xx_keypad_of_ids,
},
.probe = ep93xx_keypad_probe,
.remove = ep93xx_keypad_remove,
diff --git a/include/linux/soc/cirrus/ep93xx.h b/include/linux/soc/cirrus/ep93xx.h
index 267529ee2b3d..4327f66b7cf5 100644
--- a/include/linux/soc/cirrus/ep93xx.h
+++ b/include/linux/soc/cirrus/ep93xx.h
@@ -16,8 +16,6 @@ int ep93xx_pwm_acquire_gpio(struct platform_device *pdev);
void ep93xx_pwm_release_gpio(struct platform_device *pdev);
int ep93xx_ide_acquire_gpio(struct platform_device *pdev);
void ep93xx_ide_release_gpio(struct platform_device *pdev);
-int ep93xx_keypad_acquire_gpio(struct platform_device *pdev);
-void ep93xx_keypad_release_gpio(struct platform_device *pdev);
int ep93xx_i2s_acquire(void);
void ep93xx_i2s_release(void);
unsigned int ep93xx_chip_revision(void);
@@ -27,8 +25,6 @@ static inline int ep93xx_pwm_acquire_gpio(struct platform_device *pdev) { return
static inline void ep93xx_pwm_release_gpio(struct platform_device *pdev) {}
static inline int ep93xx_ide_acquire_gpio(struct platform_device *pdev) { return 0; }
static inline void ep93xx_ide_release_gpio(struct platform_device *pdev) {}
-static inline int ep93xx_keypad_acquire_gpio(struct platform_device *pdev) { return 0; }
-static inline void ep93xx_keypad_release_gpio(struct platform_device *pdev) {}
static inline int ep93xx_i2s_acquire(void) { return 0; }
static inline void ep93xx_i2s_release(void) {}
static inline unsigned int ep93xx_chip_revision(void) { return 0; }
--
2.39.2
^ permalink raw reply related
* [PATCH v4 29/42] dt-bindings: input: Add Cirrus EP93xx keypad
From: Nikita Shubin via B4 Relay @ 2023-09-15 8:11 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Alexander Sverdlin
Cc: linux-input, devicetree, linux-kernel, Arnd Bergmann,
Alexander Sverdlin
In-Reply-To: <20230915-ep93xx-v4-0-a1d779dcec10@maquefel.me>
From: Nikita Shubin <nikita.shubin@maquefel.me>
Add YAML bindings for ep93xx SoC keypad.
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
.../bindings/input/cirrus,ep9307-keypad.yaml | 87 ++++++++++++++++++++++
1 file changed, 87 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/cirrus,ep9307-keypad.yaml b/Documentation/devicetree/bindings/input/cirrus,ep9307-keypad.yaml
new file mode 100644
index 000000000000..ac020c9f564a
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/cirrus,ep9307-keypad.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/cirrus,ep9307-keypad.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cirrus ep93xx keypad
+
+maintainers:
+ - Alexander Sverdlin <alexander.sverdlin@gmail.com>
+
+allOf:
+ - $ref: /schemas/input/matrix-keymap.yaml#
+
+description:
+ The KPP is designed to interface with a keypad matrix with 2-point contact
+ or 3-point contact keys. The KPP is designed to simplify the software task
+ of scanning a keypad matrix. The KPP is capable of detecting, debouncing,
+ and decoding one or multiple keys pressed simultaneously on a keypad.
+
+properties:
+ compatible:
+ oneOf:
+ - const: cirrus,ep9307-keypad
+ - items:
+ - enum:
+ - cirrus,ep9312-keypad
+ - cirrus,ep9315-keypad
+ - const: cirrus,ep9307-keypad
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ debounce-delay-ms:
+ description: |
+ Time in microseconds that key must be pressed or
+ released for state change interrupt to trigger.
+
+ cirrus,prescale:
+ description: row/column counter pre-scaler load value
+ $ref: /schemas/types.yaml#/definitions/uint16
+ maximum: 1023
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - linux,keymap
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/cirrus,ep9301-clk.h>
+ #include <dt-bindings/input/input.h>
+ keypad@800f0000 {
+ compatible = "cirrus,ep9307-keypad";
+ reg = <0x800f0000 0x0c>;
+ interrupt-parent = <&vic0>;
+ interrupts = <29>;
+ clocks = <&eclk EP93XX_CLK_KEYPAD>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&keypad_default_pins>;
+ linux,keymap = <KEY_UP>,
+ <KEY_DOWN>,
+ <KEY_VOLUMEDOWN>,
+ <KEY_HOME>,
+ <KEY_RIGHT>,
+ <KEY_LEFT>,
+ <KEY_ENTER>,
+ <KEY_VOLUMEUP>,
+ <KEY_F6>,
+ <KEY_F8>,
+ <KEY_F9>,
+ <KEY_F10>,
+ <KEY_F1>,
+ <KEY_F2>,
+ <KEY_F3>,
+ <KEY_POWER>;
+ };
--
2.39.2
^ permalink raw reply related
* [PATCH v4 00/42] ep93xx device tree conversion
From: Nikita Shubin via B4 Relay @ 2023-09-15 8:10 UTC (permalink / raw)
To: Hartley Sweeten, Alexander Sverdlin, Russell King,
Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Andy Shevchenko, Michael Turquette, Stephen Boyd, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Nikita Shubin,
Sebastian Reichel, Daniel Lezcano, Thomas Gleixner,
Alessandro Zummo, Alexandre Belloni, Wim Van Sebroeck,
Guenter Roeck, Thierry Reding, Uwe Kleine-König, Mark Brown,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vinod Koul, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Liam Girdwood, Jaroslav Kysela, Takashi Iwai
Cc: linux-arm-kernel, linux-kernel, linux-gpio, linux-clk, devicetree,
linux-pm, linux-rtc, linux-watchdog, linux-pwm, linux-spi, netdev,
dmaengine, linux-mtd, linux-ide, linux-input, alsa-devel,
Arnd Bergmann, Bartosz Golaszewski, Krzysztof Kozlowski,
Andy Shevchenko, Andrew Lunn
This series aims to convert ep93xx from platform to full device tree support.
The main goal is to receive ACK's to take it via Arnd's arm-soc branch.
Major changes:
- drop newline at the end from each YAML files
- rename dma and clk bindings headers to match first compatible
- shrink SoC exported functions number to only 2
- dropped some ep93xx_pata fixes from these series
- dropped m48t86 stuff from these series
Bit thanks to Andy Shevchenko for thorough review.
Some parts are were sent in advance, so these series depends on the
following:
Link: https://lore.kernel.org/all/20230823-m48t86_device_tree-v2-0-21ff275f949d@maquefel.me/
Link: https://lore.kernel.org/all/20230823-ep93xx_pata_fixes-v1-0-d7e7229be148@maquefel.me/
---
Changes in v4:
- gpio: ep93xx: split device in multiple
- s/generic_handle_irq/generic_handle_domain_irq/
- s/int offset/irq_hw_number_t offset/ though now it looks a bit odd to me
- drop i = 0
- drop 'error'
- use dev_err_probe withour printing devname once again
dt-bindings: clock: Add Cirrus EP93xx
- renamed cirrus,ep93xx-clock.h -> cirrus,ep9301-clk.h
clk: ep93xx: add DT support for Cirrus EP93xx
- drop unused includes
- use .name only for xtali, pll1, pll2 parents
- convert // to /*
- pass clk_parent_data instead of char* clock name
dt-bindings: pinctrl: Add Cirrus EP93xx
- s/additionalProperties/unevaluatedProperties/
dt-bindings: soc: Add Cirrus EP93xx
- move syscon to soc directory
- add vendor prefix
- make reboot same style as pinctrl, clk
- use absolute path for ref
- expand example
soc: Add SoC driver for Cirrus ep93xx
- s/0xf0000000/GENMASK(31, 28)/
- s/ret/ep93xx_chip_revision(map)/
- drop symbol exports
- convert to platform driver
dt-bindings: rtc: Add Cirrus EP93xx
- allOf: with $ref to rtc.yaml
- s/additionalProperties/unevaluatedProperties/
dt-bindings: watchdog: Add Cirrus EP93x
- drop description
- reword
power: reset: Add a driver for the ep93xx reset
- lets use 'GPL-2.0+' instead of '(GPL-2.0)'
- s/of_device/of/
- drop mdelay with warning
- return 0 at the end
net: cirrus: add DT support for Cirrus EP93xx
- fix leaking np
mtd: nand: add support for ts72xx
- +bits.h
- drop comment
- ok to fwnode_get_next_child_node
- use goto to put handle and nand and report error
ARM: dts: add Cirrus EP93XX SoC .dtsi
- add simple-bus for ebi, as we don't require to setup anything
- add arm,pl011 compatible to uart nodes
- drop i2c-gpio, as it's isn't used anywhere
ARM: dts: ep93xx: add ts7250 board
- generic node name for temperature-sensor
- drop i2c
- move nand, rtc, watchdog to ebi node
- Link to v3: https://lore.kernel.org/r/20230605-ep93xx-v3-0-3d63a5f1103e@maquefel.me
---
Alexander Sverdlin (2):
ARM: dts: ep93xx: Add EDB9302 DT
ASoC: cirrus: edb93xx: Delete driver
Nikita Shubin (40):
gpio: ep93xx: split device in multiple
ARM: ep93xx: add swlocked prototypes
dt-bindings: clock: Add Cirrus EP93xx
clk: ep93xx: add DT support for Cirrus EP93xx
dt-bindings: pinctrl: Add Cirrus EP93xx
pinctrl: add a Cirrus ep93xx SoC pin controller
dt-bindings: power: reset: Add ep93xx reset
power: reset: Add a driver for the ep93xx reset
dt-bindings: soc: Add Cirrus EP93xx
soc: Add SoC driver for Cirrus ep93xx
dt-bindings: timers: Add Cirrus EP93xx
clocksource: ep93xx: Add driver for Cirrus Logic EP93xx
dt-bindings: rtc: Add Cirrus EP93xx
rtc: ep93xx: add DT support for Cirrus EP93xx
dt-bindings: watchdog: Add Cirrus EP93x
watchdog: ep93xx: add DT support for Cirrus EP93xx
dt-bindings: pwm: Add Cirrus EP93xx
pwm: ep93xx: add DT support for Cirrus EP93xx
dt-bindings: spi: Add Cirrus EP93xx
spi: ep93xx: add DT support for Cirrus EP93xx
dt-bindings: net: Add Cirrus EP93xx
net: cirrus: add DT support for Cirrus EP93xx
dt-bindings: dma: Add Cirrus EP93xx
dma: cirrus: add DT support for Cirrus EP93xx
dt-bindings: mtd: Add ts7200 nand-controller
mtd: nand: add support for ts72xx
dt-bindings: ata: Add Cirrus EP93xx
ata: pata_ep93xx: add device tree support
dt-bindings: input: Add Cirrus EP93xx keypad
input: keypad: ep93xx: add DT support for Cirrus EP93xx
dt-bindings: wdt: Add ts72xx
wdt: ts72xx: add DT support for ts72xx
gpio: ep93xx: add DT support for gpio-ep93xx
ARM: dts: add Cirrus EP93XX SoC .dtsi
ARM: dts: ep93xx: add ts7250 board
ARM: ep93xx: DT for the Cirrus ep93xx SoC platforms
pwm: ep93xx: drop legacy pinctrl
ata: pata_ep93xx: remove legacy pinctrl use
ARM: ep93xx: delete all boardfiles
ARM: ep93xx: soc: drop defines
.../bindings/arm/cirrus/cirrus,ep9301.yaml | 38 +
.../bindings/ata/cirrus,ep9312-pata.yaml | 42 +
.../bindings/clock/cirrus,ep9301-clk.yaml | 46 +
.../bindings/dma/cirrus,ep9301-dma-m2m.yaml | 69 +
.../bindings/dma/cirrus,ep9301-dma-m2p.yaml | 121 ++
.../bindings/input/cirrus,ep9307-keypad.yaml | 87 ++
.../devicetree/bindings/mtd/technologic,nand.yaml | 45 +
.../devicetree/bindings/net/cirrus,ep9301-eth.yaml | 59 +
.../bindings/pinctrl/cirrus,ep9301-pinctrl.yaml | 57 +
.../bindings/power/reset/cirrus,ep9301-reboot.yaml | 34 +
.../devicetree/bindings/pwm/cirrus,ep9301-pwm.yaml | 46 +
.../devicetree/bindings/rtc/cirrus,ep9301-rtc.yaml | 42 +
.../bindings/soc/cirrus/cirrus,ep9301-syscon.yaml | 71 +
.../devicetree/bindings/spi/cirrus,ep9301-spi.yaml | 61 +
.../bindings/timer/cirrus,ep9301-timer.yaml | 49 +
.../bindings/watchdog/cirrus,ep9301-wdt.yaml | 42 +
.../bindings/watchdog/technologic,ts7200-wdt.yaml | 45 +
arch/arm/Makefile | 1 -
arch/arm/boot/dts/cirrus/Makefile | 4 +
arch/arm/boot/dts/cirrus/ep93xx-bk3.dts | 124 ++
arch/arm/boot/dts/cirrus/ep93xx-edb9302.dts | 180 +++
arch/arm/boot/dts/cirrus/ep93xx-ts7250.dts | 143 ++
arch/arm/boot/dts/cirrus/ep93xx.dtsi | 454 +++++++
arch/arm/mach-ep93xx/Kconfig | 20 +-
arch/arm/mach-ep93xx/Makefile | 11 -
arch/arm/mach-ep93xx/clock.c | 733 ----------
arch/arm/mach-ep93xx/core.c | 1017 --------------
arch/arm/mach-ep93xx/dma.c | 114 --
arch/arm/mach-ep93xx/edb93xx.c | 344 -----
arch/arm/mach-ep93xx/ep93xx-regs.h | 38 -
arch/arm/mach-ep93xx/gpio-ep93xx.h | 111 --
arch/arm/mach-ep93xx/hardware.h | 25 -
arch/arm/mach-ep93xx/irqs.h | 76 --
arch/arm/mach-ep93xx/platform.h | 42 -
arch/arm/mach-ep93xx/soc.h | 212 ---
arch/arm/mach-ep93xx/ts72xx.c | 422 ------
arch/arm/mach-ep93xx/ts72xx.h | 94 --
arch/arm/mach-ep93xx/vision_ep9307.c | 311 -----
drivers/ata/pata_ep93xx.c | 33 +-
drivers/clk/Kconfig | 8 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-ep93xx.c | 753 +++++++++++
drivers/clocksource/Kconfig | 11 +
drivers/clocksource/Makefile | 1 +
.../clocksource}/timer-ep93xx.c | 155 ++-
drivers/dma/ep93xx_dma.c | 125 +-
drivers/gpio/gpio-ep93xx.c | 331 ++---
drivers/input/keyboard/ep93xx_keypad.c | 74 +-
drivers/mtd/nand/raw/Kconfig | 7 +
drivers/mtd/nand/raw/Makefile | 1 +
drivers/mtd/nand/raw/technologic-nand-controller.c | 166 +++
drivers/net/ethernet/cirrus/ep93xx_eth.c | 63 +-
drivers/pinctrl/Kconfig | 7 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-ep93xx.c | 1429 ++++++++++++++++++++
drivers/power/reset/Kconfig | 10 +
drivers/power/reset/Makefile | 1 +
drivers/power/reset/ep93xx-restart.c | 85 ++
drivers/pwm/pwm-ep93xx.c | 26 +-
drivers/rtc/rtc-ep93xx.c | 8 +
drivers/soc/Kconfig | 1 +
drivers/soc/Makefile | 1 +
drivers/soc/cirrus/Kconfig | 12 +
drivers/soc/cirrus/Makefile | 2 +
drivers/soc/cirrus/soc-ep93xx.c | 215 +++
drivers/spi/spi-ep93xx.c | 28 +-
drivers/watchdog/ep93xx_wdt.c | 8 +
drivers/watchdog/ts72xx_wdt.c | 8 +
include/dt-bindings/clock/cirrus,ep9301-clk.h | 41 +
include/dt-bindings/dma/cirrus,ep93xx-dma.h | 26 +
include/linux/platform_data/dma-ep93xx.h | 25 +-
include/linux/platform_data/eth-ep93xx.h | 10 -
include/linux/platform_data/keypad-ep93xx.h | 32 -
include/linux/soc/cirrus/ep93xx.h | 29 +-
sound/soc/cirrus/Kconfig | 9 -
sound/soc/cirrus/Makefile | 4 -
sound/soc/cirrus/edb93xx.c | 117 --
77 files changed, 5126 insertions(+), 4168 deletions(-)
---
base-commit: bdc09c8e8b16d494ccd1c56e903e78dd76455a35
change-id: 20230605-ep93xx-01c76317e2d2
Best regards,
--
Nikita Shubin <nikita.shubin@maquefel.me>
^ 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