* [PATCH 0/2] Add Awinic AW9523(B) I2C GPIO Expander driver
@ 2021-01-09 14:02 AngeloGioacchino Del Regno
2021-01-09 14:02 ` [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander AngeloGioacchino Del Regno
2021-01-09 14:02 ` [PATCH 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B AngeloGioacchino Del Regno
0 siblings, 2 replies; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-09 14:02 UTC (permalink / raw)
To: linux-gpio
Cc: konrad.dybcio, marijn.suijten, martin.botka, phone-devel,
linux-kernel, devicetree, robh+dt, linus.walleij,
AngeloGioacchino Del Regno
This adds support for the Awinic AW9523/AW9523B I2C GPIO Expander, as
found in the F(x)Tec Pro1 smartphone (there, it's used to drive a
keyboard matrix).
This driver was tested on the aforementioned smartphone.
AngeloGioacchino Del Regno (2):
pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B
.../pinctrl/awinic,aw9523-pinctrl.yaml | 111 ++
drivers/pinctrl/Kconfig | 17 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-aw9523.c | 1070 +++++++++++++++++
4 files changed, 1199 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
create mode 100644 drivers/pinctrl/pinctrl-aw9523.c
--
2.29.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-09 14:02 [PATCH 0/2] Add Awinic AW9523(B) I2C GPIO Expander driver AngeloGioacchino Del Regno
@ 2021-01-09 14:02 ` AngeloGioacchino Del Regno
2021-01-09 17:24 ` kernel test robot
2021-01-09 22:11 ` Linus Walleij
2021-01-09 14:02 ` [PATCH 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B AngeloGioacchino Del Regno
1 sibling, 2 replies; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-09 14:02 UTC (permalink / raw)
To: linux-gpio
Cc: konrad.dybcio, marijn.suijten, martin.botka, phone-devel,
linux-kernel, devicetree, robh+dt, linus.walleij,
AngeloGioacchino Del Regno
The Awinic AW9523(B) is a multi-function I2C gpio expander in a
TQFN-24L package, featuring PWM (max 37mA per pin, or total max
power 3.2Watts) for LED driving capability.
It has two ports with 8 pins per port (for a total of 16 pins),
configurable as either PWM with 1/256 stepping or GPIO input/output,
1.8V logic input; each GPIO can be configured as input or output
independently from each other.
This IC also has an internal interrupt controller, which is capable
of generating an interrupt for each GPIO, depending on the
configuration, and will raise an interrupt on the INTN pin to
advertise this to an external interrupt controller.
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
drivers/pinctrl/Kconfig | 17 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-aw9523.c | 1070 ++++++++++++++++++++++++++++++
3 files changed, 1088 insertions(+)
create mode 100644 drivers/pinctrl/pinctrl-aw9523.c
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 815095326e2d..7d91e75576f1 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -110,6 +110,23 @@ config PINCTRL_AMD
Requires ACPI/FDT device enumeration code to set up a platform
device.
+config PINCTRL_AW9523
+ bool "Awinic AW9523/AW9523B I2C GPIO expander pinctrl driver"
+ depends on OF && I2C
+ select PINMUX
+ select PINCONF
+ select GENERIC_PINCONF
+ select GPIOLIB
+ select GPIOLIB_IRQCHIP
+ select REGMAP
+ help
+ The Awinic AW9523/AW9523B is a multi-function I2C GPIO
+ expander with PWM functionality. This driver bundles a
+ pinctrl driver to select the function muxing and a GPIO
+ driver to handle GPIO, when the GPIO function is selected.
+
+ Say yes to enable pinctrl and GPIO support for the AW9523(B).
+
config PINCTRL_BM1880
bool "Bitmain BM1880 Pinctrl driver"
depends on OF && (ARCH_BITMAIN || COMPILE_TEST)
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index f53933b2ff02..5696cd6593d5 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_PINCTRL_AXP209) += pinctrl-axp209.o
obj-$(CONFIG_PINCTRL_AT91) += pinctrl-at91.o
obj-$(CONFIG_PINCTRL_AT91PIO4) += pinctrl-at91-pio4.o
obj-$(CONFIG_PINCTRL_AMD) += pinctrl-amd.o
+obj-$(CONFIG_PINCTRL_AW9523) += pinctrl-aw9523.o
obj-$(CONFIG_PINCTRL_BM1880) += pinctrl-bm1880.o
obj-$(CONFIG_PINCTRL_DA850_PUPD) += pinctrl-da850-pupd.o
obj-$(CONFIG_PINCTRL_DA9062) += pinctrl-da9062.o
diff --git a/drivers/pinctrl/pinctrl-aw9523.c b/drivers/pinctrl/pinctrl-aw9523.c
new file mode 100644
index 000000000000..a6004c2a3f8e
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-aw9523.c
@@ -0,0 +1,1070 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Awinic AW9523B i2c pin controller driver
+ * Copyright (c) 2020, AngeloGioacchino Del Regno
+ * <angelogioacchino.delregno@somainline.org>
+ */
+
+#include <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/regulator/consumer.h>
+
+#include "core.h"
+#include "pinconf.h"
+#include "pinctrl-utils.h"
+
+#define AW9523_MAX_FUNCS 2
+#define AW9523_NUM_PORTS 2
+#define AW9523_PINS_PER_PORT 8
+
+/*
+ * HW needs at least 20uS for reset and at least 1-2uS to recover from
+ * reset, but we have to account for eventual board quirks, if any:
+ * for this reason, keep reset asserted for 50uS and wait for 20uS
+ * to recover from the reset.
+ */
+#define AW9523_HW_RESET_US 50
+#define AW9523_HW_RESET_RECOVERY_US 20
+
+/* Port 0: P0_0...P0_7 - Port 1: P1_0...P1_7 */
+#define AW9523_PIN_TO_PORT(pin) (pin >> 3)
+#define AW9523_REG_IN_STATE(pin) (0x00 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_OUT_STATE(pin) (0x02 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_CONF_STATE(pin) (0x04 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_INTR_DIS(pin) (0x06 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_CHIPID 0x10
+#define AW9523_VAL_EXPECTED_CHIPID 0x23
+
+#define AW9523_REG_GCR 0x11
+#define AW9523_GCR_ISEL_MASK GENMASK(0, 1)
+#define AW9523_GCR_GPOMD_MASK BIT(4)
+
+#define AW9523_REG_PORT_MODE(pin) (0x12 + AW9523_PIN_TO_PORT(pin))
+#define AW9523_REG_SOFT_RESET 0x7f
+#define AW9523_VAL_RESET 0x00
+
+/*
+ * struct aw9523_irq - Interrupt controller structure
+ * @lock: mutex locking for the irq bus
+ * @irqchip: structure holding irqchip params
+ * @old_masked: stores the previous interrupt mask for bit comparison
+ * @masked: stores the interrupt mask to program to the HW
+ * @cached_gpio: stores the previous gpio status for bit comparison
+ */
+struct aw9523_irq {
+ struct mutex lock;
+ struct irq_chip *irqchip;
+ DECLARE_BITMAP(old_masked[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT);
+ DECLARE_BITMAP(masked[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT);
+ u16 cached_gpio;
+};
+
+/*
+ * struct aw9523_pinmux - Pin mux params
+ * @name: Name of the mux
+ * @grps: Groups of the mux
+ * @num_grps: Number of groups (sizeof array grps)
+ */
+struct aw9523_pinmux {
+ const char *name;
+ const char * const *grps;
+ const u8 num_grps;
+};
+
+/*
+ * struct aw9523 - Main driver structure
+ * @dev: device handle
+ * @regmap: regmap handle for current device
+ * @i2c_lock: Mutex lock for i2c operations
+ * @reset_gpio: Hardware reset (RSTN) signal GPIO
+ * @vio_vreg: VCC regulator (Optional)
+ * @pctl: pinctrl handle for current device
+ * @gpio: structure holding gpiochip params
+ * @irq: Interrupt controller structure
+ */
+struct aw9523 {
+ struct device *dev;
+ struct regmap *regmap;
+ struct mutex i2c_lock;
+ struct gpio_desc *reset_gpio;
+ struct regulator *vio_vreg;
+ struct pinctrl_dev *pctl;
+ struct gpio_chip gpio;
+ struct aw9523_irq *irq;
+ DECLARE_BITMAP(direction_in[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT);
+};
+
+static const struct pinctrl_pin_desc aw9523_pins[] = {
+ /* Port 0 */
+ PINCTRL_PIN(0, "gpio0"),
+ PINCTRL_PIN(1, "gpio1"),
+ PINCTRL_PIN(2, "gpio2"),
+ PINCTRL_PIN(3, "gpio3"),
+ PINCTRL_PIN(4, "gpio4"),
+ PINCTRL_PIN(5, "gpio5"),
+ PINCTRL_PIN(6, "gpio6"),
+ PINCTRL_PIN(7, "gpio7"),
+
+ /* Port 1 */
+ PINCTRL_PIN(8, "gpio8"),
+ PINCTRL_PIN(9, "gpio9"),
+ PINCTRL_PIN(10, "gpio10"),
+ PINCTRL_PIN(11, "gpio11"),
+ PINCTRL_PIN(12, "gpio12"),
+ PINCTRL_PIN(13, "gpio13"),
+ PINCTRL_PIN(14, "gpio14"),
+ PINCTRL_PIN(15, "gpio15"),
+};
+
+static int aw9523_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+ return ARRAY_SIZE(aw9523_pins);
+}
+
+static const char *aw9523_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+ unsigned int selector)
+{
+ return aw9523_pins[selector].name;
+}
+
+static int aw9523_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+ unsigned int selector,
+ const unsigned int **pins,
+ unsigned int *num_pins)
+{
+ *pins = &aw9523_pins[selector].number;
+ *num_pins = 1;
+ return 0;
+}
+
+static const struct pinctrl_ops aw9523_pinctrl_ops = {
+ .get_groups_count = aw9523_pinctrl_get_groups_count,
+ .get_group_pins = aw9523_pinctrl_get_group_pins,
+ .get_group_name = aw9523_pinctrl_get_group_name,
+ .dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
+ .dt_free_map = pinconf_generic_dt_free_map,
+};
+
+static const char * const gpio_pwm_groups[] = {
+ "gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5",
+ "gpio6", "gpio7", "gpio8", "gpio9", "gpio10", "gpio11",
+ "gpio12", "gpio13", "gpio14", "gpio15"
+};
+
+/* Warning: Do NOT reorder this array */
+static const struct aw9523_pinmux aw9523_pmx[] = {
+ {
+ .name = "pwm",
+ .grps = gpio_pwm_groups,
+ .num_grps = ARRAY_SIZE(gpio_pwm_groups),
+ },
+ {
+ .name = "gpio",
+ .grps = gpio_pwm_groups,
+ .num_grps = ARRAY_SIZE(gpio_pwm_groups),
+ },
+};
+
+static int aw9523_pmx_get_funcs_count(struct pinctrl_dev *pctl)
+{
+ return ARRAY_SIZE(aw9523_pmx);
+}
+
+static const char *aw9523_pmx_get_fname(struct pinctrl_dev *pctl,
+ unsigned int sel)
+{
+ return aw9523_pmx[sel].name;
+}
+
+static int aw9523_pmx_get_groups(struct pinctrl_dev *pctl, unsigned int sel,
+ const char * const **groups,
+ unsigned int * const num_groups)
+{
+ *groups = aw9523_pmx[sel].grps;
+ *num_groups = aw9523_pmx[sel].num_grps;
+ return 0;
+}
+
+static int aw9523_pmx_set_mux(struct pinctrl_dev *pctl, unsigned int fsel,
+ unsigned int grp)
+{
+ struct aw9523 *awi = pinctrl_dev_get_drvdata(pctl);
+ int ret, pin = aw9523_pins[grp].number % AW9523_PINS_PER_PORT;
+
+ if (fsel >= ARRAY_SIZE(aw9523_pmx))
+ return -EINVAL;
+
+ /*
+ * This maps directly to the aw9523_pmx array: programming a
+ * high bit means "gpio" and a low bit means "pwm".
+ */
+ mutex_lock(&awi->i2c_lock);
+ ret = regmap_update_bits(awi->regmap, AW9523_REG_PORT_MODE(pin),
+ BIT(pin), (fsel ? BIT(pin) : 0));
+ mutex_unlock(&awi->i2c_lock);
+ return ret;
+}
+
+static const struct pinmux_ops aw9523_pinmux_ops = {
+ .get_functions_count = aw9523_pmx_get_funcs_count,
+ .get_function_name = aw9523_pmx_get_fname,
+ .get_function_groups = aw9523_pmx_get_groups,
+ .set_mux = aw9523_pmx_set_mux,
+};
+
+static int aw9523_pcfg_param_to_reg(enum pin_config_param pcp, int pin, u8 *r)
+{
+ u8 reg;
+
+ switch (pcp) {
+ case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ case PIN_CONFIG_BIAS_PULL_UP:
+ reg = AW9523_REG_IN_STATE(pin);
+ break;
+ case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+ case PIN_CONFIG_DRIVE_PUSH_PULL:
+ reg = AW9523_REG_GCR;
+ break;
+ case PIN_CONFIG_INPUT_ENABLE:
+ case PIN_CONFIG_OUTPUT_ENABLE:
+ reg = AW9523_REG_CONF_STATE(pin);
+ break;
+ case PIN_CONFIG_OUTPUT:
+ reg = AW9523_REG_OUT_STATE(pin);
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+ *r = reg;
+
+ return 0;
+}
+
+static int aw9523_pconf_get(struct pinctrl_dev *pctldev, unsigned int pin,
+ unsigned long *config)
+{
+ struct aw9523 *awi = pinctrl_dev_get_drvdata(pctldev);
+ enum pin_config_param param = pinconf_to_config_param(*config);
+ int hw_pin = pin % AW9523_PINS_PER_PORT;
+ unsigned int val;
+ u8 reg;
+ int rc;
+
+ rc = aw9523_pcfg_param_to_reg(param, pin, ®);
+ if (rc)
+ return rc;
+
+ mutex_lock(&awi->i2c_lock);
+ rc = regmap_read(awi->regmap, reg, &val);
+ mutex_unlock(&awi->i2c_lock);
+ if (rc)
+ return rc;
+
+ switch (param) {
+ case PIN_CONFIG_BIAS_PULL_UP:
+ case PIN_CONFIG_INPUT_ENABLE:
+ case PIN_CONFIG_OUTPUT:
+ val &= BIT(hw_pin);
+ break;
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ case PIN_CONFIG_OUTPUT_ENABLE:
+ val &= BIT(hw_pin);
+ val = !val;
+ break;
+ case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+ if (pin >= AW9523_PINS_PER_PORT)
+ val = 0;
+ else
+ val = !FIELD_GET(AW9523_GCR_GPOMD_MASK, val);
+ break;
+ case PIN_CONFIG_DRIVE_PUSH_PULL:
+ if (pin >= AW9523_PINS_PER_PORT)
+ val = 1;
+ else
+ val = FIELD_GET(AW9523_GCR_GPOMD_MASK, val);
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+ if (val < 1)
+ return -EINVAL;
+
+ *config = pinconf_to_config_packed(param, !!val);
+
+ return rc;
+}
+
+static int aw9523_pconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
+ unsigned long *configs, unsigned int num_configs)
+{
+ struct aw9523 *awi = pinctrl_dev_get_drvdata(pctldev);
+ enum pin_config_param param;
+ int hw_pin = pin % AW9523_PINS_PER_PORT;
+ u32 arg;
+ u8 reg;
+ unsigned int mask, val;
+ int i, rc;
+
+ mutex_lock(&awi->i2c_lock);
+ for (i = 0; i < num_configs; i++) {
+ param = pinconf_to_config_param(configs[i]);
+ arg = pinconf_to_config_argument(configs[i]);
+
+ rc = aw9523_pcfg_param_to_reg(param, pin, ®);
+ if (rc) {
+ goto end;
+ }
+
+ switch (param) {
+ case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+ case PIN_CONFIG_BIAS_PULL_DOWN:
+ case PIN_CONFIG_OUTPUT_ENABLE:
+ mask = BIT(hw_pin);
+ val = arg ? 0 : BIT(hw_pin);
+ break;
+ case PIN_CONFIG_OUTPUT:
+ /* First, enable pin output */
+ rc = regmap_update_bits(awi->regmap,
+ AW9523_REG_CONF_STATE(pin),
+ BIT(hw_pin), 0);
+ if (rc)
+ goto end;
+ /* Then, fall through to config output level */
+ fallthrough;
+ case PIN_CONFIG_BIAS_PULL_UP:
+ case PIN_CONFIG_INPUT_ENABLE:
+ mask = BIT(hw_pin);
+ val = arg ? BIT(hw_pin) : 0;
+ break;
+ case PIN_CONFIG_DRIVE_OPEN_DRAIN:
+ /* Open-Drain is supported only on port 0 */
+ if (pin >= AW9523_PINS_PER_PORT) {
+ rc = -ENOTSUPP;
+ goto end;
+ }
+ mask = AW9523_GCR_GPOMD_MASK;
+ val = 0;
+ break;
+ case PIN_CONFIG_DRIVE_PUSH_PULL:
+ /* Port 1 is always Push-Pull */
+ if (pin >= AW9523_PINS_PER_PORT) {
+ mask = 0;
+ val = 0;
+ continue;
+ }
+ mask = AW9523_GCR_GPOMD_MASK;
+ val = AW9523_GCR_GPOMD_MASK;
+ break;
+ default:
+ rc = -ENOTSUPP;
+ goto end;
+ }
+
+ rc = regmap_update_bits(awi->regmap, reg, mask, val);
+ if (rc)
+ goto end;
+ }
+end:
+ mutex_unlock(&awi->i2c_lock);
+ return rc;
+}
+
+static const struct pinconf_ops aw9523_pinconf_ops = {
+ .pin_config_get = aw9523_pconf_get,
+ .pin_config_set = aw9523_pconf_set,
+ .is_generic = true,
+};
+
+/*
+ * __aw9523_gpio_get_direction - Get pin direction
+ * @regmap: Regmap structure
+ * @pin: gpiolib pin number
+ * @hwp: pin index in port register
+ *
+ * Return: Pin direction for success or negative number for error
+ */
+static int __aw9523_gpio_get_direction(struct regmap *regmap, u8 pin, u8 hwp)
+{
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(regmap, AW9523_REG_CONF_STATE(pin), &val);
+ if (ret)
+ return ret;
+
+ return val & BIT(hwp) ? GPIO_LINE_DIRECTION_IN :
+ GPIO_LINE_DIRECTION_OUT;
+}
+
+/*
+ * __aw5923_get_port_state - Get input or output state for entire port
+ * @regmap: Regmap structure
+ * @pin: gpiolib pin number
+ * @hw_pin: hw pin index, used to retrieve port number
+ * @state: returned port state
+ *
+ * Return: Zero for success or negative number for error
+ */
+static int __aw9523_get_port_state(struct regmap *regmap, u8 pin,
+ u8 hw_pin, unsigned int *state)
+{
+ u8 reg;
+ int dir;
+
+ dir = __aw9523_gpio_get_direction(regmap, pin, hw_pin);
+ if (dir < 0)
+ return dir;
+
+ if (dir == GPIO_LINE_DIRECTION_IN)
+ reg = AW9523_REG_IN_STATE(pin);
+ else
+ reg = AW9523_REG_OUT_STATE(pin);
+
+ return regmap_read(regmap, reg, state);
+}
+
+static int aw9523_gpio_irq_type(struct irq_data *d, unsigned int type)
+{
+ switch (type) {
+ case IRQ_TYPE_NONE:
+ case IRQ_TYPE_LEVEL_MASK:
+ case IRQ_TYPE_LEVEL_HIGH:
+ case IRQ_TYPE_LEVEL_LOW:
+ case IRQ_TYPE_EDGE_BOTH:
+ case IRQ_TYPE_EDGE_RISING:
+ case IRQ_TYPE_EDGE_FALLING:
+ return 0;
+ default:
+ return -EINVAL;
+ };
+}
+
+/*
+ * aw9523_irq_mask - Mask interrupt
+ * @d: irq data
+ *
+ * Sets which interrupt to mask in the bitmap;
+ * The interrupt will be masked when unlocking the irq bus.
+ */
+static void aw9523_irq_mask(struct irq_data *d)
+{
+ struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+ unsigned int n = d->hwirq % AW9523_PINS_PER_PORT;
+ int port = AW9523_PIN_TO_PORT(d->hwirq);
+
+ set_bit(n, awi->irq->masked[port]);
+}
+
+/*
+ * aw9523_irq_unmask - Unmask interrupt
+ * @d: irq data
+ *
+ * Sets which interrupt to unmask in the bitmap;
+ * The interrupt will be masked when unlocking the irq bus.
+ */
+static void aw9523_irq_unmask(struct irq_data *d)
+{
+ struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+ unsigned int n = d->hwirq % AW9523_PINS_PER_PORT;
+ int port = AW9523_PIN_TO_PORT(d->hwirq);
+
+ clear_bit(n, awi->irq->masked[port]);
+}
+
+static irqreturn_t aw9523_irq_thread_func(int irq, void *dev_id)
+{
+ struct aw9523 *awi = (struct aw9523 *)dev_id;
+ unsigned long n, val = 0;
+ unsigned long changed_gpio;
+ unsigned int tmp, port_pin, i, ret;
+
+ for (i = 0; i < AW9523_NUM_PORTS; i++) {
+ port_pin = i * AW9523_PINS_PER_PORT;
+ ret = regmap_read(awi->regmap,
+ AW9523_REG_IN_STATE(port_pin),
+ &tmp);
+ if (ret)
+ return ret;
+
+ val |= (u8)tmp << (i * 8);
+ }
+
+ /* Handle GPIO input release interrupt as well */
+ changed_gpio = awi->irq->cached_gpio ^ val;
+ awi->irq->cached_gpio = val;
+
+ /*
+ * To avoid up to four *slow* i2c reads from any driver hooked
+ * up to our interrupts, just check for the irq_find_mapping
+ * result: if the interrupt is not mapped, then we don't want
+ * to care about it.
+ */
+ for_each_set_bit(n, &changed_gpio, awi->gpio.ngpio) {
+ tmp = irq_find_mapping(awi->gpio.irq.domain, n);
+ if (tmp <= 0)
+ continue;
+ handle_nested_irq(tmp);
+ }
+
+ return IRQ_HANDLED;
+}
+
+/*
+ * aw9523_irq_bus_lock - Grab lock for interrupt operation
+ * @d: irq data
+ */
+static void aw9523_irq_bus_lock(struct irq_data *d)
+{
+ struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+
+ mutex_lock(&awi->irq->lock);
+}
+
+/*
+ * aw9523_irq_bus_sync_unlock - Synchronize state and unlock
+ * @d: irq data
+ *
+ * Writes the interrupt mask bits (found in the bit map) to the
+ * hardware, then unlocks the bus.
+ */
+static void aw9523_irq_bus_sync_unlock(struct irq_data *d)
+{
+ struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
+ int i;
+
+ for (i = 0; i < AW9523_NUM_PORTS; i++) {
+ if (bitmap_equal(awi->irq->masked[i], awi->irq->old_masked[i],
+ AW9523_PINS_PER_PORT))
+ continue;
+ regmap_write(awi->regmap,
+ AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT * i),
+ *awi->irq->masked[i]);
+ bitmap_copy(awi->irq->old_masked[i], awi->irq->masked[i],
+ AW9523_PINS_PER_PORT);
+ }
+ mutex_unlock(&awi->irq->lock);
+}
+
+static int aw9523_gpio_get_direction(struct gpio_chip *chip,
+ unsigned int offset)
+{
+ struct aw9523 *awi = gpiochip_get_data(chip);
+ u8 hw_pin = offset % AW9523_PINS_PER_PORT;
+
+ return __aw9523_gpio_get_direction(awi->regmap, offset, hw_pin);
+}
+
+static int aw9523_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+ struct aw9523 *awi = gpiochip_get_data(chip);
+ u8 hw_pin = offset % AW9523_PINS_PER_PORT;
+ unsigned int val;
+ int ret;
+
+ mutex_lock(&awi->i2c_lock);
+ ret = __aw9523_get_port_state(awi->regmap, offset, hw_pin, &val);
+ mutex_unlock(&awi->i2c_lock);
+ if (ret)
+ return ret;
+
+ return !!(val & BIT(hw_pin));
+}
+
+/**
+ * _aw9523_gpio_get_multiple - Get I/O state for an entire port
+ * @regmap: Regmap structure
+ * @pin: gpiolib pin number
+ * @hw_pin: hw pin index, used to retrieve port number
+ * @state: returned port I/O state
+ *
+ * Return: Zero for success or negative number for error
+ */
+static int _aw9523_gpio_get_multiple(struct aw9523 *awi, u8 hw_pin,
+ u8 *state, u8 mask)
+{
+ u32 dir_in, val;
+ u8 m;
+ int ret;
+
+ /* Registers are 8-bits wide */
+ ret = regmap_read(awi->regmap, AW9523_REG_CONF_STATE(hw_pin), &dir_in);
+ if (ret)
+ return ret;
+ *state = 0;
+
+ m = mask & dir_in;
+ if (m) {
+ ret = regmap_read(awi->regmap, AW9523_REG_IN_STATE(hw_pin),
+ &val);
+ if (ret)
+ return ret;
+ *state |= (u8)val & m;
+ }
+
+ m = mask & ~dir_in;
+ if (m) {
+ ret = regmap_read(awi->regmap, AW9523_REG_OUT_STATE(hw_pin),
+ &val);
+ if (ret)
+ return ret;
+ *state |= (u8)val & m;
+ }
+
+ return 0;
+}
+
+static int aw9523_gpio_get_multiple(struct gpio_chip *chip,
+ unsigned long *mask,
+ unsigned long *bits)
+{
+ struct aw9523 *awi = gpiochip_get_data(chip);
+ u8 m, state = 0;
+ int ret;
+
+ mutex_lock(&awi->i2c_lock);
+
+ /* Port 0 (gpio 0-7) */
+ m = *mask & U8_MAX;
+ if (m) {
+ ret = _aw9523_gpio_get_multiple(awi, 0, &state, m);
+ if (ret)
+ goto out;
+ }
+ *bits = state;
+
+ /* Port 1 (gpio 8-15) */
+ m = (*mask >> 8) & U8_MAX;
+ if (m) {
+ ret = _aw9523_gpio_get_multiple(awi, AW9523_PINS_PER_PORT,
+ &state, m);
+ if (ret)
+ goto out;
+
+ *bits |= (state << 8);
+ }
+out:
+ mutex_unlock(&awi->i2c_lock);
+ return ret;
+}
+
+static void aw9523_gpio_set_multiple(struct gpio_chip *chip,
+ unsigned long *mask,
+ unsigned long *bits)
+{
+ struct aw9523 *awi = gpiochip_get_data(chip);
+ u8 mask_lo, mask_hi, bits_lo, bits_hi;
+ unsigned int reg;
+ int ret = 0;
+
+ mask_lo = *mask & U8_MAX;
+ mask_hi = (*mask >> 8) & U8_MAX;
+ mutex_lock(&awi->i2c_lock);
+ if (mask_hi) {
+ reg = AW9523_REG_OUT_STATE(AW9523_PINS_PER_PORT);
+ bits_hi = (*bits >> 8) & U8_MAX;
+
+ ret = regmap_write_bits(awi->regmap, reg, mask_hi, bits_hi);
+ if (ret) {
+ dev_warn(awi->dev, "Cannot write port1 out level\n");
+ goto out;
+ }
+ }
+ if (mask_lo) {
+ reg = AW9523_REG_OUT_STATE(0);
+ bits_lo = *bits & U8_MAX;
+ ret = regmap_write_bits(awi->regmap, reg, mask_lo, bits_lo);
+ if (ret)
+ dev_warn(awi->dev, "Cannot write port0 out level\n");
+ }
+out:
+ mutex_unlock(&awi->i2c_lock);
+}
+
+static void aw9523_gpio_set(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ struct aw9523 *awi = gpiochip_get_data(chip);
+ u8 hw_pin = offset % AW9523_PINS_PER_PORT;
+
+ mutex_lock(&awi->i2c_lock);
+ regmap_update_bits(awi->regmap, AW9523_REG_OUT_STATE(offset),
+ BIT(hw_pin), value ? BIT(hw_pin) : 0);
+ mutex_unlock(&awi->i2c_lock);
+}
+
+
+static int aw9523_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+ struct aw9523 *awi = gpiochip_get_data(chip);
+ u8 hw_pin = offset % AW9523_PINS_PER_PORT;
+ int port = AW9523_PIN_TO_PORT(offset);
+
+ set_bit(offset, awi->direction_in[port]);
+
+ return regmap_update_bits(awi->regmap, AW9523_REG_CONF_STATE(offset),
+ BIT(hw_pin), BIT(hw_pin));
+}
+
+static int aw9523_direction_output(struct gpio_chip *chip,
+ unsigned int offset, int value)
+{
+ struct aw9523 *awi = gpiochip_get_data(chip);
+ u8 hw_pin = offset % AW9523_PINS_PER_PORT;
+ int port = AW9523_PIN_TO_PORT(offset);
+ int ret;
+
+ ret = regmap_update_bits(awi->regmap, AW9523_REG_OUT_STATE(offset),
+ BIT(hw_pin), value ? BIT(hw_pin) : 0);
+ if (ret)
+ return ret;
+
+ if (test_bit(offset, awi->direction_in[port])) {
+ ret = regmap_update_bits(awi->regmap,
+ AW9523_REG_CONF_STATE(offset),
+ BIT(hw_pin), 0);
+ clear_bit(offset, awi->direction_in[port]);
+ }
+
+ return ret;
+}
+
+static int __aw9523_hw_reset(struct aw9523 *awi)
+{
+ unsigned int chip_id;
+ int ret;
+
+ /*
+ * If the chip is already configured for any reason, then we
+ * will probably succeed in sending the soft reset signal to
+ * the hardware through I2C: this operation takes less time
+ * compared to a full HW reset and it gives the same results.
+ */
+ ret = regmap_write(awi->regmap, AW9523_REG_SOFT_RESET, 0);
+ if (ret == 0)
+ goto done;
+
+ dev_dbg(awi->dev, "Cannot execute soft reset: trying hard reset\n");
+ ret = gpiod_direction_output(awi->reset_gpio, 0);
+ if (ret)
+ return ret;
+
+ /* The reset pulse has to be longer than 20uS due to deglitch */
+ usleep_range(AW9523_HW_RESET_US, AW9523_HW_RESET_US + 1);
+
+ ret = gpiod_direction_output(awi->reset_gpio, 1);
+ if (ret)
+ return ret;
+done:
+ /* The HW needs at least 1uS to reliably recover after reset */
+ usleep_range(AW9523_HW_RESET_RECOVERY_US,
+ AW9523_HW_RESET_RECOVERY_US + 1);
+
+ /* Check the ChipID */
+ ret = regmap_read(awi->regmap, AW9523_REG_CHIPID, &chip_id);
+ if (ret) {
+ dev_err(awi->dev, "Cannot read Chip ID: %d\n", ret);
+ return ret;
+ }
+ if (chip_id != AW9523_VAL_EXPECTED_CHIPID) {
+ dev_err(awi->dev, "Bad ChipID; read 0x%x, expected 0x%x\n",
+ chip_id, AW9523_VAL_EXPECTED_CHIPID);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int aw9523_hw_reset(struct aw9523 *awi)
+{
+ int ret, max_retries = 2;
+
+ /* Sometimes the chip needs more than one reset cycle */
+ do {
+ ret = __aw9523_hw_reset(awi);
+ if (ret == 0)
+ break;
+ max_retries--;
+ } while (max_retries);
+
+ return ret;
+}
+
+static int aw9523_init_gpiochip(struct aw9523 *awi, unsigned int npins)
+{
+ struct device *dev = awi->dev;
+ struct gpio_chip *gpiochip = &awi->gpio;
+
+ gpiochip->label = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+ if (!gpiochip->label)
+ return -ENOMEM;
+
+ gpiochip->base = -1;
+ gpiochip->ngpio = npins;
+ gpiochip->get_direction = aw9523_gpio_get_direction;
+ gpiochip->direction_input = aw9523_direction_input;
+ gpiochip->direction_output = aw9523_direction_output;
+ gpiochip->get = aw9523_gpio_get;
+ gpiochip->get_multiple = aw9523_gpio_get_multiple;
+ gpiochip->set = aw9523_gpio_set;
+ gpiochip->set_multiple = aw9523_gpio_set_multiple;
+ gpiochip->set_config = gpiochip_generic_config;
+ gpiochip->parent = dev;
+ gpiochip->of_node = dev->of_node;
+ gpiochip->owner = THIS_MODULE;
+ gpiochip->can_sleep = true;
+
+ return 0;
+}
+
+static int aw9523_init_irq(struct aw9523 *awi, int irq)
+{
+ struct device *dev = awi->dev;
+ struct gpio_irq_chip *gpioirq;
+ struct irq_chip *irqchip;
+ int i, ret;
+
+ if (!device_property_read_bool(dev, "interrupt-controller"))
+ return 0;
+
+ irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
+ if (!irqchip)
+ return -ENOMEM;
+
+ awi->irq = devm_kzalloc(dev, sizeof(*awi->irq), GFP_KERNEL);
+ if (!awi->irq)
+ return -ENOMEM;
+
+ irqchip->name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+ if (!irqchip->name)
+ return -ENOMEM;
+
+ irqchip->irq_mask = aw9523_irq_mask;
+ irqchip->irq_unmask = aw9523_irq_unmask;
+ irqchip->irq_bus_lock = aw9523_irq_bus_lock;
+ irqchip->irq_bus_sync_unlock = aw9523_irq_bus_sync_unlock;
+ irqchip->irq_set_type = aw9523_gpio_irq_type;
+
+ for (i = 0; i < AW9523_NUM_PORTS; i++) {
+ bitmap_fill(awi->irq->masked[i], AW9523_PINS_PER_PORT);
+ bitmap_fill(awi->irq->old_masked[i], AW9523_PINS_PER_PORT);
+ }
+ awi->irq->irqchip = irqchip;
+ mutex_init(&awi->irq->lock);
+
+ ret = devm_request_threaded_irq(dev, irq, NULL, aw9523_irq_thread_func,
+ IRQF_ONESHOT, dev_name(dev), awi);
+ if (ret) {
+ dev_err(dev, "Failed to request irq %d\n", irq);
+ return ret;
+ }
+
+ gpioirq = &awi->gpio.irq;
+ gpioirq->chip = irqchip;
+ gpioirq->parent_handler = NULL;
+ gpioirq->num_parents = 0;
+ gpioirq->parents = NULL;
+ gpioirq->parent_domain = NULL;
+ gpioirq->default_type = IRQ_TYPE_NONE;
+ gpioirq->handler = handle_simple_irq;
+ gpioirq->threaded = true;
+ gpioirq->first = 0;
+
+ return 0;
+}
+
+static int aw9523_hw_init(struct aw9523 *awi)
+{
+ u8 p1_pin = AW9523_PINS_PER_PORT;
+ unsigned int val;
+ int i, ret;
+
+ /* Bring up the chip */
+ ret = aw9523_hw_reset(awi);
+ if (ret) {
+ dev_err(awi->dev, "HW Reset failed: %d\n", ret);
+ return ret;
+ }
+
+ /*
+ * This is the expected chip and it is running: it's time to
+ * set a safe default configuration in case the user doesn't
+ * configure (all of the available) pins in this chip.
+ */
+
+ /* Set all pins as GPIO */
+ ret = regmap_write(awi->regmap, AW9523_REG_PORT_MODE(0), U8_MAX);
+ if (ret)
+ return ret;
+ ret = regmap_write(awi->regmap, AW9523_REG_PORT_MODE(p1_pin), U8_MAX);
+ if (ret)
+ return ret;
+
+ /* Set Open-Drain mode on Port 0 (Port 1 is always P-P) */
+ ret = regmap_update_bits(awi->regmap, AW9523_REG_GCR,
+ AW9523_GCR_GPOMD_MASK, 0);
+ if (ret)
+ return ret;
+
+ /* Set all pins as inputs */
+ ret = regmap_write(awi->regmap, AW9523_REG_CONF_STATE(0), U8_MAX);
+ if (ret)
+ return ret;
+ ret = regmap_write(awi->regmap, AW9523_REG_CONF_STATE(p1_pin), U8_MAX);
+ if (ret)
+ return ret;
+ for (i = 0; i < AW9523_NUM_PORTS; i++)
+ bitmap_fill(awi->direction_in[i], AW9523_PINS_PER_PORT);
+
+ /* Disable all interrupts to avoid unreasoned wakeups */
+ ret = regmap_write(awi->regmap, AW9523_REG_INTR_DIS(0), U8_MAX);
+ if (ret)
+ return ret;
+ ret = regmap_write(awi->regmap, AW9523_REG_INTR_DIS(p1_pin), U8_MAX);
+ if (ret)
+ return ret;
+
+ /* Clear setup-generated interrupts by performing a port state read */
+ ret = __aw9523_get_port_state(awi->regmap, 0, 0, &val);
+ if (ret)
+ return ret;
+ ret = __aw9523_get_port_state(awi->regmap, p1_pin, 0, &val);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static const struct regmap_config aw9523_regmap = {
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .cache_type = REGCACHE_NONE,
+ .disable_locking = true,
+
+ .max_register = AW9523_REG_SOFT_RESET,
+};
+
+static int aw9523_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct device *dev = &client->dev;
+ struct pinctrl_desc *pdesc;
+ struct aw9523 *awi;
+ int ret;
+
+ awi = devm_kzalloc(dev, sizeof(*awi), GFP_KERNEL);
+ if (!awi)
+ return -ENOMEM;
+
+ i2c_set_clientdata(client, awi);
+
+ awi->dev = dev;
+ awi->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(awi->reset_gpio))
+ return PTR_ERR(awi->reset_gpio);
+ gpiod_set_consumer_name(awi->reset_gpio, "aw9523 reset");
+
+ awi->regmap = devm_regmap_init_i2c(client, &aw9523_regmap);
+ if (IS_ERR(awi->regmap))
+ return PTR_ERR(awi->regmap);
+
+ awi->vio_vreg = devm_regulator_get_optional(dev, "vio");
+ if (IS_ERR(awi->vio_vreg)) {
+ if (PTR_ERR(awi->vio_vreg) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ awi->vio_vreg = NULL;
+ } else {
+ ret = regulator_enable(awi->vio_vreg);
+ if (ret)
+ return ret;
+ }
+
+
+ mutex_init(&awi->i2c_lock);
+ lockdep_set_subclass(&awi->i2c_lock,
+ i2c_adapter_depth(client->adapter));
+
+ pdesc = devm_kzalloc(dev, sizeof(*pdesc), GFP_KERNEL);
+ if (!pdesc)
+ return -ENOMEM;
+
+ ret = aw9523_hw_init(awi);
+ if (ret)
+ goto err_disable_vregs;
+
+ pdesc->name = dev_name(dev);
+ pdesc->owner = THIS_MODULE;
+ pdesc->pctlops = &aw9523_pinctrl_ops;
+ pdesc->pmxops = &aw9523_pinmux_ops;
+ pdesc->confops = &aw9523_pinconf_ops;
+ pdesc->pins = aw9523_pins;
+ pdesc->npins = ARRAY_SIZE(aw9523_pins);
+
+ ret = aw9523_init_gpiochip(awi, pdesc->npins);
+ if (ret)
+ goto err_disable_vregs;
+
+ if (client->irq) {
+ ret = aw9523_init_irq(awi, client->irq);
+ if (ret)
+ goto err_disable_vregs;
+ }
+
+ awi->pctl = devm_pinctrl_register(dev, pdesc, awi);
+ if (IS_ERR(awi->pctl)) {
+ ret = PTR_ERR(awi->pctl);
+ dev_err(dev, "Cannot register pinctrl: %d", ret);
+ goto err_disable_vregs;
+ }
+
+ ret = devm_gpiochip_add_data(dev, &awi->gpio, awi);
+ if (ret)
+ goto err_disable_vregs;
+
+ return ret;
+
+err_disable_vregs:
+ if (awi->vio_vreg)
+ regulator_disable(awi->vio_vreg);
+ return ret;
+}
+
+static const struct i2c_device_id aw9523_i2c_id_table[] = {
+ { "aw9523_i2c", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, aw9523_i2c_id_table);
+
+static const struct of_device_id of_aw9523_i2c_match[] = {
+ { .compatible = "awinic,aw9523-pinctrl", },
+};
+MODULE_DEVICE_TABLE(of, of_aw9523_i2c_match);
+
+static struct i2c_driver aw9523_driver = {
+ .driver = {
+ .name = "aw9523-pinctrl",
+ .of_match_table = of_aw9523_i2c_match,
+ },
+ .probe = aw9523_probe,
+ .id_table = aw9523_i2c_id_table,
+};
+module_i2c_driver(aw9523_driver);
+
+MODULE_DESCRIPTION("Awinic AW9523 I2C GPIO Expander driver");
+MODULE_AUTHOR("AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:aw9523");
--
2.29.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B
2021-01-09 14:02 [PATCH 0/2] Add Awinic AW9523(B) I2C GPIO Expander driver AngeloGioacchino Del Regno
2021-01-09 14:02 ` [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander AngeloGioacchino Del Regno
@ 2021-01-09 14:02 ` AngeloGioacchino Del Regno
2021-01-09 22:14 ` Linus Walleij
2021-01-10 17:18 ` Rob Herring
1 sibling, 2 replies; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-09 14:02 UTC (permalink / raw)
To: linux-gpio
Cc: konrad.dybcio, marijn.suijten, martin.botka, phone-devel,
linux-kernel, devicetree, robh+dt, linus.walleij,
AngeloGioacchino Del Regno
Add bindings for the Awinic AW9523/AW9523B I2C GPIO Expander driver.
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
---
.../pinctrl/awinic,aw9523-pinctrl.yaml | 111 ++++++++++++++++++
1 file changed, 111 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
diff --git a/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
new file mode 100644
index 000000000000..1352b3adb566
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
@@ -0,0 +1,111 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/awinic,aw9523-pinctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Awinic AW9523/AW9523B I2C GPIO Expander
+
+maintainers:
+ - AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
+
+description: |
+ The Awinic AW9523/AW9523B I2C GPIO Expander featuring 16 multi-function
+ I/O, 256 steps PWM mode and interrupt support.
+
+properties:
+ compatible:
+ const: awinic,aw9523-pinctrl
+
+ reg:
+ maxItems: 1
+
+ '#gpio-cells':
+ description: |
+ Specifying the pin number and flags, as defined in
+ include/dt-bindings/gpio/gpio.h
+ const: 2
+
+ gpio-controller: true
+
+ gpio-ranges:
+ maxItems: 1
+
+ interrupt-controller: true
+
+ interrupts:
+ maxItems: 1
+ description: Specifies the INTN pin IRQ.
+
+ '#interrupt-cells':
+ description:
+ Specifies the PIN numbers and Flags, as defined in defined in
+ include/dt-bindings/interrupt-controller/irq.h
+ const: 2
+
+#PIN CONFIGURATION NODES
+patternProperties:
+ '^.*$':
+ if:
+ type: object
+ then:
+ properties:
+ pins:
+ description:
+ List of gpio pins affected by the properties specified in
+ this subnode.
+ items:
+ pattern: "^gpio([0-9]|1[0-5])$"
+ minItems: 1
+ maxItems: 16
+
+ function:
+ description:
+ Specify the alternative function to be configured for the
+ specified pins.
+
+ enum: [ gpio, pwm ]
+
+ bias-disable: true
+ bias-pull-down: true
+ bias-pull-up: true
+ drive-open-drain: true
+ drive-push-pull: true
+ input-enable: true
+ output-high: true
+ output-low: true
+
+ required:
+ - pins
+ - function
+
+ additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - gpio-controller
+ - '#gpio-cells'
+ - gpio-ranges
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c_node {
+ gpio-expander@58 {
+ compatible = "awinic,aw9523-pinctrl";
+ reg = <0x58>;
+ interrupt-parent = <&tlmm>;
+ interrupts = <50 IRQ_TYPE_EDGE_FALLING>;
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-ranges = <&tlmm 0 0 16>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ reset-gpios = <&tlmm 51 GPIO_ACTIVE_HIGH>;
+ };
+ };
--
2.29.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-09 14:02 ` [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander AngeloGioacchino Del Regno
@ 2021-01-09 17:24 ` kernel test robot
2021-01-09 22:11 ` Linus Walleij
1 sibling, 0 replies; 19+ messages in thread
From: kernel test robot @ 2021-01-09 17:24 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, linux-gpio
Cc: kbuild-all, konrad.dybcio, marijn.suijten, martin.botka,
phone-devel, linux-kernel, devicetree, robh+dt, linus.walleij,
AngeloGioacchino Del Regno
[-- Attachment #1: Type: text/plain, Size: 4686 bytes --]
Hi AngeloGioacchino,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pinctrl/devel]
[also build test ERROR on v5.11-rc2 next-20210108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/AngeloGioacchino-Del-Regno/Add-Awinic-AW9523-B-I2C-GPIO-Expander-driver/20210109-220525
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: sparc-randconfig-r025-20210110 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/9cf5980cd08c30aec01a24e284a0553396a1fa64
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review AngeloGioacchino-Del-Regno/Add-Awinic-AW9523-B-I2C-GPIO-Expander-driver/20210109-220525
git checkout 9cf5980cd08c30aec01a24e284a0553396a1fa64
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/pinctrl/pinctrl-aw9523.c: In function 'aw9523_pconf_get':
drivers/pinctrl/pinctrl-aw9523.c:292:11: error: implicit declaration of function 'FIELD_GET'; did you mean 'FOLL_GET'? [-Werror=implicit-function-declaration]
292 | val = !FIELD_GET(AW9523_GCR_GPOMD_MASK, val);
| ^~~~~~~~~
| FOLL_GET
drivers/pinctrl/pinctrl-aw9523.c: In function 'aw9523_init_irq':
>> drivers/pinctrl/pinctrl-aw9523.c:880:9: error: 'struct gpio_irq_chip' has no member named 'parent_domain'
880 | gpioirq->parent_domain = NULL;
| ^~
cc1: some warnings being treated as errors
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for FRAME_POINTER
Depends on DEBUG_KERNEL && (M68K || UML || SUPERH) || ARCH_WANT_FRAME_POINTERS || MCOUNT
Selected by
- LOCKDEP && DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT && !MIPS && !PPC && !ARM && !S390 && !MICROBLAZE && !ARC && !X86
- FAULT_INJECTION_STACKTRACE_FILTER && FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT && !X86_64 && !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM && !ARC && !X86
vim +880 drivers/pinctrl/pinctrl-aw9523.c
832
833 static int aw9523_init_irq(struct aw9523 *awi, int irq)
834 {
835 struct device *dev = awi->dev;
836 struct gpio_irq_chip *gpioirq;
837 struct irq_chip *irqchip;
838 int i, ret;
839
840 if (!device_property_read_bool(dev, "interrupt-controller"))
841 return 0;
842
843 irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
844 if (!irqchip)
845 return -ENOMEM;
846
847 awi->irq = devm_kzalloc(dev, sizeof(*awi->irq), GFP_KERNEL);
848 if (!awi->irq)
849 return -ENOMEM;
850
851 irqchip->name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
852 if (!irqchip->name)
853 return -ENOMEM;
854
855 irqchip->irq_mask = aw9523_irq_mask;
856 irqchip->irq_unmask = aw9523_irq_unmask;
857 irqchip->irq_bus_lock = aw9523_irq_bus_lock;
858 irqchip->irq_bus_sync_unlock = aw9523_irq_bus_sync_unlock;
859 irqchip->irq_set_type = aw9523_gpio_irq_type;
860
861 for (i = 0; i < AW9523_NUM_PORTS; i++) {
862 bitmap_fill(awi->irq->masked[i], AW9523_PINS_PER_PORT);
863 bitmap_fill(awi->irq->old_masked[i], AW9523_PINS_PER_PORT);
864 }
865 awi->irq->irqchip = irqchip;
866 mutex_init(&awi->irq->lock);
867
868 ret = devm_request_threaded_irq(dev, irq, NULL, aw9523_irq_thread_func,
869 IRQF_ONESHOT, dev_name(dev), awi);
870 if (ret) {
871 dev_err(dev, "Failed to request irq %d\n", irq);
872 return ret;
873 }
874
875 gpioirq = &awi->gpio.irq;
876 gpioirq->chip = irqchip;
877 gpioirq->parent_handler = NULL;
878 gpioirq->num_parents = 0;
879 gpioirq->parents = NULL;
> 880 gpioirq->parent_domain = NULL;
881 gpioirq->default_type = IRQ_TYPE_NONE;
882 gpioirq->handler = handle_simple_irq;
883 gpioirq->threaded = true;
884 gpioirq->first = 0;
885
886 return 0;
887 }
888
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 20788 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
@ 2021-01-09 17:24 ` kernel test robot
0 siblings, 0 replies; 19+ messages in thread
From: kernel test robot @ 2021-01-09 17:24 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 4799 bytes --]
Hi AngeloGioacchino,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pinctrl/devel]
[also build test ERROR on v5.11-rc2 next-20210108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/AngeloGioacchino-Del-Regno/Add-Awinic-AW9523-B-I2C-GPIO-Expander-driver/20210109-220525
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: sparc-randconfig-r025-20210110 (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/9cf5980cd08c30aec01a24e284a0553396a1fa64
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review AngeloGioacchino-Del-Regno/Add-Awinic-AW9523-B-I2C-GPIO-Expander-driver/20210109-220525
git checkout 9cf5980cd08c30aec01a24e284a0553396a1fa64
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/pinctrl/pinctrl-aw9523.c: In function 'aw9523_pconf_get':
drivers/pinctrl/pinctrl-aw9523.c:292:11: error: implicit declaration of function 'FIELD_GET'; did you mean 'FOLL_GET'? [-Werror=implicit-function-declaration]
292 | val = !FIELD_GET(AW9523_GCR_GPOMD_MASK, val);
| ^~~~~~~~~
| FOLL_GET
drivers/pinctrl/pinctrl-aw9523.c: In function 'aw9523_init_irq':
>> drivers/pinctrl/pinctrl-aw9523.c:880:9: error: 'struct gpio_irq_chip' has no member named 'parent_domain'
880 | gpioirq->parent_domain = NULL;
| ^~
cc1: some warnings being treated as errors
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for FRAME_POINTER
Depends on DEBUG_KERNEL && (M68K || UML || SUPERH) || ARCH_WANT_FRAME_POINTERS || MCOUNT
Selected by
- LOCKDEP && DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT && !MIPS && !PPC && !ARM && !S390 && !MICROBLAZE && !ARC && !X86
- FAULT_INJECTION_STACKTRACE_FILTER && FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT && !X86_64 && !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM && !ARC && !X86
vim +880 drivers/pinctrl/pinctrl-aw9523.c
832
833 static int aw9523_init_irq(struct aw9523 *awi, int irq)
834 {
835 struct device *dev = awi->dev;
836 struct gpio_irq_chip *gpioirq;
837 struct irq_chip *irqchip;
838 int i, ret;
839
840 if (!device_property_read_bool(dev, "interrupt-controller"))
841 return 0;
842
843 irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
844 if (!irqchip)
845 return -ENOMEM;
846
847 awi->irq = devm_kzalloc(dev, sizeof(*awi->irq), GFP_KERNEL);
848 if (!awi->irq)
849 return -ENOMEM;
850
851 irqchip->name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
852 if (!irqchip->name)
853 return -ENOMEM;
854
855 irqchip->irq_mask = aw9523_irq_mask;
856 irqchip->irq_unmask = aw9523_irq_unmask;
857 irqchip->irq_bus_lock = aw9523_irq_bus_lock;
858 irqchip->irq_bus_sync_unlock = aw9523_irq_bus_sync_unlock;
859 irqchip->irq_set_type = aw9523_gpio_irq_type;
860
861 for (i = 0; i < AW9523_NUM_PORTS; i++) {
862 bitmap_fill(awi->irq->masked[i], AW9523_PINS_PER_PORT);
863 bitmap_fill(awi->irq->old_masked[i], AW9523_PINS_PER_PORT);
864 }
865 awi->irq->irqchip = irqchip;
866 mutex_init(&awi->irq->lock);
867
868 ret = devm_request_threaded_irq(dev, irq, NULL, aw9523_irq_thread_func,
869 IRQF_ONESHOT, dev_name(dev), awi);
870 if (ret) {
871 dev_err(dev, "Failed to request irq %d\n", irq);
872 return ret;
873 }
874
875 gpioirq = &awi->gpio.irq;
876 gpioirq->chip = irqchip;
877 gpioirq->parent_handler = NULL;
878 gpioirq->num_parents = 0;
879 gpioirq->parents = NULL;
> 880 gpioirq->parent_domain = NULL;
881 gpioirq->default_type = IRQ_TYPE_NONE;
882 gpioirq->handler = handle_simple_irq;
883 gpioirq->threaded = true;
884 gpioirq->first = 0;
885
886 return 0;
887 }
888
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 20788 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-09 14:02 ` [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander AngeloGioacchino Del Regno
2021-01-09 17:24 ` kernel test robot
@ 2021-01-09 22:11 ` Linus Walleij
2021-01-09 23:11 ` AngeloGioacchino Del Regno
1 sibling, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2021-01-09 22:11 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: open list:GPIO SUBSYSTEM, konrad.dybcio, marijn.suijten,
martin.botka, phone-devel, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring
On Sat, Jan 9, 2021 at 3:02 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
> The Awinic AW9523(B) is a multi-function I2C gpio expander in a
> TQFN-24L package, featuring PWM (max 37mA per pin, or total max
> power 3.2Watts) for LED driving capability.
>
> It has two ports with 8 pins per port (for a total of 16 pins),
> configurable as either PWM with 1/256 stepping or GPIO input/output,
> 1.8V logic input; each GPIO can be configured as input or output
> independently from each other.
>
> This IC also has an internal interrupt controller, which is capable
> of generating an interrupt for each GPIO, depending on the
> configuration, and will raise an interrupt on the INTN pin to
> advertise this to an external interrupt controller.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
Okay!
Overall this driver is in good shape.
The major review comment is that it'd be nice if you look into
using regmaps register cache instead of rolling your own,
and also possibly using regmaps locking rather than your own
as a result of that.
> +config PINCTRL_AW9523
> + bool "Awinic AW9523/AW9523B I2C GPIO expander pinctrl driver"
> + depends on OF && I2C
> + select PINMUX
> + select PINCONF
> + select GENERIC_PINCONF
> + select GPIOLIB
> + select GPIOLIB_IRQCHIP
> + select REGMAP
> + help
> + The Awinic AW9523/AW9523B is a multi-function I2C GPIO
> + expander with PWM functionality. This driver bundles a
> + pinctrl driver to select the function muxing and a GPIO
> + driver to handle GPIO, when the GPIO function is selected.
> +
> + Say yes to enable pinctrl and GPIO support for the AW9523(B).
This:
+ DECLARE_BITMAP(old_masked[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT);
+ DECLARE_BITMAP(masked[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT)
(...)
+ DECLARE_BITMAP(direction_in[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT);
And this looks like a reimplementation of the existing register cache
in regmap. So use regmaps regcache instead. (More notes on that
below.)
This looks good. Right dependencies and helpers.
> + int hw_pin = pin % AW9523_PINS_PER_PORT;
This makes me a bit wary.
Is that really the "hardware pin" as it looks? It looks more like
the bit number 0..7 in the register for that port. I would just name these
"regbit" or just "n" like you do in the irq code.
> +/*
> + * __aw9523_gpio_get_direction - Get pin direction
> + * @regmap: Regmap structure
> + * @pin: gpiolib pin number
> + * @hwp: pin index in port register
> + *
> + * Return: Pin direction for success or negative number for error
> + */
> +static int __aw9523_gpio_get_direction(struct regmap *regmap, u8 pin, u8 hwp)
Nitpick: I kind of dislike __underscore functions because they have
ambiguous semantics. Sometimes it is a compiler thing. Sometimes
it is an inner function from something wrapped, i.e. it depends on
context what these underscores
mean. What about finding a better name that says what the function
is doing?
> +static int __aw9523_get_port_state(struct regmap *regmap, u8 pin,
> + u8 hw_pin, unsigned int *state)
Same.
> +static int aw9523_gpio_irq_type(struct irq_data *d, unsigned int type)
> +{
> + switch (type) {
> + case IRQ_TYPE_NONE:
> + case IRQ_TYPE_LEVEL_MASK:
> + case IRQ_TYPE_LEVEL_HIGH:
> + case IRQ_TYPE_LEVEL_LOW:
> + case IRQ_TYPE_EDGE_BOTH:
> + case IRQ_TYPE_EDGE_RISING:
> + case IRQ_TYPE_EDGE_FALLING:
> + return 0;
Does this hardware really support all these edge types without any
software configuration whatsoever. That looks weird.
> +static irqreturn_t aw9523_irq_thread_func(int irq, void *dev_id)
> +{
> + struct aw9523 *awi = (struct aw9523 *)dev_id;
> + unsigned long n, val = 0;
> + unsigned long changed_gpio;
> + unsigned int tmp, port_pin, i, ret;
> +
> + for (i = 0; i < AW9523_NUM_PORTS; i++) {
> + port_pin = i * AW9523_PINS_PER_PORT;
> + ret = regmap_read(awi->regmap,
> + AW9523_REG_IN_STATE(port_pin),
> + &tmp);
> + if (ret)
> + return ret;
> +
> + val |= (u8)tmp << (i * 8);
> + }
Can you convince me that these are not just consecutive registers
that could be read in one go with regmap_bulk_read()?
(I could not unwind the macros in my head, and you have the
datasheet I suppose.)
> +/*
> + * aw9523_irq_bus_sync_unlock - Synchronize state and unlock
> + * @d: irq data
> + *
> + * Writes the interrupt mask bits (found in the bit map) to the
> + * hardware, then unlocks the bus.
> + */
> +static void aw9523_irq_bus_sync_unlock(struct irq_data *d)
> +{
> + struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
> + int i;
> +
> + for (i = 0; i < AW9523_NUM_PORTS; i++) {
> + if (bitmap_equal(awi->irq->masked[i], awi->irq->old_masked[i],
> + AW9523_PINS_PER_PORT))
> + continue;
> + regmap_write(awi->regmap,
> + AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT * i),
> + *awi->irq->masked[i]);
> + bitmap_copy(awi->irq->old_masked[i], awi->irq->masked[i],
> + AW9523_PINS_PER_PORT);
> + }
> + mutex_unlock(&awi->irq->lock);
> +}
These copies in the state that you write out at sync unlock.
Can this not be done using the async facility in regmap?
regmap_write_async()/regcache_mark_dirty() in all the IRQ
config etc functions, followed by a simple
regcache_sync() here makes it unnecessary to keep your
own register cache I believe?
At least that is how I always thought it was supposed to be
used.
> +static int aw9523_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct aw9523 *awi = gpiochip_get_data(chip);
> + u8 hw_pin = offset % AW9523_PINS_PER_PORT;
> + int port = AW9523_PIN_TO_PORT(offset);
> +
> + set_bit(offset, awi->direction_in[port]);
This direction_in state seems to be another reimplementation of regmaps
register cache.
> +static int aw9523_hw_reset(struct aw9523 *awi)
> +{
> + int ret, max_retries = 2;
> +
> + /* Sometimes the chip needs more than one reset cycle */
> + do {
> + ret = __aw9523_hw_reset(awi);
Please give a better name to the inner function. Like
aw9523_drive_reset_gpio() or so.
> + for (i = 0; i < AW9523_NUM_PORTS; i++) {
> + bitmap_fill(awi->irq->masked[i], AW9523_PINS_PER_PORT);
> + bitmap_fill(awi->irq->old_masked[i], AW9523_PINS_PER_PORT);
> + }
This is another of these complications of reimplementing regmaps
register cache.
> +static const struct regmap_config aw9523_regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .cache_type = REGCACHE_NONE,
By using some elaborate caching here instead of implementing
your own, the driver can be simplified.
> + .disable_locking = true,
Are you sure you are not just reimplementing this locking
with your mutex?
> +static struct i2c_driver aw9523_driver = {
> + .driver = {
> + .name = "aw9523-pinctrl",
> + .of_match_table = of_aw9523_i2c_match,
> + },
> + .probe = aw9523_probe,
A lot of people (especially on Qualcomm platforms, which is used in the
DT binding example) are working to modularize pin controllers.
This controller on a slow bus should be able to support .remove() I
think?
You should even be able to insmod/rmmod it at runtime for testing.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-09 17:24 ` kernel test robot
@ 2021-01-09 22:12 ` Linus Walleij
-1 siblings, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2021-01-09 22:12 UTC (permalink / raw)
To: kernel test robot
Cc: AngeloGioacchino Del Regno, open list:GPIO SUBSYSTEM, kbuild-all,
konrad.dybcio, marijn.suijten, martin.botka, phone-devel,
linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring
On Sat, Jan 9, 2021 at 6:24 PM kernel test robot <lkp@intel.com> wrote:
> > 880 gpioirq->parent_domain = NULL;
The autobuilder is complaining because your irq chip is not
hierarchical and this is only used for hierarchical irqchips.
I think you can just delete this line.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
@ 2021-01-09 22:12 ` Linus Walleij
0 siblings, 0 replies; 19+ messages in thread
From: Linus Walleij @ 2021-01-09 22:12 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 314 bytes --]
On Sat, Jan 9, 2021 at 6:24 PM kernel test robot <lkp@intel.com> wrote:
> > 880 gpioirq->parent_domain = NULL;
The autobuilder is complaining because your irq chip is not
hierarchical and this is only used for hierarchical irqchips.
I think you can just delete this line.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B
2021-01-09 14:02 ` [PATCH 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B AngeloGioacchino Del Regno
@ 2021-01-09 22:14 ` Linus Walleij
2021-01-09 23:13 ` AngeloGioacchino Del Regno
2021-01-10 17:18 ` Rob Herring
1 sibling, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2021-01-09 22:14 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: open list:GPIO SUBSYSTEM, konrad.dybcio, marijn.suijten,
martin.botka, phone-devel, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring
Hi Angelo,
thanks for your patch!
On Sat, Jan 9, 2021 at 3:02 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
> +#PIN CONFIGURATION NODES
> +patternProperties:
> + '^.*$':
> + if:
> + type: object
> + then:
> + properties:
> + pins:
> + description:
> + List of gpio pins affected by the properties specified in
> + this subnode.
> + items:
> + pattern: "^gpio([0-9]|1[0-5])$"
> + minItems: 1
> + maxItems: 16
> +
> + function:
> + description:
> + Specify the alternative function to be configured for the
> + specified pins.
> +
> + enum: [ gpio, pwm ]
> +
> + bias-disable: true
> + bias-pull-down: true
> + bias-pull-up: true
> + drive-open-drain: true
> + drive-push-pull: true
> + input-enable: true
> + output-high: true
> + output-low: true
> +
> + required:
> + - pins
> + - function
Is it possible to just $ref /pinctrl/pincfg-node.yaml# for some of this?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-09 22:11 ` Linus Walleij
@ 2021-01-09 23:11 ` AngeloGioacchino Del Regno
2021-01-10 0:24 ` Linus Walleij
0 siblings, 1 reply; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-09 23:11 UTC (permalink / raw)
To: Linus Walleij
Cc: open list:GPIO SUBSYSTEM, konrad.dybcio, marijn.suijten,
martin.botka, phone-devel, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring
Il 09/01/21 23:11, Linus Walleij ha scritto:
> On Sat, Jan 9, 2021 at 3:02 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
>
>> The Awinic AW9523(B) is a multi-function I2C gpio expander in a
>> TQFN-24L package, featuring PWM (max 37mA per pin, or total max
>> power 3.2Watts) for LED driving capability.
>>
>> It has two ports with 8 pins per port (for a total of 16 pins),
>> configurable as either PWM with 1/256 stepping or GPIO input/output,
>> 1.8V logic input; each GPIO can be configured as input or output
>> independently from each other.
>>
>> This IC also has an internal interrupt controller, which is capable
>> of generating an interrupt for each GPIO, depending on the
>> configuration, and will raise an interrupt on the INTN pin to
>> advertise this to an external interrupt controller.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
>
> Okay!
>
> Overall this driver is in good shape.
>
> The major review comment is that it'd be nice if you look into
> using regmaps register cache instead of rolling your own,
> and also possibly using regmaps locking rather than your own
> as a result of that.
>
Actually, I really tried to use regmap's FLAT register cache and after
many, many tries... I had to give up. I just couldn't get it working. :(
>> +config PINCTRL_AW9523
>> + bool "Awinic AW9523/AW9523B I2C GPIO expander pinctrl driver"
>> + depends on OF && I2C
>> + select PINMUX
>> + select PINCONF
>> + select GENERIC_PINCONF
>> + select GPIOLIB
>> + select GPIOLIB_IRQCHIP
>> + select REGMAP
>> + help
>> + The Awinic AW9523/AW9523B is a multi-function I2C GPIO
>> + expander with PWM functionality. This driver bundles a
>> + pinctrl driver to select the function muxing and a GPIO
>> + driver to handle GPIO, when the GPIO function is selected.
>> +
>> + Say yes to enable pinctrl and GPIO support for the AW9523(B).
>
> This:
>
> + DECLARE_BITMAP(old_masked[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT);
> + DECLARE_BITMAP(masked[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT)
> (...)
> + DECLARE_BITMAP(direction_in[AW9523_NUM_PORTS], AW9523_PINS_PER_PORT);
>
> And this looks like a reimplementation of the existing register cache
> in regmap. So use regmaps regcache instead. (More notes on that
> below.)
>
> This looks good. Right dependencies and helpers.
>
>> + int hw_pin = pin % AW9523_PINS_PER_PORT;
>
> This makes me a bit wary.
>
> Is that really the "hardware pin" as it looks? It looks more like
> the bit number 0..7 in the register for that port. I would just name these
> "regbit" or just "n" like you do in the irq code.
>
Yes this is the bit number 0..7, you've understood that right. I guess
renaming it to "regbit" is a good choice, makes it more understandable!
>> +/*
>> + * __aw9523_gpio_get_direction - Get pin direction
>> + * @regmap: Regmap structure
>> + * @pin: gpiolib pin number
>> + * @hwp: pin index in port register
>> + *
>> + * Return: Pin direction for success or negative number for error
>> + */
>> +static int __aw9523_gpio_get_direction(struct regmap *regmap, u8 pin, u8 hwp)
>
> Nitpick: I kind of dislike __underscore functions because they have
> ambiguous semantics. Sometimes it is a compiler thing. Sometimes
> it is an inner function from something wrapped, i.e. it depends on
> context what these underscores
> mean. What about finding a better name that says what the function
> is doing?
>
My initial idea was aw9523_get_pin_direction... then I changed it to
include the word "gpio" in an attempt to make it less confusing. Let's
go for the initial one then!
>> +static int __aw9523_get_port_state(struct regmap *regmap, u8 pin,
>> + u8 hw_pin, unsigned int *state)
>
> Same.
>
...And here I had another function without __prefix, which was then
merged into another one as having it separated made no sense, then I
forgot to remove the underscores. Oops! Removed!
>> +static int aw9523_gpio_irq_type(struct irq_data *d, unsigned int type)
>> +{
>> + switch (type) {
>> + case IRQ_TYPE_NONE:
>> + case IRQ_TYPE_LEVEL_MASK:
>> + case IRQ_TYPE_LEVEL_HIGH:
>> + case IRQ_TYPE_LEVEL_LOW:
>> + case IRQ_TYPE_EDGE_BOTH:
>> + case IRQ_TYPE_EDGE_RISING:
>> + case IRQ_TYPE_EDGE_FALLING:
>> + return 0;
>
> Does this hardware really support all these edge types without any
> software configuration whatsoever. That looks weird.
>
And it would indeed be weird: I've rechecked the datasheet again and
only LEVEL interrupts are supported. As stated there: "When AW9523B
detect port change, any input state from high-level to low-level or from
low-level to high-level will generate interrupt after 8us internal
deglitch."
I wonder what happened with my brain, there...
>> +static irqreturn_t aw9523_irq_thread_func(int irq, void *dev_id)
>> +{
>> + struct aw9523 *awi = (struct aw9523 *)dev_id;
>> + unsigned long n, val = 0;
>> + unsigned long changed_gpio;
>> + unsigned int tmp, port_pin, i, ret;
>> +
>> + for (i = 0; i < AW9523_NUM_PORTS; i++) {
>> + port_pin = i * AW9523_PINS_PER_PORT;
>> + ret = regmap_read(awi->regmap,
>> + AW9523_REG_IN_STATE(port_pin),
>> + &tmp);
>> + if (ret)
>> + return ret;
>> +
>> + val |= (u8)tmp << (i * 8);
>> + }
>
> Can you convince me that these are not just consecutive registers
> that could be read in one go with regmap_bulk_read()?
> (I could not unwind the macros in my head, and you have the
> datasheet I suppose.)
>
I cannot and I would never convince you of something wrong: yes, this is
a read of two (and only two) consecutive registers. Here, I didn't go
for regmap_bulk_read in favor of a "paranoid" performance optimization
of this operation: in regmap_bulk_read we have 2 if branches, 1 if-else
branch, plus another "implicit" (regmap_get_offset) if-else branch, and
a switch. That's exactly what I'm avoiding with this for loop... for 1.5
times.
...And that's the full story: all about keeping overhead as minimal as
possible.
However, if it's really necessary to get that (even if very small)
overhead, I can switch that to a regmap_bulk_read call... but from my
perspective, having less instructions is better for many reasons.
A typical case of "less is more", I guess?
>> +/*
>> + * aw9523_irq_bus_sync_unlock - Synchronize state and unlock
>> + * @d: irq data
>> + *
>> + * Writes the interrupt mask bits (found in the bit map) to the
>> + * hardware, then unlocks the bus.
>> + */
>> +static void aw9523_irq_bus_sync_unlock(struct irq_data *d)
>> +{
>> + struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
>> + int i;
>> +
>> + for (i = 0; i < AW9523_NUM_PORTS; i++) {
>> + if (bitmap_equal(awi->irq->masked[i], awi->irq->old_masked[i],
>> + AW9523_PINS_PER_PORT))
>> + continue;
>> + regmap_write(awi->regmap,
>> + AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT * i),
>> + *awi->irq->masked[i]);
>> + bitmap_copy(awi->irq->old_masked[i], awi->irq->masked[i],
>> + AW9523_PINS_PER_PORT);
>> + }
>> + mutex_unlock(&awi->irq->lock);
>> +}
>
> These copies in the state that you write out at sync unlock.
>
> Can this not be done using the async facility in regmap?
>
> regmap_write_async()/regcache_mark_dirty() in all the IRQ
> config etc functions, followed by a simple
> regcache_sync() here makes it unnecessary to keep your
> own register cache I believe?
>
> At least that is how I always thought it was supposed to be
> used.
>
As I wrote earlier, unfortunately I tried hard... but I couldn't succeed...
>> +static int aw9523_direction_input(struct gpio_chip *chip, unsigned int offset)
>> +{
>> + struct aw9523 *awi = gpiochip_get_data(chip);
>> + u8 hw_pin = offset % AW9523_PINS_PER_PORT;
>> + int port = AW9523_PIN_TO_PORT(offset);
>> +
>> + set_bit(offset, awi->direction_in[port]);
>
> This direction_in state seems to be another reimplementation of regmaps
> register cache.
>
>> +static int aw9523_hw_reset(struct aw9523 *awi)
>> +{
>> + int ret, max_retries = 2;
>> +
>> + /* Sometimes the chip needs more than one reset cycle */
>> + do {
>> + ret = __aw9523_hw_reset(awi);
>
> Please give a better name to the inner function. Like
> aw9523_drive_reset_gpio() or so.
>
I like it. aw9523_drive_reset_gpio it is!
>> + for (i = 0; i < AW9523_NUM_PORTS; i++) {
>> + bitmap_fill(awi->irq->masked[i], AW9523_PINS_PER_PORT);
>> + bitmap_fill(awi->irq->old_masked[i], AW9523_PINS_PER_PORT);
>> + }
>
> This is another of these complications of reimplementing regmaps
> register cache.
>
>> +static const struct regmap_config aw9523_regmap = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> +
>> + .cache_type = REGCACHE_NONE,
>
> By using some elaborate caching here instead of implementing
> your own, the driver can be simplified.
>
>> + .disable_locking = true,
>
> Are you sure you are not just reimplementing this locking
> with your mutex?
>
Yes, I am using more specialized locking, which results in less
lock-unlock operations in many cases, bringing *a lot* less overhead.
Using the regmap locking, my keyboard matrix was a lot slower: I really
had the need to optimize this driver's performance as much as possible.
>> +static struct i2c_driver aw9523_driver = {
>> + .driver = {
>> + .name = "aw9523-pinctrl",
>> + .of_match_table = of_aw9523_i2c_match,
>> + },
>> + .probe = aw9523_probe,
>
> A lot of people (especially on Qualcomm platforms, which is used in the
> DT binding example) are working to modularize pin controllers.
>
> This controller on a slow bus should be able to support .remove() I
> think?
>
> You should even be able to insmod/rmmod it at runtime for testing.
>
Actually, yes. I will add a .remove callback.
You will get a V2 of this driver tomorrow!
-- Angelo
> Yours,
> Linus Walleij
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-09 22:12 ` Linus Walleij
(?)
@ 2021-01-09 23:12 ` AngeloGioacchino Del Regno
-1 siblings, 0 replies; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-09 23:12 UTC (permalink / raw)
To: Linus Walleij, kernel test robot
Cc: open list:GPIO SUBSYSTEM, kbuild-all, konrad.dybcio,
marijn.suijten, martin.botka, phone-devel, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring
Il 09/01/21 23:12, Linus Walleij ha scritto:
> On Sat, Jan 9, 2021 at 6:24 PM kernel test robot <lkp@intel.com> wrote:
>
>> > 880 gpioirq->parent_domain = NULL;
>
> The autobuilder is complaining because your irq chip is not
> hierarchical and this is only used for hierarchical irqchips.
> I think you can just delete this line.
>
That's a development leftover. Big oops! Removed in V2 :)
> Yours,
> Linus Walleij
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B
2021-01-09 22:14 ` Linus Walleij
@ 2021-01-09 23:13 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-09 23:13 UTC (permalink / raw)
To: Linus Walleij
Cc: open list:GPIO SUBSYSTEM, konrad.dybcio, marijn.suijten,
martin.botka, phone-devel, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring
Il 09/01/21 23:14, Linus Walleij ha scritto:
> Hi Angelo,
>
> thanks for your patch!
>
> On Sat, Jan 9, 2021 at 3:02 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
>
>> +#PIN CONFIGURATION NODES
>> +patternProperties:
>> + '^.*$':
>> + if:
>> + type: object
>> + then:
>> + properties:
>> + pins:
>> + description:
>> + List of gpio pins affected by the properties specified in
>> + this subnode.
>> + items:
>> + pattern: "^gpio([0-9]|1[0-5])$"
>> + minItems: 1
>> + maxItems: 16
>> +
>> + function:
>> + description:
>> + Specify the alternative function to be configured for the
>> + specified pins.
>> +
>> + enum: [ gpio, pwm ]
>> +
>> + bias-disable: true
>> + bias-pull-down: true
>> + bias-pull-up: true
>> + drive-open-drain: true
>> + drive-push-pull: true
>> + input-enable: true
>> + output-high: true
>> + output-low: true
>> +
>> + required:
>> + - pins
>> + - function
>
> Is it possible to just $ref /pinctrl/pincfg-node.yaml# for some of this?
>
Sure, I will try to reference that for V2!
> Yours,
> Linus Walleij
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-09 23:11 ` AngeloGioacchino Del Regno
@ 2021-01-10 0:24 ` Linus Walleij
2021-01-10 14:32 ` AngeloGioacchino Del Regno
0 siblings, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2021-01-10 0:24 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Mark Brown
Cc: open list:GPIO SUBSYSTEM, konrad.dybcio, marijn.suijten,
martin.botka, phone-devel, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring
On Sun, Jan 10, 2021 at 12:11 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
> Il 09/01/21 23:11, Linus Walleij ha scritto:
> > The major review comment is that it'd be nice if you look into
> > using regmaps register cache instead of rolling your own,
> > and also possibly using regmaps locking rather than your own
> > as a result of that.
> >
> Actually, I really tried to use regmap's FLAT register cache and after
> many, many tries... I had to give up. I just couldn't get it working. :(
This needs to be root-caused. The register cache in regmap is for
using not for decoration.
What is the problems you are seeing?
If it is fundamentally so that regmap has limitations that is one thing,
but I want to rule out that we're just not using it wrong or that there
is a bug in it that we should fix.
Looping in Mark Brown the regmap maintainer.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-10 0:24 ` Linus Walleij
@ 2021-01-10 14:32 ` AngeloGioacchino Del Regno
2021-01-10 14:56 ` AngeloGioacchino Del Regno
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-10 14:32 UTC (permalink / raw)
To: Linus Walleij, Mark Brown
Cc: open list:GPIO SUBSYSTEM, konrad.dybcio, marijn.suijten,
martin.botka, phone-devel, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring
Il 10/01/21 01:24, Linus Walleij ha scritto:
> On Sun, Jan 10, 2021 at 12:11 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
>> Il 09/01/21 23:11, Linus Walleij ha scritto:
>
>>> The major review comment is that it'd be nice if you look into
>>> using regmaps register cache instead of rolling your own,
>>> and also possibly using regmaps locking rather than your own
>>> as a result of that.
>>>
>> Actually, I really tried to use regmap's FLAT register cache and after
>> many, many tries... I had to give up. I just couldn't get it working. :(
>
> This needs to be root-caused. The register cache in regmap is for
> using not for decoration.
>
> What is the problems you are seeing?
>
> If it is fundamentally so that regmap has limitations that is one thing,
> but I want to rule out that we're just not using it wrong or that there
> is a bug in it that we should fix.
>
> Looping in Mark Brown the regmap maintainer.
>
> Yours,
> Linus Walleij
>
Oh don't misunderstand me, I would really be happy to make use of the
regmap register cache, it's a nice one.
So, I've retried some basic usage of the regcache, relevant snippets here:
static bool aw9523_volatile_reg(struct device *dev, unsigned int reg)
{
return reg == AW9523_REG_IN_STATE(0) ||
reg == AW9523_REG_IN_STATE(AW9523_PINS_PER_PORT) ||
reg == AW9523_REG_CHIPID;
}
static const struct regmap_config aw9523_regmap = {
....
.volatile_reg = aw9523_volatile_reg,
.cache_type = REGCACHE_FLAT,
....
}
Since REG_IN_STATE is used to read the GPIO input level, it's not
cacheable, then CHIPID was set as not cacheable for safety: that may be
avoided, but that may make no sense.. since it's a one-time readout for
init putposes, it'd be useless to keep it cached.
Then, the set_bit/clear_bit in aw9523_irq_mask(), aw9523_irq_unmask were
replaced with calls to regmap_update_bits_async, example:
regmap_update_bits_async(awi->regmap,
AW9523_REG_INTR_DIS(d->hwirq),
BIT(n), BIT(n));
Where of course the value is either BIT(n) or 0 for mask and unmask
respectively.
Also, the bus_sync_unlock callback was changed as follows:
static void aw9523_irq_mask(struct irq_data *d)
{
struct aw9523 *awi =
gpiochip_get_data(irq_data_get_irq_chip_data(d));
unsigned int n = d->hwirq % AW9523_PINS_PER_PORT;
regmap_update_bits_async(awi->regmap,
AW9523_REG_INTR_DIS(d->hwirq),
BIT(n), BIT(n));
}
One of the biggest / oddest issues that I get when trying to use
regcache is that I'm getting badbadbad scheduling while atomic warnings
all over and I don't get why, since regcache_default_sync is just
calling _regmap_write, which is exactly what (non _prefix) regmap_write
also calls...
As a reference, this is one out of "many" (as you can imagine) stacktraces:
<3>[ 1.061428] BUG: scheduling while atomic: kworker/3:1/119/0x00000000
<4>[ 1.061575] Modules linked in:
<4>[ 1.061716] CPU: 3 PID: 119 Comm: kworker/3:1 Tainted: G W
5.10.0-rc3-31949-ge1680e3101bc-dirty #1162
<4>[ 1.061956] Hardware name: F(x)tec Pro1 (QX1000) (DT)
<4>[ 1.062081] Workqueue: events deferred_probe_work_func
<4>[ 1.062205] Call trace:
<4>[ 1.062333] dump_backtrace+0x0/0x1e0
<4>[ 1.062439] show_stack+0x14/0x60
<4>[ 1.062539] dump_stack+0xd4/0x12c
<4>[ 1.062680] __schedule_bug+0x50/0x70
<4>[ 1.062785] __schedule+0x618/0x650
<4>[ 1.062888] schedule+0x6c/0xf8
<4>[ 1.062985] schedule_timeout+0x1d0/0x260
<4>[ 1.063134] wait_for_completion_timeout+0x8c/0x110
<4>[ 1.063257] qup_i2c_wait_for_complete.isra.18+0x1c/0x80
<4>[ 1.063429] qup_i2c_xfer_v2_msg+0x2d4/0x3f0
<4>[ 1.063543] qup_i2c_xfer_v2+0x290/0xa28
<4>[ 1.063652] __i2c_transfer+0x16c/0x380
<4>[ 1.063798] i2c_transfer+0x5c/0x138
<4>[ 1.063903] i2c_transfer_buffer_flags+0x58/0x80
<4>[ 1.064060] regmap_i2c_write+0x1c/0x50
<4>[ 1.064168] _regmap_raw_write_impl+0x35c/0x688
<4>[ 1.064285] _regmap_bus_raw_write+0x64/0x80
<4>[ 1.064440] _regmap_write+0x58/0xa8
<4>[ 1.064545] regcache_default_sync+0xcc/0x1a0
<4>[ 1.064660] regcache_sync_region+0xdc/0xe8
<4>[ 1.064811] aw9523_irq_bus_sync_unlock+0x30/0x48
<4>[ 1.064931] __setup_irq+0x798/0x890
<4>[ 1.065034] request_threaded_irq+0xe0/0x198
<4>[ 1.065188] devm_request_threaded_irq+0x78/0xf8
<4>[ 1.065311] gpio_keyboard_probe+0x2a8/0x468
<4>[ 1.065465] platform_drv_probe+0x50/0xa0
<4>[ 1.065576] really_probe+0x290/0x4e8
<4>[ 1.065682] driver_probe_device+0xf4/0x160
<4>[ 1.065834] __device_attach_driver+0x98/0x110
<4>[ 1.065950] bus_for_each_drv+0x64/0xc8
<4>[ 1.066063] __device_attach+0xe4/0x168
<4>[ 1.066211] device_initial_probe+0x10/0x18
<4>[ 1.066325] bus_probe_device+0x90/0x98
<4>[ 1.066434] deferred_probe_work_func+0x88/0xe0
<4>[ 1.066591] process_one_work+0x1e4/0x358
<4>[ 1.066702] worker_thread+0x208/0x478
<4>[ 1.073273] kthread+0x14c/0x150
<4>[ 1.079858] ret_from_fork+0x10/0x18
P.S.: Infinite thanks for being so nice and helpful!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-10 14:32 ` AngeloGioacchino Del Regno
@ 2021-01-10 14:56 ` AngeloGioacchino Del Regno
2021-01-10 19:35 ` Linus Walleij
2021-01-11 13:10 ` Mark Brown
2 siblings, 0 replies; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-10 14:56 UTC (permalink / raw)
To: Linus Walleij, Mark Brown
Cc: open list:GPIO SUBSYSTEM, konrad.dybcio, marijn.suijten,
martin.botka, phone-devel, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring
Il 10/01/21 15:32, AngeloGioacchino Del Regno ha scritto:
> Il 10/01/21 01:24, Linus Walleij ha scritto:
>> On Sun, Jan 10, 2021 at 12:11 AM AngeloGioacchino Del Regno
>> <angelogioacchino.delregno@somainline.org> wrote:
>>> Il 09/01/21 23:11, Linus Walleij ha scritto:
>>
>>>> The major review comment is that it'd be nice if you look into
>>>> using regmaps register cache instead of rolling your own,
>>>> and also possibly using regmaps locking rather than your own
>>>> as a result of that.
>>>>
>>> Actually, I really tried to use regmap's FLAT register cache and after
>>> many, many tries... I had to give up. I just couldn't get it working. :(
>>
>> This needs to be root-caused. The register cache in regmap is for
>> using not for decoration.
>>
>> What is the problems you are seeing?
>>
>> If it is fundamentally so that regmap has limitations that is one thing,
>> but I want to rule out that we're just not using it wrong or that there
>> is a bug in it that we should fix.
>>
>> Looping in Mark Brown the regmap maintainer.
>>
>> Yours,
>> Linus Walleij
>>
>
> Oh don't misunderstand me, I would really be happy to make use of the
> regmap register cache, it's a nice one.
>
> So, I've retried some basic usage of the regcache, relevant snippets here:
> static bool aw9523_volatile_reg(struct device *dev, unsigned int reg)
>
> {
>
> return reg == AW9523_REG_IN_STATE(0) ||
>
> reg == AW9523_REG_IN_STATE(AW9523_PINS_PER_PORT) ||
>
> reg == AW9523_REG_CHIPID;
>
> }
>
> static const struct regmap_config aw9523_regmap = {
>
> ....
> .volatile_reg = aw9523_volatile_reg,
>
>
> .cache_type = REGCACHE_FLAT,
>
> ....
>
> }
>
> Since REG_IN_STATE is used to read the GPIO input level, it's not
> cacheable, then CHIPID was set as not cacheable for safety: that may be
> avoided, but that may make no sense.. since it's a one-time readout for
> init putposes, it'd be useless to keep it cached.
>
> Then, the set_bit/clear_bit in aw9523_irq_mask(), aw9523_irq_unmask were
> replaced with calls to regmap_update_bits_async, example:
>
> regmap_update_bits_async(awi->regmap,
>
> AW9523_REG_INTR_DIS(d->hwirq),
>
> BIT(n), BIT(n));
>
>
>
> Where of course the value is either BIT(n) or 0 for mask and unmask
> respectively.
> Also, the bus_sync_unlock callback was changed as follows:
>
> static void aw9523_irq_mask(struct irq_data *d)
>
> {
>
> struct aw9523 *awi =
> gpiochip_get_data(irq_data_get_irq_chip_data(d));
>
> unsigned int n = d->hwirq % AW9523_PINS_PER_PORT;
>
>
>
> regmap_update_bits_async(awi->regmap,
>
> AW9523_REG_INTR_DIS(d->hwirq),
>
> BIT(n), BIT(n));
>
> }
Sorry, wrong paste! This is the right one:
static void aw9523_irq_bus_sync_unlock(struct irq_data *d)
{
struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
regcache_mark_dirty(awi->regmap);
regcache_sync_region(awi->regmap, AW9523_REG_INTR_DIS(0),
AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT));
mutex_unlock(&awi->irq->lock);
}
>
> One of the biggest / oddest issues that I get when trying to use
> regcache is that I'm getting badbadbad scheduling while atomic warnings
> all over and I don't get why, since regcache_default_sync is just
> calling _regmap_write, which is exactly what (non _prefix) regmap_write
> also calls...
>
> As a reference, this is one out of "many" (as you can imagine) stacktraces:
>
> <3>[ 1.061428] BUG: scheduling while atomic: kworker/3:1/119/0x00000000
>
> <4>[ 1.061575] Modules linked in:
>
> <4>[ 1.061716] CPU: 3 PID: 119 Comm: kworker/3:1 Tainted: G W
> 5.10.0-rc3-31949-ge1680e3101bc-dirty #1162
>
> <4>[ 1.061956] Hardware name: F(x)tec Pro1 (QX1000) (DT)
>
> <4>[ 1.062081] Workqueue: events deferred_probe_work_func
>
> <4>[ 1.062205] Call trace:
>
> <4>[ 1.062333] dump_backtrace+0x0/0x1e0
>
> <4>[ 1.062439] show_stack+0x14/0x60
>
> <4>[ 1.062539] dump_stack+0xd4/0x12c
>
> <4>[ 1.062680] __schedule_bug+0x50/0x70
>
> <4>[ 1.062785] __schedule+0x618/0x650
>
> <4>[ 1.062888] schedule+0x6c/0xf8
>
> <4>[ 1.062985] schedule_timeout+0x1d0/0x260
>
> <4>[ 1.063134] wait_for_completion_timeout+0x8c/0x110
>
> <4>[ 1.063257] qup_i2c_wait_for_complete.isra.18+0x1c/0x80
>
> <4>[ 1.063429] qup_i2c_xfer_v2_msg+0x2d4/0x3f0
>
> <4>[ 1.063543] qup_i2c_xfer_v2+0x290/0xa28
>
> <4>[ 1.063652] __i2c_transfer+0x16c/0x380
>
> <4>[ 1.063798] i2c_transfer+0x5c/0x138
>
> <4>[ 1.063903] i2c_transfer_buffer_flags+0x58/0x80
>
> <4>[ 1.064060] regmap_i2c_write+0x1c/0x50
>
> <4>[ 1.064168] _regmap_raw_write_impl+0x35c/0x688
>
> <4>[ 1.064285] _regmap_bus_raw_write+0x64/0x80
>
> <4>[ 1.064440] _regmap_write+0x58/0xa8
>
> <4>[ 1.064545] regcache_default_sync+0xcc/0x1a0
>
> <4>[ 1.064660] regcache_sync_region+0xdc/0xe8
>
> <4>[ 1.064811] aw9523_irq_bus_sync_unlock+0x30/0x48
>
> <4>[ 1.064931] __setup_irq+0x798/0x890
>
> <4>[ 1.065034] request_threaded_irq+0xe0/0x198
>
> <4>[ 1.065188] devm_request_threaded_irq+0x78/0xf8
>
> <4>[ 1.065311] gpio_keyboard_probe+0x2a8/0x468
>
> <4>[ 1.065465] platform_drv_probe+0x50/0xa0
>
> <4>[ 1.065576] really_probe+0x290/0x4e8
>
> <4>[ 1.065682] driver_probe_device+0xf4/0x160
>
> <4>[ 1.065834] __device_attach_driver+0x98/0x110
>
> <4>[ 1.065950] bus_for_each_drv+0x64/0xc8
>
> <4>[ 1.066063] __device_attach+0xe4/0x168
>
> <4>[ 1.066211] device_initial_probe+0x10/0x18
>
> <4>[ 1.066325] bus_probe_device+0x90/0x98
>
> <4>[ 1.066434] deferred_probe_work_func+0x88/0xe0
>
> <4>[ 1.066591] process_one_work+0x1e4/0x358
>
> <4>[ 1.066702] worker_thread+0x208/0x478
>
> <4>[ 1.073273] kthread+0x14c/0x150
>
> <4>[ 1.079858] ret_from_fork+0x10/0x18
>
>
>
> P.S.: Infinite thanks for being so nice and helpful!
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B
2021-01-09 14:02 ` [PATCH 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B AngeloGioacchino Del Regno
2021-01-09 22:14 ` Linus Walleij
@ 2021-01-10 17:18 ` Rob Herring
1 sibling, 0 replies; 19+ messages in thread
From: Rob Herring @ 2021-01-10 17:18 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: linux-kernel, robh+dt, martin.botka, marijn.suijten, devicetree,
linus.walleij, phone-devel, konrad.dybcio, linux-gpio
On Sat, 09 Jan 2021 15:02:04 +0100, AngeloGioacchino Del Regno wrote:
> Add bindings for the Awinic AW9523/AW9523B I2C GPIO Expander driver.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
> ---
> .../pinctrl/awinic,aw9523-pinctrl.yaml | 111 ++++++++++++++++++
> 1 file changed, 111 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
./Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml:102:1: [error] syntax error: found character '\t' that cannot start any token (syntax)
dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml: while scanning a block scalar
in "<unicode string>", line 94, column 5
found a tab character where an indentation space is expected
in "<unicode string>", line 102, column 1
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.yaml
Traceback (most recent call last):
File "/usr/local/bin/dt-extract-example", line 45, in <module>
binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 343, in load
return constructor.get_single_data()
File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
node = self.composer.get_single_node()
File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node
File "_ruamel_yaml.pyx", line 773, in _ruamel_yaml.CParser._compose_node
File "_ruamel_yaml.pyx", line 848, in _ruamel_yaml.CParser._compose_sequence_node
File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: while scanning a block scalar
in "<unicode string>", line 94, column 5
found a tab character where an indentation space is expected
in "<unicode string>", line 102, column 1
make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.example.dts] Error 1
make[1]: *** Deleting file 'Documentation/devicetree/bindings/pinctrl/awinic,aw9523-pinctrl.example.dts'
make: *** [Makefile:1370: dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1424120
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-10 14:32 ` AngeloGioacchino Del Regno
2021-01-10 14:56 ` AngeloGioacchino Del Regno
@ 2021-01-10 19:35 ` Linus Walleij
2021-01-11 17:54 ` AngeloGioacchino Del Regno
2021-01-11 13:10 ` Mark Brown
2 siblings, 1 reply; 19+ messages in thread
From: Linus Walleij @ 2021-01-10 19:35 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: Mark Brown, open list:GPIO SUBSYSTEM, konrad.dybcio,
marijn.suijten, martin.botka, phone-devel, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring
On Sun, Jan 10, 2021 at 3:32 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
> So, I've retried some basic usage of the regcache, relevant snippets here:
> static bool aw9523_volatile_reg(struct device *dev, unsigned int reg)
> {
>
> return reg == AW9523_REG_IN_STATE(0) ||
> reg == AW9523_REG_IN_STATE(AW9523_PINS_PER_PORT) ||
> reg == AW9523_REG_CHIPID;
> }
(...)
> Since REG_IN_STATE is used to read the GPIO input level, it's not
> cacheable,
Fair enough.
> then CHIPID was set as not cacheable for safety: that may be
> avoided, but that may make no sense.. since it's a one-time readout for
> init putposes, it'd be useless to keep it cached.
I guess.
> Then, the set_bit/clear_bit in aw9523_irq_mask(), aw9523_irq_unmask were
> replaced with calls to regmap_update_bits_async, example:
>
> regmap_update_bits_async(awi->regmap,
> AW9523_REG_INTR_DIS(d->hwirq),
> BIT(n), BIT(n));
>
> Where of course the value is either BIT(n) or 0 for mask and unmask
> respectively.
> Also, the bus_sync_unlock callback was changed as follows:
>
> static void aw9523_irq_bus_sync_unlock(struct irq_data *d)
>
> {
> struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
> regcache_mark_dirty(awi->regmap);
> regcache_sync_region(awi->regmap, AW9523_REG_INTR_DIS(0),
> AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT));
> mutex_unlock(&awi->irq->lock);
(...)
> One of the biggest / oddest issues that I get when trying to use
> regcache is that I'm getting badbadbad scheduling while atomic warnings
> all over and I don't get why, since regcache_default_sync is just
> calling _regmap_write, which is exactly what (non _prefix) regmap_write
> also calls...
OK that is the real problem to solve then.
> As a reference, this is one out of "many" (as you can imagine) stacktraces:
>
> <3>[ 1.061428] BUG: scheduling while atomic: kworker/3:1/119/0x00000000
(...)
> <4>[ 1.063134] wait_for_completion_timeout+0x8c/0x110
> <4>[ 1.063257] qup_i2c_wait_for_complete.isra.18+0x1c/0x80
> <4>[ 1.063429] qup_i2c_xfer_v2_msg+0x2d4/0x3f0
> <4>[ 1.063543] qup_i2c_xfer_v2+0x290/0xa28
> <4>[ 1.063652] __i2c_transfer+0x16c/0x380
> <4>[ 1.063798] i2c_transfer+0x5c/0x138
> <4>[ 1.063903] i2c_transfer_buffer_flags+0x58/0x80
> <4>[ 1.064060] regmap_i2c_write+0x1c/0x50
> <4>[ 1.064168] _regmap_raw_write_impl+0x35c/0x688
> <4>[ 1.064285] _regmap_bus_raw_write+0x64/0x80
> <4>[ 1.064440] _regmap_write+0x58/0xa8
> <4>[ 1.064545] regcache_default_sync+0xcc/0x1a0
> <4>[ 1.064660] regcache_sync_region+0xdc/0xe8
> <4>[ 1.064811] aw9523_irq_bus_sync_unlock+0x30/0x48
> <4>[ 1.064931] __setup_irq+0x798/0x890
> <4>[ 1.065034] request_threaded_irq+0xe0/0x198
> <4>[ 1.065188] devm_request_threaded_irq+0x78/0xf8
> <4>[ 1.065311] gpio_keyboard_probe+0x2a8/0x468
scheduling while atomic happens when this trace gets called with interrupts
disabled, usually because someone has taken a spinlock.
Looking in __setup_irq() it looks safe.
I would turn on lock debugging (lockdep) and see if I can find it that way.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-10 14:32 ` AngeloGioacchino Del Regno
2021-01-10 14:56 ` AngeloGioacchino Del Regno
2021-01-10 19:35 ` Linus Walleij
@ 2021-01-11 13:10 ` Mark Brown
2 siblings, 0 replies; 19+ messages in thread
From: Mark Brown @ 2021-01-11 13:10 UTC (permalink / raw)
To: AngeloGioacchino Del Regno
Cc: Linus Walleij, open list:GPIO SUBSYSTEM, konrad.dybcio,
marijn.suijten, martin.botka, phone-devel, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring
[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]
On Sun, Jan 10, 2021 at 03:32:47PM +0100, AngeloGioacchino Del Regno wrote:
> Then, the set_bit/clear_bit in aw9523_irq_mask(), aw9523_irq_unmask were
> replaced with calls to regmap_update_bits_async, example:
Why are you trying to use the _async() versions here? Don't you need
the I/O to complete synchronously, and what would be the benefit of
dealing with the complexity of making things async? The genirq
framework is definitely going to expect things to be complete before you
return to it...
Note also that this only really does anything for SPI, I2C doesn't offer
an aynchronous API.
> <4>[ 1.064060] regmap_i2c_write+0x1c/0x50
regmap will only use spinlocks if you explictly ask it to do so by
setting fast_io. You can't use spinlock based locking for your register
cache if your device is attached via I2C, I2C is a very slow bus and I2C
controllers will need to sleep. I'd be very surprised if there were any
benefit from using a flat cache with such a device frankly, the I/O is
going to be so slow that any performance gain from using a flat cache is
lost in the noise.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander
2021-01-10 19:35 ` Linus Walleij
@ 2021-01-11 17:54 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 19+ messages in thread
From: AngeloGioacchino Del Regno @ 2021-01-11 17:54 UTC (permalink / raw)
To: Linus Walleij
Cc: Mark Brown, open list:GPIO SUBSYSTEM, konrad.dybcio,
marijn.suijten, martin.botka, phone-devel, linux-kernel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Rob Herring, marek.vasut
Il 10/01/21 20:35, Linus Walleij ha scritto:
> On Sun, Jan 10, 2021 at 3:32 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@somainline.org> wrote:
>
>> So, I've retried some basic usage of the regcache, relevant snippets here:
>> static bool aw9523_volatile_reg(struct device *dev, unsigned int reg)
>> {
>>
>> return reg == AW9523_REG_IN_STATE(0) ||
>> reg == AW9523_REG_IN_STATE(AW9523_PINS_PER_PORT) ||
>> reg == AW9523_REG_CHIPID;
>> }
> (...)
>> Since REG_IN_STATE is used to read the GPIO input level, it's not
>> cacheable,
>
> Fair enough.
>
>> then CHIPID was set as not cacheable for safety: that may be
>> avoided, but that may make no sense.. since it's a one-time readout for
>> init putposes, it'd be useless to keep it cached.
>
> I guess.
>
>> Then, the set_bit/clear_bit in aw9523_irq_mask(), aw9523_irq_unmask were
>> replaced with calls to regmap_update_bits_async, example:
>>
>> regmap_update_bits_async(awi->regmap,
>> AW9523_REG_INTR_DIS(d->hwirq),
>> BIT(n), BIT(n));
>>
>> Where of course the value is either BIT(n) or 0 for mask and unmask
>> respectively.
>> Also, the bus_sync_unlock callback was changed as follows:
>>
>> static void aw9523_irq_bus_sync_unlock(struct irq_data *d)
>>
>> {
>> struct aw9523 *awi = gpiochip_get_data(irq_data_get_irq_chip_data(d));
>> regcache_mark_dirty(awi->regmap);
>> regcache_sync_region(awi->regmap, AW9523_REG_INTR_DIS(0),
>> AW9523_REG_INTR_DIS(AW9523_PINS_PER_PORT));
>> mutex_unlock(&awi->irq->lock);
> (...)
>> One of the biggest / oddest issues that I get when trying to use
>> regcache is that I'm getting badbadbad scheduling while atomic warnings
>> all over and I don't get why, since regcache_default_sync is just
>> calling _regmap_write, which is exactly what (non _prefix) regmap_write
>> also calls...
>
> OK that is the real problem to solve then.
>
>> As a reference, this is one out of "many" (as you can imagine) stacktraces:
>>
>> <3>[ 1.061428] BUG: scheduling while atomic: kworker/3:1/119/0x00000000
> (...)
>> <4>[ 1.063134] wait_for_completion_timeout+0x8c/0x110
>> <4>[ 1.063257] qup_i2c_wait_for_complete.isra.18+0x1c/0x80
>> <4>[ 1.063429] qup_i2c_xfer_v2_msg+0x2d4/0x3f0
>> <4>[ 1.063543] qup_i2c_xfer_v2+0x290/0xa28
>> <4>[ 1.063652] __i2c_transfer+0x16c/0x380
>> <4>[ 1.063798] i2c_transfer+0x5c/0x138
>> <4>[ 1.063903] i2c_transfer_buffer_flags+0x58/0x80
>> <4>[ 1.064060] regmap_i2c_write+0x1c/0x50
>> <4>[ 1.064168] _regmap_raw_write_impl+0x35c/0x688
>> <4>[ 1.064285] _regmap_bus_raw_write+0x64/0x80
>> <4>[ 1.064440] _regmap_write+0x58/0xa8
>> <4>[ 1.064545] regcache_default_sync+0xcc/0x1a0
>> <4>[ 1.064660] regcache_sync_region+0xdc/0xe8
>> <4>[ 1.064811] aw9523_irq_bus_sync_unlock+0x30/0x48
>> <4>[ 1.064931] __setup_irq+0x798/0x890
>> <4>[ 1.065034] request_threaded_irq+0xe0/0x198
>> <4>[ 1.065188] devm_request_threaded_irq+0x78/0xf8
>> <4>[ 1.065311] gpio_keyboard_probe+0x2a8/0x468
>
> scheduling while atomic happens when this trace gets called with interrupts
> disabled, usually because someone has taken a spinlock.
>
> Looking in __setup_irq() it looks safe.
>
> I would turn on lock debugging (lockdep) and see if I can find it that way.
>
> Yours,
> Linus Walleij
>
Hey!
Good news around the corner!
So, the issues were relative to the gpio matrix_keypad driver, which is
protecting with spinlocks (!), "throwing" us in atomic context and
obviously producing this kind of havoc.
Regarding this, I feel like we should bring this to the attention of the
matrix_keypad driver maintainer, Marek Vasut, which I'm including to the
Cc list of this email... but at the same time, that driver seems to be
largely outdated and for this reason I've decided to make one on-the-fly
that uses modern APIs instead and also seems to solve slowness issues on
my KB matrix connected to the AW9523.
Back to our topic, I have solved the issues that were preventing the
usage of a FLAT regcache, cleaned up a bit and tested the entire thing
again.
This works even better than before.
The V2 of this series is coming in a few minutes.
A huge thank you for your help!
-- Angelo
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-01-11 17:55 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-09 14:02 [PATCH 0/2] Add Awinic AW9523(B) I2C GPIO Expander driver AngeloGioacchino Del Regno
2021-01-09 14:02 ` [PATCH 1/2] pinctrl: Add driver for Awinic AW9523/B I2C GPIO Expander AngeloGioacchino Del Regno
2021-01-09 17:24 ` kernel test robot
2021-01-09 17:24 ` kernel test robot
2021-01-09 22:12 ` Linus Walleij
2021-01-09 22:12 ` Linus Walleij
2021-01-09 23:12 ` AngeloGioacchino Del Regno
2021-01-09 22:11 ` Linus Walleij
2021-01-09 23:11 ` AngeloGioacchino Del Regno
2021-01-10 0:24 ` Linus Walleij
2021-01-10 14:32 ` AngeloGioacchino Del Regno
2021-01-10 14:56 ` AngeloGioacchino Del Regno
2021-01-10 19:35 ` Linus Walleij
2021-01-11 17:54 ` AngeloGioacchino Del Regno
2021-01-11 13:10 ` Mark Brown
2021-01-09 14:02 ` [PATCH 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B AngeloGioacchino Del Regno
2021-01-09 22:14 ` Linus Walleij
2021-01-09 23:13 ` AngeloGioacchino Del Regno
2021-01-10 17:18 ` Rob Herring
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.