* Re: [PATCH v3 18/23] regulator: add s2dos05 regulator support
From: Krzysztof Kozlowski @ 2024-06-20 16:08 UTC (permalink / raw)
To: Dzmitry Sankouski, Sebastian Reichel, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Dmitry Torokhov, Pavel Machek,
Liam Girdwood, Mark Brown, Uwe Kleine-König, Konrad Dybcio,
Chanwoo Choi, phone-devel
Cc: linux-pm, linux-kernel, linux-arm-msm, linux-clk, dri-devel,
devicetree, linux-input, linux-leds, linux-pwm, linux-samsung-soc
In-Reply-To: <20240618-starqltechn_integration_upstream-v3-18-e3f6662017ac@gmail.com>
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> S2dos05 has 1 buck and 4 LDO regulators, used for powering
> panel/touchscreen.
>
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> diff --git a/drivers/regulator/s2dos05-regulator.c b/drivers/regulator/s2dos05-regulator.c
> new file mode 100644
> index 000000000000..3c58a1bd2262
> --- /dev/null
> +++ b/drivers/regulator/s2dos05-regulator.c
> @@ -0,0 +1,362 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * s2dos05.c - Regulator driver for the Samsung s2dos05
> + *
> + * Copyright (C) 2016 Samsung Electronics
Not happy. You upstream old issues. :( Please drop all junk Samsung code.
> + * Copyright (C) 2023 Dzmitry Sankouski <dsankouski@gmail.com>
> + *
> + */
...
> +
> + return ret;
> +
> +err_data:
> + devm_kfree(dev, (void *)s2dos05);
Why?
> + kfree(s2dos05);
Please test this first. This is obviously wrong and having such trivial
issue makes me question what else is in this Samsung code.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 17/23] mfd: add s2dos series core driver
From: Krzysztof Kozlowski @ 2024-06-20 16:07 UTC (permalink / raw)
To: Dzmitry Sankouski, Sebastian Reichel, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Dmitry Torokhov, Pavel Machek,
Liam Girdwood, Mark Brown, Uwe Kleine-König, Konrad Dybcio,
Chanwoo Choi, phone-devel
Cc: linux-pm, linux-kernel, linux-arm-msm, linux-clk, dri-devel,
devicetree, linux-input, linux-leds, linux-pwm, linux-samsung-soc
In-Reply-To: <20240618-starqltechn_integration_upstream-v3-17-e3f6662017ac@gmail.com>
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> S2DOS05 is a panel/touchscreen PMIC, often found in
> Samsung phones. We define 2 sub-devices for which drivers will
> be added in subsequent patches.
>
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> ---
> MAINTAINERS | 1 +
> drivers/mfd/Kconfig | 13 +++
> drivers/mfd/Makefile | 2 +-
> drivers/mfd/s2dos-core.c | 141 +++++++++++++++++++++++++++++++++
> include/linux/mfd/samsung/s2dos-core.h | 21 +++++
> include/linux/mfd/samsung/s2dos05.h | 115 +++++++++++++++++++++++++++
> 6 files changed, 292 insertions(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f3c245d432d9..b53462684a30 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19908,6 +19908,7 @@ F: Documentation/devicetree/bindings/regulator/samsung,s2dos*.yaml
> F: Documentation/devicetree/bindings/regulator/samsung,s2m*.yaml
> F: Documentation/devicetree/bindings/regulator/samsung,s5m*.yaml
> F: drivers/clk/clk-s2mps11.c
> +F: drivers/mfd/s2dos*.c
> F: drivers/mfd/sec*.c
> F: drivers/regulator/s2m*.c
> F: drivers/regulator/s5m*.c
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index c4eb8ff2dcad..517d8778d7a8 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1278,6 +1278,19 @@ config MFD_RN5T618
> additional drivers must be enabled in order to use the
> functionality of the device.
>
> +config MFD_S2DOS_CORE
> + tristate "Samsung Electronics PMIC Series Support"
Nope, that's a strong NAK.
No need for one more clone of Samsung PMIC driver.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 14/23] power: supply: max77705: Add charger driver for Maxim 77705
From: Krzysztof Kozlowski @ 2024-06-20 16:06 UTC (permalink / raw)
To: Dzmitry Sankouski, Sebastian Reichel, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Dmitry Torokhov, Pavel Machek,
Liam Girdwood, Mark Brown, Uwe Kleine-König, Konrad Dybcio,
Chanwoo Choi, phone-devel
Cc: linux-pm, linux-kernel, linux-arm-msm, linux-clk, dri-devel,
devicetree, linux-input, linux-leds, linux-pwm, linux-samsung-soc
In-Reply-To: <20240618-starqltechn_integration_upstream-v3-14-e3f6662017ac@gmail.com>
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> Add driver for Maxim 77705 switch-mode charger (part of max77705
> MFD driver) providing power supply class information to userspace.
>
> The driver is configured through DTS (battery and system related
> settings). Also, POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE property
> is writable, which allows to configure charge end at less than 100%
>
...
> + *
> + * Copyright (C) 2015 Samsung Electronics, Inc.
> + *
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
Yeah, please do not upstream 2015 Samsung code. Instead, take a new,
clean, decent driver and use it as template.
Otherwise you ask us to do exactly the same review we already gave last
10 years. Or review on issues we fixed last 10 years.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 13/23] input: add max77705 haptic driver
From: Krzysztof Kozlowski @ 2024-06-20 16:04 UTC (permalink / raw)
To: Dzmitry Sankouski, Sebastian Reichel, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Dmitry Torokhov, Pavel Machek,
Liam Girdwood, Mark Brown, Uwe Kleine-König, Konrad Dybcio,
Chanwoo Choi, phone-devel
Cc: linux-pm, linux-kernel, linux-arm-msm, linux-clk, dri-devel,
devicetree, linux-input, linux-leds, linux-pwm, linux-samsung-soc
In-Reply-To: <20240618-starqltechn_integration_upstream-v3-13-e3f6662017ac@gmail.com>
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> Add support for haptic controller on MAX77705 Multifunction
> device.
>
> This driver supports external pwm and LRA (Linear Resonant Actuator) motor.
> User can control the haptic device via force feedback framework.
>
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> ---
> +static int max77705_haptic_bias(struct max77705_haptic *haptic, bool on)
> +{
> + int error;
> +
> + error = regmap_update_bits(haptic->regmap_haptic,
> + MAX77705_PMIC_REG_MAINCTRL1,
> + MAX77705_MAINCTRL1_BIASEN_MASK,
> + on << MAX77705_MAINCTRL1_BIASEN_SHIFT);
> +
> + if (error) {
> + dev_err(haptic->dev, "failed to %s bias: %d\n",
> + on ? "enable" : "disable", error);
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static int max77705_haptic_configure(struct max77705_haptic *haptic,
> + bool enable)
> +{
> + unsigned int value, config_reg;
> + int error;
> +
> + value = ((haptic->type << MAX77705_CONFIG2_MODE_SHIFT) |
> + (enable << MAX77705_CONFIG2_MEN_SHIFT) |
> + (haptic->mode << MAX77705_CONFIG2_HTYP_SHIFT) |
> + MAX77705_HAPTIC_PWM_DIVISOR_128);
> + config_reg = MAX77705_PMIC_REG_MCONFIG;
> +
> + error = regmap_write(haptic->regmap_haptic,
> + config_reg, value);
> + if (error) {
> + dev_err(haptic->dev,
> + "failed to update haptic config: %d\n", error);
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static void max77705_haptic_enable(struct max77705_haptic *haptic)
> +{
> + int error;
> +
> + if (haptic->enabled)
> + return;
> +
> + error = pwm_enable(haptic->pwm_dev);
> + if (error) {
> + dev_err(haptic->dev,
> + "failed to enable haptic pwm device: %d\n", error);
> + return;
> + }
> +
> + error = max77705_haptic_configure(haptic, true);
> + if (error)
> + goto err_enable_config;
> +
> + haptic->enabled = true;
> +
> + return;
> +
> +err_enable_config:
> + pwm_disable(haptic->pwm_dev);
> +}
> +
> +static void max77705_haptic_disable(struct max77705_haptic *haptic)
> +{
> + int error;
> +
> + if (!haptic->enabled)
> + return;
> +
> + error = max77705_haptic_configure(haptic, false);
> + if (error)
> + return;
> +
> + pwm_disable(haptic->pwm_dev);
> + haptic->enabled = false;
> +}
> +
> +static void max77705_haptic_play_work(struct work_struct *work)
> +{
> + struct max77705_haptic *haptic =
> + container_of(work, struct max77705_haptic, work);
> + int error;
> +
> + error = max77705_haptic_set_duty_cycle(haptic);
> + if (error) {
> + dev_err(haptic->dev, "failed to set duty cycle: %d\n", error);
> + return;
> + }
> +
> + if (haptic->magnitude)
> + max77705_haptic_enable(haptic);
> + else
> + max77705_haptic_disable(haptic);
> +}
> +
> +static int max77705_haptic_play_effect(struct input_dev *dev, void *data,
> + struct ff_effect *effect)
> +{
> + struct max77705_haptic *haptic = input_get_drvdata(dev);
> + struct pwm_args pargs;
> + u64 period_mag_multi;
> +
> + haptic->magnitude = effect->u.rumble.strong_magnitude;
> + if (!haptic->magnitude)
> + haptic->magnitude = effect->u.rumble.weak_magnitude;
> +
> + /*
> + * The magnitude comes from force-feedback interface.
> + * The formula to convert magnitude to pwm_duty as follows:
> + * - pwm_duty = (magnitude * pwm_period) / MAX_MAGNITUDE(0xFFFF)
> + */
> + pr_info("magnitude: %d(%x)", haptic->magnitude, haptic->magnitude);
Do not use pr_xxx in your drivers. That's a generic comment so please
apply it everywhere. Anyway driver should be silent.
> + pwm_get_args(haptic->pwm_dev, &pargs);
> + period_mag_multi = (u64)pargs.period * haptic->magnitude;
> + haptic->pwm_duty = (unsigned int)(period_mag_multi >>
> + MAX_MAGNITUDE_SHIFT);
> +
> + schedule_work(&haptic->work);
> +
> + return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(max77705_haptic_pm_ops,
> + max77705_haptic_suspend,
> + max77705_haptic_resume);
> +
> +static const struct of_device_id of_max77705_haptic_dt_match[] = {
> + { .compatible = "maxim,max77705-haptic", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, of_max77705_haptic_dt_match);
> +
> +static struct platform_driver max77705_haptic_driver = {
> + .driver = {
> + .name = "max77705-haptic",
> + .pm = pm_sleep_ptr(&max77705_haptic_pm_ops),
> + .of_match_table = of_max77705_haptic_dt_match,
> + },
> + .probe = max77705_haptic_probe,
> + .remove_new = max77705_haptic_remove,
> +};
> +module_platform_driver(max77705_haptic_driver);
> +
> +MODULE_AUTHOR("Dzmitry Sankouski <dsankouski@gmail.com>");
> +MODULE_AUTHOR("Jaewon Kim <jaewon02.kim@samsung.com>");
> +MODULE_AUTHOR("Krzysztof Kozlowski <krzk@kernel.org>");
I doubt that this driver is needed. Everything is copy of max777693.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 12/23] mfd: Add new driver for MAX77705 PMIC
From: Krzysztof Kozlowski @ 2024-06-20 16:02 UTC (permalink / raw)
To: Dzmitry Sankouski, Sebastian Reichel, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Dmitry Torokhov, Pavel Machek,
Liam Girdwood, Mark Brown, Uwe Kleine-König, Konrad Dybcio,
Chanwoo Choi, phone-devel
Cc: linux-pm, linux-kernel, linux-arm-msm, linux-clk, dri-devel,
devicetree, linux-input, linux-leds, linux-pwm, linux-samsung-soc,
Rose Hudson
In-Reply-To: <20240618-starqltechn_integration_upstream-v3-12-e3f6662017ac@gmail.com>
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> Add the core MFD driver for max77705 PMIC. We define five sub-devices
> for which the drivers will be added in subsequent patches.
>
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> Co-authored-by: Rose Hudson <rose@krx.sh>
> ---
> MAINTAINERS | 2 +
> drivers/mfd/Kconfig | 12 ++
> drivers/mfd/Makefile | 3 +
> drivers/mfd/max77705-core.c | 278 ++++++++++++++++++++++++++++++++
> drivers/mfd/max77705-irq.c | 299 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77705-private.h | 281 ++++++++++++++++++++++++++++++++
> include/linux/mfd/max77705.h | 20 +++
> 7 files changed, 895 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fae3b8ea9ce4..f66f08825db9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13690,6 +13690,7 @@ F: drivers/*/*max77843.c
> F: drivers/*/max14577*.c
> F: drivers/*/max77686*.c
> F: drivers/*/max77693*.c
> +F: drivers/*/max77705*.c
> F: drivers/clk/clk-max77686.c
> F: drivers/extcon/extcon-max14577.c
> F: drivers/extcon/extcon-max77693.c
> @@ -13697,6 +13698,7 @@ F: drivers/rtc/rtc-max77686.c
> F: include/linux/mfd/max14577*.h
> F: include/linux/mfd/max77686*.h
> F: include/linux/mfd/max77693*.h
> +F: include/linux/mfd/max77705*.h
>
> MAXIRADIO FM RADIO RECEIVER DRIVER
> M: Hans Verkuil <hverkuil@xs4all.nl>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 266b4f54af60..c4eb8ff2dcad 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -880,6 +880,18 @@ config MFD_MAX77693
> additional drivers must be enabled in order to use the functionality
> of the device.
>
> +config MFD_MAX77705
> + tristate "Maxim Semiconductor MAX77705 PMIC Support"
> + depends on I2C
> + select MFD_CORE
> + help
> + Say yes here to add support for Maxim Semiconductor MAX77705.
> + This is a Power Management IC with Charger, safe LDOs, Flash, Haptic
> + and MUIC controls on chip.
> + This driver provides common support for accessing the device;
> + additional drivers must be enabled in order to use the functionality
> + of the device.
> +
> config MFD_MAX77714
> tristate "Maxim Semiconductor MAX77714 PMIC Support"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index db1ba39de3b5..bf65cc36d59c 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -162,6 +162,8 @@ obj-$(CONFIG_MFD_MAX77620) += max77620.o
> obj-$(CONFIG_MFD_MAX77650) += max77650.o
> obj-$(CONFIG_MFD_MAX77686) += max77686.o
> obj-$(CONFIG_MFD_MAX77693) += max77693.o
> +max77705-objs := max77705-core.o max77705-irq.o
> +obj-$(CONFIG_MFD_MAX77705) += max77705.o
> obj-$(CONFIG_MFD_MAX77714) += max77714.o
> obj-$(CONFIG_MFD_MAX77843) += max77843.o
> obj-$(CONFIG_MFD_MAX8907) += max8907.o
> @@ -226,6 +228,7 @@ obj-$(CONFIG_MFD_RK8XX_I2C) += rk8xx-i2c.o
> obj-$(CONFIG_MFD_RK8XX_SPI) += rk8xx-spi.o
> obj-$(CONFIG_MFD_RN5T618) += rn5t618.o
> obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o
> +obj-$(CONFIG_MFD_S2DOS05) += s2dos05.o
??? That's not related.
> obj-$(CONFIG_MFD_SYSCON) += syscon.o
> obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o
> obj-$(CONFIG_MFD_VEXPRESS_SYSREG) += vexpress-sysreg.o
> diff --git a/drivers/mfd/max77705-core.c b/drivers/mfd/max77705-core.c
> new file mode 100644
> index 000000000000..7cb71a0a9688
> --- /dev/null
> +++ b/drivers/mfd/max77705-core.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * max77705.c - mfd core driver for the Maxim 77705
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Copyright (C) 2024 Dzmitry Sankouski <dsankouski@gmail.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/mutex.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/max77705.h>
> +#include <linux/mfd/max77705-private.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/debugfs.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +
> +#define I2C_ADDR_PMIC (0xCC >> 1) /* Top sys, Haptic */
> +#define I2C_ADDR_MUIC (0x4A >> 1)
> +#define I2C_ADDR_CHG (0xD2 >> 1)
> +#define I2C_ADDR_FG (0x6C >> 1)
> +#define I2C_ADDR_DEBUG (0xC4 >> 1)
> +
> +static struct dentry *debugfs_file;
> +
> +static int max77705_debugfs_show(struct seq_file *s, void *data)
> +{
> + struct max77705_dev *max77705 = s->private;
> + struct regmap *regmap = max77705->regmap;
> + unsigned int i, reg, reg_data, pmic_id, pmic_rev;
> + int regs[] = {
> + MAX77705_PMIC_REG_MAINCTRL1,
> + MAX77705_PMIC_REG_MCONFIG,
> + MAX77705_PMIC_REG_MCONFIG2,
> + MAX77705_PMIC_REG_INTSRC,
> + MAX77705_PMIC_REG_INTSRC_MASK,
> + MAX77705_PMIC_REG_SYSTEM_INT,
> + MAX77705_PMIC_REG_SYSTEM_INT_MASK,
> + MAX77705_RGBLED_REG_LEDEN,
> + MAX77705_RGBLED_REG_LED0BRT,
> + MAX77705_RGBLED_REG_LED1BRT,
> + MAX77705_RGBLED_REG_LED2BRT,
> + MAX77705_RGBLED_REG_LED3BRT,
> + MAX77705_RGBLED_REG_LEDBLNK
> + };
> +
> + regmap_read(regmap, MAX77705_PMIC_REG_PMICID1, &pmic_id);
> + regmap_read(regmap, MAX77705_PMIC_REG_PMICREV, &pmic_rev);
> + seq_printf(s, "MAX77705, pmic id: %d, pmic rev: %d\n",
> + pmic_id, pmic_rev);
> + seq_puts(s, "===================\n");
> + for (i = 0; i < ARRAY_SIZE(regs); i++) {
> + reg = regs[i];
> + regmap_read(regmap, reg, ®_data);
> + seq_printf(s, "0x%02x:\t0x%02x\n", reg, reg_data);
> + }
> +
> + seq_puts(s, "\n");
> + return 0;
Why do you need this code? Regmap has its own debugging code.
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(max77705_debugfs);
> +
> +static const struct regmap_config max77705_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = MAX77705_PMIC_REG_END,
> +};
> +
> +static const struct regmap_config max77705_leds_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = MAX77705_LED_REG_END,
> +};
> +
> +static const struct regmap_config max77705_fg_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = MAX77705_FG_END,
> +};
> +
> +static struct mfd_cell max77705_devs[] = {
> + {
> + .name = "leds-max77705-rgb",
> + .of_compatible = "maxim,max77705-led",
> + },
> + {
> + .name = "max77705-fuelgauge",
> + .of_compatible = "maxim,max77705-fg",
> + },
> + {
> + .name = "max77705-charger",
> + .of_compatible = "maxim,max77705-charger",
> + },
> + {
> + .name = "max77705-haptic",
> + .of_compatible = "maxim,max77705-haptic",
> + },
> +};
> +
> +static int max77705_i2c_probe(struct i2c_client *i2c)
> +{
> + struct max77705_dev *max77705;
> + struct max77705_platform_data *pdata = i2c->dev.platform_data;
This is not used. You need to clean the driver from such obsoletes from
2010.
> +
> + unsigned int reg_data;
> + int ret = 0;
> +
> + max77705 = kzalloc(sizeof(struct max77705_dev), GFP_KERNEL);
sizeof(*)
devm_kzalloc, unless you cannot use it because (missing comment)?
Please take the newest driver as your template. Do not upstream some
old, crappy code. Things changed over last 10 years, conventions changed.
> + if (!max77705)
> + return -ENOMEM;
> +
> + max77705->pdata = pdata;
Drop, not used.
> + max77705->dev = &i2c->dev;
> + max77705->i2c = i2c;
> + max77705->irq = i2c->irq;
> +
> + max77705->regmap = devm_regmap_init_i2c(max77705->i2c, &max77705_regmap_config);
> + if (IS_ERR(max77705->regmap)) {
> + ret = PTR_ERR(max77705->regmap);
> + dev_err(max77705->dev, "failed to allocate register map: %d\n",
> + ret);
Not sure if we print allocation errors... but if you print, then:
ret = dev_err_probe
> + return ret;
> + }
> +
> + max77705->regmap_leds = devm_regmap_init_i2c(max77705->i2c, &max77705_leds_regmap_config);
> + if (IS_ERR(max77705->regmap_leds)) {
> + ret = PTR_ERR(max77705->regmap_leds);
> + dev_err(max77705->dev, "failed to allocate register map: %d\n",
> + ret);
> + return ret;
> + }
> +
> + i2c_set_clientdata(i2c, max77705);
> +
> + if (regmap_read(max77705->regmap, MAX77705_PMIC_REG_PMICREV, ®_data) < 0) {
> + dev_err(max77705->dev,
> + "device not found on this channel (this is not an error)\n");
> + ret = -ENODEV;
> + goto err;
> + } else {
> + /* print rev */
> + max77705->pmic_rev = (reg_data & MAX77705_REVISION_MASK);
> + max77705->pmic_ver = ((reg_data & MAX77705_VERSION_MASK) >> MAX77705_VERSION_SHIFT);
> + dev_info(max77705->dev, "%s device found: rev.0x%x, ver.0x%x\n",
> + __func__, max77705->pmic_rev, max77705->pmic_ver);
dev_dbg
> + }
> +
> + max77705->charger = devm_i2c_new_dummy_device(max77705->dev, i2c->adapter, I2C_ADDR_CHG);
> + i2c_set_clientdata(max77705->charger, max77705);
> + max77705->regmap_charger = devm_regmap_init_i2c(max77705->charger, &max77705_regmap_config);
> + if (IS_ERR(max77705->regmap)) {
> + ret = PTR_ERR(max77705->regmap);
> + dev_err(max77705->dev, "failed to allocate register map: %d\n",
> + ret);
> + return ret;
> + }
> +
> + max77705->fuelgauge = devm_i2c_new_dummy_device(max77705->dev, i2c->adapter, I2C_ADDR_FG);
> + i2c_set_clientdata(max77705->fuelgauge, max77705);
> + max77705->regmap_fg = devm_regmap_init_i2c(max77705->fuelgauge, &max77705_fg_regmap_config);
> + if (IS_ERR(max77705->regmap_fg)) {
> + ret = PTR_ERR(max77705->regmap_fg);
> + dev_err(max77705->dev, "failed to allocate register map: %d\n",
> + ret);
> + return ret;
> + }
> +
> + if (likely(i2c->irq > 0))
Drop likely
> + max77705->irq = i2c->irq;
Why do you need it?
> + else {
> + dev_err(max77705->dev, "failed to get irq number\n");
> + return -EINVAL;
> + }
> +
> + max77705->irq_base = irq_alloc_descs(-1, 0, MAX77705_IRQ_NR, -1);
Why this is not using simple regmap_irq_chip?
> + if (unlikely(max77705->irq_base < 0)) {
> + dev_err(max77705->dev, "irq_alloc_descs fail: %d\n", max77705->irq_base);
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + disable_irq(max77705->irq);
Heh?
> + ret = max77705_irq_init(max77705);
> + if (ret) {
> + dev_err(max77705->dev, "failed to init irq system: %d\n", ret);
> + return ret;
> + }
> +
> + ret = mfd_add_devices(max77705->dev, -1, max77705_devs,
> + ARRAY_SIZE(max77705_devs), NULL, 0, NULL);
> + if (ret < 0)
> + goto err_mfd;
> +
> + debugfs_file = debugfs_create_file("max77705-regs",
> + 0664, NULL, (void *)max77705,
> + &max77705_debugfs_fops);
> + if (!debugfs_file)
> + dev_err(max77705->dev, "Failed to create debugfs file\n");
Drop dev_err
> +
> + device_init_wakeup(max77705->dev, true);
> +
> + return ret;
> +
> +err_mfd:
> + mfd_remove_devices(max77705->dev);
> +err:
> + kfree(max77705);
> + return ret;
> +}
> +
> +static void max77705_i2c_remove(struct i2c_client *i2c)
> +{
> + struct max77705_dev *max77705 = i2c_get_clientdata(i2c);
> +
> + if (debugfs_file)
> + debugfs_remove(debugfs_file);
> +
> + device_init_wakeup(max77705->dev, 0);
> + mfd_remove_devices(max77705->dev);
No freeing here? Sorry, this code is really poor.
> +}
> +
> +static int __maybe_unused max77705_suspend(struct device *dev)
> +{
> + struct i2c_client *i2c = to_i2c_client(dev);
> + struct max77705_dev *max77705 = i2c_get_clientdata(i2c);
> +
> + disable_irq(max77705->irq);
> + if (device_may_wakeup(dev))
> + enable_irq_wake(max77705->irq);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused max77705_resume(struct device *dev)
> +{
> + struct i2c_client *i2c = to_i2c_client(dev);
> + struct max77705_dev *max77705 = i2c_get_clientdata(i2c);
> +
> + if (device_may_wakeup(dev))
> + disable_irq_wake(max77705->irq);
> + enable_irq(max77705->irq);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(max77705_pm, max77705_suspend, max77705_resume);
> +
> +static const struct of_device_id max77705_i2c_dt_ids[] = {
> + { .compatible = "maxim,max77705" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, max77705_i2c_dt_ids);
> +
> +static struct i2c_driver max77705_i2c_driver = {
> + .driver = {
> + .name = MFD_DEV_NAME,
> + .pm = &max77705_pm,
> + .of_match_table = max77705_i2c_dt_ids,
> + .suppress_bind_attrs = true,
Nope, drop. Fix the code instead.
> + },
> + .probe = max77705_i2c_probe,
> + .remove = max77705_i2c_remove,
> +};
> +module_i2c_driver(max77705_i2c_driver);
> +
> +MODULE_DESCRIPTION("MAXIM 77705 multi-function core driver");
> +MODULE_AUTHOR("Dzmitry Sankouski <dsankouski@gmail.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/max77705-irq.c b/drivers/mfd/max77705-irq.c
> new file mode 100644
> index 000000000000..0da17fc95e18
> --- /dev/null
> +++ b/drivers/mfd/max77705-irq.c
> @@ -0,0 +1,299 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * max77705-irq.c - Interrupt controller support for MAX77705
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Copyright (C) 2024 Dzmitry Sankouski <dsankouski@gmail.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/max77705.h>
> +#include <linux/mfd/max77705-private.h>
> +#include <linux/regmap.h>
> +
> +static const u8 max77705_mask_reg[] = {
> + [SYS_INT] = MAX77705_PMIC_REG_SYSTEM_INT_MASK,
> + [CHG_INT] = MAX77705_CHG_REG_INT_MASK,
> + [FUEL_INT] = MAX77705_REG_INVALID,
> +};
> +
> +static struct regmap *get_i2c(struct max77705_dev *max77705,
> + enum max77705_irq_source src)
> +{
> + switch (src) {
> + case SYS_INT:
> + return max77705->regmap;
> + case FUEL_INT:
> + return max77705->regmap_fg;
> + case CHG_INT:
> + return max77705->regmap_charger;
> + default:
> + return ERR_PTR(-EINVAL);
> + }
> +}
> +
> +struct max77705_irq_data {
> + int mask;
> + enum max77705_irq_source group;
> +};
> +
> +static const struct max77705_irq_data max77705_irqs[] = {
> + [MAX77705_SYSTEM_IRQ_BSTEN_INT] = { .group = SYS_INT, .mask = BIT(3) },
> + [MAX77705_SYSTEM_IRQ_SYSUVLO_INT] = { .group = SYS_INT, .mask = BIT(4) },
> + [MAX77705_SYSTEM_IRQ_SYSOVLO_INT] = { .group = SYS_INT, .mask = BIT(5) },
> + [MAX77705_SYSTEM_IRQ_TSHDN_INT] = { .group = SYS_INT, .mask = BIT(6) },
> + [MAX77705_SYSTEM_IRQ_TM_INT] = { .group = SYS_INT, .mask = BIT(7) },
> +
> + [MAX77705_CHG_IRQ_BYP_I] = { .group = CHG_INT, .mask = BIT(0) },
> + [MAX77705_CHG_IRQ_BAT_I] = { .group = CHG_INT, .mask = BIT(3) },
> + [MAX77705_CHG_IRQ_CHG_I] = { .group = CHG_INT, .mask = BIT(4) },
> + [MAX77705_CHG_IRQ_WCIN_I] = { .group = CHG_INT, .mask = BIT(5) },
> + [MAX77705_CHG_IRQ_CHGIN_I] = { .group = CHG_INT, .mask = BIT(6) },
> + [MAX77705_CHG_IRQ_AICL_I] = { .group = CHG_INT, .mask = BIT(7) },
> +
> + [MAX77705_FG_IRQ_ALERT] = { .group = FUEL_INT, .mask = BIT(1) },
> +};
> +
> +static void max77705_irq_lock(struct irq_data *data)
> +{
> + struct max77705_dev *max77705 = irq_get_chip_data(data->irq);
> +
> + mutex_lock(&max77705->irqlock);
> +}
> +
> +static void max77705_irq_sync_unlock(struct irq_data *data)
> +{
> + struct max77705_dev *max77705 = irq_get_chip_data(data->irq);
> + int i;
> +
> + for (i = 0; i < MAX77705_IRQ_GROUP_NR; i++) {
> + u8 mask_reg = max77705_mask_reg[i];
> + struct regmap *i2c = get_i2c(max77705, i);
> +
> + if (mask_reg == MAX77705_REG_INVALID ||
> + IS_ERR_OR_NULL(i2c))
> + continue;
> + max77705->irq_masks_cache[i] = max77705->irq_masks_cur[i];
> +
> + regmap_write(i2c, max77705_mask_reg[i],
> + max77705->irq_masks_cur[i]);
> + }
> +
> + mutex_unlock(&max77705->irqlock);
Just use regmap_irq_chip.
> +
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/max77705-private.h b/include/linux/mfd/max77705-private.h
> new file mode 100644
> index 000000000000..bd6ab9c582b8
> --- /dev/null
> +++ b/include/linux/mfd/max77705-private.h
> @@ -0,0 +1,281 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * max77705-private.h
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
OMG, how this code is?
I already said but above is the nail to the coffin: sorry, start from
scratch and do not try to upstream junk downstream code.
Amount of work you require from reviewers on silly things which we
solved long time ago, is just tremendous.
That's not how upstreaming works.
> + */
> +
> +#ifndef __LINUX_MFD_MAX77705_PRIV_H
> +#define __LINUX_MFD_MAX77705_PRIV_H
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/gpio.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +
> +#define MAX77705_REG_INVALID (0xff)
> +#define MAX77705_IRQSRC_CHG BIT(0)
> +#define MAX77705_IRQSRC_TOP BIT(1)
> +#define MAX77705_IRQSRC_FG BIT(2)
> +#define MAX77705_IRQSRC_USBC BIT(3)
> +
> +/* STATUS_REG */
> +#define MAX77705_BAT_ABSENT_MASK BIT(3)
> +/* MAX77705 MAINCTRL1 register */
> +#define MAX77705_MAINCTRL1_BIASEN_SHIFT 7
> +#define MAX77705_MAINCTRL1_BIASEN_MASK BIT(MAX77705_MAINCTRL1_BIASEN_SHIFT)
> +
> +/* max77705-haptic configuration register */
> +#define MAX77705_CONFIG2_MEN_SHIFT 6
> +#define MAX77705_CONFIG2_MODE_SHIFT 7
> +#define MAX77705_CONFIG2_HTYP_SHIFT 5
> +
> +#define MAX77705_REVISION_MASK 7
> +#define MAX77705_VERSION_MASK 0xF8
> +#define MAX77705_VERSION_SHIFT 3
> +
> +enum max77705_hw_rev {
> + MAX77705_PASS1 = 1,
> + MAX77705_PASS2,
> + MAX77705_PASS3,
> +};
> +
> +enum max77705_reg {
> + /* Slave addr = 0xCC */
> + /* PMIC Top-Level Registers */
> + MAX77705_PMIC_REG_PMICID1 = 0x00,
> + MAX77705_PMIC_REG_PMICREV = 0x01,
> + MAX77705_PMIC_REG_MAINCTRL1 = 0x02,
> + MAX77705_PMIC_REG_INTSRC = 0x22,
That's not enum but defines. This applies to all your register definitions.
> + MAX77705_PMIC_REG_INTSRC_MASK = 0x23,
> + MAX77705_PMIC_REG_SYSTEM_INT = 0x24,
> + MAX77705_PMIC_REG_RESERVED_25 = 0x25,
> + MAX77705_PMIC_REG_SYSTEM_INT_MASK = 0x26,
> + MAX77705_PMIC_REG_RESERVED_27 = 0x27,
> + MAX77705_PMIC_REG_RESERVED_28 = 0x28,
> + MAX77705_PMIC_REG_RESERVED_29 = 0x29,
> + MAX77705_PMIC_REG_BOOSTCONTROL1 = 0x4C,
> + MAX77705_PMIC_REG_BSTOUT_MASK = 0x03,
> + MAX77705_PMIC_REG_BOOSTCONTROL2 = 0x4F,
> + MAX77705_PMIC_REG_FORCE_EN_MASK = 0x08,
> + MAX77705_PMIC_REG_SW_RESET = 0x50,
> + MAX77705_PMIC_REG_USBC_RESET = 0x51,
> +
> + /* Haptic motor driver Registers */
> + MAX77705_PMIC_REG_MCONFIG = 0x10,
> + MAX77705_PMIC_REG_MCONFIG2 = 0x11,
> +
> + MAX77705_CHG_REG_INT = 0xB0,
> + MAX77705_CHG_REG_INT_MASK = 0xB1,
> + MAX77705_CHG_REG_INT_OK = 0xB2,
> + MAX77705_CHG_REG_DETAILS_00 = 0xB3,
> + MAX77705_CHG_REG_DETAILS_01 = 0xB4,
> + MAX77705_CHG_REG_DETAILS_02 = 0xB5,
> + MAX77705_CHG_REG_DTLS_03 = 0xB6,
> + MAX77705_CHG_REG_CNFG_00 = 0xB7,
> + MAX77705_CHG_REG_CNFG_01 = 0xB8,
> + MAX77705_CHG_REG_CNFG_02 = 0xB9,
> + MAX77705_CHG_REG_CNFG_03 = 0xBA,
> + MAX77705_CHG_REG_CNFG_04 = 0xBB,
> + MAX77705_CHG_REG_CNFG_05 = 0xBC,
> + MAX77705_CHG_REG_CNFG_06 = 0xBD,
> + MAX77705_CHG_REG_CNFG_07 = 0xBE,
> + MAX77705_CHG_REG_CNFG_08 = 0xBF,
> + MAX77705_CHG_REG_CNFG_09 = 0xC0,
> + MAX77705_CHG_REG_CNFG_10 = 0xC1,
> + MAX77705_CHG_REG_CNFG_11 = 0xC2,
> + MAX77705_CHG_REG_CNFG_12 = 0xC3,
> + MAX77705_CHG_REG_CNFG_13 = 0xC4,
> + MAX77705_CHG_REG_CNFG_14 = 0xC5,
> + MAX77705_CHG_REG_SAFEOUT_CTRL = 0xC6,
> +
> + MAX77705_PMIC_REG_END,
> +};
> +
> +enum max77705_charger_battery_state {
> + MAX77705_BATTERY_NOBAT,
> + MAX77705_BATTERY_PREQUALIFICATION,
> + MAX77705_BATTERY_DEAD,
> + MAX77705_BATTERY_GOOD,
> + MAX77705_BATTERY_LOWVOLTAGE,
> + MAX77705_BATTERY_OVERVOLTAGE,
> + MAX77705_BATTERY_RESERVED,
> +};
> +
> +enum max77705_charger_charge_type {
> + MAX77705_CHARGER_CONSTANT_CURRENT = 1,
> + MAX77705_CHARGER_CONSTANT_VOLTAGE,
> + MAX77705_CHARGER_END_OF_CHARGE,
> + MAX77705_CHARGER_DONE,
> +};
> +
> +/* Slave addr = 0x6C : Fuelgauge */
> +enum max77705_fuelgauge_reg {
> + STATUS_REG = 0x00,
> + VALRT_THRESHOLD_REG = 0x01,
> + TALRT_THRESHOLD_REG = 0x02,
> + SALRT_THRESHOLD_REG = 0x03,
> + REMCAP_REP_REG = 0x05,
> + SOCREP_REG = 0x06,
> + TEMPERATURE_REG = 0x08,
> + VCELL_REG = 0x09,
> + TIME_TO_EMPTY_REG = 0x11,
> + FULLSOCTHR_REG = 0x13,
> + CURRENT_REG = 0x0A,
> + AVG_CURRENT_REG = 0x0B,
> + SOCMIX_REG = 0x0D,
> + SOCAV_REG = 0x0E,
> + REMCAP_MIX_REG = 0x0F,
> + FULLCAP_REG = 0x10,
> + RFAST_REG = 0x15,
> + AVR_TEMPERATURE_REG = 0x16,
> + CYCLES_REG = 0x17,
> + DESIGNCAP_REG = 0x18,
> + AVR_VCELL_REG = 0x19,
> + TIME_TO_FULL_REG = 0x20,
> + CONFIG_REG = 0x1D,
> + ICHGTERM_REG = 0x1E,
> + REMCAP_AV_REG = 0x1F,
> + FULLCAP_NOM_REG = 0x23,
> + LEARN_CFG_REG = 0x28,
> + FILTER_CFG_REG = 0x29,
> + MISCCFG_REG = 0x2B,
> + QRTABLE20_REG = 0x32,
> + FULLCAP_REP_REG = 0x35,
> + RCOMP_REG = 0x38,
> + VEMPTY_REG = 0x3A,
> + FSTAT_REG = 0x3D,
> + DISCHARGE_THRESHOLD_REG = 0x40,
> + QRTABLE30_REG = 0x42,
> + ISYS_REG = 0x43,
> + DQACC_REG = 0x45,
> + DPACC_REG = 0x46,
> + AVGISYS_REG = 0x4B,
> + QH_REG = 0x4D,
> + VSYS_REG = 0xB1,
> + TALRTTH2_REG = 0xB2,
> + /* "not used REG(0xB2)" is for checking fuelgague init result. */
> + FG_INIT_RESULT_REG = TALRTTH2_REG,
> + VBYP_REG = 0xB3,
> + CONFIG2_REG = 0xBB,
> + IIN_REG = 0xD0,
> + OCV_REG = 0xEE,
> + VFOCV_REG = 0xFB,
> + VFSOC_REG = 0xFF,
> +
> + MAX77705_FG_END,
> +};
> +
> +enum max77705_irq_source {
> + SYS_INT = 0,
> + CHG_INT,
> + FUEL_INT,
> + MAX77705_IRQ_GROUP_NR,
> +};
> +
> +
> +#define MAX77705_REG_MAINCTRL1_BIASEN BIT(7)
> +
> +/* Slave addr = 0x94: RGB LED */
> +enum max77705_led_reg {
> + MAX77705_RGBLED_REG_LEDEN = 0x30,
> + MAX77705_RGBLED_REG_LED0BRT = 0x31,
> + MAX77705_RGBLED_REG_LED1BRT = 0x32,
> + MAX77705_RGBLED_REG_LED2BRT = 0x33,
> + MAX77705_RGBLED_REG_LED3BRT = 0x34,
> + MAX77705_RGBLED_REG_LEDRMP = 0x36,
> + MAX77705_RGBLED_REG_LEDBLNK = 0x38,
> + MAX77705_LED_REG_END,
> +};
> +
> +enum max77705_irq {
> + /* PMIC; TOPSYS */
> + MAX77705_SYSTEM_IRQ_BSTEN_INT,
> + MAX77705_SYSTEM_IRQ_SYSUVLO_INT,
> + MAX77705_SYSTEM_IRQ_SYSOVLO_INT,
> + MAX77705_SYSTEM_IRQ_TSHDN_INT,
> + MAX77705_SYSTEM_IRQ_TM_INT,
> +
> + /* PMIC; Charger */
> + MAX77705_CHG_IRQ_BYP_I,
> + MAX77705_CHG_IRQ_BAT_I,
> + MAX77705_CHG_IRQ_CHG_I,
> + MAX77705_CHG_IRQ_WCIN_I,
> + MAX77705_CHG_IRQ_CHGIN_I,
> + MAX77705_CHG_IRQ_AICL_I,
> +
> + /* Fuelgauge */
> + MAX77705_FG_IRQ_ALERT,
> +
> + MAX77705_IRQ_NR,
> +};
> +
> +struct max77705_dev {
> + struct device *dev;
> + struct i2c_client *i2c; /* 0xCC; Haptic, PMIC */
> + struct i2c_client *charger; /* 0xD2; Charger */
> + struct i2c_client *fuelgauge; /* 0x6C; Fuelgauge */
> + struct i2c_client *muic; /* 0x4A; MUIC */
> + struct i2c_client *debug; /* 0xC4; Debug */
> + struct mutex i2c_lock;
> +
> + struct regmap *regmap;
> + struct regmap *regmap_fg;
> + struct regmap *regmap_charger;
> + struct regmap *regmap_leds;
> +
> + int type;
> +
> + int irq;
> + int irq_base;
> + int irq_masks_cur[MAX77705_IRQ_GROUP_NR];
> + int irq_masks_cache[MAX77705_IRQ_GROUP_NR];
> + bool wakeup;
> + struct mutex irqlock;
> +
> +#ifdef CONFIG_HIBERNATION
> + /* For hibernation */
> + u8 reg_pmic_dump[MAX77705_PMIC_REG_END];
> + u8 reg_muic_dump[MAX77705_USBC_REG_END];
> + u8 reg_led_dump[MAX77705_LED_REG_END];
> +#endif
> +
> + /* pmic VER/REV register */
> + u8 pmic_rev; /* pmic Rev */
> + u8 pmic_ver; /* pmic version */
Why do you need to store it?
> +
> + u8 cc_booting_complete;
That's nowhere used. Just like many other fields here. Clean up your
driver before posting it upstream.
> +
> + wait_queue_head_t queue_empty_wait_q;
> + int doing_irq;
> + int is_usbc_queue;
> +
> + struct max77705_platform_data *pdata;
> +};
> +
> +enum max77705_types {
> + TYPE_MAX77705,
Drop. What is this for?
> +};
> +
> +
> +/**
> + * Unmask sub device interrupts on device level
> + *
> + * @param max77705 - device structure
> + * @param mask - sub device interrupts to unmask
> + */
> +extern inline int max77705_irq_unmask_subdevice(struct max77705_dev *max77705, unsigned int mask);
See coding style: limit is 80. checkpatch is not coding style.
> +
> +/**
> + * Same as max77705_irq_unmask_device, but for masking.
> + */
> +extern inline int max77705_irq_mask_subdevice(struct max77705_dev *max77705, unsigned int mask);
> +
> +
> +extern int max77705_irq_init(struct max77705_dev *max77705);
Drop externs everywhere.
> +
> +#endif /* __LINUX_MFD_MAX77705_PRIV_H */
> diff --git a/include/linux/mfd/max77705.h b/include/linux/mfd/max77705.h
> new file mode 100644
> index 000000000000..e2d757c77973
> --- /dev/null
> +++ b/include/linux/mfd/max77705.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * max77705.h - Driver for the Maxim 77705
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __MAX77705_H__
> +#define __MAX77705_H__
> +
> +#define MFD_DEV_NAME "max77705"
> +
> +struct max77705_platform_data {
> + struct power_supply_battery_info *bat_info;
Entire platform data is redundant. Drop.
> +};
> +
> +#endif /* __MAX77705_H__ */
> +
>
Do not keep redundant blank lines. Drop.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 10/23] dt-bindings: regulator: add samsung,s2dos05
From: Krzysztof Kozlowski @ 2024-06-20 15:51 UTC (permalink / raw)
To: Dzmitry Sankouski, Sebastian Reichel, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Dmitry Torokhov, Pavel Machek,
Liam Girdwood, Mark Brown, Uwe Kleine-König, Konrad Dybcio,
Chanwoo Choi, phone-devel
Cc: linux-pm, linux-kernel, linux-arm-msm, linux-clk, dri-devel,
devicetree, linux-input, linux-leds, linux-pwm, linux-samsung-soc
In-Reply-To: <20240618-starqltechn_integration_upstream-v3-10-e3f6662017ac@gmail.com>
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> add samsung,s2dos05 regulator binding part
Make it a proper sentence.
>
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> ---
> .../bindings/regulator/samsung,s2dos05.yaml | 36 ++++++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 37 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/regulator/samsung,s2dos05.yaml b/Documentation/devicetree/bindings/regulator/samsung,s2dos05.yaml
> new file mode 100644
> index 000000000000..4b8e4389329c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/samsung,s2dos05.yaml
> @@ -0,0 +1,36 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/samsung,s2dos05.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung S2DOS05 Power Management IC Regulators
> +
> +maintainers:
> + - Dzmitry Sankouski <dsankouski@gmail.com>
> +
> +description:
> + This is a part of device tree bindings for S2M and S5M family of Power
> + Management IC (PMIC).
No, it's not.
> +
> + Has 4 LDO and 1 BUCK regulators, provides ELVDD, ELVSS, AVDD lines.
> +
> + See also Documentation/devicetree/bindings/mfd/samsung,s2dos05.yaml for
> + additional information and example.
> +
> +properties:
> + compatible:
> + const: samsung,s2dos05-regulator
Drop the compatible and then squash it in the MFD binding.
> +
> +patternProperties:
> + "^buck1|ldo[1-4]$":
You did not test the patches.
I will not continue with the review, because testing is a requirement.
Otherwise it is just a waste of my time.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 09/23] dt-bindings: mfd: add samsung,s2dos05
From: Krzysztof Kozlowski @ 2024-06-20 15:49 UTC (permalink / raw)
To: Dzmitry Sankouski, Sebastian Reichel, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Dmitry Torokhov, Pavel Machek,
Liam Girdwood, Mark Brown, Uwe Kleine-König, Konrad Dybcio,
Chanwoo Choi, phone-devel
Cc: linux-pm, linux-kernel, linux-arm-msm, linux-clk, dri-devel,
devicetree, linux-input, linux-leds, linux-pwm, linux-samsung-soc
In-Reply-To: <20240618-starqltechn_integration_upstream-v3-9-e3f6662017ac@gmail.com>
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> add samsung,s2dos05 core MFD module binding
>
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> ---
> .../devicetree/bindings/mfd/samsung,s2dos05.yaml | 89 ++++++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 90 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/samsung,s2dos05.yaml b/Documentation/devicetree/bindings/mfd/samsung,s2dos05.yaml
> new file mode 100644
> index 000000000000..f2ef5171cc40
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/samsung,s2dos05.yaml
> @@ -0,0 +1,89 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/samsung,s2dos05.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung S2DOS05 Power Management IC
> +
> +maintainers:
> + - Dzmitry Sankouski <dsankouski@gmail.com>
> +
> +description:
> + This is a part of device tree bindings for S2M and S5M family of Power
> + Management IC (PMIC).
No, it is not.
> +
> + The S2DOS05 is a companion power management IC for the panel and touchscreen
> + in smart phones. Provides voltage and current regulators and adc for power/current
> + measurements.
> +
> +properties:
> + compatible:
> + const: samsung,s2dos05-pmic
s2dos05 cannot be anything else than pmic. Drop the suffix.
> +
> + reg:
> + maxItems: 1
> +
> + regulators:
> + $ref: /schemas/regulator/samsung,s2dos05.yaml
> + description: List of regulators and its properties
> +
> +required:
> + - compatible
> + - reg
> + - regulators
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pmic@60 {
> + compatible = "samsung,s2dos05";
> + reg = <0x60>;
> +
> + regulators {
Messed indentation. Everywhere, each of your patch.
> + s2dos05_ldo1: s2dos05-ldo1 {
Just "ldo1" and drop unused labels.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 06/23] dt-bindings: power: supply: add maxim,max77705 charger
From: Krzysztof Kozlowski @ 2024-06-20 15:47 UTC (permalink / raw)
To: Dzmitry Sankouski, Sebastian Reichel, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Dmitry Torokhov, Pavel Machek,
Liam Girdwood, Mark Brown, Uwe Kleine-König, Konrad Dybcio,
Chanwoo Choi, phone-devel
Cc: linux-pm, linux-kernel, linux-arm-msm, linux-clk, dri-devel,
devicetree, linux-input, linux-leds, linux-pwm, linux-samsung-soc
In-Reply-To: <20240618-starqltechn_integration_upstream-v3-6-e3f6662017ac@gmail.com>
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> add maxim,max77705 charger binding part
Make it a proper sentence.
>
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> ---
> .../power/supply/maxim,max77705-charger.yaml | 30 ++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max77705-charger.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max77705-charger.yaml
> new file mode 100644
> index 000000000000..2b805da2a328
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/maxim,max77705-charger.yaml
> @@ -0,0 +1,30 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/maxim,max77705-charger.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX77705 and Companion Power Management IC charger
> +
> +maintainers:
> + - Dzmitry Sankouski <dsankouski@gmail.com>
> +
> +description: |
> + This is a part of device tree bindings for Maxim MAX77705 multi functional device.
> +
> + See also Documentation/devicetree/bindings/mfd/maxim,max77705.yaml for
> + additional information and example.
> +
> +allOf:
> + - $ref: power-supply.yaml#
> +
> +properties:
> + compatible:
> + const: maxim,max77705-charger
Looks pointless. Merge it to parent node.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 05/23] dt-bindings: input: add maxim,max77705-haptic
From: Krzysztof Kozlowski @ 2024-06-20 15:47 UTC (permalink / raw)
To: Dzmitry Sankouski, Sebastian Reichel, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Dmitry Torokhov, Pavel Machek,
Liam Girdwood, Mark Brown, Uwe Kleine-König, Konrad Dybcio,
Chanwoo Choi, phone-devel
Cc: linux-pm, linux-kernel, linux-arm-msm, linux-clk, dri-devel,
devicetree, linux-input, linux-leds, linux-pwm, linux-samsung-soc
In-Reply-To: <20240618-starqltechn_integration_upstream-v3-5-e3f6662017ac@gmail.com>
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> maxim,max77705-haptic is MAX77705 pmic binding part
>
> +
> +maintainers:
> + - Dzmitry Sankouski <dsankouski@gmail.com>
> +
> +description: |
> + This is a part of device tree bindings for Maxim MAX77705 multi functional device.
Wrap according to Linux coding style, so at 80. All your bindings have
the same issue.
> +
> + See also Documentation/devicetree/bindings/mfd/maxim,max77705.yaml for
> + additional information and example.
> +
> +properties:
> + compatible:
> + const: maxim,max77705-haptic
> +
> + haptic-supply: true
> +
> + pwms: true
maxItems
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 04/23] dt-bindings: mfd: add maxim,max77705
From: Krzysztof Kozlowski @ 2024-06-20 15:45 UTC (permalink / raw)
To: Dzmitry Sankouski, Sebastian Reichel, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Dmitry Torokhov, Pavel Machek,
Liam Girdwood, Mark Brown, Uwe Kleine-König, Konrad Dybcio,
Chanwoo Choi, phone-devel
Cc: linux-pm, linux-kernel, linux-arm-msm, linux-clk, dri-devel,
devicetree, linux-input, linux-leds, linux-pwm, linux-samsung-soc
In-Reply-To: <20240618-starqltechn_integration_upstream-v3-4-e3f6662017ac@gmail.com>
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> maxim,max77705 is MAX77705 pmic binding part
>
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> ---
> .../devicetree/bindings/mfd/maxim,max77705.yaml | 112 +++++++++++++++++++++
Your patch order is totally messed. Not tested by automation. Only
limited review follows.
> MAINTAINERS | 1 +
> 2 files changed, 113 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/maxim,max77705.yaml b/Documentation/devicetree/bindings/mfd/maxim,max77705.yaml
> new file mode 100644
> index 000000000000..b54408e3d792
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/maxim,max77705.yaml
> @@ -0,0 +1,112 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/maxim,max77705.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX77705 Companion Power Management IC and USB Type-C interface IC
> +
> +maintainers:
> + - Dzmitry Sankouski <dsankouski@gmail.com>
> +
> +description: |
> + This is a part of device tree bindings for Maxim MAX77705 multi functional device.
> +
> + The Maxim MAX77705 is a Companion Power Management and Type-C interface IC which
> + includes charger, fuelgauge, LED, haptic motor driver and Type-C management IC.
> +
> +properties:
> + compatible:
> + const: maxim,max77705
> +
> + reg:
> + description:
> + I2C device address.
Drop description, obvious.
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + interrupt-names:
> + description:
> + MAX77705 shared irq.
> + items:
> + - const: max77705_irq
Drop entire property, not really useful.
> +
> + charger:
> + $ref: /schemas/power/supply/maxim,max77705-charger.yaml
> +
> + fuelgauge:
> + $ref: /schemas/power/supply/maxim,max77705-fg.yaml
> +
> + haptic:
> + $ref: /schemas/input/maxim,max77705.yaml
> +
> + leds:
> + $ref: /schemas/leds/maxim,max77705.yaml
> +
> +
> +required:
> + - compatible
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/leds/common.h>
> +
> + i2c14 {
i2c
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + pmic@66 {
> + compatible = "maxim,max77705";
> + reg = <0x66>;
Totally messed indentation. Fix your code.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 03/23] dt-bindings: panel: add Samsung s6e3ha8
From: Krzysztof Kozlowski @ 2024-06-20 15:44 UTC (permalink / raw)
To: Dzmitry Sankouski, Sebastian Reichel, Bjorn Andersson,
Michael Turquette, Stephen Boyd, Neil Armstrong, Jessica Zhang,
Sam Ravnborg, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Daniel Vetter, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones, Dmitry Torokhov, Pavel Machek,
Liam Girdwood, Mark Brown, Uwe Kleine-König, Konrad Dybcio,
Chanwoo Choi, phone-devel
Cc: linux-pm, linux-kernel, linux-arm-msm, linux-clk, dri-devel,
devicetree, linux-input, linux-leds, linux-pwm, linux-samsung-soc
In-Reply-To: <20240618-starqltechn_integration_upstream-v3-3-e3f6662017ac@gmail.com>
On 18/06/2024 15:59, Dzmitry Sankouski wrote:
> Add binding for the Samsung s6e3ha8 panel found in the Samsung S9.
>
> Signed-off-by: Dzmitry Sankouski <dsankouski@gmail.com>
> ---
> .../bindings/display/panel/samsung,s6e3ha8.yaml | 76 ++++++++++++++++++++++
> MAINTAINERS | 5 ++
> 2 files changed, 81 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6e3ha8.yaml b/Documentation/devicetree/bindings/display/panel/samsung,s6e3ha8.yaml
> new file mode 100644
> index 000000000000..9d7d747264dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/samsung,s6e3ha8.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/samsung,s6e3ha8.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung s6e3ha8 AMOLED DSI panel
> +
> +description: The s6e3ha8 is a 1440x2960 DPI display panel from Samsung Mobile
> + Displays (SMD).
> +
> +maintainers:
> + - Dzmitry Sankouski <dsankouski@gmail.com>
> +
> +allOf:
> + - $ref: panel-common.yaml#
> +
> +properties:
> + compatible:
> + const: samsung,s6e3ha8
> +
> + reg:
> + maxItems: 1
> +
> + reset-gpios:
> + description: reset gpio
Pointless description. I think this can be reset-gpios: true, because
gpio-consumer-common provides constrain.
> +
> + port: true
> +
> + vdd3-supply:
> + description: VDD regulator
> +
> + vci-supply:
> + description: VCI regulator
> +
> + vddr-supply:
> + description: VDDR regulator
> +
> +required:
> + - compatible
> + - reset-gpios
> + - vdd3-supply
> + - vddr-supply
> + - vci-supply
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + dsi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + panel@0 {
> + compatible = "samsung,s6e3ha8";
> + reg = <0>;
Messed indentation. Keep consistent one.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 1/1] Input: ads7846 - Use spi_device_id table
From: Dmitry Torokhov @ 2024-06-20 15:42 UTC (permalink / raw)
To: Alexander Stein; +Cc: linux-input, linux-kernel
In-Reply-To: <20240619122703.2081476-1-alexander.stein@ew.tq-group.com>
On Wed, Jun 19, 2024 at 02:27:02PM +0200, Alexander Stein wrote:
> As the driver supports more devices over time the single MODULE_ALIAS
> is complete and raises several warnings:
> SPI driver ads7846 has no spi_device_id for ti,tsc2046
> SPI driver ads7846 has no spi_device_id for ti,ads7843
> SPI driver ads7846 has no spi_device_id for ti,ads7845
> SPI driver ads7846 has no spi_device_id for ti,ads7873
>
> Fix this by adding a spi_device_id table and removing the manual
> MODULE_ALIAS.
>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4 1/4] Input: atmel_mxt_ts - add power off and power on functions
From: Dmitry Torokhov @ 2024-06-20 15:37 UTC (permalink / raw)
To: Stefan Eichenberger
Cc: nick, robh, krzysztof.kozlowski+dt, conor+dt, nicolas.ferre,
alexandre.belloni, claudiu.beznea, linus.walleij, linux-input,
devicetree, linux-arm-kernel, linux-kernel, Stefan Eichenberger
In-Reply-To: <20240417090527.15357-2-eichest@gmail.com>
Hi Stefan,
On Wed, Apr 17, 2024 at 11:05:24AM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
>
> Add a separate function for power off and power on instead of calling
> regulator_bulk_enable and regulator_bulk_disable directly.
>
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 59 +++++++++++++++---------
> 1 file changed, 37 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 542a31448c8f..52867ce3b9b6 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1307,6 +1307,38 @@ static int mxt_soft_reset(struct mxt_data *data)
> return 0;
> }
>
> +static int mxt_power_on(struct mxt_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: %d\n",
> + error);
> + return error;
> + }
> +
> + msleep(MXT_BACKUP_TIME);
> +
> + if (data->reset_gpio) {
> + /* Wait a while and then de-assert the RESET GPIO line */
> + msleep(MXT_RESET_GPIO_TIME);
> + gpiod_set_value(data->reset_gpio, 0);
> + msleep(MXT_RESET_INVALID_CHG);
> + }
> +
> + return 0;
> +}
> +
> +static void mxt_power_off(struct mxt_data *data)
> +{
> + if (data->reset_gpio)
> + gpiod_set_value(data->reset_gpio, 1);
> +
> + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> +}
> +
> static void mxt_update_crc(struct mxt_data *data, u8 cmd, u8 value)
> {
> /*
> @@ -3305,25 +3337,9 @@ static int mxt_probe(struct i2c_client *client)
> return error;
> }
>
> - error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> - data->regulators);
> - if (error) {
> - dev_err(&client->dev, "failed to enable regulators: %d\n",
> - error);
> + error = mxt_power_on(data);
> + if (error)
> return error;
> - }
> - /*
> - * The device takes 40ms to come up after power-on according
> - * to the mXT224 datasheet, page 13.
> - */
> - msleep(MXT_BACKUP_TIME);
> -
> - if (data->reset_gpio) {
> - /* Wait a while and then de-assert the RESET GPIO line */
> - msleep(MXT_RESET_GPIO_TIME);
> - gpiod_set_value(data->reset_gpio, 0);
> - msleep(MXT_RESET_INVALID_CHG);
> - }
>
> /*
> * Controllers like mXT1386 have a dedicated WAKE line that could be
> @@ -3361,8 +3377,8 @@ static int mxt_probe(struct i2c_client *client)
> mxt_free_input_device(data);
> mxt_free_object_table(data);
> err_disable_regulators:
> - regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> - data->regulators);
> + mxt_power_off(data);
> +
> return error;
> }
>
> @@ -3374,8 +3390,7 @@ static void mxt_remove(struct i2c_client *client)
> sysfs_remove_group(&client->dev.kobj, &mxt_attr_group);
> mxt_free_input_device(data);
> mxt_free_object_table(data);
> - regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> - data->regulators);
> + mxt_power_off(data);
This change means that on unbind we will leave with GPIO line asserted.
Won't this potentially cause some current leakage? Why do we need to
have reset asserted here?
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH v3 0/5] himax_hx83112b: add support for HX83100A
From: Felix Kaechele @ 2024-06-20 14:50 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
Original discussion around v2 here: https://lore.kernel.org/all/20240511121245.109644-1-felix@kaechele.ca/
Changes from v2:
- reworded dt-bindings commit message to specifiy how the HX83100A is different
from other chips in this series to justify having its own compatible string
- added Acked-By from Conor that was given in the v2 thread to commit message
I've done some more testing in the meantime and haven't found any issues.
I haven't heard back from anyone with an HX83112B, but the HX83100A works as
intended with these changes and I assume the HX83112B will too.
Thanks,
Felix
Felix Kaechele (5):
dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A
input: himax_hx83112b: use more descriptive register defines
input: himax_hx83112b: implement MCU register reading
input: himax_hx83112b: add himax_chip struct for multi-chip support
input: himax_hx83112b: add support for HX83100A
.../input/touchscreen/himax,hx83112b.yaml | 1 +
drivers/input/touchscreen/himax_hx83112b.c | 135 ++++++++++++++----
2 files changed, 110 insertions(+), 26 deletions(-)
base-commit: 9b9247397e2e20016031e59f76dae563b79b6ee2
--
2.45.2
^ permalink raw reply
* [PATCH v3 2/5] input: himax_hx83112b: use more descriptive register defines
From: Felix Kaechele @ 2024-06-20 14:50 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <20240620145019.156187-1-felix@kaechele.ca>
Himax uses an AHB-style bus to communicate with different parts of the
display driver and touch controller system.
Use more descriptive names for the register and address defines.
The names were taken from a driver submission for the similar HX83102J
chip.
Signed-off-by: Felix Kaechele <felix@kaechele.ca>
Link: https://lore.kernel.org/all/TY0PR06MB561105A3386E9D76F429110D9E0F2@TY0PR06MB5611.apcprd06.prod.outlook.com/
---
drivers/input/touchscreen/himax_hx83112b.c | 23 ++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index bafabd06dabc..d6c4a68eac23 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -24,12 +24,14 @@
#define HIMAX_MAX_POINTS 10
-#define HIMAX_REG_CFG_SET_ADDR 0x00
-#define HIMAX_REG_CFG_INIT_READ 0x0c
-#define HIMAX_REG_CFG_READ_VALUE 0x08
-#define HIMAX_REG_READ_EVENT 0x30
+#define HIMAX_AHB_ADDR_BYTE_0 0x00
+#define HIMAX_AHB_ADDR_RDATA_BYTE_0 0x08
+#define HIMAX_AHB_ADDR_ACCESS_DIRECTION 0x0c
+#define HIMAX_AHB_ADDR_EVENT_STACK 0x30
-#define HIMAX_CFG_PRODUCT_ID 0x900000d0
+#define HIMAX_AHB_CMD_ACCESS_DIRECTION_READ 0x00
+
+#define HIMAX_REG_ADDR_ICID 0x900000d0
#define HIMAX_INVALID_COORD 0xffff
@@ -67,15 +69,16 @@ static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
{
int error;
- error = regmap_write(ts->regmap, HIMAX_REG_CFG_SET_ADDR, address);
+ error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_BYTE_0, address);
if (error)
return error;
- error = regmap_write(ts->regmap, HIMAX_REG_CFG_INIT_READ, 0x0);
+ error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_ACCESS_DIRECTION,
+ HIMAX_AHB_CMD_ACCESS_DIRECTION_READ);
if (error)
return error;
- error = regmap_read(ts->regmap, HIMAX_REG_CFG_READ_VALUE, dst);
+ error = regmap_read(ts->regmap, HIMAX_AHB_ADDR_RDATA_BYTE_0, dst);
if (error)
return error;
@@ -101,7 +104,7 @@ static int himax_read_product_id(struct himax_ts_data *ts, u32 *product_id)
{
int error;
- error = himax_read_config(ts, HIMAX_CFG_PRODUCT_ID, product_id);
+ error = himax_read_config(ts, HIMAX_REG_ADDR_ICID, product_id);
if (error)
return error;
@@ -235,7 +238,7 @@ static int himax_handle_input(struct himax_ts_data *ts)
int error;
struct himax_event event;
- error = regmap_raw_read(ts->regmap, HIMAX_REG_READ_EVENT, &event,
+ error = regmap_raw_read(ts->regmap, HIMAX_AHB_ADDR_EVENT_STACK, &event,
sizeof(event));
if (error) {
dev_err(&ts->client->dev, "Failed to read input event: %d\n",
--
2.45.2
^ permalink raw reply related
* [PATCH v3 3/5] input: himax_hx83112b: implement MCU register reading
From: Felix Kaechele @ 2024-06-20 14:50 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <20240620145019.156187-1-felix@kaechele.ca>
Implement reading from the MCU in a more universal fashion. This allows
properly handling reads of more than 4 bytes using the AHB FIFO
implemented in the chip.
Signed-off-by: Felix Kaechele <felix@kaechele.ca>
---
drivers/input/touchscreen/himax_hx83112b.c | 50 ++++++++++++++++++++--
1 file changed, 47 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index d6c4a68eac23..c9cc69bf242a 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -27,9 +27,13 @@
#define HIMAX_AHB_ADDR_BYTE_0 0x00
#define HIMAX_AHB_ADDR_RDATA_BYTE_0 0x08
#define HIMAX_AHB_ADDR_ACCESS_DIRECTION 0x0c
+#define HIMAX_AHB_ADDR_INCR4 0x0d
+#define HIMAX_AHB_ADDR_CONTI 0x13
#define HIMAX_AHB_ADDR_EVENT_STACK 0x30
#define HIMAX_AHB_CMD_ACCESS_DIRECTION_READ 0x00
+#define HIMAX_AHB_CMD_INCR4 0x10
+#define HIMAX_AHB_CMD_CONTI 0x31
#define HIMAX_REG_ADDR_ICID 0x900000d0
@@ -65,10 +69,34 @@ static const struct regmap_config himax_regmap_config = {
.val_format_endian = REGMAP_ENDIAN_LITTLE,
};
-static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
+static int himax_bus_enable_burst(struct himax_ts_data *ts)
{
int error;
+ error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_CONTI,
+ HIMAX_AHB_CMD_CONTI);
+ if (error)
+ return error;
+
+ error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_INCR4,
+ HIMAX_AHB_CMD_INCR4);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static int himax_bus_read(struct himax_ts_data *ts, u32 address, void *dst,
+ size_t length)
+{
+ int error;
+
+ if (length > 4) {
+ error = himax_bus_enable_burst(ts);
+ if (error)
+ return error;
+ }
+
error = regmap_write(ts->regmap, HIMAX_AHB_ADDR_BYTE_0, address);
if (error)
return error;
@@ -78,7 +106,23 @@ static int himax_read_config(struct himax_ts_data *ts, u32 address, u32 *dst)
if (error)
return error;
- error = regmap_read(ts->regmap, HIMAX_AHB_ADDR_RDATA_BYTE_0, dst);
+ if (length > 4)
+ error = regmap_noinc_read(ts->regmap, HIMAX_AHB_ADDR_RDATA_BYTE_0,
+ dst, length);
+ else
+ error = regmap_read(ts->regmap, HIMAX_AHB_ADDR_RDATA_BYTE_0,
+ dst);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static int himax_read_mcu(struct himax_ts_data *ts, u32 address, u32 *dst)
+{
+ int error;
+
+ error = himax_bus_read(ts, address, dst, sizeof(dst));
if (error)
return error;
@@ -104,7 +148,7 @@ static int himax_read_product_id(struct himax_ts_data *ts, u32 *product_id)
{
int error;
- error = himax_read_config(ts, HIMAX_REG_ADDR_ICID, product_id);
+ error = himax_read_mcu(ts, HIMAX_REG_ADDR_ICID, product_id);
if (error)
return error;
--
2.45.2
^ permalink raw reply related
* [PATCH v3 1/5] dt-bindings: input: touchscreen: himax,hx83112b: add HX83100A
From: Felix Kaechele @ 2024-06-20 14:50 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
Cc: Conor Dooley
In-Reply-To: <20240620145019.156187-1-felix@kaechele.ca>
Add a compatible string for the Himax HX83100A touch controller.
The HX83100A presents touch events on the internal bus rather than
offering a dedicated event register like the other chips in this family
do.
Signed-off-by: Felix Kaechele <felix@kaechele.ca>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
.../devicetree/bindings/input/touchscreen/himax,hx83112b.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
index f42b23d532eb..f5cfacb5e966 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
@@ -15,6 +15,7 @@ allOf:
properties:
compatible:
enum:
+ - himax,hx83100a
- himax,hx83112b
reg:
--
2.45.2
^ permalink raw reply related
* [PATCH v3 5/5] input: himax_hx83112b: add support for HX83100A
From: Felix Kaechele @ 2024-06-20 14:50 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
Cc: Paul Gale
In-Reply-To: <20240620145019.156187-1-felix@kaechele.ca>
The HX83100A is a bit of an outlier in the Himax HX831xxx series of
touch controllers as it requires reading touch events through the AHB
interface of the MCU rather than providing a dedicated FIFO address like
the other chips do.
This patch implements the specific read function and introduces the
HX83100A chip with an appropriate i2c ID and DT compatible string.
The HX83100A doesn't have a straightforward way to do chip
identification, which is why it is not implemented in this patch.
Tested on: Lenovo ThinkSmart View (CD-18781Y) / Innolux P080DDD-AB2 LCM
Signed-off-by: Felix Kaechele <felix@kaechele.ca>
Tested-by: Paul Gale <paul@siliconpixel.com>
---
drivers/input/touchscreen/himax_hx83112b.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index 5092a357c332..9ed3bccde4ac 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -4,6 +4,9 @@
*
* Copyright (C) 2022 Job Noorman <job@noorman.info>
*
+ * HX83100A support
+ * Copyright (C) 2024 Felix Kaechele <felix@kaechele.ca>
+ *
* This code is based on "Himax Android Driver Sample Code for QCT platform":
*
* Copyright (C) 2017 Himax Corporation.
@@ -35,6 +38,8 @@
#define HIMAX_REG_ADDR_ICID 0x900000d0
+#define HX83100A_REG_FW_EVENT_STACK 0x90060000
+
#define HIMAX_INVALID_COORD 0xffff
struct himax_event_point {
@@ -288,6 +293,12 @@ static int himax_read_events(struct himax_ts_data *ts,
length);
}
+static int hx83100a_read_events(struct himax_ts_data *ts,
+ struct himax_event *event, size_t length)
+{
+ return himax_bus_read(ts, HX83100A_REG_FW_EVENT_STACK, event, length);
+};
+
static int himax_handle_input(struct himax_ts_data *ts)
{
int error;
@@ -394,6 +405,10 @@ static int himax_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(himax_pm_ops, himax_suspend, himax_resume);
+static const struct himax_chip hx83100a_chip = {
+ .read_events = hx83100a_read_events,
+};
+
static const struct himax_chip hx83112b_chip = {
.id = 0x83112b,
.check_id = himax_check_product_id,
@@ -401,6 +416,7 @@ static const struct himax_chip hx83112b_chip = {
};
static const struct i2c_device_id himax_ts_id[] = {
+ { "hx83100a", (kernel_ulong_t)&hx83100a_chip },
{ "hx83112b", (kernel_ulong_t)&hx83112b_chip },
{ /* sentinel */ }
};
@@ -408,6 +424,7 @@ MODULE_DEVICE_TABLE(i2c, himax_ts_id);
#ifdef CONFIG_OF
static const struct of_device_id himax_of_match[] = {
+ { .compatible = "himax,hx83100a", .data = &hx83100a_chip },
{ .compatible = "himax,hx83112b", .data = &hx83112b_chip },
{ /* sentinel */ }
};
--
2.45.2
^ permalink raw reply related
* [PATCH v3 4/5] input: himax_hx83112b: add himax_chip struct for multi-chip support
From: Felix Kaechele @ 2024-06-20 14:50 UTC (permalink / raw)
To: Job Noorman, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-input, devicetree, linux-kernel
In-Reply-To: <20240620145019.156187-1-felix@kaechele.ca>
In preparation for HX83100A support allow defining separate functions
for specific chip operations.
Signed-off-by: Felix Kaechele <felix@kaechele.ca>
---
drivers/input/touchscreen/himax_hx83112b.c | 51 +++++++++++++++-------
1 file changed, 35 insertions(+), 16 deletions(-)
diff --git a/drivers/input/touchscreen/himax_hx83112b.c b/drivers/input/touchscreen/himax_hx83112b.c
index c9cc69bf242a..5092a357c332 100644
--- a/drivers/input/touchscreen/himax_hx83112b.c
+++ b/drivers/input/touchscreen/himax_hx83112b.c
@@ -20,8 +20,6 @@
#include <linux/kernel.h>
#include <linux/regmap.h>
-#define HIMAX_ID_83112B 0x83112b
-
#define HIMAX_MAX_POINTS 10
#define HIMAX_AHB_ADDR_BYTE_0 0x00
@@ -55,7 +53,16 @@ struct himax_event {
static_assert(sizeof(struct himax_event) == 56);
+struct himax_ts_data;
+struct himax_chip {
+ u32 id;
+ int (*check_id)(struct himax_ts_data *ts);
+ int (*read_events)(struct himax_ts_data *ts, struct himax_event *event,
+ size_t length);
+};
+
struct himax_ts_data {
+ const struct himax_chip *chip;
struct gpio_desc *gpiod_rst;
struct input_dev *input_dev;
struct i2c_client *client;
@@ -167,15 +174,12 @@ static int himax_check_product_id(struct himax_ts_data *ts)
dev_dbg(&ts->client->dev, "Product id: %x\n", product_id);
- switch (product_id) {
- case HIMAX_ID_83112B:
+ if (product_id == ts->chip->id)
return 0;
- default:
- dev_err(&ts->client->dev,
- "Unknown product id: %x\n", product_id);
- return -EINVAL;
- }
+ dev_err(&ts->client->dev, "Unknown product id: %x\n",
+ product_id);
+ return -EINVAL;
}
static int himax_input_register(struct himax_ts_data *ts)
@@ -277,13 +281,19 @@ static bool himax_verify_checksum(struct himax_ts_data *ts,
return true;
}
+static int himax_read_events(struct himax_ts_data *ts,
+ struct himax_event *event, size_t length)
+{
+ return regmap_raw_read(ts->regmap, HIMAX_AHB_ADDR_EVENT_STACK, event,
+ length);
+}
+
static int himax_handle_input(struct himax_ts_data *ts)
{
int error;
struct himax_event event;
- error = regmap_raw_read(ts->regmap, HIMAX_AHB_ADDR_EVENT_STACK, &event,
- sizeof(event));
+ error = ts->chip->read_events(ts, &event, sizeof(event));
if (error) {
dev_err(&ts->client->dev, "Failed to read input event: %d\n",
error);
@@ -329,6 +339,7 @@ static int himax_probe(struct i2c_client *client)
i2c_set_clientdata(client, ts);
ts->client = client;
+ ts->chip = i2c_get_match_data(client);
ts->regmap = devm_regmap_init_i2c(client, &himax_regmap_config);
error = PTR_ERR_OR_ZERO(ts->regmap);
@@ -346,9 +357,11 @@ static int himax_probe(struct i2c_client *client)
himax_reset(ts);
- error = himax_check_product_id(ts);
- if (error)
- return error;
+ if (ts->chip->check_id) {
+ error = himax_check_product_id(ts);
+ if (error)
+ return error;
+ }
error = himax_input_register(ts);
if (error)
@@ -381,15 +394,21 @@ static int himax_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(himax_pm_ops, himax_suspend, himax_resume);
+static const struct himax_chip hx83112b_chip = {
+ .id = 0x83112b,
+ .check_id = himax_check_product_id,
+ .read_events = himax_read_events,
+};
+
static const struct i2c_device_id himax_ts_id[] = {
- { "hx83112b" },
+ { "hx83112b", (kernel_ulong_t)&hx83112b_chip },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(i2c, himax_ts_id);
#ifdef CONFIG_OF
static const struct of_device_id himax_of_match[] = {
- { .compatible = "himax,hx83112b" },
+ { .compatible = "himax,hx83112b", .data = &hx83112b_chip },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, himax_of_match);
--
2.45.2
^ permalink raw reply related
* Re: [bug report] HID: bpf: remove tracing HID-BPF capability
From: Benjamin Tissoires @ 2024-06-20 11:01 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-input
In-Reply-To: <00f7b624-219f-4a05-a7ad-5335f15a41c7@moroto.mountain>
Hi,
On Jun 20 2024, Dan Carpenter wrote:
> Hello Benjamin Tissoires,
>
> Commit 4a86220e046d ("HID: bpf: remove tracing HID-BPF capability")
> from Jun 8, 2024 (linux-next), leads to the following Smatch static
> checker warning:
>
> drivers/hid/bpf/hid_bpf_dispatch.c:65 dispatch_hid_bpf_device_event()
> error: uninitialized symbol 'ret'.
>
> drivers/hid/bpf/hid_bpf_dispatch.c
> 26 dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type, u8 *data,
> 27 u32 *size, int interrupt)
> 28 {
> 29 struct hid_bpf_ctx_kern ctx_kern = {
> 30 .ctx = {
> 31 .hid = hdev,
> 32 .allocated_size = hdev->bpf.allocated_data,
> 33 .size = *size,
> 34 },
> 35 .data = hdev->bpf.device_data,
> 36 };
> 37 struct hid_bpf_ops *e;
> 38 int ret;
> 39
> 40 if (type >= HID_REPORT_TYPES)
> 41 return ERR_PTR(-EINVAL);
> 42
> 43 /* no program has been attached yet */
> 44 if (!hdev->bpf.device_data)
> 45 return data;
> 46
> 47 memset(ctx_kern.data, 0, hdev->bpf.allocated_data);
> 48 memcpy(ctx_kern.data, data, *size);
> 49
> 50 rcu_read_lock();
> 51 list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
> 52 if (e->hid_device_event) {
> 53 ret = e->hid_device_event(&ctx_kern.ctx, type);
> 54 if (ret < 0) {
> 55 rcu_read_unlock();
> 56 return ERR_PTR(ret);
> 57 }
> 58
> 59 if (ret)
> 60 ctx_kern.ctx.retval = ret;
> 61 }
>
> Can all the e->hid_device_event pointers be NULL?
They can, but then hdev->bpf.device_data will also set to NULL in 99% of
the normal cases. The only case where device_data is set to a value and
hid_device_event is never set anywhere is after the last bpf filter has
been removed.
>
> 62 }
> 63 rcu_read_unlock();
> 64
> 65 if (ret) {
> ^^^
> If so then ret is uninitialized
Yep, good point. Let me fix that in my followup series.
>
> 66 if (ret > ctx_kern.ctx.allocated_size)
> 67 return ERR_PTR(-EINVAL);
> 68
> 69 *size = ret;
>
>
> regards,
> dan carpenter
Thanks!
Cheers,
Benjamin
^ permalink raw reply
* [bug report] HID: bpf: remove tracing HID-BPF capability
From: Dan Carpenter @ 2024-06-20 8:50 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input
Hello Benjamin Tissoires,
Commit 4a86220e046d ("HID: bpf: remove tracing HID-BPF capability")
from Jun 8, 2024 (linux-next), leads to the following Smatch static
checker warning:
drivers/hid/bpf/hid_bpf_dispatch.c:65 dispatch_hid_bpf_device_event()
error: uninitialized symbol 'ret'.
drivers/hid/bpf/hid_bpf_dispatch.c
26 dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type, u8 *data,
27 u32 *size, int interrupt)
28 {
29 struct hid_bpf_ctx_kern ctx_kern = {
30 .ctx = {
31 .hid = hdev,
32 .allocated_size = hdev->bpf.allocated_data,
33 .size = *size,
34 },
35 .data = hdev->bpf.device_data,
36 };
37 struct hid_bpf_ops *e;
38 int ret;
39
40 if (type >= HID_REPORT_TYPES)
41 return ERR_PTR(-EINVAL);
42
43 /* no program has been attached yet */
44 if (!hdev->bpf.device_data)
45 return data;
46
47 memset(ctx_kern.data, 0, hdev->bpf.allocated_data);
48 memcpy(ctx_kern.data, data, *size);
49
50 rcu_read_lock();
51 list_for_each_entry_rcu(e, &hdev->bpf.prog_list, list) {
52 if (e->hid_device_event) {
53 ret = e->hid_device_event(&ctx_kern.ctx, type);
54 if (ret < 0) {
55 rcu_read_unlock();
56 return ERR_PTR(ret);
57 }
58
59 if (ret)
60 ctx_kern.ctx.retval = ret;
61 }
Can all the e->hid_device_event pointers be NULL?
62 }
63 rcu_read_unlock();
64
65 if (ret) {
^^^
If so then ret is uninitialized
66 if (ret > ctx_kern.ctx.allocated_size)
67 return ERR_PTR(-EINVAL);
68
69 *size = ret;
regards,
dan carpenter
^ permalink raw reply
* [hid:for-next 1/2] include/linux/platform_data/x86/asus-wmi.h:174:64: warning: missing braces around initializer
From: kernel test robot @ 2024-06-20 8:37 UTC (permalink / raw)
To: Luke D. Jones; +Cc: oe-kbuild-all, linux-input, Jiri Kosina
tree: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
head: fa9057ccb9c323816e0f2599a2d91f5c856b0004
commit: bda38f9a41d89b47dce54932f88d0e0edee00cae [1/2] HID: asus: use hid for brightness control on keyboard
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20240620/202406201647.E2BChvPJ-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240620/202406201647.E2BChvPJ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406201647.E2BChvPJ-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/hid/hid-asus.c:29:
>> include/linux/platform_data/x86/asus-wmi.h:174:64: warning: missing braces around initializer [-Wmissing-braces]
174 | static const struct dmi_system_id asus_use_hid_led_dmi_ids[] = {
| ^
vim +174 include/linux/platform_data/x86/asus-wmi.h
173
> 174 static const struct dmi_system_id asus_use_hid_led_dmi_ids[] = {
175 {
176 .matches = {
177 DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Zephyrus"),
178 },
179 },
180 {
181 .matches = {
182 DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Strix"),
183 },
184 },
185 {
186 .matches = {
187 DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Flow"),
188 },
189 },
190 {
191 .matches = {
192 DMI_MATCH(DMI_BOARD_NAME, "GA403"),
193 },
194 },
195 {
196 .matches = {
197 DMI_MATCH(DMI_BOARD_NAME, "GU605"),
198 },
199 },
200 {
201 .matches = {
202 DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
203 },
204 },
205 NULL,
206 };
207
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* [hid:for-next 1/2] include/linux/platform_data/x86/asus-wmi.h:205:2: warning: suggest braces around initialization of subobject
From: kernel test robot @ 2024-06-20 6:17 UTC (permalink / raw)
To: Luke D. Jones; +Cc: llvm, oe-kbuild-all, linux-input, Jiri Kosina
tree: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
head: fa9057ccb9c323816e0f2599a2d91f5c856b0004
commit: bda38f9a41d89b47dce54932f88d0e0edee00cae [1/2] HID: asus: use hid for brightness control on keyboard
config: i386-buildonly-randconfig-006-20240620 (https://download.01.org/0day-ci/archive/20240620/202406201404.NGl8SOi4-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240620/202406201404.NGl8SOi4-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406201404.NGl8SOi4-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/hid/hid-asus.c:29:
>> include/linux/platform_data/x86/asus-wmi.h:205:2: warning: suggest braces around initialization of subobject [-Wmissing-braces]
205 | NULL,
| ^~~~
| { }
include/linux/stddef.h:8:14: note: expanded from macro 'NULL'
8 | #define NULL ((void *)0)
| ^~~~~~~~~~~
1 warning generated.
vim +205 include/linux/platform_data/x86/asus-wmi.h
173
174 static const struct dmi_system_id asus_use_hid_led_dmi_ids[] = {
175 {
176 .matches = {
177 DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Zephyrus"),
178 },
179 },
180 {
181 .matches = {
182 DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Strix"),
183 },
184 },
185 {
186 .matches = {
187 DMI_MATCH(DMI_PRODUCT_FAMILY, "ROG Flow"),
188 },
189 },
190 {
191 .matches = {
192 DMI_MATCH(DMI_BOARD_NAME, "GA403"),
193 },
194 },
195 {
196 .matches = {
197 DMI_MATCH(DMI_BOARD_NAME, "GU605"),
198 },
199 },
200 {
201 .matches = {
202 DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
203 },
204 },
> 205 NULL,
206 };
207
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v4 4/4] Input: atmel_mxt_ts - add support for poweroff-sleep
From: Dmitry Torokhov @ 2024-06-20 1:12 UTC (permalink / raw)
To: Stefan Eichenberger
Cc: nick, robh, krzysztof.kozlowski+dt, conor+dt, nicolas.ferre,
alexandre.belloni, claudiu.beznea, linus.walleij, linux-input,
devicetree, linux-arm-kernel, linux-kernel, Stefan Eichenberger
In-Reply-To: <20240417090527.15357-5-eichest@gmail.com>
Hi Stefan,
On Wed, Apr 17, 2024 at 11:05:27AM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
>
> Add support for poweroff-sleep to the Atmel maXTouch driver. This allows
> us to power off the input device entirely and only power it on when it
> is opened. This will also automatically power it off when we suspend the
> system.
>
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
> drivers/input/touchscreen/atmel_mxt_ts.c | 71 +++++++++++++++++++-----
> 1 file changed, 57 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 7c807d1f1f9b..f92808be3f5b 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -317,6 +317,7 @@ struct mxt_data {
> struct gpio_desc *reset_gpio;
> struct gpio_desc *wake_gpio;
> bool use_retrigen_workaround;
> + bool poweroff_sleep;
Why is this separate from "enum mxt_suspend_mode suspend_mode"? Can we
make MXT_SUSPEND_POWEROFF and use it in mxt_start() and others? It still
can be driven by the "atmel,poweroff-sleep" device property.
>
> /* Cached parameters from object table */
> u16 T5_address;
> @@ -2277,6 +2278,19 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
> release_firmware(cfg);
> }
>
> +static int mxt_initialize_after_resume(struct mxt_data *data)
> +{
> + const struct firmware *fw;
> +
> + mxt_acquire_irq(data);
> +
> + firmware_request_nowarn(&fw, MXT_CFG_NAME, &data->client->dev);
> +
> + mxt_config_cb(fw, data);
Is this really required? As far as I know all maXTouch controllers have
NVRAM for their configs and should not lose configuration even if power
is cut off. In fact, the whole automatic request of firmware/config upon
probe I think was a mistake and I would like to get rid of it. In fact,
on Chrome OS the version of the driver in use does not do that and
instead relies on userspace to check if firmware update is needed.
If this is actually required you need to add error handling.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] Input: ims-pcu - annotate struct ims_pcu_flash_fmt with __counted_by
From: Gustavo A. R. Silva @ 2024-06-19 21:26 UTC (permalink / raw)
To: Javier Carrasco, Dmitry Torokhov, Kees Cook, Gustavo A. R. Silva
Cc: linux-input, linux-kernel, linux-hardening
In-Reply-To: <20240619-ims-pcu-counted_by-v1-1-3ee0ead2e57d@gmail.com>
On 19/06/24 23:13, Javier Carrasco wrote:
> Use the __counted_by compiler attribute for the data[] flexible array
> member to improve the results of array bound sanitizers.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks
--
Gustavo
> ---
> drivers/input/misc/ims-pcu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 408a586f8c36..91f8ad826238 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -761,7 +761,7 @@ static int ims_pcu_switch_to_bootloader(struct ims_pcu *pcu)
> struct ims_pcu_flash_fmt {
> __le32 addr;
> u8 len;
> - u8 data[];
> + u8 data[] __counted_by(len);
> };
>
> static unsigned int ims_pcu_count_fw_records(const struct firmware *fw)
>
^ 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