* Re: [PATCH 1/2] Input: cs40l26: Support for CS40L26 Boosted Haptic Amplifier
From: Fred Treven @ 2023-04-14 20:51 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Charles Keepax, dmitry.torokhov@gmail.com, Ben Bright,
James Ogletree, lee@kernel.org, jdelvare@suse.de, joel@jms.id.au,
cy_huang@richtek.com, rdunlap@infradead.org,
eajames@linux.ibm.com, ping.bai@nxp.com, msp@baylibre.com,
arnd@arndb.de, bartosz.golaszewski@linaro.org,
linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
patches@opensource.cirrus.com
In-Reply-To: <ZDYakQMOPsPTbGe0@nixie71>
>> +
>> + return cs40l26_probe(cs40l26, pdata);
>> +}
>> +
>> +static void cs40l26_i2c_remove(struct i2c_client *client)
>> +{
>> + struct cs40l26_private *cs40l26 = i2c_get_clientdata(client);
>> +
>> + cs40l26_remove(cs40l26);
>> +}
>> +
>> +static struct i2c_driver cs40l26_i2c_driver = {
>> + .driver = {
>> + .name = "cs40l26",
>> + .of_match_table = cs40l26_of_match,
>> + .pm = &cs40l26_pm_ops,
>
> Please guard this with the new pm_sleep_ptr(), as not all platforms would
> define CONFIG_PM. More comments in the core driver.
Understood, and I certainly agree with this change. One thing I’m unsure of is what the effect would be on the driver if CONFIG_PM is not set in the .config. Surely, this driver would not work as expected right? Should I add a dependency in the kconfig to avoid building the driver without CONFIG_PM?
>
>> +{
>> + size_t len_words = len_bytes / sizeof(__be32);
>> + struct cs_dsp_coeff_ctl *ctl;
>> + __be32 *val;
>> + int i, ret;
>> +
>> + ctl = cs_dsp_get_ctl(dsp, name, WMFW_ADSP2_XM, algo_id);
>> + if (IS_ERR_OR_NULL(ctl)) {
>> + dev_err(dsp->dev, "Failed to find fw ctl %s\n", name);
>> + return -ENOENT;
>> + }
>> +
>> + val = kzalloc(len_bytes, GFP_KERNEL);
>> + if (!val)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < len_words; i++)
>> + val[i] = cpu_to_be32(buf[i]);
>> +
>> + ret = cs_dsp_coeff_write_ctrl(ctl, off_words, val, len_bytes);
>> + if (ret)
>> + dev_err(dsp->dev, "Failed to write fw ctl %s: %d\n", name, ret);
>> +
>> + kfree(val);
>> +
>> + return ret;
>> +}
>> +
>> +static inline int cs40l26_fw_ctl_write(struct cs_dsp *dsp, const char * const name,
>> + unsigned int algo_id, u32 val)
>> +{
>> + return cs40l26_fw_ctl_write_raw(dsp, name, algo_id, 0, sizeof(u32), &val);
>> +}
>> +
>> +static int cs40l26_fw_ctl_read_raw(struct cs_dsp *dsp, const char * const name,
>> + unsigned int algo_id, unsigned int off_words, size_t len_bytes, u32 *buf)
>> +{
>> + size_t len_words = len_bytes / sizeof(u32);
>> + struct cs_dsp_coeff_ctl *ctl;
>> + int i, ret;
>> +
>> + ctl = cs_dsp_get_ctl(dsp, name, WMFW_ADSP2_XM, algo_id);
>> + if (IS_ERR_OR_NULL(ctl)) {
>> + dev_err(dsp->dev, "Failed to find fw ctl %s\n", name);
>> + return -ENOENT;
>> + }
>> +
>> + ret = cs_dsp_coeff_read_ctrl(ctl, off_words, buf, len_bytes);
>> + if (ret) {
>> + dev_err(dsp->dev, "Failed to read fw ctl %s: %d\n", name, ret);
>> + return ret;
>> + }
>> +
>> + for (i = 0; i < len_words; i++)
>> + buf[i] = be32_to_cpu(buf[i]);
>> +
>> + return 0;
>> +}
>> +
>> +static inline int cs40l26_fw_ctl_read(struct cs_dsp *dsp, const char * const name,
>> + unsigned int algo_id, u32 *buf)
>> +{
>> + return cs40l26_fw_ctl_read_raw(dsp, name, algo_id, 0, sizeof(u32), buf);
>> +}
>
> None of these four functions seem particularly specific to L26; is there any
> reason they don't belong in cs_dsp or wm_adsp? In fact, some of the functions
> throughout those drivers seem to be doing similar work.
>
> Maybe out of scope for this particular submission, but is there not any room
> for re-use here?
>
I wanted to avoid making too many changes to the firmware drivers in this initial submission, and I think I’d like to keep it as-is for now, but I think moving this combination to cs_dsp is the right move. Let me know if you feel that it would be better to make the change now rather than later.
> On Tue, Apr 11, 2023 at 09:27:08AM +0000, Charles Keepax wrote:
>> On Mon, Apr 10, 2023 at 07:31:56PM -0500, Jeff LaBundy wrote:
>>> On Mon, Apr 10, 2023 at 08:56:34AM +0000, Charles Keepax wrote:
>>>> On Sat, Apr 08, 2023 at 10:44:39PM -0500, Jeff LaBundy wrote:
>>>> I would far rather not have every single attempt to communicate
>>>> with the device wrapped in a retry if the communication failed
>>>> incase the device is hibernating. It seems much cleaner, and less
>>>> likely to risk odd behaviour, to know we have brought the device
>>>> out of hibernation.
>>
>>> A common way to deal with this is that of [1], where the bus calls
>>> are simply wrapped with all retry logic limited to two places (read
>>> and write). These functions could also print the register address
>>> in case of failure, solving the problem of having dozens of custom
>>> error messages thorughout the driver.
>>
>> I suspect this really comes down to a matter of taste, but my
>> thoughts would be that the code is shorter that way, but not
>> necessarily simpler. This feels far more error prone and likely
>> to encounter issues where the device hibernates at a time someone
>> hadn't properly thought through. I am far more comfortable with
>> the device is blocked from hibernating whilst the driver is
>> actively engaged with it and it keeps any special handling for
>> exiting hibernate in one place.
>
> Fair enough. I do concede that having this control in the driver as
> opposed to DSP FW is more nimble and makes it easier to respond to
> customer issues; I'm sure your battle scars will agree :)
I concur with Charles here, and it seems like you’re also ok with this so I will leave it as-is.
>>> +static int cs40l26_irq_update_mask(struct cs40l26_private *cs40l26, u32 reg, u32 val, u32 bit_mask)
>>> +{
>>> + u32 eint_reg, cur_mask, new_mask;
>>> + int ret;
>>> +
>>> + if (reg == CS40L26_IRQ1_MASK_1) {
>>> + eint_reg = CS40L26_IRQ1_EINT_1;
>>> + } else if (reg == CS40L26_IRQ1_MASK_2) {
>>> + eint_reg = CS40L26_IRQ1_EINT_2;
>>> + } else {
>>> + dev_err(cs40l26->dev, "Invalid IRQ mask reg: 0x%08X\n", reg);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ret = regmap_read(cs40l26->regmap, reg, &cur_mask);
>>> + if (ret) {
>>> + dev_err(cs40l26->dev, "Failed to get IRQ mask\n");
>>
>> Having a custom error message for every possible failed register read
>> does not ultimately aid in debugging and unnecessarily grows the size
>> of the driver.
>>
>> If a specific message is absolutely necessary, then add wrappers around
>> regmap_read/write and print the failed address. However, this does not
>> seem necessary either. Simply propagate the error code all the way up
>> to the caller, whether it is probe or a sysfs attribute.
>>
>> Stated another way:
>>
>> error = regmap_...(...);
>> if (error)
>> return error;
>>
>
> Not sure I feel super strongly on this one, but I do find when
> debugging issues on drivers that do this that I usually end up
> adding the printks back in.
I went ahead and implemented the change Jeff suggested here; it does streamline the driver to a good degree, and I think it’s worth making the adjustment.
>>> +static int cs40l26_dsp_state_get(struct cs40l26_private *cs40l26, u8 *state)
>>> +{
>>> + bool mutex_available = !mutex_is_locked(&cs40l26->dsp.pwr_lock);
>>
>> This is dangerous and a sign that locks are not properly managed. What would
>> be a case where you do not know the state of the lock upon entering this function?
>> If you do not know whether the mutex is locked inside this function, it is not the
>> proper place to grab it.
>>
>
> Yes I whole heartedly agree here this should not be done this
> way.
I certainly understand the concern here, but I wanted to provide some context. Since cs40l26_dsp_state_get() is called both in the cs_dsp_pre_run() callback as well as outside of this, there are instances where pwr_lock needs to be grabbed and times when it is already taken (in the instance of the callback invocation). Because of this, attempting to take the lock during pre_run causes a deadlock. I felt that it was quite clear when the lock would be available and when it wouldn’t be and to avoid the deadlock, I checked whether or not the lock was available. What do y’all feel is the best way to handle this? Some separate variable to determine if we are in the pre_run sequence or not? A separate function for dsp_state_get() for when it is not invoked from the callback?
>
>> +
>> + cs40l26_pm_runtime_teardown(cs40l26);
>> +
>> + if (cs40l26->dsp.running)
>> + cs_dsp_stop(&cs40l26->dsp);
>> + if (cs40l26->dsp.booted)
>> + cs_dsp_power_down(&cs40l26->dsp);
>> + if (&cs40l26->dsp)
>> + cs_dsp_remove(&cs40l26->dsp);
>> +
>> + if (cs40l26->vibe_workqueue) {
>> + cancel_work_sync(&cs40l26->erase_work);
>> + cancel_work_sync(&cs40l26->set_gain_work);
>> + cancel_work_sync(&cs40l26->upload_work);
>> + cancel_work_sync(&cs40l26->vibe_start_work);
>> + cancel_work_sync(&cs40l26->vibe_stop_work);
>> + destroy_workqueue(cs40l26->vibe_workqueue);
>> + }
>> +
>> + mutex_destroy(&cs40l26->lock);
>
> This ultimately does nothing.
Could you please clarify a bit what you mean here? Does the mutex not need to be destroyed explicitly? What about the work queue? Is it canceled/destroyed automatically when the module is removed? Are the cs_dsp functions not necessary to gracefully stop the DSP etc.?
In v2 of this patch, I plan to implement all of the suggestions in your comments that I didn’t question or disagree with here. Thanks for your in-depth analysis.
Best regards,
Fred
^ permalink raw reply
* [PATCH 0/2] Input: cap11xx add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-04-14 23:38 UTC (permalink / raw)
To: dmitry.torokhov, krzysztof.kozlowski+dt
Cc: jiriv, devicetree, linux-input, linux-kernel, robh+dt,
u.kleine-koenig
PATCH 1 - add documentation for new options
PATCH 2 - add support for advanced settings into driver
Jiri Valek - 2N (2):
dt-bindings: input: microchip,cap11xx: add advanced sensitivity
settings
Input: cap11xx - add advanced sensitivity settings
.../bindings/input/microchip,cap11xx.yaml | 64 ++++-
drivers/input/keyboard/cap11xx.c | 263 +++++++++++++++---
2 files changed, 278 insertions(+), 49 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH 1/2] dt-bindings: input: microchip,cap11xx: add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-04-14 23:38 UTC (permalink / raw)
To: dmitry.torokhov, krzysztof.kozlowski+dt
Cc: jiriv, devicetree, linux-input, linux-kernel, robh+dt,
u.kleine-koenig
In-Reply-To: <20230414233815.4004526-1-jiriv@axis.com>
Add support for advanced sensitivity settings and signal guard feature.
Signed-off-by: Jiri Valek - 2N <jiriv@axis.com>
---
.../bindings/input/microchip,cap11xx.yaml | 64 ++++++++++++++++++-
1 file changed, 61 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
index 5fa625b5c5fb..08e28226a164 100644
--- a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
+++ b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
@@ -45,13 +45,13 @@ properties:
Enables the Linux input system's autorepeat feature on the input device.
linux,keycodes:
- minItems: 6
- maxItems: 6
+ minItems: 3
+ maxItems: 8
description: |
Specifies an array of numeric keycode values to
be used for the channels. If this property is
omitted, KEY_A, KEY_B, etc are used as defaults.
- The array must have exactly six entries.
+ The number of entries must correspond to the number of channels.
microchip,sensor-gain:
$ref: /schemas/types.yaml#/definitions/uint32
@@ -70,6 +70,58 @@ properties:
open drain. This property allows using the active
high push-pull output.
+ microchip,sensitivity-delta-sense:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 32
+ enum: [1, 2, 4, 8, 16, 32, 64, 128]
+ description: |
+ Optional parameter. Controls the sensitivity multiplier of a touch detection.
+ At the more sensitive settings, touches are detected for a smaller delta
+ capacitance corresponding to a “lighter” touch.
+
+ microchip,sensitivity-base-shift:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 256
+ enum: [1, 2, 4, 8, 16, 32, 64, 128, 256]
+ description: |
+ Optional parameter. Controls data scaling factor.
+ The higher the value of these bits, the larger the range and the lower
+ the resolution of the data presented. These settings will not affect
+ touch detection or sensitivity.
+
+ microchip,signal-guard:
+ minItems: 3
+ maxItems: 8
+ enum: [0, 1]
+ default: 0
+ description: |
+ Optional parameter supported only for CAP129x.
+ The signal guard isolates the signal from virtual grounds.
+ If enabled then the behavior of the channel is changed to signal guard.
+ The number of entries must correspond to the number of channels.
+
+ microchip,input-treshold:
+ minItems: 3
+ maxItems: 8
+ minimum: 0
+ maximum: 127
+ default: 64
+ description: |
+ Optional parameter. Specifies the delta threshold that is used to
+ determine if a touch has been detected.
+ The number of entries must correspond to the number of channels.
+
+ microchip,calib-sensitivity:
+ minItems: 3
+ maxItems: 8
+ enum: [1, 2, 4]
+ default: 1
+ description: |
+ Optional parameter supported only for CAP129x. Specifies an array of
+ numeric values that controls the gain used by the calibration routine to
+ enable sensor inputs to be more sensitive for proximity detection.
+ The number of entries must correspond to the number of channels.
+
patternProperties:
"^led@[0-7]$":
type: object
@@ -122,6 +174,12 @@ examples:
reg = <0x28>;
autorepeat;
microchip,sensor-gain = <2>;
+ microchip,sensitivity-delta-sense = <16>;
+ microchip,sensitivity-base-shift = <128>;
+
+ microchip,signal-guard = <0>, <0>, <0>, <0>, <0>, <0>;
+ microchip,input-treshold = <21>, <18>, <46>, <46>, <46>, <21>;
+ microchip,calib-sensitivityj = <1>, <2>, <2>, <1>, <1>, <2>;
linux,keycodes = <103>, /* KEY_UP */
<106>, /* KEY_RIGHT */
--
2.25.1
^ permalink raw reply related
* [PATCH 2/2] Input: cap11xx - add advanced sensitivity settings
From: Jiri Valek - 2N @ 2023-04-14 23:38 UTC (permalink / raw)
To: dmitry.torokhov, krzysztof.kozlowski+dt
Cc: jiriv, devicetree, linux-input, linux-kernel, robh+dt,
u.kleine-koenig
In-Reply-To: <20230414233815.4004526-1-jiriv@axis.com>
Add support for advanced sensitivity settings that allows more precise
tunig of touch buttons. Input-treshold allows to set the sensitivity for
each channel separately. Also add signal guard feature for CAP129x chips.
Signed-off-by: Jiri Valek - 2N <jiriv@axis.com>
---
drivers/input/keyboard/cap11xx.c | 263 +++++++++++++++++++++++++------
1 file changed, 217 insertions(+), 46 deletions(-)
diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
index 040696d0e49c..d387754e76f5 100644
--- a/drivers/input/keyboard/cap11xx.c
+++ b/drivers/input/keyboard/cap11xx.c
@@ -14,6 +14,7 @@
#include <linux/regmap.h>
#include <linux/i2c.h>
#include <linux/gpio/consumer.h>
+#include <linux/bitfield.h>
#define CAP11XX_REG_MAIN_CONTROL 0x00
#define CAP11XX_REG_MAIN_CONTROL_GAIN_SHIFT (6)
@@ -24,6 +25,8 @@
#define CAP11XX_REG_NOISE_FLAG_STATUS 0x0a
#define CAP11XX_REG_SENOR_DELTA(X) (0x10 + (X))
#define CAP11XX_REG_SENSITIVITY_CONTROL 0x1f
+#define CAP11XX_REG_SENSITIVITY_CONTROL_DELTA_SENSE_MASK 0x70
+#define CAP11XX_REG_SENSITIVITY_CONTROL_BASE_SHIFT_MASK 0x0F
#define CAP11XX_REG_CONFIG 0x20
#define CAP11XX_REG_SENSOR_ENABLE 0x21
#define CAP11XX_REG_SENSOR_CONFIG 0x22
@@ -32,6 +35,7 @@
#define CAP11XX_REG_CALIBRATION 0x26
#define CAP11XX_REG_INT_ENABLE 0x27
#define CAP11XX_REG_REPEAT_RATE 0x28
+#define CAP11XX_REG_SIGNAL_GUARD_ENABLE 0x29
#define CAP11XX_REG_MT_CONFIG 0x2a
#define CAP11XX_REG_MT_PATTERN_CONFIG 0x2b
#define CAP11XX_REG_MT_PATTERN 0x2d
@@ -47,6 +51,8 @@
#define CAP11XX_REG_SENSOR_BASE_CNT(X) (0x50 + (X))
#define CAP11XX_REG_LED_POLARITY 0x73
#define CAP11XX_REG_LED_OUTPUT_CONTROL 0x74
+#define CAP11XX_REG_CALIB_SENSITIVITY_CONFIG 0x80
+#define CAP11XX_REG_CALIB_SENSITIVITY_CONFIG2 0x81
#define CAP11XX_REG_LED_DUTY_CYCLE_1 0x90
#define CAP11XX_REG_LED_DUTY_CYCLE_2 0x91
@@ -78,12 +84,21 @@ struct cap11xx_led {
struct cap11xx_priv {
struct regmap *regmap;
+ struct device *dev;
struct input_dev *idev;
+ const struct cap11xx_hw_model *model;
+ u8 id;
struct cap11xx_led *leds;
int num_leds;
/* config */
+ u8 analog_gain;
+ u8 sensitivity_delta_sense;
+ u8 sensitivity_base_shift;
+ u8 signal_guard_inputs_mask;
+ u32 thresholds[8];
+ u32 calib_sensitivities[8];
u32 keycodes[];
};
@@ -181,6 +196,197 @@ static const struct regmap_config cap11xx_regmap_config = {
.volatile_reg = cap11xx_volatile_reg,
};
+static int
+cap11xx_write_calib_sens_config_1(struct cap11xx_priv *priv)
+{
+ return regmap_write(priv->regmap,
+ CAP11XX_REG_CALIB_SENSITIVITY_CONFIG,
+ (priv->calib_sensitivities[3] << 6) |
+ (priv->calib_sensitivities[2] << 4) |
+ (priv->calib_sensitivities[1] << 2) |
+ priv->calib_sensitivities[0]);
+}
+
+static int
+cap11xx_write_calib_sens_config_2(struct cap11xx_priv *priv)
+{
+ return regmap_write(priv->regmap,
+ CAP11XX_REG_CALIB_SENSITIVITY_CONFIG2,
+ (priv->calib_sensitivities[7] << 6) |
+ (priv->calib_sensitivities[6] << 4) |
+ (priv->calib_sensitivities[5] << 2) |
+ priv->calib_sensitivities[4]);
+}
+
+static int
+cap11xx_init_keys(struct cap11xx_priv *priv)
+{
+ struct device_node *node = priv->dev->of_node;
+ struct device *dev = priv->dev;
+ int i, error;
+ u32 u32_val;
+
+ if (!node) {
+ dev_err(dev, "Corresponding DT entry is not available\n");
+ return -ENODEV;
+ }
+
+ if (!of_property_read_u32(node, "microchip,sensor-gain", &u32_val)) {
+ if (priv->model->no_gain) {
+ dev_warn(dev,
+ "This model doesn't support 'sensor-gain'\n");
+ } else if (is_power_of_2(u32_val) && u32_val <= 8) {
+ priv->analog_gain = (u8)ilog2(u32_val);
+
+ error = regmap_update_bits(priv->regmap,
+ CAP11XX_REG_MAIN_CONTROL,
+ CAP11XX_REG_MAIN_CONTROL_GAIN_MASK,
+ priv->analog_gain << CAP11XX_REG_MAIN_CONTROL_GAIN_SHIFT);
+ if (error)
+ return error;
+ } else {
+ dev_err(dev, "Invalid sensor-gain value %u\n", u32_val);
+ return -EINVAL;
+ }
+ }
+
+ if (of_property_read_bool(node, "microchip,irq-active-high")) {
+ if (priv->id == CAP1106 ||
+ priv->id == CAP1126 ||
+ priv->id == CAP1188) {
+ error = regmap_update_bits(priv->regmap,
+ CAP11XX_REG_CONFIG2,
+ CAP11XX_REG_CONFIG2_ALT_POL,
+ 0);
+ if (error)
+ return error;
+ } else {
+ dev_warn(dev,
+ "This model doesn't support 'irq-active-high'\n");
+ }
+ }
+
+ if (!of_property_read_u32(node,
+ "microchip,sensitivity-delta-sense", &u32_val)) {
+ if (!is_power_of_2(u32_val) || u32_val > 128) {
+ dev_err(dev, "Invalid sensitivity-delta-sense value %u\n", u32_val);
+ return -EINVAL;
+ }
+
+ priv->sensitivity_delta_sense = (u8)ilog2(u32_val);
+ u32_val = ~(FIELD_PREP(CAP11XX_REG_SENSITIVITY_CONTROL_DELTA_SENSE_MASK,
+ priv->sensitivity_delta_sense));
+
+ error = regmap_update_bits(priv->regmap,
+ CAP11XX_REG_SENSITIVITY_CONTROL,
+ CAP11XX_REG_SENSITIVITY_CONTROL_DELTA_SENSE_MASK,
+ u32_val);
+ if (error)
+ return error;
+ }
+
+ if (!of_property_read_u32(node,
+ "microchip,sensitivity-base-shift", &u32_val)) {
+ if (!is_power_of_2(u32_val) || u32_val > 256) {
+ dev_err(dev, "Invalid sensitivity-base-shift value %u\n", u32_val);
+ return -EINVAL;
+ }
+
+ priv->sensitivity_base_shift = (u8)ilog2(u32_val);
+ u32_val = FIELD_PREP(CAP11XX_REG_SENSITIVITY_CONTROL_BASE_SHIFT_MASK,
+ priv->sensitivity_base_shift);
+
+ error = regmap_update_bits(priv->regmap,
+ CAP11XX_REG_SENSITIVITY_CONTROL,
+ CAP11XX_REG_SENSITIVITY_CONTROL_BASE_SHIFT_MASK,
+ u32_val);
+ if (error)
+ return error;
+ }
+
+ if (!of_property_read_u32_array(node, "microchip,input-treshold",
+ priv->thresholds, priv->model->num_channels)) {
+ for (i = 0; i < priv->model->num_channels; i++) {
+ if (priv->thresholds[i] > 127) {
+ dev_err(dev, "Invalid input-treshold value %u\n",
+ priv->thresholds[i]);
+ return -EINVAL;
+ }
+
+ error = regmap_write(priv->regmap,
+ CAP11XX_REG_SENSOR_THRESH(i),
+ priv->thresholds[i]);
+ if (error)
+ return error;
+ }
+ }
+
+ if (!of_property_read_u32_array(node, "microchip,calib-sensitivity",
+ priv->calib_sensitivities, priv->model->num_channels)) {
+ if (priv->id == CAP1293 || priv->id == CAP1298) {
+ for (i = 0; i < priv->model->num_channels; i++) {
+ if (!is_power_of_2(priv->calib_sensitivities[i]) ||
+ priv->calib_sensitivities[i] > 4) {
+ dev_err(dev, "Invalid calib-sensitivity value %u\n",
+ priv->calib_sensitivities[i]);
+ return -EINVAL;
+ }
+ priv->calib_sensitivities[i] = ilog2(priv->calib_sensitivities[i]);
+ }
+
+ error = cap11xx_write_calib_sens_config_1(priv);
+ if (error)
+ return error;
+
+ if (priv->id == CAP1298) {
+ error = cap11xx_write_calib_sens_config_2(priv);
+ if (error)
+ return error;
+ }
+ } else {
+ dev_warn(dev,
+ "This model doesn't support 'calib-sensitivity'\n");
+ }
+ }
+
+ for (i = 0; i < priv->model->num_channels; i++) {
+ if (!of_property_read_u32_index(node, "microchip,signal-guard",
+ i, &u32_val)) {
+ if (u32_val > 1)
+ return -EINVAL;
+ if (u32_val)
+ priv->signal_guard_inputs_mask |= 0x01 << i;
+ }
+ }
+
+ if (priv->signal_guard_inputs_mask) {
+ if (priv->id == CAP1293 || priv->id == CAP1298) {
+ error = regmap_write(priv->regmap,
+ CAP11XX_REG_SIGNAL_GUARD_ENABLE,
+ priv->signal_guard_inputs_mask);
+ if (error)
+ return error;
+ } else {
+ dev_warn(dev,
+ "This model doesn't support 'signal-guard'\n");
+ }
+ }
+
+ /* Provide some useful defaults */
+ for (i = 0; i < priv->model->num_channels; i++)
+ priv->keycodes[i] = KEY_A + i;
+
+ of_property_read_u32_array(node, "linux,keycodes",
+ priv->keycodes, priv->model->num_channels);
+
+ /* Disable autorepeat. The Linux input system has its own handling. */
+ error = regmap_write(priv->regmap, CAP11XX_REG_REPEAT_RATE, 0);
+ if (error)
+ return error;
+
+ return 0;
+}
+
static irqreturn_t cap11xx_thread_func(int irq_num, void *data)
{
struct cap11xx_priv *priv = data;
@@ -332,11 +538,9 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
const struct i2c_device_id *id = i2c_client_get_device_id(i2c_client);
struct device *dev = &i2c_client->dev;
struct cap11xx_priv *priv;
- struct device_node *node;
const struct cap11xx_hw_model *cap;
- int i, error, irq, gain = 0;
+ int i, error, irq;
unsigned int val, rev;
- u32 gain32;
if (id->driver_data >= ARRAY_SIZE(cap11xx_devices)) {
dev_err(dev, "Invalid device ID %lu\n", id->driver_data);
@@ -355,6 +559,8 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
if (!priv)
return -ENOMEM;
+ priv->dev = dev;
+
priv->regmap = devm_regmap_init_i2c(i2c_client, &cap11xx_regmap_config);
if (IS_ERR(priv->regmap))
return PTR_ERR(priv->regmap);
@@ -384,50 +590,15 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
return error;
dev_info(dev, "CAP11XX detected, model %s, revision 0x%02x\n",
- id->name, rev);
- node = dev->of_node;
-
- if (!of_property_read_u32(node, "microchip,sensor-gain", &gain32)) {
- if (cap->no_gain)
- dev_warn(dev,
- "This version doesn't support sensor gain\n");
- else if (is_power_of_2(gain32) && gain32 <= 8)
- gain = ilog2(gain32);
- else
- dev_err(dev, "Invalid sensor-gain value %d\n", gain32);
- }
+ id->name, rev);
- if (id->driver_data == CAP1106 ||
- id->driver_data == CAP1126 ||
- id->driver_data == CAP1188) {
- if (of_property_read_bool(node, "microchip,irq-active-high")) {
- error = regmap_update_bits(priv->regmap,
- CAP11XX_REG_CONFIG2,
- CAP11XX_REG_CONFIG2_ALT_POL,
- 0);
- if (error)
- return error;
- }
- }
+ priv->model = cap;
+ priv->id = id->driver_data;
- /* Provide some useful defaults */
- for (i = 0; i < cap->num_channels; i++)
- priv->keycodes[i] = KEY_A + i;
-
- of_property_read_u32_array(node, "linux,keycodes",
- priv->keycodes, cap->num_channels);
-
- if (!cap->no_gain) {
- error = regmap_update_bits(priv->regmap,
- CAP11XX_REG_MAIN_CONTROL,
- CAP11XX_REG_MAIN_CONTROL_GAIN_MASK,
- gain << CAP11XX_REG_MAIN_CONTROL_GAIN_SHIFT);
- if (error)
- return error;
- }
+ dev_info(dev, "CAP11XX device detected, model %s, revision 0x%02x\n",
+ id->name, rev);
- /* Disable autorepeat. The Linux input system has its own handling. */
- error = regmap_write(priv->regmap, CAP11XX_REG_REPEAT_RATE, 0);
+ error = cap11xx_init_keys(priv);
if (error)
return error;
@@ -439,7 +610,7 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
priv->idev->id.bustype = BUS_I2C;
priv->idev->evbit[0] = BIT_MASK(EV_KEY);
- if (of_property_read_bool(node, "autorepeat"))
+ if (of_property_read_bool(dev->of_node, "autorepeat"))
__set_bit(EV_REP, priv->idev->evbit);
for (i = 0; i < cap->num_channels; i++)
@@ -474,7 +645,7 @@ static int cap11xx_i2c_probe(struct i2c_client *i2c_client)
if (error)
return error;
- irq = irq_of_parse_and_map(node, 0);
+ irq = irq_of_parse_and_map(dev->of_node, 0);
if (!irq) {
dev_err(dev, "Unable to parse or map IRQ\n");
return -ENXIO;
--
2.25.1
^ permalink raw reply related
* [PATCH v3 0/5] Add support for Focaltech FTS Touchscreen
From: Joel Selvaraj @ 2023-04-15 2:02 UTC (permalink / raw)
To: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Jeff LaBundy, Markuss Broks,
Jean Delvare, Max Krummenacher, Chris Morgan, Job Noorman,
Alistair Francis, Hans de Goede, Maxime Ripard
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel, Joel Selvaraj
Changes in v3:(Suggested by Krzysztof Kozlowski and Konrad Dybcio)
--------------
- dts: removed the invalid "input-enable" property
- dts: replace interrupts with interrupts-extended
- dts: removed redundant dma configuration
- dts: reorder pinctrl and pinctrl-names
- bindings: moved unevaluatedProperties after required
- bindings: make interrupts a required property (new change from my end)
- bindings: update example based on dts changes
I have made the interrupts a required property in the bindings as the driver
will not function without an interrupt. Because of this new change, I have not
picked up the reviewed by tag of Krzysztof Kozlowski for the bindings.
Changes in v2:
--------------
1. dt-bindings changes (Suggested by Krzysztof Kozlowski)
- changed file name from focaltech,fts.yaml to focaltech,fts5452.yaml
- removed focaltech,max-touch-number property, handled in driver now
- removed touchscreen-* properties and used unevaluatedProperties: false
instead of additionalProperties: false
- fixed the example dts node name to be generic
2. FTS Touchscreen driver changes (Suggested by Markuss Broks and Jeff LaBundy)
- removed repeated license terms since SPDX tag is used
- includes are now sorted
- added the missing input_mt_sync_frame when reporting touch
- focaltech,max-touch-number is no longer read from dts and instead
specified in the driver as compatible data.
- removed redundant __set_bits
- input_mt_init_slots is now called after the axes are defined
- irq handler now returns IRQ_NONE when there is an i2c error
- other minor fixes and refactoring as suggested
- renamed driver filename from focaltech_fts.c to focaltech_fts5452.c
(Suggested by Krzysztof Kozlowski)
3. dts changes (Suggested by Krzysztof Kozlowski)
- use generic touchscreen nodes
- removed focaltech,max-touch-number property
- irq type was specified wrongly for Poco F1 in v1. Changed the irq
type to IRQ_TYPE_EDGE_FALLING as that is correct.
Some Clarifications on v1 comments:
-----------------------------------
1. Jeff LaBundy suggested I could read chip id with the following:
__be16 val;
regmap_raw_read(data->regmap, FTS_REG_CHIP_ID_H, &val, sizeof(val));
But this is not possible because FTS_REG_CHIP_ID_H and FTS_REG_CHIP_ID_L
are not continuous register, therefore reading it together as 16-bit values
will not work. So I went with what Markuss Broks suggested:
regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
id |= val << 8;
2. As Markuss Broks suggested, I tried to cast the buffer to struct, but
unfortunately was not able to successfully do it. The buffer layout is
weirdly split into 4 bits and 12 bits at someplaces which makes it hard
to cast into a struct. For example, we can note
type = buf[base + 3] >> 6
x = ((buf[base + 3] & 0x0F) << 8) + (buf[base + 4] & 0xFF);
Here at buffer index 3, the first two bits (>>6) are used for denoting
event type. The next two bits are not used. But the last 4 bits (&0x0F)
of buffer[3] are added with buffer index 4 to get the x position.
I don't know how to handle these when casting to a struct. I tried
experimenting with bitfields in struct, but to no avail. So I am sticking
with my initial implementation for now.
Kindly let me know if any further improvements are needed. Thanks.
The Focaltech FTS driver supports several variants of focaltech
touchscreens found in ~2018 era smartphones including variants found on
the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
This driver is loosely based on the original driver from Focaltech and
the patches submitted by Caleb Connolly previously[1] but has been
simplified and largely reworked.
[1] https://patchwork.kernel.org/project/linux-input/patch/20220123173650.290349-3-caleb@connolly.tech/
Joel Selvaraj (5):
dt-bindings: input: touchscreen: add focaltech,fts5452 touchscreen
Input: add driver for Focaltech FTS touchscreen
arm64: dts: qcom: sdm845-xiaomi-beryllium-common: add touchscreen
related nodes
arm64: dts: qcom: sdm845-xiaomi-beryllium-ebbg: introduce support for
fts touchscreen
arm64: dts: qcom: sdm845-shift-axolotl: update focaltech touchscreen
properties
.../input/touchscreen/focaltech,fts5452.yaml | 71 +++
MAINTAINERS | 8 +
.../boot/dts/qcom/sdm845-shift-axolotl.dts | 18 +-
.../qcom/sdm845-xiaomi-beryllium-common.dtsi | 37 ++
.../dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts | 21 +
drivers/input/touchscreen/Kconfig | 12 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
8 files changed, 590 insertions(+), 10 deletions(-)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c
--
2.40.0
^ permalink raw reply
* [PATCH v3 1/5] dt-bindings: input: touchscreen: add focaltech,fts5452 touchscreen
From: Joel Selvaraj @ 2023-04-15 2:02 UTC (permalink / raw)
To: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Jeff LaBundy, Markuss Broks,
Jean Delvare, Max Krummenacher, Chris Morgan, Job Noorman,
Alistair Francis, Hans de Goede, Maxime Ripard
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel, Joel Selvaraj
In-Reply-To: <20230415020222.216232-1-joelselvaraj.oss@gmail.com>
Document the Focaltech FTS touchscreen driver.
Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
Signed-off-by: Caleb Connolly <caleb@connolly.tech>
---
.../input/touchscreen/focaltech,fts5452.yaml | 71 +++++++++++++++++++
1 file changed, 71 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
diff --git a/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml b/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
new file mode 100644
index 000000000000..28270dc5ed67
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/focaltech,fts5452.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Focaltech FTS I2C Touchscreen Controller
+
+maintainers:
+ - Joel Selvaraj <joelselvaraj.oss@gmail.com>
+ - Caleb Connolly <caleb@connolly.tech>
+
+allOf:
+ - $ref: touchscreen.yaml#
+
+properties:
+ compatible:
+ enum:
+ - focaltech,fts5452
+ - focaltech,fts8719
+
+ reg:
+ const: 0x38
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+
+ avdd-supply:
+ description: regulator supplying analog power (2.6V to 3.3V).
+
+ vddio-supply:
+ description: regulator supplying IO power (1.8V).
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - touchscreen-size-x
+ - touchscreen-size-y
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@38 {
+ compatible = "focaltech,fts5452";
+ reg = <0x38>;
+
+ interrupts-extended = <&tlmm 125 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
+
+ avdd-supply = <&vreg_l28a_3p0>;
+ vddio-supply = <&vreg_l14a_1p88>;
+
+ pinctrl-0 = <&ts_int_active &ts_reset_active>;
+ pinctrl-1 = <&ts_int_suspend &ts_reset_suspend>;
+ pinctrl-names = "default", "suspend";
+
+ touchscreen-size-x = <1080>;
+ touchscreen-size-y = <2160>;
+ };
+ };
--
2.40.0
^ permalink raw reply related
* [PATCH v3 2/5] Input: add driver for Focaltech FTS touchscreen
From: Joel Selvaraj @ 2023-04-15 2:02 UTC (permalink / raw)
To: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Jeff LaBundy, Markuss Broks,
Jean Delvare, Max Krummenacher, Chris Morgan, Job Noorman,
Alistair Francis, Hans de Goede, Maxime Ripard
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel, Joel Selvaraj
In-Reply-To: <20230415020222.216232-1-joelselvaraj.oss@gmail.com>
The Focaltech FTS driver supports several variants of focaltech
touchscreens found in ~2018 era smartphones including variants found on
the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
This driver is loosely based on the original driver from Focaltech
but has been simplified and largely reworked.
Co-developed-by: Caleb Connolly <caleb@connolly.tech>
Signed-off-by: Caleb Connolly <caleb@connolly.tech>
Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
---
MAINTAINERS | 8 +
drivers/input/touchscreen/Kconfig | 12 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
4 files changed, 453 insertions(+)
create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7ec4ce64f66d..1a3ea61e1f52 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8028,6 +8028,14 @@ L: linux-input@vger.kernel.org
S: Maintained
F: drivers/input/joystick/fsia6b.c
+FOCALTECH FTS5452 TOUCHSCREEN DRIVER
+M: Joel Selvaraj <joelselvaraj.oss@gmail.com>
+M: Caleb Connolly <caleb@connolly.tech>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
+F: drivers/input/touchscreen/focaltech_fts5452.c
+
FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
M: Geoffrey D. Bennett <g@b4.vu>
L: alsa-devel@alsa-project.org (moderated for non-subscribers)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 1feecd7ed3cb..11af91504969 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
To compile this driver as a module, choose M here: the
module will be called exc3000.
+config TOUCHSCREEN_FOCALTECH_FTS5452
+ tristate "Focaltech FTS Touchscreen"
+ depends on I2C
+ help
+ Say Y here to enable support for I2C connected Focaltech FTS
+ based touch panels, including the 5452 and 8917 panels.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called focaltech_fts.
+
config TOUCHSCREEN_FUJITSU
tristate "Fujitsu serial touchscreen"
select SERIO
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 159cd5136fdb..47d78c9cff21 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO) += elo.o
obj-$(CONFIG_TOUCHSCREEN_EGALAX) += egalax_ts.o
obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
+obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS5452) += focaltech_fts5452.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
diff --git a/drivers/input/touchscreen/focaltech_fts5452.c b/drivers/input/touchscreen/focaltech_fts5452.c
new file mode 100644
index 000000000000..abf8a2f271ca
--- /dev/null
+++ b/drivers/input/touchscreen/focaltech_fts5452.c
@@ -0,0 +1,432 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * FocalTech touchscreen driver.
+ *
+ * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
+ * Copyright (C) 2018 XiaoMi, Inc.
+ * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
+ * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
+ */
+
+#include <linux/delay.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>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#define FTS_REG_CHIP_ID_H 0xA3
+#define FTS_REG_CHIP_ID_L 0x9F
+
+#define FTS_MAX_POINTS_SUPPORT 10
+#define FTS_ONE_TOUCH_LEN 6
+
+#define FTS_TOUCH_X_H_OFFSET 3
+#define FTS_TOUCH_X_L_OFFSET 4
+#define FTS_TOUCH_Y_H_OFFSET 5
+#define FTS_TOUCH_Y_L_OFFSET 6
+#define FTS_TOUCH_PRESSURE_OFFSET 7
+#define FTS_TOUCH_AREA_OFFSET 8
+#define FTS_TOUCH_TYPE_OFFSET 3
+#define FTS_TOUCH_ID_OFFSET 5
+
+#define FTS_TOUCH_DOWN 0
+#define FTS_TOUCH_UP 1
+#define FTS_TOUCH_CONTACT 2
+
+#define FTS_INTERVAL_READ_REG_MS 100
+#define FTS_TIMEOUT_READ_REG_MS 2000
+
+#define FTS_DRIVER_NAME "fts-i2c"
+
+static const u16 fts_chip_types[] = {
+ 0x5452,
+ 0x8719,
+};
+
+struct fts_ts_data {
+ struct i2c_client *client;
+ struct input_dev *input_dev;
+ struct regmap *regmap;
+ int irq;
+ struct regulator_bulk_data regulators[2];
+ u8 max_touch_points;
+ u8 *point_buf;
+ int point_buf_size;
+ struct gpio_desc *reset_gpio;
+ struct touchscreen_properties prop;
+};
+
+struct fts_i2c_chip_data {
+ int max_touch_points;
+};
+
+int fts_check_status(struct fts_ts_data *data)
+{
+ int error, i = 0, count = 0;
+ unsigned int val, id;
+
+ do {
+ error = regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
+ if (error)
+ dev_err(&data->client->dev, "I2C read failed: %d\n", error);
+
+ error = regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
+ if (error)
+ dev_err(&data->client->dev, "I2C read failed: %d\n", error);
+
+ id |= val << 8;
+
+ for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
+ if (id == fts_chip_types[i])
+ return 0;
+
+ count++;
+ msleep(FTS_INTERVAL_READ_REG_MS);
+ } while ((count * FTS_INTERVAL_READ_REG_MS) < FTS_TIMEOUT_READ_REG_MS);
+
+ return -EIO;
+}
+
+static int fts_report_touch(struct fts_ts_data *data)
+{
+ struct input_dev *input_dev = data->input_dev;
+ int base;
+ unsigned int x, y, z, maj;
+ u8 slot, type;
+ int error, i = 0;
+
+ u8 *buf = data->point_buf;
+
+ memset(buf, 0, data->point_buf_size);
+
+ error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
+ if (error) {
+ dev_err(&data->client->dev, "I2C read failed: %d\n", error);
+ return error;
+ }
+
+ for (i = 0; i < data->max_touch_points; i++) {
+ base = FTS_ONE_TOUCH_LEN * i;
+
+ slot = buf[base + FTS_TOUCH_ID_OFFSET] >> 4;
+ if (slot >= data->max_touch_points)
+ break;
+
+ x = ((buf[base + FTS_TOUCH_X_H_OFFSET] & 0x0F) << 8) +
+ (buf[base + FTS_TOUCH_X_L_OFFSET] & 0xFF);
+ y = ((buf[base + FTS_TOUCH_Y_H_OFFSET] & 0x0F) << 8) +
+ (buf[base + FTS_TOUCH_Y_L_OFFSET] & 0xFF);
+
+ z = buf[base + FTS_TOUCH_PRESSURE_OFFSET];
+ if (z == 0)
+ z = 0x3f;
+
+ maj = buf[base + FTS_TOUCH_AREA_OFFSET] >> 4;
+ if (maj == 0)
+ maj = 0x09;
+
+ type = buf[base + FTS_TOUCH_TYPE_OFFSET] >> 6;
+
+ input_mt_slot(input_dev, slot);
+ if (type == FTS_TOUCH_DOWN || type == FTS_TOUCH_CONTACT) {
+ input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, true);
+ touchscreen_report_pos(data->input_dev, &data->prop, x, y, true);
+ input_report_abs(input_dev, ABS_MT_PRESSURE, z);
+ input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, maj);
+ input_report_key(data->input_dev, BTN_TOUCH, 1);
+ } else {
+ input_report_key(data->input_dev, BTN_TOUCH, 0);
+ input_mt_report_slot_inactive(input_dev);
+ }
+ }
+ input_mt_sync_frame(input_dev);
+ input_sync(input_dev);
+
+ return 0;
+}
+
+static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
+{
+ struct fts_ts_data *data = dev_id;
+
+ return fts_report_touch(data) ? IRQ_NONE : IRQ_HANDLED;
+}
+
+static void fts_power_off(void *d)
+{
+ struct fts_ts_data *data = d;
+
+ regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
+}
+
+static int fts_start(struct fts_ts_data *data)
+{
+ int error;
+
+ error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
+ data->regulators);
+ if (error) {
+ dev_err(&data->client->dev, "failed to enable regulators\n");
+ return error;
+ }
+
+ gpiod_set_value_cansleep(data->reset_gpio, 0);
+ msleep(200);
+
+ enable_irq(data->irq);
+
+ return 0;
+}
+
+static int fts_stop(struct fts_ts_data *data)
+{
+ disable_irq(data->irq);
+ gpiod_set_value_cansleep(data->reset_gpio, 1);
+ fts_power_off(data);
+
+ return 0;
+}
+
+static int fts_input_open(struct input_dev *dev)
+{
+ struct fts_ts_data *data = input_get_drvdata(dev);
+ int error;
+
+ error = fts_start(data);
+ if (error)
+ return error;
+
+ error = fts_check_status(data);
+ if (error) {
+ dev_err(&data->client->dev, "Failed to start or unsupported chip");
+ return error;
+ }
+
+ return 0;
+}
+
+static void fts_input_close(struct input_dev *dev)
+{
+ struct fts_ts_data *data = input_get_drvdata(dev);
+
+ fts_stop(data);
+}
+
+static int fts_input_init(struct fts_ts_data *data)
+{
+ struct device *dev = &data->client->dev;
+ struct input_dev *input_dev;
+ int error = 0;
+
+ input_dev = devm_input_allocate_device(dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ data->input_dev = input_dev;
+
+ input_dev->name = FTS_DRIVER_NAME;
+ input_dev->id.bustype = BUS_I2C;
+ input_dev->dev.parent = dev;
+ input_dev->open = fts_input_open;
+ input_dev->close = fts_input_close;
+ input_set_drvdata(input_dev, data);
+
+ input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
+ input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
+ input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
+
+ touchscreen_parse_properties(input_dev, true, &data->prop);
+ if (!data->prop.max_x || !data->prop.max_y) {
+ dev_err(dev,
+ "touchscreen-size-x and/or touchscreen-size-y not set in device properties\n");
+ return -EINVAL;
+ }
+
+ error = input_mt_init_slots(input_dev, data->max_touch_points,
+ INPUT_MT_DIRECT);
+ if (error)
+ return error;
+
+ data->point_buf_size = (data->max_touch_points * FTS_ONE_TOUCH_LEN) + 3;
+ data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
+ if (!data->point_buf)
+ return -ENOMEM;
+
+ error = input_register_device(input_dev);
+ if (error) {
+ dev_err(dev, "Failed to register input device\n");
+ return error;
+ }
+
+ return 0;
+}
+
+static const struct regmap_config fts_ts_i2c_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+static int fts_ts_probe(struct i2c_client *client)
+{
+ const struct i2c_device_id *id = i2c_client_get_device_id(client);
+ const struct fts_i2c_chip_data *chip_data;
+ struct fts_ts_data *data;
+ int error = 0;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+ dev_err(&client->dev, "I2C not supported");
+ return -ENODEV;
+ }
+
+ if (!client->irq) {
+ dev_err(&client->dev, "No irq specified\n");
+ return -EINVAL;
+ }
+
+ data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ chip_data = device_get_match_data(&client->dev);
+ if (!chip_data)
+ chip_data = (const struct fts_i2c_chip_data *)id->driver_data;
+ if (!chip_data || !chip_data->max_touch_points) {
+ dev_err(&client->dev, "invalid or missing chip data\n");
+ return -EINVAL;
+ }
+ if (chip_data->max_touch_points > FTS_MAX_POINTS_SUPPORT) {
+ dev_err(&client->dev,
+ "invalid chip data, max_touch_points should be less than or equal to %d\n",
+ FTS_MAX_POINTS_SUPPORT);
+ return -EINVAL;
+ }
+ data->max_touch_points = chip_data->max_touch_points;
+
+ data->client = client;
+ i2c_set_clientdata(client, data);
+
+ data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(data->reset_gpio)) {
+ error = PTR_ERR(data->reset_gpio);
+ dev_err(&client->dev, "Failed to request reset gpio, error %d\n", error);
+ return error;
+ }
+
+ data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
+ if (IS_ERR(data->regmap)) {
+ error = PTR_ERR(data->regmap);
+ dev_err(&client->dev, "regmap allocation failed, error %d\n", error);
+ return error;
+ }
+
+ /*
+ * AVDD is the analog voltage supply (2.6V to 3.3V)
+ * VDDIO is the digital voltage supply (1.8V)
+ */
+ data->regulators[0].supply = "avdd";
+ data->regulators[1].supply = "vddio";
+ error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
+ data->regulators);
+ if (error) {
+ dev_err(&client->dev, "Failed to get regulators %d\n", error);
+ return error;
+ }
+
+ error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
+ if (error) {
+ dev_err(&client->dev, "failed to install power off handler\n");
+ return error;
+ }
+
+ error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+ fts_ts_interrupt, IRQF_ONESHOT,
+ client->name, data);
+ if (error) {
+ dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
+ return error;
+ }
+
+ error = fts_input_init(data);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static int fts_pm_suspend(struct device *dev)
+{
+ struct fts_ts_data *data = dev_get_drvdata(dev);
+
+ mutex_lock(&data->input_dev->mutex);
+
+ if (input_device_enabled(data->input_dev))
+ fts_stop(data);
+
+ mutex_unlock(&data->input_dev->mutex);
+
+ return 0;
+}
+
+static int fts_pm_resume(struct device *dev)
+{
+ struct fts_ts_data *data = dev_get_drvdata(dev);
+ int error = 0;
+
+ mutex_lock(&data->input_dev->mutex);
+
+ if (input_device_enabled(data->input_dev))
+ error = fts_start(data);
+
+ mutex_unlock(&data->input_dev->mutex);
+
+ return error;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(fts_dev_pm_ops, fts_pm_suspend, fts_pm_resume);
+
+static const struct fts_i2c_chip_data fts5452_chip_data = {
+ .max_touch_points = 5,
+};
+
+static const struct fts_i2c_chip_data fts8719_chip_data = {
+ .max_touch_points = 10,
+};
+
+static const struct i2c_device_id fts_i2c_id[] = {
+ { .name = "fts5452", .driver_data = (long)&fts5452_chip_data },
+ { .name = "fts8719", .driver_data = (long)&fts8719_chip_data },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, fts_i2c_id);
+
+static const struct of_device_id fts_of_match[] = {
+ { .compatible = "focaltech,fts5452", .data = &fts5452_chip_data },
+ { .compatible = "focaltech,fts8719", .data = &fts8719_chip_data },
+ { /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, fts_of_match);
+
+static struct i2c_driver fts_ts_driver = {
+ .probe_new = fts_ts_probe,
+ .id_table = fts_i2c_id,
+ .driver = {
+ .name = FTS_DRIVER_NAME,
+ .pm = pm_sleep_ptr(&fts_dev_pm_ops),
+ .of_match_table = fts_of_match,
+ },
+};
+module_i2c_driver(fts_ts_driver);
+
+MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
+MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
+MODULE_DESCRIPTION("Focaltech Touchscreen Driver");
+MODULE_LICENSE("GPL");
--
2.40.0
^ permalink raw reply related
* [PATCH v3 3/5] arm64: dts: qcom: sdm845-xiaomi-beryllium-common: add touchscreen related nodes
From: Joel Selvaraj @ 2023-04-15 2:02 UTC (permalink / raw)
To: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Jeff LaBundy, Markuss Broks,
Jean Delvare, Max Krummenacher, Chris Morgan, Job Noorman,
Alistair Francis, Hans de Goede, Maxime Ripard
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel, Joel Selvaraj
In-Reply-To: <20230415020222.216232-1-joelselvaraj.oss@gmail.com>
Enable qupv3_id_1 and gpi_dma1 as they are required for configuring
touchscreen. Also add pinctrl configurations needed for touchscreen.
These are common for both the tianma and ebbg touchscreen variant.
In the subsequent patch, we will initially enable support for the focaltech
touchscreen used in the EBBG variant. This is done in preparation for that.
Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
---
.../qcom/sdm845-xiaomi-beryllium-common.dtsi | 37 +++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
index 5ed975cc6ecb..b34ba46080ce 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
@@ -268,6 +268,10 @@ &gmu {
status = "okay";
};
+&gpi_dma1 {
+ status = "okay";
+};
+
&gpu {
status = "okay";
@@ -376,6 +380,10 @@ &qupv3_id_0 {
status = "okay";
};
+&qupv3_id_1 {
+ status = "okay";
+};
+
&sdhc_2 {
status = "okay";
@@ -481,6 +489,35 @@ sdc2_card_det_n: sd-card-det-n-state {
function = "gpio";
bias-pull-up;
};
+
+ ts_int_default: ts-int-default-state {
+ pins = "gpio31";
+ function = "gpio";
+ drive-strength = <16>;
+ bias-pull-down;
+ };
+
+ ts_reset_default: ts-reset-default-state {
+ pins = "gpio32";
+ function = "gpio";
+ drive-strength = <16>;
+ output-high;
+ };
+
+ ts_int_sleep: ts-int-sleep-state {
+ pins = "gpio31";
+ function = "gpio";
+ drive-strength = <2>;
+ bias-pull-down;
+ };
+
+ ts_reset_sleep: ts-reset-sleep-state {
+ pins = "gpio32";
+ function = "gpio";
+ drive-strength = <2>;
+ bias-disable;
+ output-low;
+ };
};
&uart6 {
--
2.40.0
^ permalink raw reply related
* [PATCH v3 4/5] arm64: dts: qcom: sdm845-xiaomi-beryllium-ebbg: introduce support for fts touchscreen
From: Joel Selvaraj @ 2023-04-15 2:02 UTC (permalink / raw)
To: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Jeff LaBundy, Markuss Broks,
Jean Delvare, Max Krummenacher, Chris Morgan, Job Noorman,
Alistair Francis, Hans de Goede, Maxime Ripard
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel, Joel Selvaraj
In-Reply-To: <20230415020222.216232-1-joelselvaraj.oss@gmail.com>
The Poco F1 EBBG variant uses Focaltech FTS touchscreen. Introduce
support for it.
Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
---
.../dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts | 21 +++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
index 76931ebad065..26e77979cdab 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
@@ -13,3 +13,24 @@ &display_panel {
compatible = "ebbg,ft8719";
status = "okay";
};
+
+&i2c14 {
+ status = "okay";
+
+ touchscreen@38 {
+ compatible = "focaltech,fts8719";
+ reg = <0x38>;
+
+ interrupts-extended = <&tlmm 31 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
+
+ vddio-supply = <&vreg_l14a_1p8>;
+
+ pinctrl-0 = <&ts_int_default &ts_reset_default>;
+ pinctrl-1 = <&ts_int_sleep &ts_reset_sleep>;
+ pinctrl-names = "default", "sleep";
+
+ touchscreen-size-x = <1080>;
+ touchscreen-size-y = <2246>;
+ };
+};
--
2.40.0
^ permalink raw reply related
* [PATCH v3 5/5] arm64: dts: qcom: sdm845-shift-axolotl: update focaltech touchscreen properties
From: Joel Selvaraj @ 2023-04-15 2:02 UTC (permalink / raw)
To: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Jeff LaBundy, Markuss Broks,
Jean Delvare, Max Krummenacher, Chris Morgan, Job Noorman,
Alistair Francis, Hans de Goede, Maxime Ripard
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel, Joel Selvaraj
In-Reply-To: <20230415020222.216232-1-joelselvaraj.oss@gmail.com>
The touchscreen nodes were added before the driver patches were merged.
Update the focaltech touchscreen properties to match with the upstreamed
focaltech driver. Also, the touchscreen used is in axolotl is fts5452
and not fts8719.
Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
---
.../boot/dts/qcom/sdm845-shift-axolotl.dts | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
index b54e304abf71..70286e53e000 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
@@ -474,23 +474,21 @@ &i2c5 {
status = "okay";
touchscreen@38 {
- compatible = "focaltech,fts8719";
+ compatible = "focaltech,fts5452";
reg = <0x38>;
- wakeup-source;
- interrupt-parent = <&tlmm>;
- interrupts = <125 0x2>;
- vdd-supply = <&vreg_l28a_3p0>;
- vcc-i2c-supply = <&vreg_l14a_1p88>;
- pinctrl-names = "default", "suspend";
+ interrupts-extended = <&tlmm 125 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
+
+ avdd-supply = <&vreg_l28a_3p0>;
+ vddio-supply = <&vreg_l14a_1p88>;
+
pinctrl-0 = <&ts_int_active &ts_reset_active>;
pinctrl-1 = <&ts_int_suspend &ts_reset_suspend>;
+ pinctrl-names = "default", "suspend";
- reset-gpio = <&tlmm 99 GPIO_ACTIVE_HIGH>;
- irq-gpio = <&tlmm 125 GPIO_TRANSITORY>;
touchscreen-size-x = <1080>;
touchscreen-size-y = <2160>;
- focaltech,max-touch-number = <5>;
};
};
--
2.40.0
^ permalink raw reply related
* Re: [PATCH 1/2] dt-bindings: input: microchip,cap11xx: add advanced sensitivity settings
From: Krzysztof Kozlowski @ 2023-04-15 9:10 UTC (permalink / raw)
To: Jiri Valek - 2N, dmitry.torokhov, krzysztof.kozlowski+dt
Cc: devicetree, linux-input, linux-kernel, robh+dt, u.kleine-koenig
In-Reply-To: <20230414233815.4004526-2-jiriv@axis.com>
On 15/04/2023 01:38, Jiri Valek - 2N wrote:
> Add support for advanced sensitivity settings and signal guard feature.
>
> Signed-off-by: Jiri Valek - 2N <jiriv@axis.com>
> ---
> .../bindings/input/microchip,cap11xx.yaml | 64 ++++++++++++++++++-
> 1 file changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
> index 5fa625b5c5fb..08e28226a164 100644
> --- a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
> +++ b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
> @@ -45,13 +45,13 @@ properties:
> Enables the Linux input system's autorepeat feature on the input device.
>
> linux,keycodes:
> - minItems: 6
> - maxItems: 6
> + minItems: 3
> + maxItems: 8
> description: |
> Specifies an array of numeric keycode values to
> be used for the channels. If this property is
> omitted, KEY_A, KEY_B, etc are used as defaults.
> - The array must have exactly six entries.
> + The number of entries must correspond to the number of channels.
>
> microchip,sensor-gain:
> $ref: /schemas/types.yaml#/definitions/uint32
> @@ -70,6 +70,58 @@ properties:
> open drain. This property allows using the active
> high push-pull output.
>
> + microchip,sensitivity-delta-sense:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 32
> + enum: [1, 2, 4, 8, 16, 32, 64, 128]
> + description: |
Do not need '|' unless you need to preserve formatting.
> + Optional parameter. Controls the sensitivity multiplier of a touch detection.
> + At the more sensitive settings, touches are detected for a smaller delta
> + capacitance corresponding to a “lighter” touch.
> +
> + microchip,sensitivity-base-shift:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + default: 256
> + enum: [1, 2, 4, 8, 16, 32, 64, 128, 256]
> + description: |
> + Optional parameter. Controls data scaling factor.
> + The higher the value of these bits, the larger the range and the lower
> + the resolution of the data presented. These settings will not affect
> + touch detection or sensitivity.
> +
> + microchip,signal-guard:
> + minItems: 3
> + maxItems: 8
> + enum: [0, 1]
> + default: 0
This was not really tested. Missing ref, mixing scalar and array
properties. You want items with enum. And drop default.
> + description: |
> + Optional parameter supported only for CAP129x.
Then disallow it for others (allOf:if:then: ...
microchip,signal-guard:false)
> + The signal guard isolates the signal from virtual grounds.
> + If enabled then the behavior of the channel is changed to signal guard.
> + The number of entries must correspond to the number of channels.
> +
> + microchip,input-treshold:
> + minItems: 3
> + maxItems: 8
> + minimum: 0
> + maximum: 127
> + default: 64
> + description: |
> + Optional parameter. Specifies the delta threshold that is used to
> + determine if a touch has been detected.
> + The number of entries must correspond to the number of channels.
> +
> + microchip,calib-sensitivity:
> + minItems: 3
> + maxItems: 8
> + enum: [1, 2, 4]
> + default: 1
> + description: |
> + Optional parameter supported only for CAP129x. Specifies an array of
> + numeric values that controls the gain used by the calibration routine to
> + enable sensor inputs to be more sensitive for proximity detection.
> + The number of entries must correspond to the number of channels.
Most of these properties do not look like hardware properties. Policies
and runtime configuration should not be put into DT. Explain please why
these are board-specific thus suitable for DT.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 2/5] Input: add driver for Focaltech FTS touchscreen
From: Christophe JAILLET @ 2023-04-15 9:55 UTC (permalink / raw)
To: joelselvaraj.oss
Cc: agross, alistair, andersson, arnd, caleb, devicetree,
dmitry.torokhov, hdegoede, jdelvare, jeff, job, konrad.dybcio,
krzysztof.kozlowski+dt, linux-arm-msm, linux-input, linux-kernel,
macromorgan, markuss.broks, max.krummenacher, mripard,
phone-devel, robert.jarzmik, robh+dt, rydberg,
~postmarketos/upstreaming
In-Reply-To: <20230415020222.216232-3-joelselvaraj.oss@gmail.com>
Le 15/04/2023 à 04:02, Joel Selvaraj a écrit :
> The Focaltech FTS driver supports several variants of focaltech
> touchscreens found in ~2018 era smartphones including variants found on
> the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
> This driver is loosely based on the original driver from Focaltech
> but has been simplified and largely reworked.
>
> Co-developed-by: Caleb Connolly <caleb-u60PMpPBjd35c1cvEZuMuQ@public.gmane.org>
> Signed-off-by: Caleb Connolly <caleb-u60PMpPBjd35c1cvEZuMuQ@public.gmane.org>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> MAINTAINERS | 8 +
> drivers/input/touchscreen/Kconfig | 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
> 4 files changed, 453 insertions(+)
> create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7ec4ce64f66d..1a3ea61e1f52 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8028,6 +8028,14 @@ L: linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> S: Maintained
> F: drivers/input/joystick/fsia6b.c
>
> +FOCALTECH FTS5452 TOUCHSCREEN DRIVER
> +M: Joel Selvaraj <joelselvaraj.oss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> +M: Caleb Connolly <caleb-u60PMpPBjd35c1cvEZuMuQ@public.gmane.org>
> +L: linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
> +F: drivers/input/touchscreen/focaltech_fts5452.c
> +
> FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
> M: Geoffrey D. Bennett <g@b4.vu>
> L: alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org (moderated for non-subscribers)
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 1feecd7ed3cb..11af91504969 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
> To compile this driver as a module, choose M here: the
> module will be called exc3000.
>
> +config TOUCHSCREEN_FOCALTECH_FTS5452
> + tristate "Focaltech FTS Touchscreen"
> + depends on I2C
> + help
> + Say Y here to enable support for I2C connected Focaltech FTS
> + based touch panels, including the 5452 and 8917 panels.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called focaltech_fts.
focaltech_fts5452?
Or should modifications be done elsewhere so that is does not look too
5452 specific?
> +
> config TOUCHSCREEN_FUJITSU
> tristate "Fujitsu serial touchscreen"
> select SERIO
[...]
> +struct fts_i2c_chip_data {
> + int max_touch_points;
> +};
> +
> +int fts_check_status(struct fts_ts_data *data)
> +{
> + int error, i = 0, count = 0;
No need to init "i".
> + unsigned int val, id;
> +
> + do {
> + error = regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
> + if (error)
> + dev_err(&data->client->dev, "I2C read failed: %d\n", error);
> +
> + error = regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
> + if (error)
> + dev_err(&data->client->dev, "I2C read failed: %d\n", error);
> +
> + id |= val << 8;
> +
> + for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
> + if (id == fts_chip_types[i])
> + return 0;
> +
> + count++;
> + msleep(FTS_INTERVAL_READ_REG_MS);
> + } while ((count * FTS_INTERVAL_READ_REG_MS) < FTS_TIMEOUT_READ_REG_MS);
> +
> + return -EIO;
> +}
> +
> +static int fts_report_touch(struct fts_ts_data *data)
> +{
> + struct input_dev *input_dev = data->input_dev;
> + int base;
> + unsigned int x, y, z, maj;
> + u8 slot, type;
> + int error, i = 0;
No need to init "i".
> +
> + u8 *buf = data->point_buf;
> +
> + memset(buf, 0, data->point_buf_size);
> +
> + error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
> + if (error) {
> + dev_err(&data->client->dev, "I2C read failed: %d\n", error);
> + return error;
> + }
> +
> + for (i = 0; i < data->max_touch_points; i++) {
> + base = FTS_ONE_TOUCH_LEN * i;
[...]
> +static int fts_ts_probe(struct i2c_client *client)
> +{
> + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> + const struct fts_i2c_chip_data *chip_data;
> + struct fts_ts_data *data;
> + int error = 0;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(&client->dev, "I2C not supported");
> + return -ENODEV;
> + }
> +
> + if (!client->irq) {
> + dev_err(&client->dev, "No irq specified\n");
> + return -EINVAL;
> + }
> +
> + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + chip_data = device_get_match_data(&client->dev);
> + if (!chip_data)
> + chip_data = (const struct fts_i2c_chip_data *)id->driver_data;
> + if (!chip_data || !chip_data->max_touch_points) {
> + dev_err(&client->dev, "invalid or missing chip data\n");
> + return -EINVAL;
> + }
> + if (chip_data->max_touch_points > FTS_MAX_POINTS_SUPPORT) {
> + dev_err(&client->dev,
> + "invalid chip data, max_touch_points should be less than or equal to %d\n",
> + FTS_MAX_POINTS_SUPPORT);
> + return -EINVAL;
> + }
> + data->max_touch_points = chip_data->max_touch_points;
> +
> + data->client = client;
> + i2c_set_clientdata(client, data);
> +
> + data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(data->reset_gpio)) {
> + error = PTR_ERR(data->reset_gpio);
> + dev_err(&client->dev, "Failed to request reset gpio, error %d\n", error);
> + return error;
> + }
> +
> + data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
> + if (IS_ERR(data->regmap)) {
> + error = PTR_ERR(data->regmap);
> + dev_err(&client->dev, "regmap allocation failed, error %d\n", error);
> + return error;
> + }
> +
> + /*
> + * AVDD is the analog voltage supply (2.6V to 3.3V)
> + * VDDIO is the digital voltage supply (1.8V)
> + */
> + data->regulators[0].supply = "avdd";
> + data->regulators[1].supply = "vddio";
> + error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
> + data->regulators);
> + if (error) {
> + dev_err(&client->dev, "Failed to get regulators %d\n", error);
> + return error;
> + }
> +
> + error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
> + if (error) {
> + dev_err(&client->dev, "failed to install power off handler\n");
> + return error;
> + }
Is it really needed?
This could lead to disable something that is not enabled. This looks
harmless, but I wonder if it can occur?
I don't know the pm and input_dev frameworks enough to figure it myself,
so this question is just about curiousity.
CJ
> +
> + error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> + fts_ts_interrupt, IRQF_ONESHOT,
> + client->name, data);
> + if (error) {
> + dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
> + return error;
> + }
> +
> + error = fts_input_init(data);
> + if (error)
> + return error;
> +
> + return 0;
> +}
[...]
^ permalink raw reply
* Re: [PATCH v3 3/5] arm64: dts: qcom: sdm845-xiaomi-beryllium-common: add touchscreen related nodes
From: Konrad Dybcio @ 2023-04-15 10:41 UTC (permalink / raw)
To: Joel Selvaraj, Caleb Connolly, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Jeff LaBundy, Markuss Broks,
Jean Delvare, Max Krummenacher, Chris Morgan, Job Noorman,
Alistair Francis, Hans de Goede, Maxime Ripard
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230415020222.216232-4-joelselvaraj.oss@gmail.com>
On 15.04.2023 04:02, Joel Selvaraj wrote:
> Enable qupv3_id_1 and gpi_dma1 as they are required for configuring
> touchscreen. Also add pinctrl configurations needed for touchscreen.
> These are common for both the tianma and ebbg touchscreen variant.
> In the subsequent patch, we will initially enable support for the focaltech
> touchscreen used in the EBBG variant. This is done in preparation for that.
>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
Bit weird to add everything except the touchscreen, but okay..
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
> .../qcom/sdm845-xiaomi-beryllium-common.dtsi | 37 +++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
> index 5ed975cc6ecb..b34ba46080ce 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
> @@ -268,6 +268,10 @@ &gmu {
> status = "okay";
> };
>
> +&gpi_dma1 {
> + status = "okay";
> +};
> +
> &gpu {
> status = "okay";
>
> @@ -376,6 +380,10 @@ &qupv3_id_0 {
> status = "okay";
> };
>
> +&qupv3_id_1 {
> + status = "okay";
> +};
> +
> &sdhc_2 {
> status = "okay";
>
> @@ -481,6 +489,35 @@ sdc2_card_det_n: sd-card-det-n-state {
> function = "gpio";
> bias-pull-up;
> };
> +
> + ts_int_default: ts-int-default-state {
> + pins = "gpio31";
> + function = "gpio";
> + drive-strength = <16>;
> + bias-pull-down;
> + };
> +
> + ts_reset_default: ts-reset-default-state {
> + pins = "gpio32";
> + function = "gpio";
> + drive-strength = <16>;
> + output-high;
> + };
> +
> + ts_int_sleep: ts-int-sleep-state {
> + pins = "gpio31";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> +
> + ts_reset_sleep: ts-reset-sleep-state {
> + pins = "gpio32";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-disable;
> + output-low;
> + };
> };
>
> &uart6 {
^ permalink raw reply
* Re: [PATCH v3 4/5] arm64: dts: qcom: sdm845-xiaomi-beryllium-ebbg: introduce support for fts touchscreen
From: Konrad Dybcio @ 2023-04-15 10:41 UTC (permalink / raw)
To: Joel Selvaraj, Caleb Connolly, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Jeff LaBundy, Markuss Broks,
Jean Delvare, Max Krummenacher, Chris Morgan, Job Noorman,
Alistair Francis, Hans de Goede, Maxime Ripard
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230415020222.216232-5-joelselvaraj.oss@gmail.com>
On 15.04.2023 04:02, Joel Selvaraj wrote:
> The Poco F1 EBBG variant uses Focaltech FTS touchscreen. Introduce
> support for it.
>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
> .../dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts | 21 +++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> index 76931ebad065..26e77979cdab 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> @@ -13,3 +13,24 @@ &display_panel {
> compatible = "ebbg,ft8719";
> status = "okay";
> };
> +
> +&i2c14 {
> + status = "okay";
> +
> + touchscreen@38 {
> + compatible = "focaltech,fts8719";
> + reg = <0x38>;
> +
> + interrupts-extended = <&tlmm 31 IRQ_TYPE_EDGE_FALLING>;
> + reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
> +
> + vddio-supply = <&vreg_l14a_1p8>;
> +
> + pinctrl-0 = <&ts_int_default &ts_reset_default>;
> + pinctrl-1 = <&ts_int_sleep &ts_reset_sleep>;
> + pinctrl-names = "default", "sleep";
> +
> + touchscreen-size-x = <1080>;
> + touchscreen-size-y = <2246>;
> + };
> +};
^ permalink raw reply
* Re: [PATCH 1/2] Input: cs40l26: Support for CS40L26 Boosted Haptic Amplifier
From: Jeff LaBundy @ 2023-04-15 20:54 UTC (permalink / raw)
To: Fred Treven
Cc: Charles Keepax, dmitry.torokhov@gmail.com, Ben Bright,
James Ogletree, lee@kernel.org, jdelvare@suse.de, joel@jms.id.au,
cy_huang@richtek.com, rdunlap@infradead.org,
eajames@linux.ibm.com, ping.bai@nxp.com, msp@baylibre.com,
arnd@arndb.de, bartosz.golaszewski@linaro.org,
linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
patches@opensource.cirrus.com
In-Reply-To: <6ABC85BA-AF2D-4D2E-8CA8-32E372570DA0@cirrus.com>
Hi Fred,
On Fri, Apr 14, 2023 at 08:51:48PM +0000, Fred Treven wrote:
>
> >> +
> >> + return cs40l26_probe(cs40l26, pdata);
> >> +}
> >> +
> >> +static void cs40l26_i2c_remove(struct i2c_client *client)
> >> +{
> >> + struct cs40l26_private *cs40l26 = i2c_get_clientdata(client);
> >> +
> >> + cs40l26_remove(cs40l26);
> >> +}
> >> +
> >> +static struct i2c_driver cs40l26_i2c_driver = {
> >> + .driver = {
> >> + .name = "cs40l26",
> >> + .of_match_table = cs40l26_of_match,
> >> + .pm = &cs40l26_pm_ops,
> >
> > Please guard this with the new pm_sleep_ptr(), as not all platforms would
> > define CONFIG_PM. More comments in the core driver.
>
> Understood, and I certainly agree with this change. One thing I’m unsure of is what the effect would be on the driver if CONFIG_PM is not set in the .config. Surely, this driver would not work as expected right? Should I add a dependency in the kconfig to avoid building the driver without CONFIG_PM?
The PM core stubs out dummy functions for most, if not all of the API used here
for the #if !CONFIG_PM case. Please test to be sure, but it's likely the driver
still compiles without CONFIG_PM set.
However, you should evaluate the driver's functionality during this case. At a
minimum, the device should still be useable but simply not hibernate.
> >
> >> +{
> >> + size_t len_words = len_bytes / sizeof(__be32);
> >> + struct cs_dsp_coeff_ctl *ctl;
> >> + __be32 *val;
> >> + int i, ret;
> >> +
> >> + ctl = cs_dsp_get_ctl(dsp, name, WMFW_ADSP2_XM, algo_id);
> >> + if (IS_ERR_OR_NULL(ctl)) {
> >> + dev_err(dsp->dev, "Failed to find fw ctl %s\n", name);
> >> + return -ENOENT;
> >> + }
> >> +
> >> + val = kzalloc(len_bytes, GFP_KERNEL);
> >> + if (!val)
> >> + return -ENOMEM;
> >> +
> >> + for (i = 0; i < len_words; i++)
> >> + val[i] = cpu_to_be32(buf[i]);
> >> +
> >> + ret = cs_dsp_coeff_write_ctrl(ctl, off_words, val, len_bytes);
> >> + if (ret)
> >> + dev_err(dsp->dev, "Failed to write fw ctl %s: %d\n", name, ret);
> >> +
> >> + kfree(val);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static inline int cs40l26_fw_ctl_write(struct cs_dsp *dsp, const char * const name,
> >> + unsigned int algo_id, u32 val)
> >> +{
> >> + return cs40l26_fw_ctl_write_raw(dsp, name, algo_id, 0, sizeof(u32), &val);
> >> +}
> >> +
> >> +static int cs40l26_fw_ctl_read_raw(struct cs_dsp *dsp, const char * const name,
> >> + unsigned int algo_id, unsigned int off_words, size_t len_bytes, u32 *buf)
> >> +{
> >> + size_t len_words = len_bytes / sizeof(u32);
> >> + struct cs_dsp_coeff_ctl *ctl;
> >> + int i, ret;
> >> +
> >> + ctl = cs_dsp_get_ctl(dsp, name, WMFW_ADSP2_XM, algo_id);
> >> + if (IS_ERR_OR_NULL(ctl)) {
> >> + dev_err(dsp->dev, "Failed to find fw ctl %s\n", name);
> >> + return -ENOENT;
> >> + }
> >> +
> >> + ret = cs_dsp_coeff_read_ctrl(ctl, off_words, buf, len_bytes);
> >> + if (ret) {
> >> + dev_err(dsp->dev, "Failed to read fw ctl %s: %d\n", name, ret);
> >> + return ret;
> >> + }
> >> +
> >> + for (i = 0; i < len_words; i++)
> >> + buf[i] = be32_to_cpu(buf[i]);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static inline int cs40l26_fw_ctl_read(struct cs_dsp *dsp, const char * const name,
> >> + unsigned int algo_id, u32 *buf)
> >> +{
> >> + return cs40l26_fw_ctl_read_raw(dsp, name, algo_id, 0, sizeof(u32), buf);
> >> +}
> >
> > None of these four functions seem particularly specific to L26; is there any
> > reason they don't belong in cs_dsp or wm_adsp? In fact, some of the functions
> > throughout those drivers seem to be doing similar work.
> >
> > Maybe out of scope for this particular submission, but is there not any room
> > for re-use here?
> >
> I wanted to avoid making too many changes to the firmware drivers in this initial submission, and I think I’d like to keep it as-is for now, but I think moving this combination to cs_dsp is the right move. Let me know if you feel that it would be better to make the change now rather than later.
I think it's fine to clear this submission first, then look for ways to
optimize later.
>
> > On Tue, Apr 11, 2023 at 09:27:08AM +0000, Charles Keepax wrote:
> >> On Mon, Apr 10, 2023 at 07:31:56PM -0500, Jeff LaBundy wrote:
> >>> On Mon, Apr 10, 2023 at 08:56:34AM +0000, Charles Keepax wrote:
> >>>> On Sat, Apr 08, 2023 at 10:44:39PM -0500, Jeff LaBundy wrote:
> >>>> I would far rather not have every single attempt to communicate
> >>>> with the device wrapped in a retry if the communication failed
> >>>> incase the device is hibernating. It seems much cleaner, and less
> >>>> likely to risk odd behaviour, to know we have brought the device
> >>>> out of hibernation.
> >>
> >>> A common way to deal with this is that of [1], where the bus calls
> >>> are simply wrapped with all retry logic limited to two places (read
> >>> and write). These functions could also print the register address
> >>> in case of failure, solving the problem of having dozens of custom
> >>> error messages thorughout the driver.
> >>
> >> I suspect this really comes down to a matter of taste, but my
> >> thoughts would be that the code is shorter that way, but not
> >> necessarily simpler. This feels far more error prone and likely
> >> to encounter issues where the device hibernates at a time someone
> >> hadn't properly thought through. I am far more comfortable with
> >> the device is blocked from hibernating whilst the driver is
> >> actively engaged with it and it keeps any special handling for
> >> exiting hibernate in one place.
> >
> > Fair enough. I do concede that having this control in the driver as
> > opposed to DSP FW is more nimble and makes it easier to respond to
> > customer issues; I'm sure your battle scars will agree :)
>
> I concur with Charles here, and it seems like you’re also ok with this so I will leave it as-is.
I'm OK with it; it's simply not my first choice. I'm in the "less code is
the best code" camp, so I'm biased toward giving the silicon the benefit
of the doubt and letting it autosuspend on its own. This is how bare-metal
platforms are likely to use the device, and probably how it was envisioned
to be used. And if there truly is an issue with hibernation mode, it seems
you would eventually stumble upon it whether the kernel or the DSP FW is
managing the device's state. That's just my $.02.
That being said, there is nothing functionally incorrect about the current
implementation and since you are the one who is ultimately responsible for
supporting the driver, I think you should stick with what makes you most
comfortable and agile. As I mention, it can be to your advantage to have
complete explicit control over the device from the kernel.
Please do add some comments to describe how the device's state is managed
however, particularly with respect to GPIO triggers. I was also confused
as to why the kernel autosuspend timeout (2000 ms) is greater than the
device's standby-to-hibernation timeout (100 ms) because I naively assumed
the opposite would be true.
>
>
> >>> +static int cs40l26_irq_update_mask(struct cs40l26_private *cs40l26, u32 reg, u32 val, u32 bit_mask)
> >>> +{
> >>> + u32 eint_reg, cur_mask, new_mask;
> >>> + int ret;
> >>> +
> >>> + if (reg == CS40L26_IRQ1_MASK_1) {
> >>> + eint_reg = CS40L26_IRQ1_EINT_1;
> >>> + } else if (reg == CS40L26_IRQ1_MASK_2) {
> >>> + eint_reg = CS40L26_IRQ1_EINT_2;
> >>> + } else {
> >>> + dev_err(cs40l26->dev, "Invalid IRQ mask reg: 0x%08X\n", reg);
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + ret = regmap_read(cs40l26->regmap, reg, &cur_mask);
> >>> + if (ret) {
> >>> + dev_err(cs40l26->dev, "Failed to get IRQ mask\n");
> >>
> >> Having a custom error message for every possible failed register read
> >> does not ultimately aid in debugging and unnecessarily grows the size
> >> of the driver.
> >>
> >> If a specific message is absolutely necessary, then add wrappers around
> >> regmap_read/write and print the failed address. However, this does not
> >> seem necessary either. Simply propagate the error code all the way up
> >> to the caller, whether it is probe or a sysfs attribute.
> >>
> >> Stated another way:
> >>
> >> error = regmap_...(...);
> >> if (error)
> >> return error;
> >>
> >
> > Not sure I feel super strongly on this one, but I do find when
> > debugging issues on drivers that do this that I usually end up
> > adding the printks back in.
>
> I went ahead and implemented the change Jeff suggested here; it does streamline the driver to a good degree, and I think it’s worth making the adjustment.
>
> >>> +static int cs40l26_dsp_state_get(struct cs40l26_private *cs40l26, u8 *state)
> >>> +{
> >>> + bool mutex_available = !mutex_is_locked(&cs40l26->dsp.pwr_lock);
> >>
> >> This is dangerous and a sign that locks are not properly managed. What would
> >> be a case where you do not know the state of the lock upon entering this function?
> >> If you do not know whether the mutex is locked inside this function, it is not the
> >> proper place to grab it.
> >>
> >
> > Yes I whole heartedly agree here this should not be done this
> > way.
>
> I certainly understand the concern here, but I wanted to provide some context. Since cs40l26_dsp_state_get() is called both in the cs_dsp_pre_run() callback as well as outside of this, there are instances where pwr_lock needs to be grabbed and times when it is already taken (in the instance of the callback invocation). Because of this, attempting to take the lock during pre_run causes a deadlock. I felt that it was quite clear when the lock would be available and when it wouldn’t be and to avoid the deadlock, I checked whether or not the lock was available. What do y’all feel is the best way to handle this? Some separate variable to determine if we are in the pre_run sequence or not? A separate function for dsp_state_get() for when it is not invoked from the callback?
This is essentially my point. As a rule of thumb, if it takes this much
explanation to convey the design intent, it's probably unsafe and ripe
for bugs as others innocently contribute to this driver.
A second rule of thumb is that for proper encapsulation, the state of
the outside world should be the same regardless of how this function is
entered. Your last idea is the proper solution, for example:
static int __cs40l26_dsp_state_get(...)
{
/* Mutex is assumed to be locked here. */
[...]
return error;
}
static int cs40l26_dsp_state_get(...)
{
mutex_lock(...);
error = __cs40l26_dsp_state_get(...);
mutex_unlock(...);
return error;
}
If you need to get the DSP state in some other function where the lock is
held, just call __cs40l26_dsp_state_get(). Another advantage to using a
"helper" function is that the mutex is easily unlocked in both passing or
failing cases without complex conditional blocks or goto statements.
>
> >
> >> +
> >> + cs40l26_pm_runtime_teardown(cs40l26);
> >> +
> >> + if (cs40l26->dsp.running)
> >> + cs_dsp_stop(&cs40l26->dsp);
> >> + if (cs40l26->dsp.booted)
> >> + cs_dsp_power_down(&cs40l26->dsp);
> >> + if (&cs40l26->dsp)
> >> + cs_dsp_remove(&cs40l26->dsp);
> >> +
> >> + if (cs40l26->vibe_workqueue) {
> >> + cancel_work_sync(&cs40l26->erase_work);
> >> + cancel_work_sync(&cs40l26->set_gain_work);
> >> + cancel_work_sync(&cs40l26->upload_work);
> >> + cancel_work_sync(&cs40l26->vibe_start_work);
> >> + cancel_work_sync(&cs40l26->vibe_stop_work);
> >> + destroy_workqueue(cs40l26->vibe_workqueue);
> >> + }
> >> +
> >> + mutex_destroy(&cs40l26->lock);
> >
> > This ultimately does nothing.
>
> Could you please clarify a bit what you mean here? Does the mutex not need to be destroyed explicitly? What about the work queue? Is it canceled/destroyed automatically when the module is removed? Are the cs_dsp functions not necessary to gracefully stop the DSP etc.?
Sorry, I meant that mutex_destroy() is unnecessary; it's an empty function
unless mutex debugging is enabled. There is no need to take any action on
the mutex as the driver is torn down.
You do need to cancel any pending work to prevent use-after-free conditions;
the core does not do this for you. You don't necessarily need to destroy the
work queue, however, because it seems all of the means to queue work will be
gone by the time the device-managed cs40l26 pointer is freed.
>
> In v2 of this patch, I plan to implement all of the suggestions in your comments that I didn’t question or disagree with here. Thanks for your in-depth analysis.
>
> Best regards,
> Fred
>
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH] hid: hid-core: Adjust indentation in hid_add_device
From: Junyan Ye @ 2023-04-16 7:38 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, David Herrmann
Cc: hust-os-kernel-patches, Junyan Ye, Dongliang Mu, Jiri Kosina,
linux-input, linux-kernel
Smatch reported:
drivers/hid/hid-core.c:2750 hid_add_device() warn: inconsistent indenting
There is an extra space before the if statement and its braces.
Fix this warning by removing the whitespace to conform to the coding style.
Fixes: 3c86726cfe38 ("HID: make .raw_request mandatory")
Signed-off-by: Junyan Ye <yejunyan@hust.edu.cn>
Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
---
drivers/hid/hid-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 22623eb4f72f..80fc2eed6383 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2747,10 +2747,10 @@ int hid_add_device(struct hid_device *hdev)
/*
* Check for the mandatory transport channel.
*/
- if (!hdev->ll_driver->raw_request) {
+ if (!hdev->ll_driver->raw_request) {
hid_err(hdev, "transport driver missing .raw_request()\n");
return -EINVAL;
- }
+ }
/*
* Read the device report descriptor once and use as template
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v3 0/5] Add support for Focaltech FTS Touchscreen
From: Krzysztof Kozlowski @ 2023-04-16 8:41 UTC (permalink / raw)
To: Joel Selvaraj, Caleb Connolly, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Henrik Rydberg, Arnd Bergmann, Robert Jarzmik, Jeff LaBundy,
Markuss Broks, Jean Delvare, Max Krummenacher, Chris Morgan,
Job Noorman, Alistair Francis, Hans de Goede, Maxime Ripard
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230415020222.216232-1-joelselvaraj.oss@gmail.com>
On 15/04/2023 04:02, Joel Selvaraj wrote:
> Changes in v3:(Suggested by Krzysztof Kozlowski and Konrad Dybcio)
> --------------
> - dts: removed the invalid "input-enable" property
> - dts: replace interrupts with interrupts-extended
> - dts: removed redundant dma configuration
> - dts: reorder pinctrl and pinctrl-names
> - bindings: moved unevaluatedProperties after required
> - bindings: make interrupts a required property (new change from my end)
> - bindings: update example based on dts changes
>
> I have made the interrupts a required property in the bindings as the driver
> will not function without an interrupt. Because of this new change, I have not
Driver does not really matter for the bindings. The BSD driver for
example might function without an interrupt, so why requiring it? The
reason for requiring or not is in the hardware and how it works.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 1/5] dt-bindings: input: touchscreen: add focaltech,fts5452 touchscreen
From: Krzysztof Kozlowski @ 2023-04-16 8:42 UTC (permalink / raw)
To: Joel Selvaraj, Caleb Connolly, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Henrik Rydberg, Arnd Bergmann, Robert Jarzmik, Jeff LaBundy,
Markuss Broks, Jean Delvare, Max Krummenacher, Chris Morgan,
Job Noorman, Alistair Francis, Hans de Goede, Maxime Ripard
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230415020222.216232-2-joelselvaraj.oss@gmail.com>
On 15/04/2023 04:02, Joel Selvaraj wrote:
> Document the Focaltech FTS touchscreen driver.
>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> ---
> .../input/touchscreen/focaltech,fts5452.yaml | 71 +++++++++++++++++++
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* Servicio de la flota
From: Miguel Garcia @ 2023-04-17 7:55 UTC (permalink / raw)
To: linux-input
Buenos días:
Le escribo para hablarle sobre una de las mejores herramientas GPS en el mercado.
La herramienta, que me gustaría presentarle brevemente, dispone de muchas funciones útiles para su trabajo, que optimizan los procesos de transporte y le ayudan a realizar tareas de campo de manera más eficiente.
¿Quiere conocer los detalles?
Atentamente,
Miguel Garcia
^ permalink raw reply
* Re: [PATCH] Input: xpad - fix GPF in xpad_probe
From: Dan Carpenter @ 2023-04-17 9:25 UTC (permalink / raw)
To: Dongliang Mu, Vicki Pfau
Cc: Dmitry Torokhov, Pavel Rojtberg, Nate Yocom, Mattijs Korpershoek,
John Butler, Matthias Benkmann, Christopher Crockett,
Santosh De Massari, hust-os-kernel-patches,
syzbot+a3f758b8d8cb7e49afec, Pierre-Loup A. Griffais, linux-input,
linux-kernel
In-Reply-To: <20230414125603.686123-1-dzm91@hust.edu.cn>
On Fri, Apr 14, 2023 at 08:55:47PM +0800, Dongliang Mu wrote:
> In xpad_probe(), it does not allocate xpad->dev with input_dev type.
> Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
> would trigger GPF.
What is a call tree for this? Actually I found it from the bug report.
drivers/input/joystick/xpad.c
2034 if (error)
2035 dev_warn(&xpad->dev->dev,
2036 "unable to receive magic message: %d\n",
2037 error);
2038 }
>
> Fix this by allocating xpad->dev, its error handling and cleanup
> operations in the remove function.
>
> Note that this crash does not have any reproducer, so the patch
> only passes compilation testing.
The xpad->dev = input_dev; already happens in xpad_init_input(). We
shouldn't allocate it twice. I think the fix is to just use a different
device pointer for the dev_warn(). Why not use &xpad->intf->dev?
>
> Reported-by: syzbot+a3f758b8d8cb7e49afec@syzkaller.appspotmail.com
Could you use a Link tag to link to the bug report?
Link: https://groups.google.com/g/syzkaller-bugs/c/iMhTgpGuIbM
This needs a Fixes tag.
Fixes: db7220c48d8d ("Input: xpad - fix support for some third-party controllers")
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] Input: xpad - fix GPF in xpad_probe
From: Dan Carpenter @ 2023-04-17 10:42 UTC (permalink / raw)
To: Dongliang Mu, Vicki Pfau, kernel-janitors
Cc: Dmitry Torokhov, Pavel Rojtberg, Nate Yocom, Mattijs Korpershoek,
John Butler, Matthias Benkmann, Christopher Crockett,
Santosh De Massari, hust-os-kernel-patches,
syzbot+a3f758b8d8cb7e49afec, Pierre-Loup A. Griffais, linux-input,
linux-kernel
In-Reply-To: <c3e0823b-2b03-4dab-b7cb-a8bc5151f0b1@kili.mountain>
Btw, we should be thinking about how to detect these sorts of issues
using static analysis. Unfortunately, it's not as simple as saying
"We know this variable is NULL so don't dereference it." The problem
with that is that many times Smatch sees where a pointer is set to NULL
but not when it is assigned to a different value.
What we could do instead is say:
1) If a pointer is dereferenced and we know it is NULL then:
set_state_expr(my_id, expr, &suspicious);
2) If we set a pointer to non-NULL and it is marked as suspicious then
print a warning.
This would generate a warning for cases where we dereference a pointer
before it has been initialized.
It is not hard to write a Smatch check like this. The first draft
approach is only three functions long.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] Input: xpad - fix GPF in xpad_probe
From: Vicki Pfau @ 2023-04-17 11:07 UTC (permalink / raw)
To: Dongliang Mu, Dan Carpenter
Cc: Dmitry Torokhov, Pavel Rojtberg, Nate Yocom, Mattijs Korpershoek,
John Butler, Matthias Benkmann, Christopher Crockett,
Santosh De Massari, hust-os-kernel-patches,
syzbot+a3f758b8d8cb7e49afec, Pierre-Loup A. Griffais, linux-input,
linux-kernel
In-Reply-To: <57577302-8d18-231f-062b-b1d262720943@hust.edu.cn>
On 4/17/23 03:33, Dongliang Mu wrote:
>
> On 2023/4/17 18:24, Vicki Pfau wrote:
>> Hi,
>>
>> On 4/17/23 03:01, Dongliang Mu wrote:
>>> On 2023/4/17 17:25, Dan Carpenter wrote:
>>>> On Fri, Apr 14, 2023 at 08:55:47PM +0800, Dongliang Mu wrote:
>>>>> In xpad_probe(), it does not allocate xpad->dev with input_dev type.
>>>>> Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
>>>>> would trigger GPF.
>>>> What is a call tree for this? Actually I found it from the bug report.
>>>> drivers/input/joystick/xpad.c
>>>> 2034 if (error)
>>>> 2035 dev_warn(&xpad->dev->dev,
>>>> 2036 "unable to receive magic message: %d\n",
>>>> 2037 error);
>>>> 2038 }
>> Sorry, this appears to be my code, and was merged recently after a few drafts with Dmitry. This code is sensitive to being moved and only affects some controllers, so I'm looking into if I can move it into after creation of the input_dev right now. It's something I'd already thought might be necessary, but I didn't find any evidence for it before. I'll try to get back to you on that soon.
>
> If this is necessary, we can change it with another device pointer. Otherwise, we need to move it after the allocation and assignment. Or move the allocation and assignment before which is not suggested.
>
> Thanks for your reply. Do I need to submit a v2 patch? Or you will take care of it?
I'll take care of it. I have a patch prepared, but I need to do a bit more testing before I can confirm it doesn't break one specific controller. I'll try to file it as soon as possible. Do you have a timeframe you need this by?
>
> Dongliang Mu
>
>>> Hi Dan,
>>>
>>> this only occurs in linux-next tree.
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/input/joystick/xpad.c?n2053#n2053
>>>
>>>>> Fix this by allocating xpad->dev, its error handling and cleanup
>>>>> operations in the remove function.
>>>>>
>>>>> Note that this crash does not have any reproducer, so the patch
>>>>> only passes compilation testing.
>>>> The xpad->dev = input_dev; already happens in xpad_init_input(). We
>>>> shouldn't allocate it twice. I think the fix is to just use a different
>>>> device pointer for the dev_warn(). Why not use &xpad->intf->dev?
>>> Yeah, the allocation and assignment is in the last part that I missed before. We have two choices to fix this issue:
>>>
>>> 1. Change to another device pointer;
>>>
>>> 2. Move the allocation and assignment to a previous code site;
>>>
>>> If there is no other places dereferencing this pointer before the allocation and assignment, it's better to use the 1st one.
>>>
>>> Let me craft a v2 patch.
>>>
>>>>> Reported-by: syzbot+a3f758b8d8cb7e49afec@syzkaller.appspotmail.com
>>>> Could you use a Link tag to link to the bug report?
>>>> Link: https://groups.google.com/g/syzkaller-bugs/c/iMhTgpGuIbM
>>> Sure, no problem.
>>>> This needs a Fixes tag.
>>>>
>>>> Fixes: db7220c48d8d ("Input: xpad - fix support for some third-party controllers")
>>>>
>>>> regards,
>>>> dan carpenter
>>>>
>> Vicki
Vicki
^ permalink raw reply
* Re: [PATCH] Input: xpad - fix GPF in xpad_probe
From: Dongliang Mu @ 2023-04-17 11:15 UTC (permalink / raw)
To: Vicki Pfau, Dan Carpenter
Cc: Dmitry Torokhov, Pavel Rojtberg, Nate Yocom, Mattijs Korpershoek,
John Butler, Matthias Benkmann, Christopher Crockett,
Santosh De Massari, hust-os-kernel-patches,
syzbot+a3f758b8d8cb7e49afec, Pierre-Loup A. Griffais, linux-input,
linux-kernel
In-Reply-To: <69fc3a73-f18b-0268-6431-1b8b6aeed8ff@endrift.com>
On 2023/4/17 19:07, Vicki Pfau wrote:
>
> On 4/17/23 03:33, Dongliang Mu wrote:
>> On 2023/4/17 18:24, Vicki Pfau wrote:
>>> Hi,
>>>
>>> On 4/17/23 03:01, Dongliang Mu wrote:
>>>> On 2023/4/17 17:25, Dan Carpenter wrote:
>>>>> On Fri, Apr 14, 2023 at 08:55:47PM +0800, Dongliang Mu wrote:
>>>>>> In xpad_probe(), it does not allocate xpad->dev with input_dev type.
>>>>>> Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
>>>>>> would trigger GPF.
>>>>> What is a call tree for this? Actually I found it from the bug report.
>>>>> drivers/input/joystick/xpad.c
>>>>> 2034 if (error)
>>>>> 2035 dev_warn(&xpad->dev->dev,
>>>>> 2036 "unable to receive magic message: %d\n",
>>>>> 2037 error);
>>>>> 2038 }
>>> Sorry, this appears to be my code, and was merged recently after a few drafts with Dmitry. This code is sensitive to being moved and only affects some controllers, so I'm looking into if I can move it into after creation of the input_dev right now. It's something I'd already thought might be necessary, but I didn't find any evidence for it before. I'll try to get back to you on that soon.
>> If this is necessary, we can change it with another device pointer. Otherwise, we need to move it after the allocation and assignment. Or move the allocation and assignment before which is not suggested.
>>
>> Thanks for your reply. Do I need to submit a v2 patch? Or you will take care of it?
> I'll take care of it. I have a patch prepared, but I need to do a bit more testing before I can confirm it doesn't break one specific controller. I'll try to file it as soon as possible. Do you have a timeframe you need this by?
You can follow your own plan since I am an enthusiast who discovers and
patches security vulnerabilities, instead of reply on the functionability.
>
>> Dongliang Mu
>>
>>>> Hi Dan,
>>>>
>>>> this only occurs in linux-next tree.
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/input/joystick/xpad.c?n2053#n2053
>>>>
>>>>>> Fix this by allocating xpad->dev, its error handling and cleanup
>>>>>> operations in the remove function.
>>>>>>
>>>>>> Note that this crash does not have any reproducer, so the patch
>>>>>> only passes compilation testing.
>>>>> The xpad->dev = input_dev; already happens in xpad_init_input(). We
>>>>> shouldn't allocate it twice. I think the fix is to just use a different
>>>>> device pointer for the dev_warn(). Why not use &xpad->intf->dev?
>>>> Yeah, the allocation and assignment is in the last part that I missed before. We have two choices to fix this issue:
>>>>
>>>> 1. Change to another device pointer;
>>>>
>>>> 2. Move the allocation and assignment to a previous code site;
>>>>
>>>> If there is no other places dereferencing this pointer before the allocation and assignment, it's better to use the 1st one.
>>>>
>>>> Let me craft a v2 patch.
>>>>
>>>>>> Reported-by: syzbot+a3f758b8d8cb7e49afec@syzkaller.appspotmail.com
>>>>> Could you use a Link tag to link to the bug report?
>>>>> Link: https://groups.google.com/g/syzkaller-bugs/c/iMhTgpGuIbM
>>>> Sure, no problem.
>>>>> This needs a Fixes tag.
>>>>>
>>>>> Fixes: db7220c48d8d ("Input: xpad - fix support for some third-party controllers")
>>>>>
>>>>> regards,
>>>>> dan carpenter
>>>>>
>>> Vicki
> Vicki
^ permalink raw reply
* Re: [PATCH] Input: xpad - fix GPF in xpad_probe
From: Vicki Pfau @ 2023-04-17 10:24 UTC (permalink / raw)
To: Dongliang Mu, Dan Carpenter
Cc: Dmitry Torokhov, Pavel Rojtberg, Nate Yocom, Mattijs Korpershoek,
John Butler, Matthias Benkmann, Christopher Crockett,
Santosh De Massari, hust-os-kernel-patches,
syzbot+a3f758b8d8cb7e49afec, Pierre-Loup A. Griffais, linux-input,
linux-kernel
In-Reply-To: <99794af0-7367-acff-357d-1cd4fa7f832e@hust.edu.cn>
Hi,
On 4/17/23 03:01, Dongliang Mu wrote:
>
> On 2023/4/17 17:25, Dan Carpenter wrote:
>> On Fri, Apr 14, 2023 at 08:55:47PM +0800, Dongliang Mu wrote:
>>> In xpad_probe(), it does not allocate xpad->dev with input_dev type.
>>> Then, when it invokes dev_warn with 1st argument - &xpad->dev->dev, it
>>> would trigger GPF.
>> What is a call tree for this? Actually I found it from the bug report.
>> drivers/input/joystick/xpad.c
>> 2034 if (error)
>> 2035 dev_warn(&xpad->dev->dev,
>> 2036 "unable to receive magic message: %d\n",
>> 2037 error);
>> 2038 }
>
Sorry, this appears to be my code, and was merged recently after a few drafts with Dmitry. This code is sensitive to being moved and only affects some controllers, so I'm looking into if I can move it into after creation of the input_dev right now. It's something I'd already thought might be necessary, but I didn't find any evidence for it before. I'll try to get back to you on that soon.
> Hi Dan,
>
> this only occurs in linux-next tree.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/input/joystick/xpad.c?n2053#n2053
>
>>> Fix this by allocating xpad->dev, its error handling and cleanup
>>> operations in the remove function.
>>>
>>> Note that this crash does not have any reproducer, so the patch
>>> only passes compilation testing.
>> The xpad->dev = input_dev; already happens in xpad_init_input(). We
>> shouldn't allocate it twice. I think the fix is to just use a different
>> device pointer for the dev_warn(). Why not use &xpad->intf->dev?
>
> Yeah, the allocation and assignment is in the last part that I missed before. We have two choices to fix this issue:
>
> 1. Change to another device pointer;
>
> 2. Move the allocation and assignment to a previous code site;
>
> If there is no other places dereferencing this pointer before the allocation and assignment, it's better to use the 1st one.
>
> Let me craft a v2 patch.
>
>>
>>> Reported-by: syzbot+a3f758b8d8cb7e49afec@syzkaller.appspotmail.com
>> Could you use a Link tag to link to the bug report?
>> Link: https://groups.google.com/g/syzkaller-bugs/c/iMhTgpGuIbM
> Sure, no problem.
>>
>> This needs a Fixes tag.
>>
>> Fixes: db7220c48d8d ("Input: xpad - fix support for some third-party controllers")
>>
>> regards,
>> dan carpenter
>>
Vicki
^ permalink raw reply
* Re: [regression] Bug 216946 - Toshiba satellite click mini l9w-b: touchscreen: no touch events with kernel 6.1.4
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-04-17 11:55 UTC (permalink / raw)
To: Hans de Goede, Benjamin Tissoires, Linux regressions mailing list
Cc: Jiri Kosina, Dmitry Torokhov, open list:HID CORE LAYER, LKML,
Gé Koerkamp
In-Reply-To: <13c5e60d-d9ba-f840-f43e-40db957a6617@redhat.com>
On 01.03.23 12:41, Hans de Goede wrote:
> On 2/28/23 14:26, Benjamin Tissoires wrote:
>> On Tue, Feb 28, 2023 at 12:32 PM Thorsten Leemhuis
>> <regressions@leemhuis.info> wrote:
>>>
>>> On 19.01.23 16:06, Linux kernel regression tracking (Thorsten Leemhuis)
>>> wrote:
>>>> Hi, this is your Linux kernel regression tracker.
>>>>
>>>> I noticed a regression report in bugzilla.kernel.org. As many (most?)
>>>> kernel developer don't keep an eye on it, I decided to forward it by
>>>> mail. Quoting from https://bugzilla.kernel.org/show_bug.cgi?id=216946 :
>>>
>>> The reporter recently confirmed in the ticket that the issue still
>>> happens with 6.2.
>>>
>>> There wasn't any reply from any of the input developers here or in
>>> bugzilla afaics. :-/ Hmmm. Could someone from that camp maybe please
>>> take a minute and at least briefly look into this as answer something
>>> like "that might be due to a problem in subsystem 'foo'", "maybe ask bar
>>> for an option", or "we have no idea what might cause this, this needs to
>>> be bisected"? That would help a lot.
>>
>> The working dmesg shows a line with:
>> hid-generic 0018:0457:10FB.0002: input,hidraw1: I2C HID v1.00 Device
>> [SIS0817:00 0457:10FB] on i2c-SIS0817:00
>> and then
>> hid-multitouch 0018:0457:10FB.0002: input,hidraw1: I2C HID v1.00
>> Device [SIS0817:00 0457:10FB] on i2c-SIS0817:00
>>
>> But these 2 lines do not appear on the 6.1.4 logs.
>>
>> So the device is not properly enumerated by ACPI or I2C. Hans might
>> have an idea on how to debug/solve that issue.
>
> I actually have a Toshiba satellite click mini l9w-b lying around
> myself. I already made a note to try and reproduce this
>
> But I'm very much swamped with too much kernel work, so no promises
> when I will get around to this ...
Has anyone made any progress with this? Doesn't look like it, hence I
wondered if this fall through the cracks; but maybe I'm just missing
something.
Gé Koerkamp: with a bit of luck Hans sooner or later will have time to
look into this, but it might speed things up if you could try to bisect
this.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
#regzbot poke
>>> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
>>> --
>>> Everything you wanna know about Linux kernel regression tracking:
>>> https://linux-regtracking.leemhuis.info/about/#tldr
>>> If I did something stupid, please tell me, as explained on that page.
>>>
>>> #regzbot poke
>>>>> Gé Koerkamp 2023-01-17 20:21:51 UTC
>>>>>
>>>>> Created attachment 303619 [details]
>>>>> Kernel configuration for v6.1.4/ journalctl (dmesg)/ ACPIdump/lsmod
>>>>>
>>>>> Overview:
>>>>> The touchscreen does not react on touch events.
>>>>> Touchscreen display and touchpad are working.
>>>>>
>>>>> Step to reproduce:
>>>>> Open any UI page
>>>>> Try to use touch on relevant UI controls (buttons etc.)
>>>>>
>>>>> Result:
>>>>> No reaction on screen touches
>>>>>
>>>>> Expected result:
>>>>> Reaction on touched control, same as when using the touch pad or connected mouse (which do work).
>>>>>
>>>>> Build information:
>>>>> The error happens with kernel version 6.1.4
>>>>> After rebuilding with different kernel versions, it appears that it first fails with kernel 5.16
>>>>>
>>>>> Additional builds:
>>>>> The click mini l9w-b still works with kernel 5.10.y LTS and 5.15.y LTS.
>>>>>
>>>>> Important remark:
>>>>> Touchscreen still works fine with kernel 6.1.4 using
>>>>> - an HP x2 detachable 10-p0xx or
>>>>> - a Lenovo yoga 520-14ikb
>>>>>
>>>>> So it could be a hardware dependent issue
>>>>
>>>> See the ticket for more details.
>>>>
>>>>
>>>> [TLDR for the rest of this mail: I'm adding this report to the list of
>>>> tracked Linux kernel regressions; the text you find below is based on a
>>>> few templates paragraphs you might have encountered already in similar
>>>> form.]
>>>>
>>>> BTW, let me use this mail to also add the report to the list of tracked
>>>> regressions to ensure it's doesn't fall through the cracks:
>>>>
>>>> #regzbot introduced: v5.15..v5.16
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=216946
>>>> #regzbot title: hid: touchscreen broken with Toshiba satellite click
>>>> mini l9w-b
>>>> #regzbot ignore-activity
>>>>
>>>> This isn't a regression? This issue or a fix for it are already
>>>> discussed somewhere else? It was fixed already? You want to clarify when
>>>> the regression started to happen? Or point out I got the title or
>>>> something else totally wrong? Then just reply and tell me -- ideally
>>>> while also telling regzbot about it, as explained by the page listed in
>>>> the footer of this mail.
>>>>
>>>> Developers: When fixing the issue, remember to add 'Link:' tags pointing
>>>> to the report (e.g. the buzgzilla ticket and maybe this mail as well, if
>>>> this thread sees some discussion). See page linked in footer for details.
>>>>
>>>> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
>>>> --
>>>> Everything you wanna know about Linux kernel regression tracking:
>>>> https://linux-regtracking.leemhuis.info/about/#tldr
>>>> If I did something stupid, please tell me, as explained on that page.
>>>
>>
>
>
>
^ 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