Add Qualcomm Technologies, Inc. PM8008 MFD driver
v3 cover Add Qualcomm Technologies, Inc. PM8008 MFD driver
Changes from v2:
- Collected Rob Herring's Acked-by for the IRQ listing patch
- Addressed Rob's comments for the dt-bindings patch
Changes from v1:
- Removed errant Change-Id from dt-bindings IRQ patch and gathered Bjorn's Reviewed-by
- Fixed up YAML errors using make dt_binding_check
This driver is dependent on changes that have been made to the regmap-irq framework that have currently been accepted [1][2] in regmap.git upstream by Mark Brown but haven't made it to Linus' tree yet. For this reason, this driver has been based on the tip of regmap.git and not mfd.git.
Those changes, and this driver, are the result of a rewrite effort that was promised a long ago [3]. The framework changes and this driver have been tested and verified end-to-end on an internal platform.
[1] https://lore.kernel.org/lkml/20210318183607.gFxO2hoTO274vl3jUuxWbi19rq9wQELzN-y3B4jvO10@z/
[2] https://lore.kernel.org/lkml/[email protected]/
[3] https://lore.kernel.org/lkml/[email protected]/
Guru Das Srinagesh (3):
dt-bindings: mfd: pm8008: Add IRQ listing
dt-bindings: mfd: pm8008: Add bindings
mfd: pm8008: Add driver for QCOM PM8008 PMIC
.../devicetree/bindings/mfd/qcom,pm8008.yaml | 121 +++++++++ drivers/mfd/Kconfig | 15 ++ drivers/mfd/Makefile | 1 + drivers/mfd/qcom-pm8008.c | 284 +++++++++++++++++++++ include/dt-bindings/mfd/qcom-pm8008.h | 19 ++ 5 files changed, 440 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml create mode 100644 drivers/mfd/qcom-pm8008.c create mode 100644 include/dt-bindings/mfd/qcom-pm8008.h
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v3 1/3 dt-bindings: mfd: pm8008: Add IRQ listing
Add a header file listing all of the IRQs that Qualcomm Technologies, Inc. PM8008 supports. The constants defined in this file may be used in the client device tree node to specify interrupts.
--- include/dt-bindings/mfd/qcom-pm8008.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 include/dt-bindings/mfd/qcom-pm8008.h
include/dt-bindings/mfd/qcom-pm8008.h new file +19 -0
@@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* +* Copyright (c) 2021 The Linux Foundation. All rights reserved. +*/ + +#ifndef __DT_BINDINGS_MFD_QCOM_PM8008_H +#define __DT_BINDINGS_MFD_QCOM_PM8008_H + +/* PM8008 IRQ numbers */ +#define PM8008_IRQ_MISC_UVLO 0 +#define PM8008_IRQ_MISC_OVLO 1 +#define PM8008_IRQ_MISC_OTST2 2 +#define PM8008_IRQ_MISC_OTST3 3 +#define PM8008_IRQ_MISC_LDO_OCP 4 +#define PM8008_IRQ_TEMP_ALARM 5 +#define PM8008_IRQ_GPIO1 6 +#define PM8008_IRQ_GPIO2 7 + +#endif
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
PATCH v3 2/3 dt-bindings: mfd: pm8008: Add bindings
In reply to Guru Das SrinageshAdd bindings for the Qualcomm Technologies, Inc. PM8008 MFD driver.
--- .../devicetree/bindings/mfd/qcom,pm8008.yaml | 121 +++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml new file +121 -0
@@ -0,0 +1,121 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/mfd/qcom,pm8008.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm Technologies, Inc. PM8008 PMIC bindings + +maintainers: + - Guru Das Srinagesh <[email protected]> + +description: | + Qualcomm Technologies, Inc. PM8008 is a dedicated camera PMIC that integrates + all the necessary power management, housekeeping, and interface support + functions into a single IC. + +properties: + compatible: + const: qcom,pm8008 + + reg: + description: + I2C slave address. + + maxItems: 1 + + interrupts: + maxItems: 1 + + description: Parent interrupt. + + "#interrupt-cells": + const: 2 + + description: | + The first cell is the IRQ number, the second cell is the IRQ trigger + flag. All interrupts are listed in include/dt-bindings/mfd/qcom-pm8008.h. + + interrupt-controller: true + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + +patternProperties: + "^gpio@[0-9a-f]+$": + type: object + + description: | + The GPIO peripheral. This node may be specified twice, one for each GPIO. + + properties: + compatible: + const: qcom,pm8008-gpio + + reg: + description: Peripheral address of one of the two GPIO peripherals. + maxItems: 1 + + gpio-controller: true + + interrupt-controller: true + + "#interrupt-cells": + const: 2 + + "#gpio-cells": + const: 2 + + required: + - compatible + - reg + - gpio-controller + - interrupt-controller + - "#gpio-cells" + - "#interrupt-cells" + + additionalProperties: false + +required: + - compatible + - reg + - interrupts + - "#address-cells" + - "#size-cells" + - "#interrupt-cells" + +additionalProperties: false + +examples: + - | + #include <dt-bindings/mfd/qcom-pm8008.h> + #include <dt-bindings/interrupt-controller/irq.h> + qupv3_se13_i2c { + #address-cells = <1>; + #size-cells = <0>; + pm8008i@8 { + compatible = "qcom,pm8008"; + reg = <0x8>; + #address-cells = <1>; + #size-cells = <0>; + interrupt-controller; + #interrupt-cells = <2>; + + interrupt-parent = <&tlmm>; + interrupts = <32 IRQ_TYPE_EDGE_RISING>; + + gpio@c000 { + compatible = "qcom,pm8008-gpio"; + reg = <0xc000>; + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + }; + }; + }; + +...
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v3 2/3] dt-bindings: mfd: pm8008: Add bindings
Quoted 8 lines · Guru Das Srinagesh
Add bindings for the Qualcomm Technologies, Inc. PM8008 MFD driver.
Signed-off-by Guru Das Srinagesh <[email protected]>--- .../devicetree/bindings/mfd/qcom,pm8008.yaml | 121 +++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/qcom,pm8008.yaml
PATCH v3 3/3 mfd: pm8008: Add driver for QCOM PM8008 PMIC
In reply to Guru Das SrinageshQualcomm Technologies, Inc. PM8008 is a dedicated camera PMIC that integrates all the necessary power management, housekeeping, and interface support functions into a single IC. Its key features include overtemperature protection, low-dropout linear regulators, GPIOs, and an I2C interface.
Add an MFD driver to support it.
--- drivers/mfd/Kconfig | 15 +++ drivers/mfd/Makefile | 1 + drivers/mfd/qcom-pm8008.c | 284 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 300 insertions(+) create mode 100644 drivers/mfd/qcom-pm8008.c
drivers/mfd/Kconfig +15 -0
@@ -2087,6 +2087,21 @@ config MFD_ACER_A500_EC The controller itself is ENE KB930, it is running firmware customized for the specific needs of the Acer A500 hardware. +config MFD_QCOM_PM8008 + tristate "QCOM PM8008 Power Management IC" + depends on I2C && OF + select REGMAP_I2C + select REGMAP_IRQ + help + Select this option to get support for the Qualcomm Technologies, Inc. + PM8008 PMIC chip. PM8008 is a dedicated camera PMIC that integrates + all the necessary power management, housekeeping, and interface + support functions into a single IC. This driver provides common + support for accessing the device by instantiating all the child nodes + under it in the device tree. Additional drivers must be enabled in + order to use the functionality of the device. + + menu "Multimedia Capabilities Port drivers" depends on ARCH_SA1100
drivers/mfd/Makefile +1 -0
@@ -264,6 +264,7 @@ obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o obj-$(CONFIG_MFD_STMFX) += stmfx.o obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.o +obj-$(CONFIG_MFD_QCOM_PM8008) += qcom-pm8008.o obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o obj-$(CONFIG_MFD_SIMPLE_MFD_I2C) += simple-mfd-i2c.o
drivers/mfd/qcom-pm8008.c new file +284 -0
284 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH v3 3/3] mfd: pm8008: Add driver for QCOM PM8008 PMIC
Quoted 29 lines · Guru Das Srinagesh
Qualcomm Technologies, Inc. PM8008 is a dedicated camera PMIC that integrates all the necessary power management, housekeeping, and interface support functions into a single IC. Its key features include overtemperature protection, low-dropout linear regulators, GPIOs, and an I2C interface.
Add an MFD driver to support it.
Signed-off-by Guru Das Srinagesh <[email protected]>--- drivers/mfd/Kconfig | 15 +++ drivers/mfd/Makefile | 1 + drivers/mfd/qcom-pm8008.c | 284 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 300 insertions(+) create mode 100644 drivers/mfd/qcom-pm8008.cdrivers/mfd/Kconfig +5
-0@@ -2087,6 +2087,21 @@ config MFD_ACER_A500_EC The controller itself is ENE KB930, it is running firmware customized for the specific needs of the Acer A500 hardware. +config MFD_QCOM_PM8008 + tristate "QCOM PM8008 Power Management IC" + depends on I2C && OF + select REGMAP_I2C + select REGMAP_IRQ
drivers/mfd/Kconfig config MFD_ACER_A500_EC · line 2104
Quoted 10 lines
drivers/mfd/Kconfig +10
-0+ help + Select this option to get support for the Qualcomm Technologies, Inc. + PM8008 PMIC chip. PM8008 is a dedicated camera PMIC that integrates + all the necessary power management, housekeeping, and interface + support functions into a single IC. This driver provides common + support for accessing the device by instantiating all the child nodes + under it in the device tree. Additional drivers must be enabled in + order to use the functionality of the device. + +
Nit: No double-line spacings please.
Quoted 27 lines
drivers/mfd/Kconfig +0
-0menu "Multimedia Capabilities Port drivers" depends on ARCH_SA1100drivers/mfd/Makefile +1
-0@@ -264,6 +264,7 @@ obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o obj-$(CONFIG_MFD_STMFX) += stmfx.o obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.o +obj-$(CONFIG_MFD_QCOM_PM8008) += qcom-pm8008.o obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o obj-$(CONFIG_MFD_SIMPLE_MFD_I2C) += simple-mfd-i2c.odrivers/mfd/qcom-pm8008.c new file +6
-0@@ -0,0 +1,284 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (c) 2021, The Linux Foundation. All rights reserved. +*/ + +#include <linux/bitops.h>
drivers/mfd/qcom-pm8008.c line 16
Quoted 10 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/module.h> +#include <linux/of_platform.h> +#include <linux/pinctrl/consumer.h> +#include <linux/regmap.h> +#include <linux/slab.h> +#include <linux/of_device.h>
Alphabetical please.
'\n' here.
drivers/mfd/qcom-pm8008.c line 27
Quoted 11 lines
drivers/mfd/qcom-pm8008.c new file +11
-0+#include <dt-bindings/mfd/qcom-pm8008.h> + +#define I2C_INTR_STATUS_BASE 0x0550 +#define INT_RT_STS_OFFSET 0x10 +#define INT_SET_TYPE_OFFSET 0x11 +#define INT_POL_HIGH_OFFSET 0x12 +#define INT_POL_LOW_OFFSET 0x13 +#define INT_LATCHED_CLR_OFFSET 0x14 +#define INT_EN_SET_OFFSET 0x15 +#define INT_EN_CLR_OFFSET 0x16 +#define INT_LATCHED_STS_OFFSET 0x18
Can you tab these out so they all line up?
Quoted 16 lines
drivers/mfd/qcom-pm8008.c new file +16
-0+enum { +PM8008_MISC , +PM8008_TEMP_ALARM , +PM8008_GPIO1 , +PM8008_GPIO2 , +PM8008_NUM_PERIPHS , +}; + +#define PM8008_PERIPH_0_BASE 0x900 +#define PM8008_PERIPH_1_BASE 0x2400 +#define PM8008_PERIPH_2_BASE 0xC000 +#define PM8008_PERIPH_3_BASE 0xC100 + +#define PM8008_TEMP_ALARM_ADDR PM8008_PERIPH_1_BASE +#define PM8008_GPIO1_ADDR PM8008_PERIPH_2_BASE +#define PM8008_GPIO2_ADDR PM8008_PERIPH_3_BASE
drivers/mfd/qcom-pm8008.c line 54
Quoted 10 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+ +#define PM8008_STATUS_BASE (PM8008_PERIPH_0_BASE | INT_LATCHED_STS_OFFSET) +#define PM8008_MASK_BASE (PM8008_PERIPH_0_BASE | INT_EN_SET_OFFSET) +#define PM8008_UNMASK_BASE (PM8008_PERIPH_0_BASE | INT_EN_CLR_OFFSET) +#define PM8008_TYPE_BASE (PM8008_PERIPH_0_BASE | INT_SET_TYPE_OFFSET) +#define PM8008_ACK_BASE (PM8008_PERIPH_0_BASE | INT_LATCHED_CLR_OFFSET) +#define PM8008_POLARITY_HI_BASE (PM8008_PERIPH_0_BASE | INT_POL_HIGH_OFFSET) +#define PM8008_POLARITY_LO_BASE (PM8008_PERIPH_0_BASE | INT_POL_LOW_OFFSET) + +#define ADDRESS_OFFSET (paddr, base) (paddr - base)
No need for this.
drivers/mfd/qcom-pm8008.c line 57
Quoted 2 lines
drivers/mfd/qcom-pm8008.c new file +2
-0+#define PM8008_PERIPH_OFFSET (paddr) \ + ADDRESS_OFFSET(paddr, PM8008_PERIPH_0_BASE)
#define PM8008_PERIPH_OFFSET(paddr) paddr - PM8008_PERIPH_0_BASE
Quoted 22 lines
drivers/mfd/qcom-pm8008.c new file +22
-0+struct pm8008_data { +struct device *dev; +struct regmap *regmap; +int irq; +struct regmap_irq_chip_data *irq_data; +}; + +static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET (PM8008_PERIPH_0_BASE )}; +static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET (PM8008_PERIPH_1_BASE )}; +static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET (PM8008_PERIPH_2_BASE )}; +static unsigned int p3_offs[] = {PM8008_PERIPH_OFFSET (PM8008_PERIPH_3_BASE )}; + +static struct regmap_irq_sub_irq_map pm8008_sub_reg_offsets[] = { +REGMAP_IRQ_MAIN_REG_OFFSET (p0_offs), +REGMAP_IRQ_MAIN_REG_OFFSET (p1_offs), +REGMAP_IRQ_MAIN_REG_OFFSET (p2_offs), +REGMAP_IRQ_MAIN_REG_OFFSET (p3_offs), +}; + +static unsigned int pm8008_virt_regs[] = { +PM8008_POLARITY_HI_BASE , +PM8008_POLARITY_LO_BASE ,
drivers/mfd/qcom-pm8008.c line 90
Quoted 10 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+}; + +enum { +POLARITY_HI_INDEX , +POLARITY_LO_INDEX , +PM8008_NUM_VIRT_REGS , +}; + +static struct regmap_irq pm8008_irqs[] = { +/* MISC IRQs*/
Nit: Missing ' '.
Why are these comments required? Isn't the nomenclature enough?
Quoted 14 lines
drivers/mfd/qcom-pm8008.c new file +14
-0+REGMAP_IRQ_REG (PM8008_IRQ_MISC_UVLO ,PM8008_MISC ,BIT (0 )), +REGMAP_IRQ_REG (PM8008_IRQ_MISC_OVLO ,PM8008_MISC ,BIT (1 )), +REGMAP_IRQ_REG (PM8008_IRQ_MISC_OTST2 ,PM8008_MISC ,BIT (2 )), +REGMAP_IRQ_REG (PM8008_IRQ_MISC_OTST3 ,PM8008_MISC ,BIT (3 )), +REGMAP_IRQ_REG (PM8008_IRQ_MISC_LDO_OCP ,PM8008_MISC ,BIT (4 )), +/* TEMP_ALARM IRQs */ +REGMAP_IRQ_REG (PM8008_IRQ_TEMP_ALARM ,PM8008_TEMP_ALARM ,BIT (0 )), +/* GPIO1 IRQs */ +REGMAP_IRQ_REG (PM8008_IRQ_GPIO1 ,PM8008_GPIO1 ,BIT (0 )), +/* GPIO2 IRQs */ + REGMAP_IRQ_REG(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0)), +}; + +static int pm8008_set_type_virt(unsigned int **virt_buf,
drivers/mfd/qcom-pm8008.c line 114
Quoted 10 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+ unsigned int type, unsigned long hwirq, + int reg) +{ + switch (type) { +case IRQ_TYPE_EDGE_FALLING : +case IRQ_TYPE_LEVEL_LOW : + virt_buf[POLARITY_HI_INDEX ][reg] &= ~pm8008_irqs[hwirq].mask; + virt_buf[POLARITY_LO_INDEX ][reg] |= pm8008_irqs[hwirq].mask; +break ; +
Not sure these '\n's add anything except more LoC.
Quoted 119 lines
drivers/mfd/qcom-pm8008.c new file +119
-0+case IRQ_TYPE_EDGE_RISING : +case IRQ_TYPE_LEVEL_HIGH : + virt_buf[POLARITY_HI_INDEX ][reg] |= pm8008_irqs[hwirq].mask; + virt_buf[POLARITY_LO_INDEX ][reg] &= ~pm8008_irqs[hwirq].mask; +break ; + +case IRQ_TYPE_EDGE_BOTH : + virt_buf[POLARITY_HI_INDEX ][reg] |= pm8008_irqs[hwirq].mask; + virt_buf[POLARITY_LO_INDEX ][reg] |= pm8008_irqs[hwirq].mask; +break ; + +default : +return -EINVAL ; + } + +return 0 ; +} + +static struct regmap_irq_chip pm8008_irq_chip = { + .name ="pm8008_irq" , + .main_status =I2C_INTR_STATUS_BASE , + .num_main_regs =1 , + .num_virt_regs =PM8008_NUM_VIRT_REGS , + .irqs = pm8008_irqs, + .num_irqs =ARRAY_SIZE (pm8008_irqs), + .num_regs =PM8008_NUM_PERIPHS , + .not_fixed_stride = true, + .sub_reg_offsets = pm8008_sub_reg_offsets, + .set_type_virt = pm8008_set_type_virt, + .status_base =PM8008_STATUS_BASE , + .mask_base =PM8008_MASK_BASE , + .unmask_base =PM8008_UNMASK_BASE , + .type_base =PM8008_TYPE_BASE , + .ack_base =PM8008_ACK_BASE , + .virt_reg_base = pm8008_virt_regs, + .num_type_reg =PM8008_NUM_PERIPHS , +}; + +static struct regmap_config qcom_mfd_regmap_cfg = { + .reg_bits =16 , + .val_bits =8 , + .max_register =0xFFFF , +}; + +static int pm8008_init (struct pm8008_data *chip) +{ +int rc; + +/* +* Set TEMP_ALARM peripheral's TYPE so that the regmap-irq framework +* reads this as the default value instead of zero, the HW default. +* This is required to enable the writing of TYPE registers in +* regmap_irq_sync_unlock(). +*/ + rc =regmap_write (chip->regmap, + (PM8008_TEMP_ALARM_ADDR |INT_SET_TYPE_OFFSET ), +BIT (0 )); +if (rc) +return rc; + +/* Do the same for GPIO1 and GPIO2 peripherals */ + rc =regmap_write (chip->regmap, + (PM8008_GPIO1_ADDR |INT_SET_TYPE_OFFSET ),BIT (0 )); +if (rc) +return rc; + + rc =regmap_write (chip->regmap, + (PM8008_GPIO2_ADDR |INT_SET_TYPE_OFFSET ),BIT (0 )); + +return rc; +} + +static int pm8008_probe_irq_peripherals (struct pm8008_data *chip, +int client_irq) +{ +int rc, i; +struct regmap_irq_type *type; +struct regmap_irq_chip_data *irq_data; + + rc =pm8008_init (chip); +if (rc) { +dev_err (chip->dev,"Init failed: %d\n" , rc); +return rc; + } + +for (i =0 ; i <ARRAY_SIZE (pm8008_irqs); i++) { + type = &pm8008_irqs[i].type; + + type->type_reg_offset = pm8008_irqs[i].reg_offset; + type->type_rising_val = pm8008_irqs[i].mask; + type->type_falling_val = pm8008_irqs[i].mask; + type->type_level_high_val =0 ; + type->type_level_low_val =0 ; + +if (type->type_reg_offset ==PM8008_MISC ) + type->types_supported =IRQ_TYPE_EDGE_RISING ; +else + type->types_supported = (IRQ_TYPE_EDGE_BOTH | +IRQ_TYPE_LEVEL_HIGH |IRQ_TYPE_LEVEL_LOW ); + } + + rc =devm_regmap_add_irq_chip (chip->dev, chip->regmap, client_irq, +IRQF_SHARED ,0 , &pm8008_irq_chip, &irq_data); +if (rc) { +dev_err (chip->dev,"Failed to add IRQ chip: %d\n" , rc); +return rc; + } + +return 0 ; +} + +static int pm8008_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +int rc; +struct pm8008_data *chip; + + chip =devm_kzalloc (&client->dev,sizeof (*chip),GFP_KERNEL ); +if (!chip)
drivers/mfd/qcom-pm8008.c line 244
Quoted 10 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+return -ENOMEM ; + + chip->dev = &client->dev; + chip->regmap =devm_regmap_init_i2c (client, &qcom_mfd_regmap_cfg); +if (!chip->regmap) +return -ENODEV ; + +i2c_set_clientdata (client, chip); +if (of_find_property (chip->dev->of_node,"interrupt-controller" , +NULL )) {
of_property_read_bool()
No need for this '\n' - place it all one line please.
drivers/mfd/qcom-pm8008.c line 248
Quoted 4 lines
drivers/mfd/qcom-pm8008.c new file +4
-0+ rc =pm8008_probe_irq_peripherals (chip, client->irq); +if (rc) +dev_err (chip->dev,"Failed to probe irq periphs: %d\n" , + rc);
One line please.
drivers/mfd/qcom-pm8008.c line 256
Quoted 8 lines
drivers/mfd/qcom-pm8008.c new file +8
-0+ } + +return devm_of_platform_populate (chip->dev); +} + +static int pm8008_remove (struct i2c_client *client) +{ +i2c_set_clientdata (client,NULL );
This should be done for you by the Device Driver core.
drivers/mfd/qcom-pm8008.c line 270
Quoted 13 lines
drivers/mfd/qcom-pm8008.c new file +13
-0+return 0 ; +} + +static const struct of_device_id pm8008_match[] = { + { .compatible ="qcom,pm8008" , }, + { }, +}; + +static const struct i2c_device_id pm8008_id[] = { + {"pm8008" ,0 }, + { }, +}; +MODULE_DEVICE_TABLE (i2c, pm8008_id);
Use .probe_new, then you can omit this.
drivers/mfd/qcom-pm8008.c line 274
Quoted 3 lines
drivers/mfd/qcom-pm8008.c new file +3
-0+static struct i2c_driver pm8008_mfd_driver = { + .driver = { + .name ="mfd-pm8008" ,
No such thing as 'mfd', please drop that from the name.
Quoted 10 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+ .of_match_table = pm8008_match, + }, + .probe = pm8008_probe, + .remove = pm8008_remove, + .id_table = pm8008_id, +}; +module_i2c_driver (pm8008_mfd_driver); + +MODULE_LICENSE ("GPL v2" ); +MODULE_ALIAS ("i2c:qcom-pm8008" );
Signature · 4 lines
Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH v3 3/3] mfd: pm8008: Add driver for QCOM PM8008 PMIC
Quoted 35 lines · Lee Jones
Quoted 33 lines · Guru Das Srinagesh
Qualcomm Technologies, Inc. PM8008 is a dedicated camera PMIC that integrates all the necessary power management, housekeeping, and interface support functions into a single IC. Its key features include overtemperature protection, low-dropout linear regulators, GPIOs, and an I2C interface.
Add an MFD driver to support it.
Signed-off-by Guru Das Srinagesh <[email protected]>--- drivers/mfd/Kconfig | 15 +++ drivers/mfd/Makefile | 1 + drivers/mfd/qcom-pm8008.c | 284 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 300 insertions(+) create mode 100644 drivers/mfd/qcom-pm8008.cdrivers/mfd/Kconfig +9
-0@@ -2087,6 +2087,21 @@ config MFD_ACER_A500_EC The controller itself is ENE KB930, it is running firmware customized for the specific needs of the Acer A500 hardware. +config MFD_QCOM_PM8008 + tristate "QCOM PM8008 Power Management IC" + depends on I2C && OF + select REGMAP_I2C + select REGMAP_IRQ + help + Select this option to get support for the Qualcomm Technologies, Inc. + PM8008 PMIC chip. PM8008 is a dedicated camera PMIC that integrates + all the necessary power management, housekeeping, and interface
Quoted 8 lines
drivers/mfd/Kconfig config MFD_ACER_A500_EC · line 2104
Quoted 6 lines
drivers/mfd/Kconfig +6
-0+ support functions into a single IC. This driver provides common + support for accessing the device by instantiating all the child nodes + under it in the device tree. Additional drivers must be enabled in + order to use the functionality of the device. + +Nit: No double-line spacings please.
Done.
Quoted 41 lines
Quoted 28 lines
drivers/mfd/Kconfig +0
-0menu "Multimedia Capabilities Port drivers" depends on ARCH_SA1100drivers/mfd/Makefile +1
-0@@ -264,6 +264,7 @@ obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o obj-$(CONFIG_MFD_STMFX) += stmfx.o obj-$(CONFIG_MFD_KHADAS_MCU) += khadas-mcu.o obj-$(CONFIG_MFD_ACER_A500_EC) += acer-ec-a500.o +obj-$(CONFIG_MFD_QCOM_PM8008) += qcom-pm8008.o obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o obj-$(CONFIG_MFD_SIMPLE_MFD_I2C) += simple-mfd-i2c.odrivers/mfd/qcom-pm8008.c new file +6
-0@@ -0,0 +1,284 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* +* Copyright (c) 2021, The Linux Foundation. All rights reserved. +*/ + +#include <linux/bitops.h> drivers/mfd/qcom-pm8008.c line 16
Quoted 10 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/module.h> +#include <linux/of_platform.h> +#include <linux/pinctrl/consumer.h> +#include <linux/regmap.h> +#include <linux/slab.h> +#include <linux/of_device.h> Alphabetical please.
Quoted 1 line
'\n' here.
Done.
Quoted 14 lines
drivers/mfd/qcom-pm8008.c line 27
Quoted 12 lines
drivers/mfd/qcom-pm8008.c new file +11
-0+#include <dt-bindings/mfd/qcom-pm8008.h> + +#define I2C_INTR_STATUS_BASE 0x0550 +#define INT_RT_STS_OFFSET 0x10 +#define INT_SET_TYPE_OFFSET 0x11 +#define INT_POL_HIGH_OFFSET 0x12 +#define INT_POL_LOW_OFFSET 0x13 +#define INT_LATCHED_CLR_OFFSET 0x14 +#define INT_EN_SET_OFFSET 0x15 +#define INT_EN_CLR_OFFSET 0x16 +#define INT_LATCHED_STS_OFFSET 0x18Can you tab these out so they all line up?
Done.
Quoted 30 lines
Quoted 17 lines
drivers/mfd/qcom-pm8008.c new file +16
-0+enum { +PM8008_MISC , +PM8008_TEMP_ALARM , +PM8008_GPIO1 , +PM8008_GPIO2 , +PM8008_NUM_PERIPHS , +}; + +#define PM8008_PERIPH_0_BASE 0x900 +#define PM8008_PERIPH_1_BASE 0x2400 +#define PM8008_PERIPH_2_BASE 0xC000 +#define PM8008_PERIPH_3_BASE 0xC100 + +#define PM8008_TEMP_ALARM_ADDR PM8008_PERIPH_1_BASE +#define PM8008_GPIO1_ADDR PM8008_PERIPH_2_BASE +#define PM8008_GPIO2_ADDR PM8008_PERIPH_3_BASEdrivers/mfd/qcom-pm8008.c line 54
Quoted 10 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+ +#define PM8008_STATUS_BASE (PM8008_PERIPH_0_BASE | INT_LATCHED_STS_OFFSET) +#define PM8008_MASK_BASE (PM8008_PERIPH_0_BASE | INT_EN_SET_OFFSET) +#define PM8008_UNMASK_BASE (PM8008_PERIPH_0_BASE | INT_EN_CLR_OFFSET) +#define PM8008_TYPE_BASE (PM8008_PERIPH_0_BASE | INT_SET_TYPE_OFFSET) +#define PM8008_ACK_BASE (PM8008_PERIPH_0_BASE | INT_LATCHED_CLR_OFFSET) +#define PM8008_POLARITY_HI_BASE (PM8008_PERIPH_0_BASE | INT_POL_HIGH_OFFSET) +#define PM8008_POLARITY_LO_BASE (PM8008_PERIPH_0_BASE | INT_POL_LOW_OFFSET) + +#define ADDRESS_OFFSET (paddr, base) (paddr - base)No need for this.
Quoted 4 lines
drivers/mfd/qcom-pm8008.c line 57
Quoted 2 lines
drivers/mfd/qcom-pm8008.c new file +2
-0+#define PM8008_PERIPH_OFFSET (paddr) \ + ADDRESS_OFFSET(paddr, PM8008_PERIPH_0_BASE)#define
PM8008_PERIPH_OFFSET(paddr)paddr -PM8008_PERIPH_0_BASE
Done.
Quoted 36 lines
Quoted 23 lines
drivers/mfd/qcom-pm8008.c new file +22
-0+struct pm8008_data { +struct device *dev; +struct regmap *regmap; +int irq; +struct regmap_irq_chip_data *irq_data; +}; + +static unsigned int p0_offs[] = {PM8008_PERIPH_OFFSET (PM8008_PERIPH_0_BASE )}; +static unsigned int p1_offs[] = {PM8008_PERIPH_OFFSET (PM8008_PERIPH_1_BASE )}; +static unsigned int p2_offs[] = {PM8008_PERIPH_OFFSET (PM8008_PERIPH_2_BASE )}; +static unsigned int p3_offs[] = {PM8008_PERIPH_OFFSET (PM8008_PERIPH_3_BASE )}; + +static struct regmap_irq_sub_irq_map pm8008_sub_reg_offsets[] = { +REGMAP_IRQ_MAIN_REG_OFFSET (p0_offs), +REGMAP_IRQ_MAIN_REG_OFFSET (p1_offs), +REGMAP_IRQ_MAIN_REG_OFFSET (p2_offs), +REGMAP_IRQ_MAIN_REG_OFFSET (p3_offs), +}; + +static unsigned int pm8008_virt_regs[] = { +PM8008_POLARITY_HI_BASE , +PM8008_POLARITY_LO_BASE ,drivers/mfd/qcom-pm8008.c line 90
Quoted 10 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+}; + +enum { +POLARITY_HI_INDEX , +POLARITY_LO_INDEX , +PM8008_NUM_VIRT_REGS , +}; + +static struct regmap_irq pm8008_irqs[] = { +/* MISC IRQs*/ Nit: Missing ' '.
Quoted 1 line
Why are these comments required? Isn't the nomenclature enough?
Removing comments as nomenclature is enough.
Quoted 19 lines
Quoted 19 lines
drivers/mfd/qcom-pm8008.c new file +18
-0+REGMAP_IRQ_REG (PM8008_IRQ_MISC_UVLO ,PM8008_MISC ,BIT (0 )), +REGMAP_IRQ_REG (PM8008_IRQ_MISC_OVLO ,PM8008_MISC ,BIT (1 )), +REGMAP_IRQ_REG (PM8008_IRQ_MISC_OTST2 ,PM8008_MISC ,BIT (2 )), +REGMAP_IRQ_REG (PM8008_IRQ_MISC_OTST3 ,PM8008_MISC ,BIT (3 )), +REGMAP_IRQ_REG (PM8008_IRQ_MISC_LDO_OCP ,PM8008_MISC ,BIT (4 )), +/* TEMP_ALARM IRQs */ +REGMAP_IRQ_REG (PM8008_IRQ_TEMP_ALARM ,PM8008_TEMP_ALARM ,BIT (0 )), +/* GPIO1 IRQs */ +REGMAP_IRQ_REG (PM8008_IRQ_GPIO1 ,PM8008_GPIO1 ,BIT (0 )), +/* GPIO2 IRQs */ + REGMAP_IRQ_REG(PM8008_IRQ_GPIO2, PM8008_GPIO2, BIT(0)), +}; + +static int pm8008_set_type_virt (unsigned int **virt_buf, +unsigned int type,unsigned long hwirq, +int reg) +{ +switch (type) {
Quoted 8 lines
drivers/mfd/qcom-pm8008.c line 114
Quoted 6 lines
drivers/mfd/qcom-pm8008.c new file +6
-0+case IRQ_TYPE_EDGE_FALLING : +case IRQ_TYPE_LEVEL_LOW : + virt_buf[POLARITY_HI_INDEX ][reg] &= ~pm8008_irqs[hwirq].mask; + virt_buf[POLARITY_LO_INDEX ][reg] |= pm8008_irqs[hwirq].mask; +break ; +Not sure these '\n's add anything except more LoC.
I'd like to keep these newlines as I feel they improve readability.
Quoted 133 lines
Quoted 120 lines
drivers/mfd/qcom-pm8008.c new file +119
-0+case IRQ_TYPE_EDGE_RISING : +case IRQ_TYPE_LEVEL_HIGH : + virt_buf[POLARITY_HI_INDEX ][reg] |= pm8008_irqs[hwirq].mask; + virt_buf[POLARITY_LO_INDEX ][reg] &= ~pm8008_irqs[hwirq].mask; +break ; + +case IRQ_TYPE_EDGE_BOTH : + virt_buf[POLARITY_HI_INDEX ][reg] |= pm8008_irqs[hwirq].mask; + virt_buf[POLARITY_LO_INDEX ][reg] |= pm8008_irqs[hwirq].mask; +break ; + +default : +return -EINVAL ; + } + +return 0 ; +} + +static struct regmap_irq_chip pm8008_irq_chip = { + .name ="pm8008_irq" , + .main_status =I2C_INTR_STATUS_BASE , + .num_main_regs =1 , + .num_virt_regs =PM8008_NUM_VIRT_REGS , + .irqs = pm8008_irqs, + .num_irqs =ARRAY_SIZE (pm8008_irqs), + .num_regs =PM8008_NUM_PERIPHS , + .not_fixed_stride = true, + .sub_reg_offsets = pm8008_sub_reg_offsets, + .set_type_virt = pm8008_set_type_virt, + .status_base =PM8008_STATUS_BASE , + .mask_base =PM8008_MASK_BASE , + .unmask_base =PM8008_UNMASK_BASE , + .type_base =PM8008_TYPE_BASE , + .ack_base =PM8008_ACK_BASE , + .virt_reg_base = pm8008_virt_regs, + .num_type_reg =PM8008_NUM_PERIPHS , +}; + +static struct regmap_config qcom_mfd_regmap_cfg = { + .reg_bits =16 , + .val_bits =8 , + .max_register =0xFFFF , +}; + +static int pm8008_init (struct pm8008_data *chip) +{ +int rc; + +/* +* Set TEMP_ALARM peripheral's TYPE so that the regmap-irq framework +* reads this as the default value instead of zero, the HW default. +* This is required to enable the writing of TYPE registers in +* regmap_irq_sync_unlock(). +*/ + rc =regmap_write (chip->regmap, + (PM8008_TEMP_ALARM_ADDR |INT_SET_TYPE_OFFSET ), +BIT (0 )); +if (rc) +return rc; + +/* Do the same for GPIO1 and GPIO2 peripherals */ + rc =regmap_write (chip->regmap, + (PM8008_GPIO1_ADDR |INT_SET_TYPE_OFFSET ),BIT (0 )); +if (rc) +return rc; + + rc =regmap_write (chip->regmap, + (PM8008_GPIO2_ADDR |INT_SET_TYPE_OFFSET ),BIT (0 )); + +return rc; +} + +static int pm8008_probe_irq_peripherals (struct pm8008_data *chip, +int client_irq) +{ +int rc, i; +struct regmap_irq_type *type; +struct regmap_irq_chip_data *irq_data; + + rc =pm8008_init (chip); +if (rc) { +dev_err (chip->dev,"Init failed: %d\n" , rc); +return rc; + } + +for (i =0 ; i <ARRAY_SIZE (pm8008_irqs); i++) { + type = &pm8008_irqs[i].type; + + type->type_reg_offset = pm8008_irqs[i].reg_offset; + type->type_rising_val = pm8008_irqs[i].mask; + type->type_falling_val = pm8008_irqs[i].mask; + type->type_level_high_val =0 ; + type->type_level_low_val =0 ; + +if (type->type_reg_offset ==PM8008_MISC ) + type->types_supported =IRQ_TYPE_EDGE_RISING ; +else + type->types_supported = (IRQ_TYPE_EDGE_BOTH | +IRQ_TYPE_LEVEL_HIGH |IRQ_TYPE_LEVEL_LOW ); + } + + rc =devm_regmap_add_irq_chip (chip->dev, chip->regmap, client_irq, +IRQF_SHARED ,0 , &pm8008_irq_chip, &irq_data); +if (rc) { +dev_err (chip->dev,"Failed to add IRQ chip: %d\n" , rc); +return rc; + } + +return 0 ; +} + +static int pm8008_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +int rc; +struct pm8008_data *chip; + + chip =devm_kzalloc (&client->dev,sizeof (*chip),GFP_KERNEL ); +if (!chip)drivers/mfd/qcom-pm8008.c line 244
Quoted 10 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+return -ENOMEM ; + + chip->dev = &client->dev; + chip->regmap =devm_regmap_init_i2c (client, &qcom_mfd_regmap_cfg); +if (!chip->regmap) +return -ENODEV ; + +i2c_set_clientdata (client, chip); +if (of_find_property (chip->dev->of_node,"interrupt-controller" , +NULL )) {
of_property_read_bool()
Quoted 1 line
No need for this '\n' - place it all one line please.
Done.
Quoted 7 lines
drivers/mfd/qcom-pm8008.c line 248
Quoted 5 lines
drivers/mfd/qcom-pm8008.c new file +4
-0+ rc =pm8008_probe_irq_peripherals (chip, client->irq); +if (rc) +dev_err (chip->dev,"Failed to probe irq periphs: %d\n" , + rc);One line please.
Done (forgot that the 80-char limit has been increased now).
Quoted 11 lines
drivers/mfd/qcom-pm8008.c line 256
Quoted 9 lines
drivers/mfd/qcom-pm8008.c new file +8
-0+ } + +return devm_of_platform_populate (chip->dev); +} + +static int pm8008_remove (struct i2c_client *client) +{ +i2c_set_clientdata (client,NULL );This should be done for you by the Device Driver core.
Sure, I'll just drop specifying a .remove driver callback then.
Quoted 16 lines
Quoted 4 lines
drivers/mfd/qcom-pm8008.c new file +3
-0+return 0 ; +} +drivers/mfd/qcom-pm8008.c line 270
Quoted 10 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+static const struct of_device_id pm8008_match[] = { + { .compatible ="qcom,pm8008" , }, + { }, +}; + +static const struct i2c_device_id pm8008_id[] = { + {"pm8008" ,0 }, + { }, +}; +MODULE_DEVICE_TABLE (i2c, pm8008_id);Use .probe_new, then you can omit this.
Done.
Quoted 6 lines
drivers/mfd/qcom-pm8008.c line 274
Quoted 4 lines
drivers/mfd/qcom-pm8008.c new file +3
-0+static struct i2c_driver pm8008_mfd_driver = { + .driver = { + .name ="mfd-pm8008" ,No such thing as 'mfd', please drop that from the name.
Done; will simply use "pm8008" then.
Quoted 11 lines
drivers/mfd/qcom-pm8008.c line 284
Quoted 11 lines
drivers/mfd/qcom-pm8008.c new file +10
-0+ .of_match_table = pm8008_match, + }, + .probe = pm8008_probe, + .remove = pm8008_remove, + .id_table = pm8008_id, +}; +module_i2c_driver (pm8008_mfd_driver); + +MODULE_LICENSE ("GPL v2" ); +MODULE_ALIAS ("i2c:qcom-pm8008" );
Thank you.
Guru Das.
Quoted 14 lines · Guru Das Srinagesh
Changes from v2:
- Collected Rob Herring's Acked-by for the IRQ listing patch
- Addressed Rob's comments for the dt-bindings patchChanges from v1:
- Removed errant Change-Id from dt-bindings IRQ patch and gathered Bjorn's Reviewed-by
- Fixed up YAML errors using makedt_binding_checkThis driver is dependent on changes that have been made to the regmap-irq framework that have currently been accepted [1][2] in regmap.git upstream by Mark Brown but haven't made it to Linus' tree yet. For this reason, this driver has been based on the tip of regmap.git and not mfd.git.
Quoted 7 lines
Those changes, and this driver, are the result of a rewrite effort that was promised a long ago [3]. The framework changes and this driver have been tested and verified end-to-end on an internal platform.
[1] https://lore.kernel.org/lkml/20210318183607.gFxO2hoTO274vl3jUuxWbi19rq9wQELzN-y3B4jvO10@z/
[2] https://lore.kernel.org/lkml/[email protected]/
[3] https://lore.kernel.org/lkml/[email protected]/
Hi Lee, mfd reviewers,
This new driver depends on three regmap-irq framework changes that have been accepted by Mark (please see above) and hence will land only in the next rc-1 release. I just wanted to make sure that this patch series was on your radar [1]. The dt-bindings has been Acked by Rob already, and I'd be happy to address any review comments while patiently waiting for the dependencies to land.
[1] https://lore.kernel.org/lkml/20210419072229.GA4869@dell/
Thank you.
Guru Das.
Quoted 23 lines · Guru Das Srinagesh
Quoted 14 lines · Guru Das Srinagesh
Changes from v2:
- Collected Rob Herring's Acked-by for the IRQ listing patch
- Addressed Rob's comments for the dt-bindings patchChanges from v1:
- Removed errant Change-Id from dt-bindings IRQ patch and gathered Bjorn's Reviewed-by
- Fixed up YAML errors using makedt_binding_checkThis driver is dependent on changes that have been made to the regmap-irq framework that have currently been accepted [1][2] in regmap.git upstream by Mark Brown but haven't made it to Linus' tree yet. For this reason, this driver has been based on the tip of regmap.git and not mfd.git.
Quoted 7 lines
Those changes, and this driver, are the result of a rewrite effort that was promised a long ago [3]. The framework changes and this driver have been tested and verified end-to-end on an internal platform.
[1] https://lore.kernel.org/lkml/20210318183607.gFxO2hoTO274vl3jUuxWbi19rq9wQELzN-y3B4jvO10@z/
[2] https://lore.kernel.org/lkml/[email protected]/
[3] https://lore.kernel.org/lkml/[email protected]/
Quoted 8 lines
Hi Lee, mfd reviewers,
This new driver depends on three regmap-irq framework changes that have been accepted by Mark (please see above) and hence will land only in the next rc-1 release. I just wanted to make sure that this patch series was on your radar [1]. The dt-bindings has been Acked by Rob already, and I'd be happy to address any review comments while patiently waiting for the dependencies to land.
Yes, it's on my to-review list.
Linus is about to release v5.12. I'll review this set in good time.
Signature · 4 lines
Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog