[PATCH 0/6] power: supply: introduce the Qualcomm smb2

[PATCH 0/6] power: supply: introduce the Qualcomm smb2

[Caleb Connolly]

Add a driver for the Qualcomm PMI8998/SDM660 Switch-Mode Battery Charger
"2". This replaces the previous SMBB hardware found in older PMICs.

This driver provides basic support for initialising the hardware,
configuring the USB input current limit and reporting information about
the state of the charger. Features like type-c dual role support and OTG
switching will be added in future patches.

This patch series depends on my previous series adding support for
the Round Robin ADC which is used for reading the USB voltage and
current, it can be found here:
https://lore.kernel.org/linux-arm-msm/20220323162820.110806-1-caleb@connolly.tech/

---
Caleb Connolly (6):
  power: supply: add Qualcomm SMB2 driver
  arm64: dts: qcom: pmi8998: add smb2 node
  arm64: dts: sdm845-oneplus: enable smb2
  arm64: dts: qcom: sdm845-db845c: enable smb2
  arm64: dts: qcom: sdm845-xiaomi-beryllium enable smb2
  dt-bindings: power: supply: qcom,smb2: add bindings for smb2 driver

 .../bindings/power/supply/qcom,smb2.yaml      |  68 ++
 arch/arm64/boot/dts/qcom/pmi8998.dtsi         |  15 +
 arch/arm64/boot/dts/qcom/sdm845-db845c.dts    |  18 +
 .../boot/dts/qcom/sdm845-oneplus-common.dtsi  |   4 +
 .../dts/qcom/sdm845-oneplus-enchilada.dts     |   4 +
 .../boot/dts/qcom/sdm845-oneplus-fajita.dts   |   4 +
 .../boot/dts/qcom/sdm845-xiaomi-beryllium.dts |  14 +
 drivers/power/supply/Kconfig                  |   9 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/qcom_smb2.c              | 930 ++++++++++++++++++
 10 files changed, 1067 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
 create mode 100644 drivers/power/supply/qcom_smb2.c

-- 
2.35.1

[PATCH 1/6] power: supply: add Qualcomm SMB2 driver

[Caleb Connolly]

Add a driver for the SMB2 charger block found in the Qualcomm PMI8998
and PM660.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/power/supply/Kconfig     |   9 +
 drivers/power/supply/Makefile    |   1 +
 drivers/power/supply/qcom_smb2.c | 930 +++++++++++++++++++++++++++++++
 3 files changed, 940 insertions(+)
 create mode 100644 drivers/power/supply/qcom_smb2.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 1aa8323ad9f6..c72da3d75258 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -897,4 +897,13 @@ config BATTERY_UG3105
 	  device is off or suspended, the functionality of this driver is
 	  limited to reporting capacity only.
 
+config CHARGER_QCOM_SMB2
+	tristate "Qualcomm PMIC charger driver"
+	depends on MFD_SPMI_PMIC
+	depends on IIO
+	help
+	  Say Y here to enable the Qualcomm PMIC Charger driver. This
+	  adds support for the SMB2 switch mode battery charger found
+	  in PMI8998 and related PMICs.
+
 endif # POWER_SUPPLY
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 7f02f36aea55..e5db9eda6a9b 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -108,3 +108,4 @@ obj-$(CONFIG_BATTERY_ACER_A500)	+= acer_a500_battery.o
 obj-$(CONFIG_BATTERY_SURFACE)	+= surface_battery.o
 obj-$(CONFIG_CHARGER_SURFACE)	+= surface_charger.o
 obj-$(CONFIG_BATTERY_UG3105)	+= ug3105_battery.o
+obj-$(CONFIG_CHARGER_QCOM_SMB2)	+= qcom_smb2.o
diff --git a/drivers/power/supply/qcom_smb2.c b/drivers/power/supply/qcom_smb2.c
new file mode 100644
index 000000000000..63100cfc3300
--- /dev/null
+++ b/drivers/power/supply/qcom_smb2.c
@@ -0,0 +1,930 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2016-2019 The Linux Foundation. All rights reserved.
+ * Copyright (c) 2021, Linaro Ltd.
+ *  Author: Caleb Connolly <caleb.connolly@linaro.org>
+ *
+ * This driver is for the switch-mode battery charger and boost
+ * hardware found in pmi8998 and related PMICs.
+ */
+
+#include <linux/bits.h>
+#include <linux/kernel.h>
+#include <linux/minmax.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/device.h>
+#include <linux/iio/consumer.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+#include <linux/power_supply.h>
+#include <linux/module.h>
+#include <linux/math64.h>
+#include <linux/of_irq.h>
+#include <linux/interrupt.h>
+#include <linux/workqueue.h>
+
+#define BATTERY_CHARGER_STATUS_2_REG 0x07
+#define INPUT_CURRENT_LIMITED_BIT BIT(7)
+#define CHARGER_ERROR_STATUS_SFT_EXPIRE_BIT BIT(6)
+#define CHARGER_ERROR_STATUS_BAT_OV_BIT BIT(5)
+#define CHARGER_ERROR_STATUS_BAT_TERM_MISSING_BIT BIT(4)
+#define BAT_TEMP_STATUS_MASK GENMASK(3, 0)
+#define BAT_TEMP_STATUS_SOFT_LIMIT_MASK GENMASK(3, 2)
+#define BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT BIT(3)
+#define BAT_TEMP_STATUS_COLD_SOFT_LIMIT_BIT BIT(2)
+#define BAT_TEMP_STATUS_TOO_HOT_BIT BIT(1)
+#define BAT_TEMP_STATUS_TOO_COLD_BIT BIT(0)
+
+#define CHARGING_ENABLE_CMD_REG 0x42
+#define CHARGING_ENABLE_CMD_BIT BIT(0)
+
+#define CHGR_CFG2_REG 0x51
+#define CHG_EN_SRC_BIT BIT(7)
+#define CHG_EN_POLARITY_BIT BIT(6)
+#define PRETOFAST_TRANSITION_CFG_BIT BIT(5)
+#define BAT_OV_ECC_BIT BIT(4)
+#define I_TERM_BIT BIT(3)
+#define AUTO_RECHG_BIT BIT(2)
+#define EN_ANALOG_DROP_IN_VBATT_BIT BIT(1)
+#define CHARGER_INHIBIT_BIT BIT(0)
+
+#define FAST_CHARGE_CURRENT_CFG_REG 0x61
+#define FAST_CHARGE_CURRENT_SETTING_MASK GENMASK(7, 0)
+
+#define FLOAT_VOLTAGE_CFG_REG 0x70
+#define FLOAT_VOLTAGE_SETTING_MASK GENMASK(7, 0)
+
+#define FG_UPDATE_CFG_2_SEL_REG 0x7D
+#define SOC_LT_OTG_THRESH_SEL_BIT BIT(3)
+#define SOC_LT_CHG_RECHARGE_THRESH_SEL_BIT BIT(2)
+#define VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT BIT(1)
+#define IBT_LT_CHG_TERM_THRESH_SEL_BIT BIT(0)
+
+#define JEITA_EN_CFG_REG 0x90
+#define JEITA_EN_HARDLIMIT_BIT BIT(4)
+#define JEITA_EN_HOT_SL_FCV_BIT BIT(3)
+#define JEITA_EN_COLD_SL_FCV_BIT BIT(2)
+#define JEITA_EN_HOT_SL_CCC_BIT BIT(1)
+#define JEITA_EN_COLD_SL_CCC_BIT BIT(0)
+
+#define INT_RT_STS 0x310
+#define TYPE_C_CHANGE_RT_STS_BIT BIT(7)
+#define USBIN_ICL_CHANGE_RT_STS_BIT BIT(6)
+#define USBIN_SOURCE_CHANGE_RT_STS_BIT BIT(5)
+#define USBIN_PLUGIN_RT_STS_BIT BIT(4)
+#define USBIN_OV_RT_STS_BIT BIT(3)
+#define USBIN_UV_RT_STS_BIT BIT(2)
+#define USBIN_LT_3P6V_RT_STS_BIT BIT(1)
+#define USBIN_COLLAPSE_RT_STS_BIT BIT(0)
+
+#define BATTERY_CHARGER_STATUS_1_REG 0x06
+#define BVR_INITIAL_RAMP_BIT BIT(7)
+#define CC_SOFT_TERMINATE_BIT BIT(6)
+#define STEP_CHARGING_STATUS_SHIFT 3
+#define STEP_CHARGING_STATUS_MASK GENMASK(5, 3)
+#define BATTERY_CHARGER_STATUS_MASK GENMASK(2, 0)
+
+#define BATTERY_HEALTH_STATUS_REG 0x07
+
+#define OTG_CFG_REG 0x153
+#define OTG_RESERVED_MASK GENMASK(7, 6)
+#define DIS_OTG_ON_TLIM_BIT BIT(5)
+#define QUICKSTART_OTG_FASTROLESWAP_BIT BIT(4)
+#define INCREASE_DFP_TIME_BIT BIT(3)
+#define ENABLE_OTG_IN_DEBUG_MODE_BIT BIT(2)
+#define OTG_EN_SRC_CFG_BIT BIT(1)
+#define CONCURRENT_MODE_CFG_BIT BIT(0)
+
+#define OTG_ENG_OTG_CFG_REG 0x1C0
+#define ENG_BUCKBOOST_HALT1_8_MODE_BIT BIT(0)
+
+#define APSD_STATUS_REG 0x307
+#define APSD_STATUS_7_BIT BIT(7)
+#define HVDCP_CHECK_TIMEOUT_BIT BIT(6)
+#define SLOW_PLUGIN_TIMEOUT_BIT BIT(5)
+#define ENUMERATION_DONE_BIT BIT(4)
+#define VADP_CHANGE_DONE_AFTER_AUTH_BIT BIT(3)
+#define QC_AUTH_DONE_STATUS_BIT BIT(2)
+#define QC_CHARGER_BIT BIT(1)
+#define APSD_DTC_STATUS_DONE_BIT BIT(0)
+
+#define APSD_RESULT_STATUS_REG 0x308
+#define ICL_OVERRIDE_LATCH_BIT BIT(7)
+#define APSD_RESULT_STATUS_MASK GENMASK(6, 0)
+#define QC_3P0_BIT BIT(6)
+#define QC_2P0_BIT BIT(5)
+#define FLOAT_CHARGER_BIT BIT(4)
+#define DCP_CHARGER_BIT BIT(3)
+#define CDP_CHARGER_BIT BIT(2)
+#define OCP_CHARGER_BIT BIT(1)
+#define SDP_CHARGER_BIT BIT(0)
+
+#define TYPE_C_STATUS_1_REG 0x30B
+#define UFP_TYPEC_MASK GENMASK(7, 5)
+#define UFP_TYPEC_RDSTD_BIT BIT(7)
+#define UFP_TYPEC_RD1P5_BIT BIT(6)
+#define UFP_TYPEC_RD3P0_BIT BIT(5)
+#define UFP_TYPEC_FMB_255K_BIT BIT(4)
+#define UFP_TYPEC_FMB_301K_BIT BIT(3)
+#define UFP_TYPEC_FMB_523K_BIT BIT(2)
+#define UFP_TYPEC_FMB_619K_BIT BIT(1)
+#define UFP_TYPEC_OPEN_OPEN_BIT BIT(0)
+
+#define TYPE_C_STATUS_2_REG 0x30C
+#define DFP_RA_OPEN_BIT BIT(7)
+#define TIMER_STAGE_BIT BIT(6)
+#define EXIT_UFP_MODE_BIT BIT(5)
+#define EXIT_DFP_MODE_BIT BIT(4)
+#define DFP_TYPEC_MASK GENMASK(3, 0)
+#define DFP_RD_OPEN_BIT BIT(3)
+#define DFP_RD_RA_VCONN_BIT BIT(2)
+#define DFP_RD_RD_BIT BIT(1)
+#define DFP_RA_RA_BIT BIT(0)
+
+#define TYPE_C_STATUS_3_REG 0x30D
+#define ENABLE_BANDGAP_BIT BIT(7)
+#define U_USB_GND_NOVBUS_BIT BIT(6)
+#define U_USB_FLOAT_NOVBUS_BIT BIT(5)
+#define U_USB_GND_BIT BIT(4)
+#define U_USB_FMB1_BIT BIT(3)
+#define U_USB_FLOAT1_BIT BIT(2)
+#define U_USB_FMB2_BIT BIT(1)
+#define U_USB_FLOAT2_BIT BIT(0)
+
+#define TYPE_C_STATUS_4_REG 0x30E
+#define UFP_DFP_MODE_STATUS_BIT BIT(7)
+#define TYPEC_VBUS_STATUS_BIT BIT(6)
+#define TYPEC_VBUS_ERROR_STATUS_BIT BIT(5)
+#define TYPEC_DEBOUNCE_DONE_STATUS_BIT BIT(4)
+#define TYPEC_UFP_AUDIO_ADAPT_STATUS_BIT BIT(3)
+#define TYPEC_VCONN_OVERCURR_STATUS_BIT BIT(2)
+#define CC_ORIENTATION_BIT BIT(1)
+#define CC_ATTACHED_BIT BIT(0)
+
+#define TYPE_C_STATUS_5_REG 0x30F
+#define TRY_SOURCE_FAILED_BIT BIT(6)
+#define TRY_SINK_FAILED_BIT BIT(5)
+#define TIMER_STAGE_2_BIT BIT(4)
+#define TYPEC_LEGACY_CABLE_STATUS_BIT BIT(3)
+#define TYPEC_NONCOMP_LEGACY_CABLE_STATUS_BIT BIT(2)
+#define TYPEC_TRYSOURCE_DETECT_STATUS_BIT BIT(1)
+#define TYPEC_TRYSINK_DETECT_STATUS_BIT BIT(0)
+
+#define CMD_APSD_REG 0x341
+#define ICL_OVERRIDE_BIT BIT(1)
+#define APSD_RERUN_BIT BIT(0)
+
+#define TYPE_C_CFG_REG 0x358
+#define APSD_START_ON_CC_BIT BIT(7)
+#define WAIT_FOR_APSD_BIT BIT(6)
+#define FACTORY_MODE_DETECTION_EN_BIT BIT(5)
+#define FACTORY_MODE_ICL_3A_4A_BIT BIT(4)
+#define FACTORY_MODE_DIS_CHGING_CFG_BIT BIT(3)
+#define SUSPEND_NON_COMPLIANT_CFG_BIT BIT(2)
+#define VCONN_OC_CFG_BIT BIT(1)
+#define TYPE_C_OR_U_USB_BIT BIT(0)
+
+#define TYPE_C_CFG_2_REG 0x359
+#define TYPE_C_DFP_CURRSRC_MODE_BIT BIT(7)
+#define DFP_CC_1P4V_OR_1P6V_BIT BIT(6)
+#define VCONN_SOFTSTART_CFG_MASK GENMASK(5, 4)
+#define EN_TRY_SOURCE_MODE_BIT BIT(3)
+#define USB_FACTORY_MODE_ENABLE_BIT BIT(2)
+#define TYPE_C_UFP_MODE_BIT BIT(1)
+#define EN_80UA_180UA_CUR_SOURCE_BIT BIT(0)
+
+#define TYPE_C_CFG_3_REG 0x35A
+#define TVBUS_DEBOUNCE_BIT BIT(7)
+#define TYPEC_LEGACY_CABLE_INT_EN_BIT BIT(6)
+#define TYPEC_NONCOMPLIANT_LEGACY_CABLE_INT_EN_BIT BIT(5)
+#define TYPEC_TRYSOURCE_DETECT_INT_EN_BIT BIT(4)
+#define TYPEC_TRYSINK_DETECT_INT_EN_BIT BIT(3)
+#define EN_TRYSINK_MODE_BIT BIT(2)
+#define EN_LEGACY_CABLE_DETECTION_BIT BIT(1)
+#define ALLOW_PD_DRING_UFP_TCCDB_BIT BIT(0)
+
+#define USBIN_OPTIONS_1_CFG_REG 0x362
+#define CABLE_R_SEL_BIT BIT(7)
+#define HVDCP_AUTH_ALG_EN_CFG_BIT BIT(6)
+#define HVDCP_AUTONOMOUS_MODE_EN_CFG_BIT BIT(5)
+#define INPUT_PRIORITY_BIT BIT(4)
+#define AUTO_SRC_DETECT_BIT BIT(3)
+#define HVDCP_EN_BIT BIT(2)
+#define VADP_INCREMENT_VOLTAGE_LIMIT_BIT BIT(1)
+#define VADP_TAPER_TIMER_EN_BIT BIT(0)
+
+#define USBIN_OPTIONS_2_CFG_REG 0x363
+#define WIPWR_RST_EUD_CFG_BIT BIT(7)
+#define SWITCHER_START_CFG_BIT BIT(6)
+#define DCD_TIMEOUT_SEL_BIT BIT(5)
+#define OCD_CURRENT_SEL_BIT BIT(4)
+#define SLOW_PLUGIN_TIMER_EN_CFG_BIT BIT(3)
+#define FLOAT_OPTIONS_MASK GENMASK(2, 0)
+#define FLOAT_DIS_CHGING_CFG_BIT BIT(2)
+#define SUSPEND_FLOAT_CFG_BIT BIT(1)
+#define FORCE_FLOAT_SDP_CFG_BIT BIT(0)
+
+#define TAPER_TIMER_SEL_CFG_REG 0x364
+#define TYPEC_SPARE_CFG_BIT BIT(7)
+#define TYPEC_DRP_DFP_TIME_CFG_BIT BIT(5)
+#define TAPER_TIMER_SEL_MASK GENMASK(1, 0)
+
+#define USBIN_LOAD_CFG_REG 0x365
+#define USBIN_OV_CH_LOAD_OPTION_BIT BIT(7)
+#define ICL_OVERRIDE_AFTER_APSD_BIT BIT(4)
+
+#define USBIN_ICL_OPTIONS_REG 0x366
+#define CFG_USB3P0_SEL_BIT BIT(2)
+#define USB51_MODE_BIT BIT(1)
+#define USBIN_MODE_CHG_BIT BIT(0)
+
+#define TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG 0x368
+#define EXIT_SNK_BASED_ON_CC_BIT BIT(7)
+#define VCONN_EN_ORIENTATION_BIT BIT(6)
+#define TYPEC_VCONN_OVERCURR_INT_EN_BIT BIT(5)
+#define VCONN_EN_SRC_BIT BIT(4)
+#define VCONN_EN_VALUE_BIT BIT(3)
+#define TYPEC_POWER_ROLE_CMD_MASK GENMASK(2, 0)
+#define UFP_EN_CMD_BIT BIT(2)
+#define DFP_EN_CMD_BIT BIT(1)
+#define TYPEC_DISABLE_CMD_BIT BIT(0)
+
+#define USBIN_CURRENT_LIMIT_CFG_REG 0x370
+#define USBIN_CURRENT_LIMIT_MASK GENMASK(7, 0)
+
+#define USBIN_AICL_OPTIONS_CFG_REG 0x380
+#define SUSPEND_ON_COLLAPSE_USBIN_BIT BIT(7)
+#define USBIN_AICL_HDC_EN_BIT BIT(6)
+#define USBIN_AICL_START_AT_MAX_BIT BIT(5)
+#define USBIN_AICL_RERUN_EN_BIT BIT(4)
+#define USBIN_AICL_ADC_EN_BIT BIT(3)
+#define USBIN_AICL_EN_BIT BIT(2)
+#define USBIN_HV_COLLAPSE_RESPONSE_BIT BIT(1)
+#define USBIN_LV_COLLAPSE_RESPONSE_BIT BIT(0)
+
+#define DC_ENG_SSUPPLY_CFG2_REG 0x4C1
+#define ENG_SSUPPLY_IVREF_OTG_SS_MASK GENMASK(2, 0)
+#define OTG_SS_SLOW 0x3
+
+#define DCIN_AICL_REF_SEL_CFG_REG 0x481
+#define DCIN_CONT_AICL_THRESHOLD_CFG_MASK GENMASK(5, 0)
+
+#define WI_PWR_OPTIONS_REG 0x495
+#define CHG_OK_BIT BIT(7)
+#define WIPWR_UVLO_IRQ_OPT_BIT BIT(6)
+#define BUCK_HOLDOFF_ENABLE_BIT BIT(5)
+#define CHG_OK_HW_SW_SELECT_BIT BIT(4)
+#define WIPWR_RST_ENABLE_BIT BIT(3)
+#define DCIN_WIPWR_IRQ_SELECT_BIT BIT(2)
+#define AICL_SWITCH_ENABLE_BIT BIT(1)
+#define ZIN_ICL_ENABLE_BIT BIT(0)
+
+#define ICL_STATUS_REG 0x607
+#define INPUT_CURRENT_LIMIT_MASK GENMASK(7, 0)
+
+#define POWER_PATH_STATUS_REG 0x60B
+#define P_PATH_INPUT_SS_DONE_BIT BIT(7)
+#define P_PATH_USBIN_SUSPEND_STS_BIT BIT(6)
+#define P_PATH_DCIN_SUSPEND_STS_BIT BIT(5)
+#define P_PATH_USE_USBIN_BIT BIT(4)
+#define P_PATH_USE_DCIN_BIT BIT(3)
+#define P_PATH_POWER_PATH_MASK GENMASK(2, 1)
+#define P_PATH_VALID_INPUT_POWER_SOURCE_STS_BIT BIT(0)
+
+#define WD_CFG_REG 0x651
+#define WATCHDOG_TRIGGER_AFP_EN_BIT BIT(7)
+#define BARK_WDOG_INT_EN_BIT BIT(6)
+#define BITE_WDOG_INT_EN_BIT BIT(5)
+#define SFT_AFTER_WDOG_IRQ_MASK GENMASK(4, 3)
+#define WDOG_IRQ_SFT_BIT BIT(2)
+#define WDOG_TIMER_EN_ON_PLUGIN_BIT BIT(1)
+#define WDOG_TIMER_EN_BIT BIT(0)
+
+#define AICL_RERUN_TIME_CFG_REG 0x661
+#define AICL_RERUN_TIME_MASK GENMASK(1, 0)
+
+#define SDP_CURRENT_UA 500000
+#define CDP_CURRENT_UA 1500000
+#define DCP_CURRENT_UA 1500000
+
+enum charger_status {
+	TRICKLE_CHARGE = 0,
+	PRE_CHARGE,
+	FAST_CHARGE,
+	FULLON_CHARGE,
+	TAPER_CHARGE,
+	TERMINATE_CHARGE,
+	INHIBIT_CHARGE,
+	DISABLE_CHARGE,
+};
+
+struct smb2_register {
+	u16 addr;
+	u8 mask;
+	u8 val;
+};
+
+/**
+ * struct smb2_chip - smb2 chip structure
+ * @dev:		Device reference for power_supply
+ * @base:		Base address for smb2 registers
+ * @regmap:		Register map
+ * @reg_lock:		Mutex for register access
+ * @batt_info:		Battery data from DT
+ * @usbin_i_chan:	USBIN current measurement channel
+ * @usbin_v_chan:	USBIN voltage measurement channel
+ * @chg_psy:		Charger power supply instance
+ * @usb_present:	True if a charger is attached
+ */
+struct smb2_chip {
+	struct device *dev;
+	const char *name;
+	unsigned int base;
+	struct regmap *regmap;
+	struct mutex reg_lock;
+	struct power_supply_battery_info *batt_info;
+
+	struct delayed_work status_change_work;
+
+	struct iio_channel *usbin_i_chan;
+	struct iio_channel *usbin_v_chan;
+
+	struct power_supply *chg_psy;
+
+	bool usb_present;
+};
+
+static enum power_supply_property smb2_properties[] = {
+	POWER_SUPPLY_PROP_MANUFACTURER, POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_CURRENT_MAX,	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_HEALTH,	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_USB_TYPE,
+};
+
+static enum power_supply_usb_type smb2_usb_types[] = {
+	POWER_SUPPLY_USB_TYPE_SDP, POWER_SUPPLY_USB_TYPE_DCP,
+	POWER_SUPPLY_USB_TYPE_CDP, POWER_SUPPLY_USB_TYPE_C,
+	POWER_SUPPLY_USB_TYPE_PD_DRP
+};
+
+/*
+ * Qualcomm "automatic power source detection" aka APSD
+ * tells us what type of charger we're connected to.
+ */
+static int smb2_apsd_get_charger_type(struct smb2_chip *chip, int *val)
+{
+	int rc;
+	unsigned int apsd_stat, stat;
+
+	mutex_lock(&chip->reg_lock);
+
+	rc = regmap_read(chip->regmap, chip->base + APSD_STATUS_REG,
+			 &apsd_stat);
+	if (rc < 0) {
+		dev_err(chip->dev, "Failed to read apsd status, rc = %d", rc);
+		return rc;
+	}
+	if (!(apsd_stat & APSD_DTC_STATUS_DONE_BIT)) {
+		dev_err(chip->dev, "Apsd not ready");
+		return -EAGAIN;
+	}
+
+	rc = regmap_read(chip->regmap, chip->base + APSD_RESULT_STATUS_REG,
+			 &stat);
+	if (rc < 0) {
+		dev_err(chip->dev, "Failed to read apsd result, rc = %d", rc);
+		return rc;
+	}
+
+	mutex_unlock(&chip->reg_lock);
+
+	stat &= APSD_RESULT_STATUS_MASK;
+
+	if (stat & CDP_CHARGER_BIT) {
+		*val = POWER_SUPPLY_USB_TYPE_CDP;
+	} else if (stat &
+		   (DCP_CHARGER_BIT | OCP_CHARGER_BIT | FLOAT_CHARGER_BIT)) {
+		*val = POWER_SUPPLY_USB_TYPE_DCP;
+	} else { /* SDP_CHARGER_BIT (or others) */
+		*val = POWER_SUPPLY_USB_TYPE_SDP;
+	}
+
+	return 0;
+}
+
+int smb2_get_prop_usb_online(struct smb2_chip *chip, int *val)
+{
+	unsigned int stat;
+	int rc;
+
+	rc = regmap_read(chip->regmap, chip->base + POWER_PATH_STATUS_REG,
+			 &stat);
+	if (rc < 0) {
+		dev_err(chip->dev, "Couldn't read POWER_PATH_STATUS! ret=%d\n",
+			rc);
+		return rc;
+	}
+
+	*val = (stat & P_PATH_USE_USBIN_BIT) &&
+	       (stat & P_PATH_VALID_INPUT_POWER_SOURCE_STS_BIT);
+	return rc;
+}
+
+int smb2_get_prop_status(struct smb2_chip *chip, int *val)
+{
+	int usb_online_val;
+	unsigned int stat;
+	int rc;
+	bool usb_online;
+
+	mutex_lock(&chip->reg_lock);
+
+	rc = smb2_get_prop_usb_online(chip, &usb_online_val);
+	if (rc < 0) {
+		dev_err(chip->dev, "Couldn't get usb online property rc = %d\n",
+			rc);
+		return rc;
+	}
+	usb_online = (bool)usb_online_val;
+
+	if (!usb_online) {
+		*val = POWER_SUPPLY_STATUS_DISCHARGING;
+		return rc;
+	}
+
+	rc = regmap_read(chip->regmap,
+			 chip->base + BATTERY_CHARGER_STATUS_1_REG, &stat);
+	if (rc < 0) {
+		dev_err(chip->dev,
+			"Failed to read charging status ret=%d\n", rc);
+		return rc;
+	}
+
+	mutex_unlock(&chip->reg_lock);
+
+	stat = stat & BATTERY_CHARGER_STATUS_MASK;
+
+	switch (stat) {
+	case TRICKLE_CHARGE:
+	case PRE_CHARGE:
+	case FAST_CHARGE:
+	case FULLON_CHARGE:
+	case TAPER_CHARGE:
+		*val = POWER_SUPPLY_STATUS_CHARGING;
+		return rc;
+	case DISABLE_CHARGE:
+		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		return rc;
+	case TERMINATE_CHARGE:
+		*val = POWER_SUPPLY_STATUS_FULL;
+		return rc;
+	case INHIBIT_CHARGE:
+	default:
+		*val = POWER_SUPPLY_STATUS_UNKNOWN;
+		return rc;
+	}
+}
+
+static inline int smb2_get_current_limit(struct smb2_chip *chip,
+					 unsigned int *val)
+{
+	int rc = regmap_read(chip->regmap, chip->base + ICL_STATUS_REG, val);
+
+	if (rc >= 0)
+		*val *= 25000;
+	return rc;
+}
+
+static inline int smb2_set_current_limit(struct smb2_chip *chip,
+					 unsigned int val)
+{
+	unsigned char val_raw;
+
+	if (val > 4800000) {
+		dev_err(chip->dev,
+			"Can't set current limit higher than 4800000uA");
+		return -EINVAL;
+	}
+	val_raw = val / 25000;
+	return regmap_write(chip->regmap,
+			    chip->base + USBIN_CURRENT_LIMIT_CFG_REG, val_raw);
+}
+
+void smb2_status_change_work(struct work_struct *work)
+{
+	struct smb2_chip *chip =
+		container_of(work, struct smb2_chip, status_change_work.work);
+	unsigned int charger_type, current_ua;
+	int usb_online, count, rc;
+
+	mutex_lock(&chip->reg_lock);
+
+	smb2_get_prop_usb_online(chip, &usb_online);
+	if (usb_online == 0)
+		return;
+
+	for (count = 0; count < 3; count++) {
+		dev_info(chip->dev, "get charger type retry %d\n", count);
+		rc = smb2_apsd_get_charger_type(chip, &charger_type);
+		if (rc == 0)
+			break;
+		msleep(100);
+	}
+
+	if (rc < 0) {
+		rc = regmap_update_bits(chip->regmap, chip->base + CMD_APSD_REG,
+					APSD_RERUN_BIT, APSD_RERUN_BIT);
+		schedule_delayed_work(&chip->status_change_work,
+				      msecs_to_jiffies(1500));
+		return;
+	}
+
+	switch (charger_type) {
+	case POWER_SUPPLY_USB_TYPE_CDP:
+		current_ua = CDP_CURRENT_UA;
+		break;
+	case POWER_SUPPLY_USB_TYPE_DCP:
+		current_ua = DCP_CURRENT_UA;
+		break;
+	case POWER_SUPPLY_USB_TYPE_SDP:
+	default:
+		current_ua = SDP_CURRENT_UA;
+		break;
+	}
+
+	smb2_set_current_limit(chip, current_ua);
+	mutex_unlock(&chip->reg_lock);
+	power_supply_changed(chip->chg_psy);
+}
+
+int smb2_get_iio_chan(struct smb2_chip *chip, struct iio_channel *chan,
+		      int *val)
+{
+	int rc = 0;
+	union power_supply_propval status;
+
+	rc = power_supply_get_property(chip->chg_psy, POWER_SUPPLY_PROP_STATUS,
+				       &status);
+	if (rc < 0 || status.intval != POWER_SUPPLY_STATUS_CHARGING) {
+		*val = 0;
+		return 0;
+	}
+
+	if (IS_ERR(chan)) {
+		dev_err(chip->dev, "Failed to chan, err = %li", PTR_ERR(chan));
+		return PTR_ERR(chan);
+	}
+
+	rc = iio_read_channel_processed(chan, val);
+	return rc;
+}
+
+int smb2_get_prop_health(struct smb2_chip *chip, int *val)
+{
+	int rc;
+	unsigned int stat;
+
+	*val = POWER_SUPPLY_HEALTH_UNKNOWN;
+
+	rc = regmap_read(chip->regmap,
+			 chip->base + BATTERY_CHARGER_STATUS_2_REG, &stat);
+	if (rc < 0) {
+		dev_err(chip->dev, "Couldn't read charger status rc=%d\n", rc);
+		return rc;
+	}
+
+	switch (stat & BATTERY_CHARGER_STATUS_MASK) {
+	case CHARGER_ERROR_STATUS_BAT_OV_BIT:
+		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+		return 0;
+	case BAT_TEMP_STATUS_TOO_COLD_BIT:
+		*val = POWER_SUPPLY_HEALTH_COLD;
+		return 0;
+	case BAT_TEMP_STATUS_TOO_HOT_BIT:
+		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
+		return 0;
+	case BAT_TEMP_STATUS_COLD_SOFT_LIMIT_BIT:
+		*val = POWER_SUPPLY_HEALTH_COOL;
+		return 0;
+	case BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT:
+		*val = POWER_SUPPLY_HEALTH_WARM;
+		return 0;
+	default:
+		*val = POWER_SUPPLY_HEALTH_GOOD;
+		return 0;
+	}
+}
+
+static int smb2_get_property(struct power_supply *psy,
+			     enum power_supply_property psp,
+			     union power_supply_propval *val)
+{
+	struct smb2_chip *chip = power_supply_get_drvdata(psy);
+	int error = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_MANUFACTURER:
+		val->strval = "Qualcomm";
+		return 0;
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		val->strval = chip->name;
+		return 0;
+	case POWER_SUPPLY_PROP_CURRENT_MAX:
+		error = smb2_get_current_limit(chip, &val->intval);
+		return 0;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		error = smb2_get_iio_chan(chip, chip->usbin_i_chan,
+					  &val->intval);
+		return 0;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		error = smb2_get_iio_chan(chip, chip->usbin_v_chan,
+					  &val->intval);
+		return 0;
+	case POWER_SUPPLY_PROP_ONLINE:
+		error = smb2_get_prop_usb_online(chip, &val->intval);
+		return 0;
+	case POWER_SUPPLY_PROP_STATUS:
+		error = smb2_get_prop_status(chip, &val->intval);
+		return 0;
+	case POWER_SUPPLY_PROP_HEALTH:
+		error = smb2_get_prop_health(chip, &val->intval);
+		return 0;
+	case POWER_SUPPLY_PROP_USB_TYPE:
+		error = smb2_apsd_get_charger_type(chip, &val->intval);
+		return 0;
+	default:
+		dev_err(chip->dev, "invalid property: %d\n", psp);
+		return -EINVAL;
+	}
+}
+
+static int smb2_set_property(struct power_supply *psy,
+			     enum power_supply_property psp,
+			     const union power_supply_propval *val)
+{
+	struct smb2_chip *chip = power_supply_get_drvdata(psy);
+	int error = 0;
+
+	dev_info(chip->dev, "Setting property: %d", psp);
+
+	mutex_lock(&chip->reg_lock);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CURRENT_MAX:
+		error = smb2_set_current_limit(chip, val->intval);
+		break;
+	default:
+		dev_err(chip->dev, "No setter for property: %d\n", psp);
+		error = -EINVAL;
+	}
+
+	mutex_unlock(&chip->reg_lock);
+
+	return error;
+}
+
+static int smb2_property_is_writable(struct power_supply *psy,
+				     enum power_supply_property psp)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CURRENT_MAX:
+		return 1;
+	default:
+		return 0;
+	}
+}
+
+irqreturn_t smb2_handle_usb_plugin(int irq, void *data)
+{
+	struct smb2_chip *chip = data;
+	int rc;
+	unsigned int intrt_stat;
+
+	rc = regmap_read(chip->regmap, chip->base + INT_RT_STS, &intrt_stat);
+	if (rc < 0) {
+		dev_err(chip->dev,
+			"Couldn't read USB status from reg! ret=%d\n", rc);
+		return rc;
+	}
+
+	chip->usb_present = (bool)(intrt_stat & USBIN_PLUGIN_RT_STS_BIT);
+
+	power_supply_changed(chip->chg_psy);
+
+	schedule_delayed_work(&chip->status_change_work,
+			      msecs_to_jiffies(1000));
+
+	return IRQ_HANDLED;
+}
+
+static const struct power_supply_desc smb2_psy_desc = {
+	.name = "usb",
+	.type = POWER_SUPPLY_TYPE_USB,
+	.usb_types = smb2_usb_types,
+	.num_usb_types = ARRAY_SIZE(smb2_usb_types),
+	.properties = smb2_properties,
+	.num_properties = ARRAY_SIZE(smb2_properties),
+	.get_property = smb2_get_property,
+	.set_property = smb2_set_property,
+	.property_is_writeable = smb2_property_is_writable,
+};
+
+/* Init sequence derived from downstream driver */
+static struct smb2_register smb2_init_seq[] = {
+	{ .addr = AICL_RERUN_TIME_CFG_REG,
+	  .mask = AICL_RERUN_TIME_MASK,
+	  .val = 0 },
+	{ .addr = USBIN_AICL_OPTIONS_CFG_REG,
+	  .mask = USBIN_AICL_START_AT_MAX_BIT | USBIN_AICL_ADC_EN_BIT,
+	  .val = 0 },
+	/*
+	 * By default configure us as an upstream facing port
+	 * FIXME: for OTG we should set UFP_EN_CMD_BIT and DFP_EN_CMD_BIT both to 0
+	 */
+	{ .addr = TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG,
+	  .mask = TYPEC_POWER_ROLE_CMD_MASK | VCONN_EN_SRC_BIT |
+		  VCONN_EN_VALUE_BIT,
+	  .val = VCONN_EN_SRC_BIT | UFP_EN_CMD_BIT },
+	/*
+	 * disable Type-C factory mode and stay in Attached.SRC state when VCONN
+	 * over-current happens
+	 */
+	{ .addr = TYPE_C_CFG_REG,
+	  .mask = FACTORY_MODE_DETECTION_EN_BIT | VCONN_OC_CFG_BIT,
+	  .val = 0 },
+	/* Configure VBUS for software control */
+	{ .addr = OTG_CFG_REG, .mask = OTG_EN_SRC_CFG_BIT, .val = 0 },
+	{ .addr = FG_UPDATE_CFG_2_SEL_REG,
+	  .mask = SOC_LT_CHG_RECHARGE_THRESH_SEL_BIT |
+		  VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT,
+	  .val = VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT },
+	/* Enable automatic input current limit */
+	{ .addr = USBIN_AICL_OPTIONS_CFG_REG,
+	  .mask = USBIN_AICL_EN_BIT,
+	  .val = 1 },
+	{ .addr = USBIN_OPTIONS_1_CFG_REG, .mask = HVDCP_EN_BIT, .val = 0 },
+	{ .addr = CHARGING_ENABLE_CMD_REG,
+	  .mask = CHARGING_ENABLE_CMD_BIT,
+	  .val = CHARGING_ENABLE_CMD_BIT },
+	{ .addr = USBIN_LOAD_CFG_REG,
+	  .mask = ICL_OVERRIDE_AFTER_APSD_BIT,
+	  .val = ICL_OVERRIDE_AFTER_APSD_BIT },
+	{ .addr = CHGR_CFG2_REG,
+	  .mask = CHG_EN_POLARITY_BIT | CHG_EN_SRC_BIT,
+	  .val = 0 },
+	{ .addr = TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG,
+	  .mask = UFP_EN_CMD_BIT,
+	  .val = UFP_EN_CMD_BIT },
+	/* Set the default SDP charger type to a 500ma USB 2.0 port
+	 * and apply ICL override? (OnePlus 6 hack??)
+	 */
+	{ .addr = USBIN_ICL_OPTIONS_REG,
+	  .mask = USB51_MODE_BIT | USBIN_MODE_CHG_BIT,
+	  .val = USB51_MODE_BIT }, //| USBIN_MODE_CHG_BIT },
+};
+
+static int smb2_init_hw(struct smb2_chip *chip)
+{
+	int rc, i;
+
+	for (i = 0; i < ARRAY_SIZE(smb2_init_seq); i++) {
+		dev_info(chip->dev, "%d: Writing 0x%02x to 0x%02x\n", i,
+			 smb2_init_seq[i].val, smb2_init_seq[i].addr);
+		rc = regmap_update_bits(chip->regmap,
+					chip->base + smb2_init_seq[i].addr,
+					smb2_init_seq[i].mask,
+					smb2_init_seq[i].val);
+		if (rc < 0) {
+			dev_err(chip->dev,
+				"%s: Failed to write 0x%02x to 0x%02x: %d\n",
+				__func__, smb2_init_seq[i].val,
+				smb2_init_seq[i].addr, rc);
+			return rc;
+		}
+	}
+
+	smb2_set_current_limit(chip, 1950 * 1000);
+	return rc;
+}
+
+static int smb2_probe(struct platform_device *pdev)
+{
+	struct power_supply_config supply_config = {};
+	struct smb2_chip *chip;
+	int rc = 0, irq;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->dev = &pdev->dev;
+	chip->name = pdev->name;
+	mutex_init(&chip->reg_lock);
+
+	chip->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!chip->regmap) {
+		dev_err(chip->dev, "failed to locate the regmap\n");
+		return -ENODEV;
+	}
+
+	rc = device_property_read_u32(chip->dev, "reg", &chip->base);
+	if (rc < 0) {
+		dev_err(chip->dev, "Couldn't read base address from dt: %d\n",
+			rc);
+		return rc;
+	}
+
+	irq = of_irq_get_byname(pdev->dev.of_node, "usb-plugin");
+	if (irq < 0) {
+		dev_err(&pdev->dev, "Couldn't get irq usb-plugin byname\n");
+		return irq;
+	}
+
+	rc = devm_request_threaded_irq(chip->dev, irq, NULL,
+				       smb2_handle_usb_plugin, IRQF_ONESHOT,
+				       "usb-plugin", chip);
+	if (rc < 0) {
+		dev_err(chip->dev, "Couldn't request irq %d\n", irq);
+		return rc;
+	}
+
+	chip->usbin_v_chan = iio_channel_get(chip->dev, "usbin_v");
+	chip->usbin_i_chan = iio_channel_get(chip->dev, "usbin_i");
+
+	/* RRADC should probe before us, but just in case it doesn't */
+	if (PTR_ERR(chip->usbin_v_chan) == -EPROBE_DEFER ||
+	    PTR_ERR(chip->usbin_i_chan) == -EPROBE_DEFER) {
+		return -EPROBE_DEFER;
+	}
+
+	rc = smb2_init_hw(chip);
+	if (rc < 0) {
+		dev_err(chip->dev, "Couldn't init hw %d\n", rc);
+		return rc;
+	}
+
+	supply_config.drv_data = chip;
+	supply_config.of_node = pdev->dev.of_node;
+
+	chip->chg_psy = devm_power_supply_register(chip->dev, &smb2_psy_desc,
+						   &supply_config);
+	if (IS_ERR(chip->chg_psy)) {
+		dev_err(&pdev->dev, "failed to register power supply\n");
+		return PTR_ERR(chip->chg_psy);
+	}
+
+	rc = power_supply_get_battery_info(chip->chg_psy, &chip->batt_info);
+	if (rc) {
+		dev_err(&pdev->dev, "Failed to get battery info: %d\n", rc);
+		return rc;
+	}
+
+	INIT_DELAYED_WORK(&chip->status_change_work, smb2_status_change_work);
+
+	rc = chip->batt_info->voltage_max_design_uv;
+	rc = regmap_update_bits(chip->regmap,
+				chip->base + FLOAT_VOLTAGE_CFG_REG,
+				FLOAT_VOLTAGE_SETTING_MASK, rc / 1000);
+	if (rc < 0)
+		dev_err(chip->dev, "Couldn't set vbat max rc = %d\n", rc);
+
+	platform_set_drvdata(pdev, chip);
+
+	/* trigger the IRQ to set up initial state */
+	smb2_handle_usb_plugin(irq, chip);
+
+	return 0;
+}
+
+static int smb2_remove(struct platform_device *pdev)
+{
+	struct smb2_chip *chip = platform_get_drvdata(pdev);
+
+	power_supply_put_battery_info(chip->chg_psy, chip->batt_info);
+	return 0;
+}
+
+static const struct of_device_id fg_match_id_table[] = {
+	{ .compatible = "qcom,pmi8998-smb2" },
+	{ /* sentinal */ }
+};
+MODULE_DEVICE_TABLE(of, fg_match_id_table);
+
+static struct platform_driver qcom_spmi_smb2 = {
+	.probe = smb2_probe,
+	.remove = smb2_remove,
+	.driver = {
+		.name = "qcom-spmi-smb2",
+		.of_match_table = fg_match_id_table,
+	},
+};
+
+module_platform_driver(qcom_spmi_smb2);
+
+MODULE_AUTHOR("Caleb Connolly <caleb.connolly@linaro.org>");
+MODULE_DESCRIPTION("Qualcomm SMB2 Charger Driver");
+MODULE_LICENSE("GPL");
-- 
2.35.1

Re: [PATCH 1/6] power: supply: add Qualcomm SMB2 driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 33348 bytes --]

Hi Caleb,

Thanks for the driver. It looks mostly good. Please find my feedback
inline:

On Fri, Apr 01, 2022 at 09:26:38PM +0100, Caleb Connolly wrote:
> Add a driver for the SMB2 charger block found in the Qualcomm PMI8998
> and PM660.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  drivers/power/supply/Kconfig     |   9 +
>  drivers/power/supply/Makefile    |   1 +
>  drivers/power/supply/qcom_smb2.c | 930 +++++++++++++++++++++++++++++++
>  3 files changed, 940 insertions(+)
>  create mode 100644 drivers/power/supply/qcom_smb2.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 1aa8323ad9f6..c72da3d75258 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -897,4 +897,13 @@ config BATTERY_UG3105
>  	  device is off or suspended, the functionality of this driver is
>  	  limited to reporting capacity only.
>  
> +config CHARGER_QCOM_SMB2
> +	tristate "Qualcomm PMIC charger driver"
> +	depends on MFD_SPMI_PMIC
> +	depends on IIO
> +	help
> +	  Say Y here to enable the Qualcomm PMIC Charger driver. This
> +	  adds support for the SMB2 switch mode battery charger found
> +	  in PMI8998 and related PMICs.
> +
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 7f02f36aea55..e5db9eda6a9b 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -108,3 +108,4 @@ obj-$(CONFIG_BATTERY_ACER_A500)	+= acer_a500_battery.o
>  obj-$(CONFIG_BATTERY_SURFACE)	+= surface_battery.o
>  obj-$(CONFIG_CHARGER_SURFACE)	+= surface_charger.o
>  obj-$(CONFIG_BATTERY_UG3105)	+= ug3105_battery.o
> +obj-$(CONFIG_CHARGER_QCOM_SMB2)	+= qcom_smb2.o
> diff --git a/drivers/power/supply/qcom_smb2.c b/drivers/power/supply/qcom_smb2.c
> new file mode 100644
> index 000000000000..63100cfc3300
> --- /dev/null
> +++ b/drivers/power/supply/qcom_smb2.c
> @@ -0,0 +1,930 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2016-2019 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2021, Linaro Ltd.
> + *  Author: Caleb Connolly <caleb.connolly@linaro.org>
> + *
> + * This driver is for the switch-mode battery charger and boost
> + * hardware found in pmi8998 and related PMICs.
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/kernel.h>
> +#include <linux/minmax.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>

not needed?

> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>

not needed (at this point, since there is not OTG handling)

> +#include <linux/device.h>

not needed?

> +#include <linux/iio/consumer.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +#include <linux/power_supply.h>
> +#include <linux/module.h>
> +#include <linux/math64.h>

not needed?

> +#include <linux/of_irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +
> +#define BATTERY_CHARGER_STATUS_2_REG 0x07
> +#define INPUT_CURRENT_LIMITED_BIT BIT(7)
> +#define CHARGER_ERROR_STATUS_SFT_EXPIRE_BIT BIT(6)
> +#define CHARGER_ERROR_STATUS_BAT_OV_BIT BIT(5)
> +#define CHARGER_ERROR_STATUS_BAT_TERM_MISSING_BIT BIT(4)
> +#define BAT_TEMP_STATUS_MASK GENMASK(3, 0)
> +#define BAT_TEMP_STATUS_SOFT_LIMIT_MASK GENMASK(3, 2)
> +#define BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT BIT(3)
> +#define BAT_TEMP_STATUS_COLD_SOFT_LIMIT_BIT BIT(2)
> +#define BAT_TEMP_STATUS_TOO_HOT_BIT BIT(1)
> +#define BAT_TEMP_STATUS_TOO_COLD_BIT BIT(0)
> +
> +#define CHARGING_ENABLE_CMD_REG 0x42
> +#define CHARGING_ENABLE_CMD_BIT BIT(0)
> +
> +#define CHGR_CFG2_REG 0x51
> +#define CHG_EN_SRC_BIT BIT(7)
> +#define CHG_EN_POLARITY_BIT BIT(6)
> +#define PRETOFAST_TRANSITION_CFG_BIT BIT(5)
> +#define BAT_OV_ECC_BIT BIT(4)
> +#define I_TERM_BIT BIT(3)
> +#define AUTO_RECHG_BIT BIT(2)
> +#define EN_ANALOG_DROP_IN_VBATT_BIT BIT(1)
> +#define CHARGER_INHIBIT_BIT BIT(0)
> +
> +#define FAST_CHARGE_CURRENT_CFG_REG 0x61
> +#define FAST_CHARGE_CURRENT_SETTING_MASK GENMASK(7, 0)
> +
> +#define FLOAT_VOLTAGE_CFG_REG 0x70
> +#define FLOAT_VOLTAGE_SETTING_MASK GENMASK(7, 0)
> +
> +#define FG_UPDATE_CFG_2_SEL_REG 0x7D
> +#define SOC_LT_OTG_THRESH_SEL_BIT BIT(3)
> +#define SOC_LT_CHG_RECHARGE_THRESH_SEL_BIT BIT(2)
> +#define VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT BIT(1)
> +#define IBT_LT_CHG_TERM_THRESH_SEL_BIT BIT(0)
> +
> +#define JEITA_EN_CFG_REG 0x90
> +#define JEITA_EN_HARDLIMIT_BIT BIT(4)
> +#define JEITA_EN_HOT_SL_FCV_BIT BIT(3)
> +#define JEITA_EN_COLD_SL_FCV_BIT BIT(2)
> +#define JEITA_EN_HOT_SL_CCC_BIT BIT(1)
> +#define JEITA_EN_COLD_SL_CCC_BIT BIT(0)
> +
> +#define INT_RT_STS 0x310
> +#define TYPE_C_CHANGE_RT_STS_BIT BIT(7)
> +#define USBIN_ICL_CHANGE_RT_STS_BIT BIT(6)
> +#define USBIN_SOURCE_CHANGE_RT_STS_BIT BIT(5)
> +#define USBIN_PLUGIN_RT_STS_BIT BIT(4)
> +#define USBIN_OV_RT_STS_BIT BIT(3)
> +#define USBIN_UV_RT_STS_BIT BIT(2)
> +#define USBIN_LT_3P6V_RT_STS_BIT BIT(1)
> +#define USBIN_COLLAPSE_RT_STS_BIT BIT(0)
> +
> +#define BATTERY_CHARGER_STATUS_1_REG 0x06
> +#define BVR_INITIAL_RAMP_BIT BIT(7)
> +#define CC_SOFT_TERMINATE_BIT BIT(6)
> +#define STEP_CHARGING_STATUS_SHIFT 3
> +#define STEP_CHARGING_STATUS_MASK GENMASK(5, 3)
> +#define BATTERY_CHARGER_STATUS_MASK GENMASK(2, 0)
> +
> +#define BATTERY_HEALTH_STATUS_REG 0x07
> +
> +#define OTG_CFG_REG 0x153
> +#define OTG_RESERVED_MASK GENMASK(7, 6)
> +#define DIS_OTG_ON_TLIM_BIT BIT(5)
> +#define QUICKSTART_OTG_FASTROLESWAP_BIT BIT(4)
> +#define INCREASE_DFP_TIME_BIT BIT(3)
> +#define ENABLE_OTG_IN_DEBUG_MODE_BIT BIT(2)
> +#define OTG_EN_SRC_CFG_BIT BIT(1)
> +#define CONCURRENT_MODE_CFG_BIT BIT(0)
> +
> +#define OTG_ENG_OTG_CFG_REG 0x1C0
> +#define ENG_BUCKBOOST_HALT1_8_MODE_BIT BIT(0)
> +
> +#define APSD_STATUS_REG 0x307
> +#define APSD_STATUS_7_BIT BIT(7)
> +#define HVDCP_CHECK_TIMEOUT_BIT BIT(6)
> +#define SLOW_PLUGIN_TIMEOUT_BIT BIT(5)
> +#define ENUMERATION_DONE_BIT BIT(4)
> +#define VADP_CHANGE_DONE_AFTER_AUTH_BIT BIT(3)
> +#define QC_AUTH_DONE_STATUS_BIT BIT(2)
> +#define QC_CHARGER_BIT BIT(1)
> +#define APSD_DTC_STATUS_DONE_BIT BIT(0)
> +
> +#define APSD_RESULT_STATUS_REG 0x308
> +#define ICL_OVERRIDE_LATCH_BIT BIT(7)
> +#define APSD_RESULT_STATUS_MASK GENMASK(6, 0)
> +#define QC_3P0_BIT BIT(6)
> +#define QC_2P0_BIT BIT(5)
> +#define FLOAT_CHARGER_BIT BIT(4)
> +#define DCP_CHARGER_BIT BIT(3)
> +#define CDP_CHARGER_BIT BIT(2)
> +#define OCP_CHARGER_BIT BIT(1)
> +#define SDP_CHARGER_BIT BIT(0)
> +
> +#define TYPE_C_STATUS_1_REG 0x30B
> +#define UFP_TYPEC_MASK GENMASK(7, 5)
> +#define UFP_TYPEC_RDSTD_BIT BIT(7)
> +#define UFP_TYPEC_RD1P5_BIT BIT(6)
> +#define UFP_TYPEC_RD3P0_BIT BIT(5)
> +#define UFP_TYPEC_FMB_255K_BIT BIT(4)
> +#define UFP_TYPEC_FMB_301K_BIT BIT(3)
> +#define UFP_TYPEC_FMB_523K_BIT BIT(2)
> +#define UFP_TYPEC_FMB_619K_BIT BIT(1)
> +#define UFP_TYPEC_OPEN_OPEN_BIT BIT(0)
> +
> +#define TYPE_C_STATUS_2_REG 0x30C
> +#define DFP_RA_OPEN_BIT BIT(7)
> +#define TIMER_STAGE_BIT BIT(6)
> +#define EXIT_UFP_MODE_BIT BIT(5)
> +#define EXIT_DFP_MODE_BIT BIT(4)
> +#define DFP_TYPEC_MASK GENMASK(3, 0)
> +#define DFP_RD_OPEN_BIT BIT(3)
> +#define DFP_RD_RA_VCONN_BIT BIT(2)
> +#define DFP_RD_RD_BIT BIT(1)
> +#define DFP_RA_RA_BIT BIT(0)
> +
> +#define TYPE_C_STATUS_3_REG 0x30D
> +#define ENABLE_BANDGAP_BIT BIT(7)
> +#define U_USB_GND_NOVBUS_BIT BIT(6)
> +#define U_USB_FLOAT_NOVBUS_BIT BIT(5)
> +#define U_USB_GND_BIT BIT(4)
> +#define U_USB_FMB1_BIT BIT(3)
> +#define U_USB_FLOAT1_BIT BIT(2)
> +#define U_USB_FMB2_BIT BIT(1)
> +#define U_USB_FLOAT2_BIT BIT(0)
> +
> +#define TYPE_C_STATUS_4_REG 0x30E
> +#define UFP_DFP_MODE_STATUS_BIT BIT(7)
> +#define TYPEC_VBUS_STATUS_BIT BIT(6)
> +#define TYPEC_VBUS_ERROR_STATUS_BIT BIT(5)
> +#define TYPEC_DEBOUNCE_DONE_STATUS_BIT BIT(4)
> +#define TYPEC_UFP_AUDIO_ADAPT_STATUS_BIT BIT(3)
> +#define TYPEC_VCONN_OVERCURR_STATUS_BIT BIT(2)
> +#define CC_ORIENTATION_BIT BIT(1)
> +#define CC_ATTACHED_BIT BIT(0)
> +
> +#define TYPE_C_STATUS_5_REG 0x30F
> +#define TRY_SOURCE_FAILED_BIT BIT(6)
> +#define TRY_SINK_FAILED_BIT BIT(5)
> +#define TIMER_STAGE_2_BIT BIT(4)
> +#define TYPEC_LEGACY_CABLE_STATUS_BIT BIT(3)
> +#define TYPEC_NONCOMP_LEGACY_CABLE_STATUS_BIT BIT(2)
> +#define TYPEC_TRYSOURCE_DETECT_STATUS_BIT BIT(1)
> +#define TYPEC_TRYSINK_DETECT_STATUS_BIT BIT(0)
> +
> +#define CMD_APSD_REG 0x341
> +#define ICL_OVERRIDE_BIT BIT(1)
> +#define APSD_RERUN_BIT BIT(0)
> +
> +#define TYPE_C_CFG_REG 0x358
> +#define APSD_START_ON_CC_BIT BIT(7)
> +#define WAIT_FOR_APSD_BIT BIT(6)
> +#define FACTORY_MODE_DETECTION_EN_BIT BIT(5)
> +#define FACTORY_MODE_ICL_3A_4A_BIT BIT(4)
> +#define FACTORY_MODE_DIS_CHGING_CFG_BIT BIT(3)
> +#define SUSPEND_NON_COMPLIANT_CFG_BIT BIT(2)
> +#define VCONN_OC_CFG_BIT BIT(1)
> +#define TYPE_C_OR_U_USB_BIT BIT(0)
> +
> +#define TYPE_C_CFG_2_REG 0x359
> +#define TYPE_C_DFP_CURRSRC_MODE_BIT BIT(7)
> +#define DFP_CC_1P4V_OR_1P6V_BIT BIT(6)
> +#define VCONN_SOFTSTART_CFG_MASK GENMASK(5, 4)
> +#define EN_TRY_SOURCE_MODE_BIT BIT(3)
> +#define USB_FACTORY_MODE_ENABLE_BIT BIT(2)
> +#define TYPE_C_UFP_MODE_BIT BIT(1)
> +#define EN_80UA_180UA_CUR_SOURCE_BIT BIT(0)
> +
> +#define TYPE_C_CFG_3_REG 0x35A
> +#define TVBUS_DEBOUNCE_BIT BIT(7)
> +#define TYPEC_LEGACY_CABLE_INT_EN_BIT BIT(6)
> +#define TYPEC_NONCOMPLIANT_LEGACY_CABLE_INT_EN_BIT BIT(5)
> +#define TYPEC_TRYSOURCE_DETECT_INT_EN_BIT BIT(4)
> +#define TYPEC_TRYSINK_DETECT_INT_EN_BIT BIT(3)
> +#define EN_TRYSINK_MODE_BIT BIT(2)
> +#define EN_LEGACY_CABLE_DETECTION_BIT BIT(1)
> +#define ALLOW_PD_DRING_UFP_TCCDB_BIT BIT(0)
> +
> +#define USBIN_OPTIONS_1_CFG_REG 0x362
> +#define CABLE_R_SEL_BIT BIT(7)
> +#define HVDCP_AUTH_ALG_EN_CFG_BIT BIT(6)
> +#define HVDCP_AUTONOMOUS_MODE_EN_CFG_BIT BIT(5)
> +#define INPUT_PRIORITY_BIT BIT(4)
> +#define AUTO_SRC_DETECT_BIT BIT(3)
> +#define HVDCP_EN_BIT BIT(2)
> +#define VADP_INCREMENT_VOLTAGE_LIMIT_BIT BIT(1)
> +#define VADP_TAPER_TIMER_EN_BIT BIT(0)
> +
> +#define USBIN_OPTIONS_2_CFG_REG 0x363
> +#define WIPWR_RST_EUD_CFG_BIT BIT(7)
> +#define SWITCHER_START_CFG_BIT BIT(6)
> +#define DCD_TIMEOUT_SEL_BIT BIT(5)
> +#define OCD_CURRENT_SEL_BIT BIT(4)
> +#define SLOW_PLUGIN_TIMER_EN_CFG_BIT BIT(3)
> +#define FLOAT_OPTIONS_MASK GENMASK(2, 0)
> +#define FLOAT_DIS_CHGING_CFG_BIT BIT(2)
> +#define SUSPEND_FLOAT_CFG_BIT BIT(1)
> +#define FORCE_FLOAT_SDP_CFG_BIT BIT(0)
> +
> +#define TAPER_TIMER_SEL_CFG_REG 0x364
> +#define TYPEC_SPARE_CFG_BIT BIT(7)
> +#define TYPEC_DRP_DFP_TIME_CFG_BIT BIT(5)
> +#define TAPER_TIMER_SEL_MASK GENMASK(1, 0)
> +
> +#define USBIN_LOAD_CFG_REG 0x365
> +#define USBIN_OV_CH_LOAD_OPTION_BIT BIT(7)
> +#define ICL_OVERRIDE_AFTER_APSD_BIT BIT(4)
> +
> +#define USBIN_ICL_OPTIONS_REG 0x366
> +#define CFG_USB3P0_SEL_BIT BIT(2)
> +#define USB51_MODE_BIT BIT(1)
> +#define USBIN_MODE_CHG_BIT BIT(0)
> +
> +#define TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG 0x368
> +#define EXIT_SNK_BASED_ON_CC_BIT BIT(7)
> +#define VCONN_EN_ORIENTATION_BIT BIT(6)
> +#define TYPEC_VCONN_OVERCURR_INT_EN_BIT BIT(5)
> +#define VCONN_EN_SRC_BIT BIT(4)
> +#define VCONN_EN_VALUE_BIT BIT(3)
> +#define TYPEC_POWER_ROLE_CMD_MASK GENMASK(2, 0)
> +#define UFP_EN_CMD_BIT BIT(2)
> +#define DFP_EN_CMD_BIT BIT(1)
> +#define TYPEC_DISABLE_CMD_BIT BIT(0)
> +
> +#define USBIN_CURRENT_LIMIT_CFG_REG 0x370
> +#define USBIN_CURRENT_LIMIT_MASK GENMASK(7, 0)
> +
> +#define USBIN_AICL_OPTIONS_CFG_REG 0x380
> +#define SUSPEND_ON_COLLAPSE_USBIN_BIT BIT(7)
> +#define USBIN_AICL_HDC_EN_BIT BIT(6)
> +#define USBIN_AICL_START_AT_MAX_BIT BIT(5)
> +#define USBIN_AICL_RERUN_EN_BIT BIT(4)
> +#define USBIN_AICL_ADC_EN_BIT BIT(3)
> +#define USBIN_AICL_EN_BIT BIT(2)
> +#define USBIN_HV_COLLAPSE_RESPONSE_BIT BIT(1)
> +#define USBIN_LV_COLLAPSE_RESPONSE_BIT BIT(0)
> +
> +#define DC_ENG_SSUPPLY_CFG2_REG 0x4C1
> +#define ENG_SSUPPLY_IVREF_OTG_SS_MASK GENMASK(2, 0)
> +#define OTG_SS_SLOW 0x3
> +
> +#define DCIN_AICL_REF_SEL_CFG_REG 0x481
> +#define DCIN_CONT_AICL_THRESHOLD_CFG_MASK GENMASK(5, 0)
> +
> +#define WI_PWR_OPTIONS_REG 0x495
> +#define CHG_OK_BIT BIT(7)
> +#define WIPWR_UVLO_IRQ_OPT_BIT BIT(6)
> +#define BUCK_HOLDOFF_ENABLE_BIT BIT(5)
> +#define CHG_OK_HW_SW_SELECT_BIT BIT(4)
> +#define WIPWR_RST_ENABLE_BIT BIT(3)
> +#define DCIN_WIPWR_IRQ_SELECT_BIT BIT(2)
> +#define AICL_SWITCH_ENABLE_BIT BIT(1)
> +#define ZIN_ICL_ENABLE_BIT BIT(0)
> +
> +#define ICL_STATUS_REG 0x607
> +#define INPUT_CURRENT_LIMIT_MASK GENMASK(7, 0)
> +
> +#define POWER_PATH_STATUS_REG 0x60B
> +#define P_PATH_INPUT_SS_DONE_BIT BIT(7)
> +#define P_PATH_USBIN_SUSPEND_STS_BIT BIT(6)
> +#define P_PATH_DCIN_SUSPEND_STS_BIT BIT(5)
> +#define P_PATH_USE_USBIN_BIT BIT(4)
> +#define P_PATH_USE_DCIN_BIT BIT(3)
> +#define P_PATH_POWER_PATH_MASK GENMASK(2, 1)
> +#define P_PATH_VALID_INPUT_POWER_SOURCE_STS_BIT BIT(0)
> +
> +#define WD_CFG_REG 0x651
> +#define WATCHDOG_TRIGGER_AFP_EN_BIT BIT(7)
> +#define BARK_WDOG_INT_EN_BIT BIT(6)
> +#define BITE_WDOG_INT_EN_BIT BIT(5)
> +#define SFT_AFTER_WDOG_IRQ_MASK GENMASK(4, 3)
> +#define WDOG_IRQ_SFT_BIT BIT(2)
> +#define WDOG_TIMER_EN_ON_PLUGIN_BIT BIT(1)
> +#define WDOG_TIMER_EN_BIT BIT(0)
> +
> +#define AICL_RERUN_TIME_CFG_REG 0x661
> +#define AICL_RERUN_TIME_MASK GENMASK(1, 0)
> +
> +#define SDP_CURRENT_UA 500000
> +#define CDP_CURRENT_UA 1500000
> +#define DCP_CURRENT_UA 1500000
> +
> +enum charger_status {
> +	TRICKLE_CHARGE = 0,
> +	PRE_CHARGE,
> +	FAST_CHARGE,
> +	FULLON_CHARGE,
> +	TAPER_CHARGE,
> +	TERMINATE_CHARGE,
> +	INHIBIT_CHARGE,
> +	DISABLE_CHARGE,
> +};
> +
> +struct smb2_register {
> +	u16 addr;
> +	u8 mask;
> +	u8 val;
> +};
> +
> +/**
> + * struct smb2_chip - smb2 chip structure
> + * @dev:		Device reference for power_supply

name is missing

> + * @base:		Base address for smb2 registers
> + * @regmap:		Register map
> + * @reg_lock:		Mutex for register access
> + * @batt_info:		Battery data from DT

status_change_work is missing

> + * @usbin_i_chan:	USBIN current measurement channel
> + * @usbin_v_chan:	USBIN voltage measurement channel
> + * @chg_psy:		Charger power supply instance
> + * @usb_present:	True if a charger is attached
> + */
> +struct smb2_chip {
> +	struct device *dev;
> +	const char *name;
> +	unsigned int base;
> +	struct regmap *regmap;
> +	struct mutex reg_lock;
> +	struct power_supply_battery_info *batt_info;
> +
> +	struct delayed_work status_change_work;
> +
> +	struct iio_channel *usbin_i_chan;
> +	struct iio_channel *usbin_v_chan;
> +
> +	struct power_supply *chg_psy;
> +
> +	bool usb_present;
> +};
> +
> +static enum power_supply_property smb2_properties[] = {
> +	POWER_SUPPLY_PROP_MANUFACTURER, POWER_SUPPLY_PROP_MODEL_NAME,
> +	POWER_SUPPLY_PROP_CURRENT_MAX,	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_HEALTH,	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_USB_TYPE,
> +};

Make this one item per line. This results in better line diffs when
something is changed later.

> +
> +static enum power_supply_usb_type smb2_usb_types[] = {
> +	POWER_SUPPLY_USB_TYPE_SDP, POWER_SUPPLY_USB_TYPE_DCP,
> +	POWER_SUPPLY_USB_TYPE_CDP, POWER_SUPPLY_USB_TYPE_C,
> +	POWER_SUPPLY_USB_TYPE_PD_DRP
> +};

Same as before.

> +/*
> + * Qualcomm "automatic power source detection" aka APSD
> + * tells us what type of charger we're connected to.
> + */
> +static int smb2_apsd_get_charger_type(struct smb2_chip *chip, int *val)
> +{
> +	int rc;
> +	unsigned int apsd_stat, stat;
> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	rc = regmap_read(chip->regmap, chip->base + APSD_STATUS_REG,
> +			 &apsd_stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Failed to read apsd status, rc = %d", rc);
> +		return rc;

this returns without unlocking mutex...

> +	}
> +	if (!(apsd_stat & APSD_DTC_STATUS_DONE_BIT)) {
> +		dev_err(chip->dev, "Apsd not ready");
> +		return -EAGAIN;

another return with locked mutex

> +	}
> +
> +	rc = regmap_read(chip->regmap, chip->base + APSD_RESULT_STATUS_REG,
> +			 &stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Failed to read apsd result, rc = %d", rc);
> +		return rc;

another return with locked mutex

> +	}
> +
> +	mutex_unlock(&chip->reg_lock);
> +
> +	stat &= APSD_RESULT_STATUS_MASK;
> +
> +	if (stat & CDP_CHARGER_BIT) {
> +		*val = POWER_SUPPLY_USB_TYPE_CDP;
> +	} else if (stat &
> +		   (DCP_CHARGER_BIT | OCP_CHARGER_BIT | FLOAT_CHARGER_BIT)) {
> +		*val = POWER_SUPPLY_USB_TYPE_DCP;
> +	} else { /* SDP_CHARGER_BIT (or others) */
> +		*val = POWER_SUPPLY_USB_TYPE_SDP;
> +	}
> +
> +	return 0;
> +}
> +
> +int smb2_get_prop_usb_online(struct smb2_chip *chip, int *val)
> +{
> +	unsigned int stat;
> +	int rc;
> +
> +	rc = regmap_read(chip->regmap, chip->base + POWER_PATH_STATUS_REG,
> +			 &stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't read POWER_PATH_STATUS! ret=%d\n",
> +			rc);
> +		return rc;
> +	}
> +
> +	*val = (stat & P_PATH_USE_USBIN_BIT) &&
> +	       (stat & P_PATH_VALID_INPUT_POWER_SOURCE_STS_BIT);
> +	return rc;
> +}
> +
> +int smb2_get_prop_status(struct smb2_chip *chip, int *val)
> +{
> +	int usb_online_val;
> +	unsigned int stat;
> +	int rc;
> +	bool usb_online;
> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	rc = smb2_get_prop_usb_online(chip, &usb_online_val);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't get usb online property rc = %d\n",
> +			rc);
> +		return rc;

another return with locked mutex

> +	}
> +	usb_online = (bool)usb_online_val;
> +
> +	if (!usb_online) {

Just use 'if (!usb_online_val) {' and drop the explicit cast.

> +		*val = POWER_SUPPLY_STATUS_DISCHARGING;
> +		return rc;
> +	}
> +
> +	rc = regmap_read(chip->regmap,
> +			 chip->base + BATTERY_CHARGER_STATUS_1_REG, &stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev,
> +			"Failed to read charging status ret=%d\n", rc);
> +		return rc;

another return with locked mutex

> +	}
> +
> +	mutex_unlock(&chip->reg_lock);
> +
> +	stat = stat & BATTERY_CHARGER_STATUS_MASK;
> +
> +	switch (stat) {
> +	case TRICKLE_CHARGE:
> +	case PRE_CHARGE:
> +	case FAST_CHARGE:
> +	case FULLON_CHARGE:
> +	case TAPER_CHARGE:
> +		*val = POWER_SUPPLY_STATUS_CHARGING;
> +		return rc;
> +	case DISABLE_CHARGE:
> +		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		return rc;
> +	case TERMINATE_CHARGE:
> +		*val = POWER_SUPPLY_STATUS_FULL;
> +		return rc;
> +	case INHIBIT_CHARGE:
> +	default:
> +		*val = POWER_SUPPLY_STATUS_UNKNOWN;
> +		return rc;
> +	}
> +}
> +
> +static inline int smb2_get_current_limit(struct smb2_chip *chip,
> +					 unsigned int *val)
> +{
> +	int rc = regmap_read(chip->regmap, chip->base + ICL_STATUS_REG, val);
> +
> +	if (rc >= 0)
> +		*val *= 25000;
> +	return rc;
> +}
> +
> +static inline int smb2_set_current_limit(struct smb2_chip *chip,
> +					 unsigned int val)
> +{
> +	unsigned char val_raw;
> +
> +	if (val > 4800000) {
> +		dev_err(chip->dev,
> +			"Can't set current limit higher than 4800000uA");
> +		return -EINVAL;
> +	}
> +	val_raw = val / 25000;
> +	return regmap_write(chip->regmap,
> +			    chip->base + USBIN_CURRENT_LIMIT_CFG_REG, val_raw);
> +}
> +
> +void smb2_status_change_work(struct work_struct *work)
> +{
> +	struct smb2_chip *chip =
> +		container_of(work, struct smb2_chip, status_change_work.work);
> +	unsigned int charger_type, current_ua;
> +	int usb_online, count, rc;
> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	smb2_get_prop_usb_online(chip, &usb_online);
> +	if (usb_online == 0)
> +		return;

another return with locked mutex

> +
> +	for (count = 0; count < 3; count++) {
> +		dev_info(chip->dev, "get charger type retry %d\n", count);

dev_dbg()?

> +		rc = smb2_apsd_get_charger_type(chip, &charger_type);
> +		if (rc == 0)
> +			break;
> +		msleep(100);
> +	}
> +
> +	if (rc < 0) {
> +		rc = regmap_update_bits(chip->regmap, chip->base + CMD_APSD_REG,
> +					APSD_RERUN_BIT, APSD_RERUN_BIT);
> +		schedule_delayed_work(&chip->status_change_work,
> +				      msecs_to_jiffies(1500));
> +		return;

another return with locked mutex

> +	}
> +
> +	switch (charger_type) {
> +	case POWER_SUPPLY_USB_TYPE_CDP:
> +		current_ua = CDP_CURRENT_UA;
> +		break;
> +	case POWER_SUPPLY_USB_TYPE_DCP:
> +		current_ua = DCP_CURRENT_UA;
> +		break;
> +	case POWER_SUPPLY_USB_TYPE_SDP:
> +	default:
> +		current_ua = SDP_CURRENT_UA;
> +		break;
> +	}
> +
> +	smb2_set_current_limit(chip, current_ua);
> +	mutex_unlock(&chip->reg_lock);
> +	power_supply_changed(chip->chg_psy);
> +}
> +
> +int smb2_get_iio_chan(struct smb2_chip *chip, struct iio_channel *chan,
> +		      int *val)
> +{
> +	int rc = 0;
> +	union power_supply_propval status;
> +
> +	rc = power_supply_get_property(chip->chg_psy, POWER_SUPPLY_PROP_STATUS,
> +				       &status);
> +	if (rc < 0 || status.intval != POWER_SUPPLY_STATUS_CHARGING) {
> +		*val = 0;
> +		return 0;
> +	}
> +
> +	if (IS_ERR(chan)) {
> +		dev_err(chip->dev, "Failed to chan, err = %li", PTR_ERR(chan));
> +		return PTR_ERR(chan);
> +	}
> +
> +	rc = iio_read_channel_processed(chan, val);
> +	return rc;
> +}
> +
> +int smb2_get_prop_health(struct smb2_chip *chip, int *val)
> +{
> +	int rc;
> +	unsigned int stat;
> +
> +	*val = POWER_SUPPLY_HEALTH_UNKNOWN;
> +
> +	rc = regmap_read(chip->regmap,
> +			 chip->base + BATTERY_CHARGER_STATUS_2_REG, &stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't read charger status rc=%d\n", rc);
> +		return rc;
> +	}
> +
> +	switch (stat & BATTERY_CHARGER_STATUS_MASK) {
> +	case CHARGER_ERROR_STATUS_BAT_OV_BIT:
> +		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> +		return 0;
> +	case BAT_TEMP_STATUS_TOO_COLD_BIT:
> +		*val = POWER_SUPPLY_HEALTH_COLD;
> +		return 0;
> +	case BAT_TEMP_STATUS_TOO_HOT_BIT:
> +		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
> +		return 0;
> +	case BAT_TEMP_STATUS_COLD_SOFT_LIMIT_BIT:
> +		*val = POWER_SUPPLY_HEALTH_COOL;
> +		return 0;
> +	case BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT:
> +		*val = POWER_SUPPLY_HEALTH_WARM;
> +		return 0;
> +	default:
> +		*val = POWER_SUPPLY_HEALTH_GOOD;
> +		return 0;
> +	}
> +}
> +
> +static int smb2_get_property(struct power_supply *psy,
> +			     enum power_supply_property psp,
> +			     union power_supply_propval *val)
> +{
> +	struct smb2_chip *chip = power_supply_get_drvdata(psy);
> +	int error = 0;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_MANUFACTURER:
> +		val->strval = "Qualcomm";
> +		return 0;
> +	case POWER_SUPPLY_PROP_MODEL_NAME:
> +		val->strval = chip->name;
> +		return 0;
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +		error = smb2_get_current_limit(chip, &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		error = smb2_get_iio_chan(chip, chip->usbin_i_chan,
> +					  &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		error = smb2_get_iio_chan(chip, chip->usbin_v_chan,
> +					  &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		error = smb2_get_prop_usb_online(chip, &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_STATUS:
> +		error = smb2_get_prop_status(chip, &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		error = smb2_get_prop_health(chip, &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_USB_TYPE:
> +		error = smb2_apsd_get_charger_type(chip, &val->intval);
> +		return 0;
> +	default:
> +		dev_err(chip->dev, "invalid property: %d\n", psp);
> +		return -EINVAL;
> +	}
> +}
> +
> +static int smb2_set_property(struct power_supply *psy,
> +			     enum power_supply_property psp,
> +			     const union power_supply_propval *val)
> +{
> +	struct smb2_chip *chip = power_supply_get_drvdata(psy);
> +	int error = 0;
> +
> +	dev_info(chip->dev, "Setting property: %d", psp);

drop

> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +		error = smb2_set_current_limit(chip, val->intval);
> +		break;
> +	default:
> +		dev_err(chip->dev, "No setter for property: %d\n", psp);
> +		error = -EINVAL;
> +	}
> +
> +	mutex_unlock(&chip->reg_lock);
> +
> +	return error;
> +}
> +
> +static int smb2_property_is_writable(struct power_supply *psy,
> +				     enum power_supply_property psp)
> +{
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +		return 1;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +irqreturn_t smb2_handle_usb_plugin(int irq, void *data)
> +{
> +	struct smb2_chip *chip = data;
> +	int rc;
> +	unsigned int intrt_stat;
> +
> +	rc = regmap_read(chip->regmap, chip->base + INT_RT_STS, &intrt_stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev,
> +			"Couldn't read USB status from reg! ret=%d\n", rc);
> +		return rc;
> +	}
> +
> +	chip->usb_present = (bool)(intrt_stat & USBIN_PLUGIN_RT_STS_BIT);

This variable is not read anywhere. I suppose you can just drop the
regmap read from this place?

> +	power_supply_changed(chip->chg_psy);
> +
> +	schedule_delayed_work(&chip->status_change_work,
> +			      msecs_to_jiffies(1000));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct power_supply_desc smb2_psy_desc = {
> +	.name = "usb",
> +	.type = POWER_SUPPLY_TYPE_USB,
> +	.usb_types = smb2_usb_types,
> +	.num_usb_types = ARRAY_SIZE(smb2_usb_types),
> +	.properties = smb2_properties,
> +	.num_properties = ARRAY_SIZE(smb2_properties),
> +	.get_property = smb2_get_property,
> +	.set_property = smb2_set_property,
> +	.property_is_writeable = smb2_property_is_writable,
> +};
> +
> +/* Init sequence derived from downstream driver */
> +static struct smb2_register smb2_init_seq[] = {

static const?

> +	{ .addr = AICL_RERUN_TIME_CFG_REG,
> +	  .mask = AICL_RERUN_TIME_MASK,
> +	  .val = 0 },
> +	{ .addr = USBIN_AICL_OPTIONS_CFG_REG,
> +	  .mask = USBIN_AICL_START_AT_MAX_BIT | USBIN_AICL_ADC_EN_BIT,
> +	  .val = 0 },
> +	/*
> +	 * By default configure us as an upstream facing port
> +	 * FIXME: for OTG we should set UFP_EN_CMD_BIT and DFP_EN_CMD_BIT both to 0
> +	 */
> +	{ .addr = TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG,
> +	  .mask = TYPEC_POWER_ROLE_CMD_MASK | VCONN_EN_SRC_BIT |
> +		  VCONN_EN_VALUE_BIT,
> +	  .val = VCONN_EN_SRC_BIT | UFP_EN_CMD_BIT },
> +	/*
> +	 * disable Type-C factory mode and stay in Attached.SRC state when VCONN
> +	 * over-current happens
> +	 */
> +	{ .addr = TYPE_C_CFG_REG,
> +	  .mask = FACTORY_MODE_DETECTION_EN_BIT | VCONN_OC_CFG_BIT,
> +	  .val = 0 },
> +	/* Configure VBUS for software control */
> +	{ .addr = OTG_CFG_REG, .mask = OTG_EN_SRC_CFG_BIT, .val = 0 },
> +	{ .addr = FG_UPDATE_CFG_2_SEL_REG,
> +	  .mask = SOC_LT_CHG_RECHARGE_THRESH_SEL_BIT |
> +		  VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT,
> +	  .val = VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT },
> +	/* Enable automatic input current limit */
> +	{ .addr = USBIN_AICL_OPTIONS_CFG_REG,
> +	  .mask = USBIN_AICL_EN_BIT,
> +	  .val = 1 },
> +	{ .addr = USBIN_OPTIONS_1_CFG_REG, .mask = HVDCP_EN_BIT, .val = 0 },
> +	{ .addr = CHARGING_ENABLE_CMD_REG,
> +	  .mask = CHARGING_ENABLE_CMD_BIT,
> +	  .val = CHARGING_ENABLE_CMD_BIT },
> +	{ .addr = USBIN_LOAD_CFG_REG,
> +	  .mask = ICL_OVERRIDE_AFTER_APSD_BIT,
> +	  .val = ICL_OVERRIDE_AFTER_APSD_BIT },
> +	{ .addr = CHGR_CFG2_REG,
> +	  .mask = CHG_EN_POLARITY_BIT | CHG_EN_SRC_BIT,
> +	  .val = 0 },
> +	{ .addr = TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG,
> +	  .mask = UFP_EN_CMD_BIT,
> +	  .val = UFP_EN_CMD_BIT },
> +	/* Set the default SDP charger type to a 500ma USB 2.0 port
> +	 * and apply ICL override? (OnePlus 6 hack??)
> +	 */
> +	{ .addr = USBIN_ICL_OPTIONS_REG,
> +	  .mask = USB51_MODE_BIT | USBIN_MODE_CHG_BIT,
> +	  .val = USB51_MODE_BIT }, //| USBIN_MODE_CHG_BIT },
> +};
> +
> +static int smb2_init_hw(struct smb2_chip *chip)
> +{
> +	int rc, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(smb2_init_seq); i++) {
> +		dev_info(chip->dev, "%d: Writing 0x%02x to 0x%02x\n", i,
> +			 smb2_init_seq[i].val, smb2_init_seq[i].addr);

dev_dbg

> +		rc = regmap_update_bits(chip->regmap,
> +					chip->base + smb2_init_seq[i].addr,
> +					smb2_init_seq[i].mask,
> +					smb2_init_seq[i].val);
> +		if (rc < 0) {
> +			dev_err(chip->dev,
> +				"%s: Failed to write 0x%02x to 0x%02x: %d\n",
> +				__func__, smb2_init_seq[i].val,
> +				smb2_init_seq[i].addr, rc);
> +			return rc;
> +		}
> +	}
> +
> +	smb2_set_current_limit(chip, 1950 * 1000);
> +	return rc;
> +}
> +
> +static int smb2_probe(struct platform_device *pdev)
> +{
> +	struct power_supply_config supply_config = {};
> +	struct smb2_chip *chip;
> +	int rc = 0, irq;
> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->dev = &pdev->dev;
> +	chip->name = pdev->name;
> +	mutex_init(&chip->reg_lock);
> +
> +	chip->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!chip->regmap) {
> +		dev_err(chip->dev, "failed to locate the regmap\n");
> +		return -ENODEV;

dev_err_probe()

> +	}
> +
> +	rc = device_property_read_u32(chip->dev, "reg", &chip->base);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't read base address from dt: %d\n",
> +			rc);
> +		return rc;

dev_err_probe()

> +	}
> +
> +	irq = of_irq_get_byname(pdev->dev.of_node, "usb-plugin");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Couldn't get irq usb-plugin byname\n");
> +		return irq;

dev_err_probe()

> +	}
> +
> +	rc = devm_request_threaded_irq(chip->dev, irq, NULL,
> +				       smb2_handle_usb_plugin, IRQF_ONESHOT,
> +				       "usb-plugin", chip);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't request irq %d\n", irq);
> +		return rc;

dev_err_probe()

> +	}
> +
> +	chip->usbin_v_chan = iio_channel_get(chip->dev, "usbin_v");
> +	chip->usbin_i_chan = iio_channel_get(chip->dev, "usbin_i");

maybe add another _ and make this usb_in_v and usb_in_i, which is
easier to read?

> +	/* RRADC should probe before us, but just in case it doesn't */
> +	if (PTR_ERR(chip->usbin_v_chan) == -EPROBE_DEFER ||
> +	    PTR_ERR(chip->usbin_i_chan) == -EPROBE_DEFER) {
> +		return -EPROBE_DEFER;

return dev_err_probe(chip->dev, -EPROBE_DEFER, "Missing IIO device");

to get a proper info string in /sys/kernel/debug/devices_deferred
for the charger when something goes wrong and IIO never probes.

> +	}
> +
> +	rc = smb2_init_hw(chip);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't init hw %d\n", rc);
> +		return rc;

dev_err_probe

> +	}
> +
> +	supply_config.drv_data = chip;
> +	supply_config.of_node = pdev->dev.of_node;
> +
> +	chip->chg_psy = devm_power_supply_register(chip->dev, &smb2_psy_desc,
> +						   &supply_config);
> +	if (IS_ERR(chip->chg_psy)) {
> +		dev_err(&pdev->dev, "failed to register power supply\n");
> +		return PTR_ERR(chip->chg_psy);

dev_err_probe

> +	}
> +
> +	rc = power_supply_get_battery_info(chip->chg_psy, &chip->batt_info);
> +	if (rc) {
> +		dev_err(&pdev->dev, "Failed to get battery info: %d\n", rc);
> +		return rc;

dev_err_probe

> +	}
> +
> +	INIT_DELAYED_WORK(&chip->status_change_work, smb2_status_change_work);

#include <linux/devm-helpers.h>

devm_delayed_work_autocancel()

Also the delayed work must be registered before the irq, since it
might trigger instantly during probe and or trigger the IRQ handler
during driver removal resulting in queuing another delasyed work
between the autocancel and the IRQ removal.

> +	rc = chip->batt_info->voltage_max_design_uv;
> +	rc = regmap_update_bits(chip->regmap,
> +				chip->base + FLOAT_VOLTAGE_CFG_REG,
> +				FLOAT_VOLTAGE_SETTING_MASK, rc / 1000);
> +	if (rc < 0)
> +		dev_err(chip->dev, "Couldn't set vbat max rc = %d\n", rc);
> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	/* trigger the IRQ to set up initial state */
> +	smb2_handle_usb_plugin(irq, chip);
> +
> +	return 0;
> +}
> +
> +static int smb2_remove(struct platform_device *pdev)
> +{
> +	struct smb2_chip *chip = platform_get_drvdata(pdev);
> +
> +	power_supply_put_battery_info(chip->chg_psy, chip->batt_info);
> +	return 0;
> +}

This can be removed, since power_supply_get_battery_info()
now is using device managed memory allocation functions
internally.

> +
> +static const struct of_device_id fg_match_id_table[] = {
> +	{ .compatible = "qcom,pmi8998-smb2" },
> +	{ /* sentinal */ }
> +};
> +MODULE_DEVICE_TABLE(of, fg_match_id_table);
> +
> +static struct platform_driver qcom_spmi_smb2 = {
> +	.probe = smb2_probe,
> +	.remove = smb2_remove,
> +	.driver = {
> +		.name = "qcom-spmi-smb2",
> +		.of_match_table = fg_match_id_table,
> +	},
> +};
> +
> +module_platform_driver(qcom_spmi_smb2);
> +
> +MODULE_AUTHOR("Caleb Connolly <caleb.connolly@linaro.org>");
> +MODULE_DESCRIPTION("Qualcomm SMB2 Charger Driver");
> +MODULE_LICENSE("GPL");

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/6] power: supply: add Qualcomm SMB2 driver

[Caleb Connolly]

On 01/04/2022 23:53, Sebastian Reichel wrote:
> Hi Caleb,
Hi Sebastian,
> 
> Thanks for the driver. It looks mostly good. Please find my feedback
> inline:
Thanks a lot for the thorough review, I'll address your feedback and submit a v2.
> 
> On Fri, Apr 01, 2022 at 09:26:38PM +0100, Caleb Connolly wrote:
>> Add a driver for the SMB2 charger block found in the Qualcomm PMI8998
>> and PM660.
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>>   drivers/power/supply/Kconfig     |   9 +
>>   drivers/power/supply/Makefile    |   1 +
>>   drivers/power/supply/qcom_smb2.c | 930 +++++++++++++++++++++++++++++++
>>   3 files changed, 940 insertions(+)
>>   create mode 100644 drivers/power/supply/qcom_smb2.c
>>
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index 1aa8323ad9f6..c72da3d75258 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -897,4 +897,13 @@ config BATTERY_UG3105
>>   	  device is off or suspended, the functionality of this driver is
>>   	  limited to reporting capacity only.
>>   
>> +config CHARGER_QCOM_SMB2
>> +	tristate "Qualcomm PMIC charger driver"
>> +	depends on MFD_SPMI_PMIC
>> +	depends on IIO
>> +	help
>> +	  Say Y here to enable the Qualcomm PMIC Charger driver. This
>> +	  adds support for the SMB2 switch mode battery charger found
>> +	  in PMI8998 and related PMICs.
>> +
>>   endif # POWER_SUPPLY
>> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
>> index 7f02f36aea55..e5db9eda6a9b 100644
>> --- a/drivers/power/supply/Makefile
>> +++ b/drivers/power/supply/Makefile
>> @@ -108,3 +108,4 @@ obj-$(CONFIG_BATTERY_ACER_A500)	+= acer_a500_battery.o
>>   obj-$(CONFIG_BATTERY_SURFACE)	+= surface_battery.o
>>   obj-$(CONFIG_CHARGER_SURFACE)	+= surface_charger.o
>>   obj-$(CONFIG_BATTERY_UG3105)	+= ug3105_battery.o
>> +obj-$(CONFIG_CHARGER_QCOM_SMB2)	+= qcom_smb2.o
>> diff --git a/drivers/power/supply/qcom_smb2.c b/drivers/power/supply/qcom_smb2.c
>> new file mode 100644
>> index 000000000000..63100cfc3300
>> --- /dev/null
>> +++ b/drivers/power/supply/qcom_smb2.c
>> @@ -0,0 +1,930 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2016-2019 The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2021, Linaro Ltd.
>> + *  Author: Caleb Connolly <caleb.connolly@linaro.org>
>> + *
>> + * This driver is for the switch-mode battery charger and boost
>> + * hardware found in pmi8998 and related PMICs.
>> + */
>> +
>> +#include <linux/bits.h>
>> +#include <linux/kernel.h>
>> +#include <linux/minmax.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of_address.h>
> 
> not needed?
> 
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/driver.h>
> 
> not needed (at this point, since there is not OTG handling)
> 
>> +#include <linux/device.h>
> 
> not needed?
> 
>> +#include <linux/iio/consumer.h>
>> +#include <linux/mutex.h>
>> +#include <linux/types.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/module.h>
>> +#include <linux/math64.h>
> 
> not needed?
> 
>> +#include <linux/of_irq.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/workqueue.h>
>> +
>> +#define BATTERY_CHARGER_STATUS_2_REG 0x07
>> +#define INPUT_CURRENT_LIMITED_BIT BIT(7)
>> +#define CHARGER_ERROR_STATUS_SFT_EXPIRE_BIT BIT(6)
>> +#define CHARGER_ERROR_STATUS_BAT_OV_BIT BIT(5)
>> +#define CHARGER_ERROR_STATUS_BAT_TERM_MISSING_BIT BIT(4)
>> +#define BAT_TEMP_STATUS_MASK GENMASK(3, 0)
>> +#define BAT_TEMP_STATUS_SOFT_LIMIT_MASK GENMASK(3, 2)
>> +#define BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT BIT(3)
>> +#define BAT_TEMP_STATUS_COLD_SOFT_LIMIT_BIT BIT(2)
>> +#define BAT_TEMP_STATUS_TOO_HOT_BIT BIT(1)
>> +#define BAT_TEMP_STATUS_TOO_COLD_BIT BIT(0)
>> +
>> +#define CHARGING_ENABLE_CMD_REG 0x42
>> +#define CHARGING_ENABLE_CMD_BIT BIT(0)
>> +
>> +#define CHGR_CFG2_REG 0x51
>> +#define CHG_EN_SRC_BIT BIT(7)
>> +#define CHG_EN_POLARITY_BIT BIT(6)
>> +#define PRETOFAST_TRANSITION_CFG_BIT BIT(5)
>> +#define BAT_OV_ECC_BIT BIT(4)
>> +#define I_TERM_BIT BIT(3)
>> +#define AUTO_RECHG_BIT BIT(2)
>> +#define EN_ANALOG_DROP_IN_VBATT_BIT BIT(1)
>> +#define CHARGER_INHIBIT_BIT BIT(0)
>> +
>> +#define FAST_CHARGE_CURRENT_CFG_REG 0x61
>> +#define FAST_CHARGE_CURRENT_SETTING_MASK GENMASK(7, 0)
>> +
>> +#define FLOAT_VOLTAGE_CFG_REG 0x70
>> +#define FLOAT_VOLTAGE_SETTING_MASK GENMASK(7, 0)
>> +
>> +#define FG_UPDATE_CFG_2_SEL_REG 0x7D
>> +#define SOC_LT_OTG_THRESH_SEL_BIT BIT(3)
>> +#define SOC_LT_CHG_RECHARGE_THRESH_SEL_BIT BIT(2)
>> +#define VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT BIT(1)
>> +#define IBT_LT_CHG_TERM_THRESH_SEL_BIT BIT(0)
>> +
>> +#define JEITA_EN_CFG_REG 0x90
>> +#define JEITA_EN_HARDLIMIT_BIT BIT(4)
>> +#define JEITA_EN_HOT_SL_FCV_BIT BIT(3)
>> +#define JEITA_EN_COLD_SL_FCV_BIT BIT(2)
>> +#define JEITA_EN_HOT_SL_CCC_BIT BIT(1)
>> +#define JEITA_EN_COLD_SL_CCC_BIT BIT(0)
>> +
>> +#define INT_RT_STS 0x310
>> +#define TYPE_C_CHANGE_RT_STS_BIT BIT(7)
>> +#define USBIN_ICL_CHANGE_RT_STS_BIT BIT(6)
>> +#define USBIN_SOURCE_CHANGE_RT_STS_BIT BIT(5)
>> +#define USBIN_PLUGIN_RT_STS_BIT BIT(4)
>> +#define USBIN_OV_RT_STS_BIT BIT(3)
>> +#define USBIN_UV_RT_STS_BIT BIT(2)
>> +#define USBIN_LT_3P6V_RT_STS_BIT BIT(1)
>> +#define USBIN_COLLAPSE_RT_STS_BIT BIT(0)
>> +
>> +#define BATTERY_CHARGER_STATUS_1_REG 0x06
>> +#define BVR_INITIAL_RAMP_BIT BIT(7)
>> +#define CC_SOFT_TERMINATE_BIT BIT(6)
>> +#define STEP_CHARGING_STATUS_SHIFT 3
>> +#define STEP_CHARGING_STATUS_MASK GENMASK(5, 3)
>> +#define BATTERY_CHARGER_STATUS_MASK GENMASK(2, 0)
>> +
>> +#define BATTERY_HEALTH_STATUS_REG 0x07
>> +
>> +#define OTG_CFG_REG 0x153
>> +#define OTG_RESERVED_MASK GENMASK(7, 6)
>> +#define DIS_OTG_ON_TLIM_BIT BIT(5)
>> +#define QUICKSTART_OTG_FASTROLESWAP_BIT BIT(4)
>> +#define INCREASE_DFP_TIME_BIT BIT(3)
>> +#define ENABLE_OTG_IN_DEBUG_MODE_BIT BIT(2)
>> +#define OTG_EN_SRC_CFG_BIT BIT(1)
>> +#define CONCURRENT_MODE_CFG_BIT BIT(0)
>> +
>> +#define OTG_ENG_OTG_CFG_REG 0x1C0
>> +#define ENG_BUCKBOOST_HALT1_8_MODE_BIT BIT(0)
>> +
>> +#define APSD_STATUS_REG 0x307
>> +#define APSD_STATUS_7_BIT BIT(7)
>> +#define HVDCP_CHECK_TIMEOUT_BIT BIT(6)
>> +#define SLOW_PLUGIN_TIMEOUT_BIT BIT(5)
>> +#define ENUMERATION_DONE_BIT BIT(4)
>> +#define VADP_CHANGE_DONE_AFTER_AUTH_BIT BIT(3)
>> +#define QC_AUTH_DONE_STATUS_BIT BIT(2)
>> +#define QC_CHARGER_BIT BIT(1)
>> +#define APSD_DTC_STATUS_DONE_BIT BIT(0)
>> +
>> +#define APSD_RESULT_STATUS_REG 0x308
>> +#define ICL_OVERRIDE_LATCH_BIT BIT(7)
>> +#define APSD_RESULT_STATUS_MASK GENMASK(6, 0)
>> +#define QC_3P0_BIT BIT(6)
>> +#define QC_2P0_BIT BIT(5)
>> +#define FLOAT_CHARGER_BIT BIT(4)
>> +#define DCP_CHARGER_BIT BIT(3)
>> +#define CDP_CHARGER_BIT BIT(2)
>> +#define OCP_CHARGER_BIT BIT(1)
>> +#define SDP_CHARGER_BIT BIT(0)
>> +
>> +#define TYPE_C_STATUS_1_REG 0x30B
>> +#define UFP_TYPEC_MASK GENMASK(7, 5)
>> +#define UFP_TYPEC_RDSTD_BIT BIT(7)
>> +#define UFP_TYPEC_RD1P5_BIT BIT(6)
>> +#define UFP_TYPEC_RD3P0_BIT BIT(5)
>> +#define UFP_TYPEC_FMB_255K_BIT BIT(4)
>> +#define UFP_TYPEC_FMB_301K_BIT BIT(3)
>> +#define UFP_TYPEC_FMB_523K_BIT BIT(2)
>> +#define UFP_TYPEC_FMB_619K_BIT BIT(1)
>> +#define UFP_TYPEC_OPEN_OPEN_BIT BIT(0)
>> +
>> +#define TYPE_C_STATUS_2_REG 0x30C
>> +#define DFP_RA_OPEN_BIT BIT(7)
>> +#define TIMER_STAGE_BIT BIT(6)
>> +#define EXIT_UFP_MODE_BIT BIT(5)
>> +#define EXIT_DFP_MODE_BIT BIT(4)
>> +#define DFP_TYPEC_MASK GENMASK(3, 0)
>> +#define DFP_RD_OPEN_BIT BIT(3)
>> +#define DFP_RD_RA_VCONN_BIT BIT(2)
>> +#define DFP_RD_RD_BIT BIT(1)
>> +#define DFP_RA_RA_BIT BIT(0)
>> +
>> +#define TYPE_C_STATUS_3_REG 0x30D
>> +#define ENABLE_BANDGAP_BIT BIT(7)
>> +#define U_USB_GND_NOVBUS_BIT BIT(6)
>> +#define U_USB_FLOAT_NOVBUS_BIT BIT(5)
>> +#define U_USB_GND_BIT BIT(4)
>> +#define U_USB_FMB1_BIT BIT(3)
>> +#define U_USB_FLOAT1_BIT BIT(2)
>> +#define U_USB_FMB2_BIT BIT(1)
>> +#define U_USB_FLOAT2_BIT BIT(0)
>> +
>> +#define TYPE_C_STATUS_4_REG 0x30E
>> +#define UFP_DFP_MODE_STATUS_BIT BIT(7)
>> +#define TYPEC_VBUS_STATUS_BIT BIT(6)
>> +#define TYPEC_VBUS_ERROR_STATUS_BIT BIT(5)
>> +#define TYPEC_DEBOUNCE_DONE_STATUS_BIT BIT(4)
>> +#define TYPEC_UFP_AUDIO_ADAPT_STATUS_BIT BIT(3)
>> +#define TYPEC_VCONN_OVERCURR_STATUS_BIT BIT(2)
>> +#define CC_ORIENTATION_BIT BIT(1)
>> +#define CC_ATTACHED_BIT BIT(0)
>> +
>> +#define TYPE_C_STATUS_5_REG 0x30F
>> +#define TRY_SOURCE_FAILED_BIT BIT(6)
>> +#define TRY_SINK_FAILED_BIT BIT(5)
>> +#define TIMER_STAGE_2_BIT BIT(4)
>> +#define TYPEC_LEGACY_CABLE_STATUS_BIT BIT(3)
>> +#define TYPEC_NONCOMP_LEGACY_CABLE_STATUS_BIT BIT(2)
>> +#define TYPEC_TRYSOURCE_DETECT_STATUS_BIT BIT(1)
>> +#define TYPEC_TRYSINK_DETECT_STATUS_BIT BIT(0)
>> +
>> +#define CMD_APSD_REG 0x341
>> +#define ICL_OVERRIDE_BIT BIT(1)
>> +#define APSD_RERUN_BIT BIT(0)
>> +
>> +#define TYPE_C_CFG_REG 0x358
>> +#define APSD_START_ON_CC_BIT BIT(7)
>> +#define WAIT_FOR_APSD_BIT BIT(6)
>> +#define FACTORY_MODE_DETECTION_EN_BIT BIT(5)
>> +#define FACTORY_MODE_ICL_3A_4A_BIT BIT(4)
>> +#define FACTORY_MODE_DIS_CHGING_CFG_BIT BIT(3)
>> +#define SUSPEND_NON_COMPLIANT_CFG_BIT BIT(2)
>> +#define VCONN_OC_CFG_BIT BIT(1)
>> +#define TYPE_C_OR_U_USB_BIT BIT(0)
>> +
>> +#define TYPE_C_CFG_2_REG 0x359
>> +#define TYPE_C_DFP_CURRSRC_MODE_BIT BIT(7)
>> +#define DFP_CC_1P4V_OR_1P6V_BIT BIT(6)
>> +#define VCONN_SOFTSTART_CFG_MASK GENMASK(5, 4)
>> +#define EN_TRY_SOURCE_MODE_BIT BIT(3)
>> +#define USB_FACTORY_MODE_ENABLE_BIT BIT(2)
>> +#define TYPE_C_UFP_MODE_BIT BIT(1)
>> +#define EN_80UA_180UA_CUR_SOURCE_BIT BIT(0)
>> +
>> +#define TYPE_C_CFG_3_REG 0x35A
>> +#define TVBUS_DEBOUNCE_BIT BIT(7)
>> +#define TYPEC_LEGACY_CABLE_INT_EN_BIT BIT(6)
>> +#define TYPEC_NONCOMPLIANT_LEGACY_CABLE_INT_EN_BIT BIT(5)
>> +#define TYPEC_TRYSOURCE_DETECT_INT_EN_BIT BIT(4)
>> +#define TYPEC_TRYSINK_DETECT_INT_EN_BIT BIT(3)
>> +#define EN_TRYSINK_MODE_BIT BIT(2)
>> +#define EN_LEGACY_CABLE_DETECTION_BIT BIT(1)
>> +#define ALLOW_PD_DRING_UFP_TCCDB_BIT BIT(0)
>> +
>> +#define USBIN_OPTIONS_1_CFG_REG 0x362
>> +#define CABLE_R_SEL_BIT BIT(7)
>> +#define HVDCP_AUTH_ALG_EN_CFG_BIT BIT(6)
>> +#define HVDCP_AUTONOMOUS_MODE_EN_CFG_BIT BIT(5)
>> +#define INPUT_PRIORITY_BIT BIT(4)
>> +#define AUTO_SRC_DETECT_BIT BIT(3)
>> +#define HVDCP_EN_BIT BIT(2)
>> +#define VADP_INCREMENT_VOLTAGE_LIMIT_BIT BIT(1)
>> +#define VADP_TAPER_TIMER_EN_BIT BIT(0)
>> +
>> +#define USBIN_OPTIONS_2_CFG_REG 0x363
>> +#define WIPWR_RST_EUD_CFG_BIT BIT(7)
>> +#define SWITCHER_START_CFG_BIT BIT(6)
>> +#define DCD_TIMEOUT_SEL_BIT BIT(5)
>> +#define OCD_CURRENT_SEL_BIT BIT(4)
>> +#define SLOW_PLUGIN_TIMER_EN_CFG_BIT BIT(3)
>> +#define FLOAT_OPTIONS_MASK GENMASK(2, 0)
>> +#define FLOAT_DIS_CHGING_CFG_BIT BIT(2)
>> +#define SUSPEND_FLOAT_CFG_BIT BIT(1)
>> +#define FORCE_FLOAT_SDP_CFG_BIT BIT(0)
>> +
>> +#define TAPER_TIMER_SEL_CFG_REG 0x364
>> +#define TYPEC_SPARE_CFG_BIT BIT(7)
>> +#define TYPEC_DRP_DFP_TIME_CFG_BIT BIT(5)
>> +#define TAPER_TIMER_SEL_MASK GENMASK(1, 0)
>> +
>> +#define USBIN_LOAD_CFG_REG 0x365
>> +#define USBIN_OV_CH_LOAD_OPTION_BIT BIT(7)
>> +#define ICL_OVERRIDE_AFTER_APSD_BIT BIT(4)
>> +
>> +#define USBIN_ICL_OPTIONS_REG 0x366
>> +#define CFG_USB3P0_SEL_BIT BIT(2)
>> +#define USB51_MODE_BIT BIT(1)
>> +#define USBIN_MODE_CHG_BIT BIT(0)
>> +
>> +#define TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG 0x368
>> +#define EXIT_SNK_BASED_ON_CC_BIT BIT(7)
>> +#define VCONN_EN_ORIENTATION_BIT BIT(6)
>> +#define TYPEC_VCONN_OVERCURR_INT_EN_BIT BIT(5)
>> +#define VCONN_EN_SRC_BIT BIT(4)
>> +#define VCONN_EN_VALUE_BIT BIT(3)
>> +#define TYPEC_POWER_ROLE_CMD_MASK GENMASK(2, 0)
>> +#define UFP_EN_CMD_BIT BIT(2)
>> +#define DFP_EN_CMD_BIT BIT(1)
>> +#define TYPEC_DISABLE_CMD_BIT BIT(0)
>> +
>> +#define USBIN_CURRENT_LIMIT_CFG_REG 0x370
>> +#define USBIN_CURRENT_LIMIT_MASK GENMASK(7, 0)
>> +
>> +#define USBIN_AICL_OPTIONS_CFG_REG 0x380
>> +#define SUSPEND_ON_COLLAPSE_USBIN_BIT BIT(7)
>> +#define USBIN_AICL_HDC_EN_BIT BIT(6)
>> +#define USBIN_AICL_START_AT_MAX_BIT BIT(5)
>> +#define USBIN_AICL_RERUN_EN_BIT BIT(4)
>> +#define USBIN_AICL_ADC_EN_BIT BIT(3)
>> +#define USBIN_AICL_EN_BIT BIT(2)
>> +#define USBIN_HV_COLLAPSE_RESPONSE_BIT BIT(1)
>> +#define USBIN_LV_COLLAPSE_RESPONSE_BIT BIT(0)
>> +
>> +#define DC_ENG_SSUPPLY_CFG2_REG 0x4C1
>> +#define ENG_SSUPPLY_IVREF_OTG_SS_MASK GENMASK(2, 0)
>> +#define OTG_SS_SLOW 0x3
>> +
>> +#define DCIN_AICL_REF_SEL_CFG_REG 0x481
>> +#define DCIN_CONT_AICL_THRESHOLD_CFG_MASK GENMASK(5, 0)
>> +
>> +#define WI_PWR_OPTIONS_REG 0x495
>> +#define CHG_OK_BIT BIT(7)
>> +#define WIPWR_UVLO_IRQ_OPT_BIT BIT(6)
>> +#define BUCK_HOLDOFF_ENABLE_BIT BIT(5)
>> +#define CHG_OK_HW_SW_SELECT_BIT BIT(4)
>> +#define WIPWR_RST_ENABLE_BIT BIT(3)
>> +#define DCIN_WIPWR_IRQ_SELECT_BIT BIT(2)
>> +#define AICL_SWITCH_ENABLE_BIT BIT(1)
>> +#define ZIN_ICL_ENABLE_BIT BIT(0)
>> +
>> +#define ICL_STATUS_REG 0x607
>> +#define INPUT_CURRENT_LIMIT_MASK GENMASK(7, 0)
>> +
>> +#define POWER_PATH_STATUS_REG 0x60B
>> +#define P_PATH_INPUT_SS_DONE_BIT BIT(7)
>> +#define P_PATH_USBIN_SUSPEND_STS_BIT BIT(6)
>> +#define P_PATH_DCIN_SUSPEND_STS_BIT BIT(5)
>> +#define P_PATH_USE_USBIN_BIT BIT(4)
>> +#define P_PATH_USE_DCIN_BIT BIT(3)
>> +#define P_PATH_POWER_PATH_MASK GENMASK(2, 1)
>> +#define P_PATH_VALID_INPUT_POWER_SOURCE_STS_BIT BIT(0)
>> +
>> +#define WD_CFG_REG 0x651
>> +#define WATCHDOG_TRIGGER_AFP_EN_BIT BIT(7)
>> +#define BARK_WDOG_INT_EN_BIT BIT(6)
>> +#define BITE_WDOG_INT_EN_BIT BIT(5)
>> +#define SFT_AFTER_WDOG_IRQ_MASK GENMASK(4, 3)
>> +#define WDOG_IRQ_SFT_BIT BIT(2)
>> +#define WDOG_TIMER_EN_ON_PLUGIN_BIT BIT(1)
>> +#define WDOG_TIMER_EN_BIT BIT(0)
>> +
>> +#define AICL_RERUN_TIME_CFG_REG 0x661
>> +#define AICL_RERUN_TIME_MASK GENMASK(1, 0)
>> +
>> +#define SDP_CURRENT_UA 500000
>> +#define CDP_CURRENT_UA 1500000
>> +#define DCP_CURRENT_UA 1500000
>> +
>> +enum charger_status {
>> +	TRICKLE_CHARGE = 0,
>> +	PRE_CHARGE,
>> +	FAST_CHARGE,
>> +	FULLON_CHARGE,
>> +	TAPER_CHARGE,
>> +	TERMINATE_CHARGE,
>> +	INHIBIT_CHARGE,
>> +	DISABLE_CHARGE,
>> +};
>> +
>> +struct smb2_register {
>> +	u16 addr;
>> +	u8 mask;
>> +	u8 val;
>> +};
>> +
>> +/**
>> + * struct smb2_chip - smb2 chip structure
>> + * @dev:		Device reference for power_supply
> 
> name is missing
> 
>> + * @base:		Base address for smb2 registers
>> + * @regmap:		Register map
>> + * @reg_lock:		Mutex for register access
>> + * @batt_info:		Battery data from DT
> 
> status_change_work is missing
> 
>> + * @usbin_i_chan:	USBIN current measurement channel
>> + * @usbin_v_chan:	USBIN voltage measurement channel
>> + * @chg_psy:		Charger power supply instance
>> + * @usb_present:	True if a charger is attached
>> + */
>> +struct smb2_chip {
>> +	struct device *dev;
>> +	const char *name;
>> +	unsigned int base;
>> +	struct regmap *regmap;
>> +	struct mutex reg_lock;
>> +	struct power_supply_battery_info *batt_info;
>> +
>> +	struct delayed_work status_change_work;
>> +
>> +	struct iio_channel *usbin_i_chan;
>> +	struct iio_channel *usbin_v_chan;
>> +
>> +	struct power_supply *chg_psy;
>> +
>> +	bool usb_present;
>> +};
>> +
>> +static enum power_supply_property smb2_properties[] = {
>> +	POWER_SUPPLY_PROP_MANUFACTURER, POWER_SUPPLY_PROP_MODEL_NAME,
>> +	POWER_SUPPLY_PROP_CURRENT_MAX,	POWER_SUPPLY_PROP_CURRENT_NOW,
>> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,	POWER_SUPPLY_PROP_STATUS,
>> +	POWER_SUPPLY_PROP_HEALTH,	POWER_SUPPLY_PROP_ONLINE,
>> +	POWER_SUPPLY_PROP_USB_TYPE,
>> +};
> 
> Make this one item per line. This results in better line diffs when
> something is changed later.
> 
>> +
>> +static enum power_supply_usb_type smb2_usb_types[] = {
>> +	POWER_SUPPLY_USB_TYPE_SDP, POWER_SUPPLY_USB_TYPE_DCP,
>> +	POWER_SUPPLY_USB_TYPE_CDP, POWER_SUPPLY_USB_TYPE_C,
>> +	POWER_SUPPLY_USB_TYPE_PD_DRP
>> +};
> 
> Same as before.
> 
>> +/*
>> + * Qualcomm "automatic power source detection" aka APSD
>> + * tells us what type of charger we're connected to.
>> + */
>> +static int smb2_apsd_get_charger_type(struct smb2_chip *chip, int *val)
>> +{
>> +	int rc;
>> +	unsigned int apsd_stat, stat;
>> +
>> +	mutex_lock(&chip->reg_lock);
>> +
>> +	rc = regmap_read(chip->regmap, chip->base + APSD_STATUS_REG,
>> +			 &apsd_stat);
>> +	if (rc < 0) {
>> +		dev_err(chip->dev, "Failed to read apsd status, rc = %d", rc);
>> +		return rc;
> 
> this returns without unlocking mutex...
> 
>> +	}
>> +	if (!(apsd_stat & APSD_DTC_STATUS_DONE_BIT)) {
>> +		dev_err(chip->dev, "Apsd not ready");
>> +		return -EAGAIN;
> 
> another return with locked mutex
> 
>> +	}
>> +
>> +	rc = regmap_read(chip->regmap, chip->base + APSD_RESULT_STATUS_REG,
>> +			 &stat);
>> +	if (rc < 0) {
>> +		dev_err(chip->dev, "Failed to read apsd result, rc = %d", rc);
>> +		return rc;
> 
> another return with locked mutex
> 
>> +	}
>> +
>> +	mutex_unlock(&chip->reg_lock);
>> +
>> +	stat &= APSD_RESULT_STATUS_MASK;
>> +
>> +	if (stat & CDP_CHARGER_BIT) {
>> +		*val = POWER_SUPPLY_USB_TYPE_CDP;
>> +	} else if (stat &
>> +		   (DCP_CHARGER_BIT | OCP_CHARGER_BIT | FLOAT_CHARGER_BIT)) {
>> +		*val = POWER_SUPPLY_USB_TYPE_DCP;
>> +	} else { /* SDP_CHARGER_BIT (or others) */
>> +		*val = POWER_SUPPLY_USB_TYPE_SDP;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int smb2_get_prop_usb_online(struct smb2_chip *chip, int *val)
>> +{
>> +	unsigned int stat;
>> +	int rc;
>> +
>> +	rc = regmap_read(chip->regmap, chip->base + POWER_PATH_STATUS_REG,
>> +			 &stat);
>> +	if (rc < 0) {
>> +		dev_err(chip->dev, "Couldn't read POWER_PATH_STATUS! ret=%d\n",
>> +			rc);
>> +		return rc;
>> +	}
>> +
>> +	*val = (stat & P_PATH_USE_USBIN_BIT) &&
>> +	       (stat & P_PATH_VALID_INPUT_POWER_SOURCE_STS_BIT);
>> +	return rc;
>> +}
>> +
>> +int smb2_get_prop_status(struct smb2_chip *chip, int *val)
>> +{
>> +	int usb_online_val;
>> +	unsigned int stat;
>> +	int rc;
>> +	bool usb_online;
>> +
>> +	mutex_lock(&chip->reg_lock);
>> +
>> +	rc = smb2_get_prop_usb_online(chip, &usb_online_val);
>> +	if (rc < 0) {
>> +		dev_err(chip->dev, "Couldn't get usb online property rc = %d\n",
>> +			rc);
>> +		return rc;
> 
> another return with locked mutex
> 
>> +	}
>> +	usb_online = (bool)usb_online_val;
>> +
>> +	if (!usb_online) {
> 
> Just use 'if (!usb_online_val) {' and drop the explicit cast.
> 
>> +		*val = POWER_SUPPLY_STATUS_DISCHARGING;
>> +		return rc;
>> +	}
>> +
>> +	rc = regmap_read(chip->regmap,
>> +			 chip->base + BATTERY_CHARGER_STATUS_1_REG, &stat);
>> +	if (rc < 0) {
>> +		dev_err(chip->dev,
>> +			"Failed to read charging status ret=%d\n", rc);
>> +		return rc;
> 
> another return with locked mutex
> 
>> +	}
>> +
>> +	mutex_unlock(&chip->reg_lock);
>> +
>> +	stat = stat & BATTERY_CHARGER_STATUS_MASK;
>> +
>> +	switch (stat) {
>> +	case TRICKLE_CHARGE:
>> +	case PRE_CHARGE:
>> +	case FAST_CHARGE:
>> +	case FULLON_CHARGE:
>> +	case TAPER_CHARGE:
>> +		*val = POWER_SUPPLY_STATUS_CHARGING;
>> +		return rc;
>> +	case DISABLE_CHARGE:
>> +		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
>> +		return rc;
>> +	case TERMINATE_CHARGE:
>> +		*val = POWER_SUPPLY_STATUS_FULL;
>> +		return rc;
>> +	case INHIBIT_CHARGE:
>> +	default:
>> +		*val = POWER_SUPPLY_STATUS_UNKNOWN;
>> +		return rc;
>> +	}
>> +}
>> +
>> +static inline int smb2_get_current_limit(struct smb2_chip *chip,
>> +					 unsigned int *val)
>> +{
>> +	int rc = regmap_read(chip->regmap, chip->base + ICL_STATUS_REG, val);
>> +
>> +	if (rc >= 0)
>> +		*val *= 25000;
>> +	return rc;
>> +}
>> +
>> +static inline int smb2_set_current_limit(struct smb2_chip *chip,
>> +					 unsigned int val)
>> +{
>> +	unsigned char val_raw;
>> +
>> +	if (val > 4800000) {
>> +		dev_err(chip->dev,
>> +			"Can't set current limit higher than 4800000uA");
>> +		return -EINVAL;
>> +	}
>> +	val_raw = val / 25000;
>> +	return regmap_write(chip->regmap,
>> +			    chip->base + USBIN_CURRENT_LIMIT_CFG_REG, val_raw);
>> +}
>> +
>> +void smb2_status_change_work(struct work_struct *work)
>> +{
>> +	struct smb2_chip *chip =
>> +		container_of(work, struct smb2_chip, status_change_work.work);
>> +	unsigned int charger_type, current_ua;
>> +	int usb_online, count, rc;
>> +
>> +	mutex_lock(&chip->reg_lock);
>> +
>> +	smb2_get_prop_usb_online(chip, &usb_online);
>> +	if (usb_online == 0)
>> +		return;
> 
> another return with locked mutex
> 
>> +
>> +	for (count = 0; count < 3; count++) {
>> +		dev_info(chip->dev, "get charger type retry %d\n", count);
> 
> dev_dbg()?
> 
>> +		rc = smb2_apsd_get_charger_type(chip, &charger_type);
>> +		if (rc == 0)
>> +			break;
>> +		msleep(100);
>> +	}
>> +
>> +	if (rc < 0) {
>> +		rc = regmap_update_bits(chip->regmap, chip->base + CMD_APSD_REG,
>> +					APSD_RERUN_BIT, APSD_RERUN_BIT);
>> +		schedule_delayed_work(&chip->status_change_work,
>> +				      msecs_to_jiffies(1500));
>> +		return;
> 
> another return with locked mutex
> 
>> +	}
>> +
>> +	switch (charger_type) {
>> +	case POWER_SUPPLY_USB_TYPE_CDP:
>> +		current_ua = CDP_CURRENT_UA;
>> +		break;
>> +	case POWER_SUPPLY_USB_TYPE_DCP:
>> +		current_ua = DCP_CURRENT_UA;
>> +		break;
>> +	case POWER_SUPPLY_USB_TYPE_SDP:
>> +	default:
>> +		current_ua = SDP_CURRENT_UA;
>> +		break;
>> +	}
>> +
>> +	smb2_set_current_limit(chip, current_ua);
>> +	mutex_unlock(&chip->reg_lock);
>> +	power_supply_changed(chip->chg_psy);
>> +}
>> +
>> +int smb2_get_iio_chan(struct smb2_chip *chip, struct iio_channel *chan,
>> +		      int *val)
>> +{
>> +	int rc = 0;
>> +	union power_supply_propval status;
>> +
>> +	rc = power_supply_get_property(chip->chg_psy, POWER_SUPPLY_PROP_STATUS,
>> +				       &status);
>> +	if (rc < 0 || status.intval != POWER_SUPPLY_STATUS_CHARGING) {
>> +		*val = 0;
>> +		return 0;
>> +	}
>> +
>> +	if (IS_ERR(chan)) {
>> +		dev_err(chip->dev, "Failed to chan, err = %li", PTR_ERR(chan));
>> +		return PTR_ERR(chan);
>> +	}
>> +
>> +	rc = iio_read_channel_processed(chan, val);
>> +	return rc;
>> +}
>> +
>> +int smb2_get_prop_health(struct smb2_chip *chip, int *val)
>> +{
>> +	int rc;
>> +	unsigned int stat;
>> +
>> +	*val = POWER_SUPPLY_HEALTH_UNKNOWN;
>> +
>> +	rc = regmap_read(chip->regmap,
>> +			 chip->base + BATTERY_CHARGER_STATUS_2_REG, &stat);
>> +	if (rc < 0) {
>> +		dev_err(chip->dev, "Couldn't read charger status rc=%d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	switch (stat & BATTERY_CHARGER_STATUS_MASK) {
>> +	case CHARGER_ERROR_STATUS_BAT_OV_BIT:
>> +		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>> +		return 0;
>> +	case BAT_TEMP_STATUS_TOO_COLD_BIT:
>> +		*val = POWER_SUPPLY_HEALTH_COLD;
>> +		return 0;
>> +	case BAT_TEMP_STATUS_TOO_HOT_BIT:
>> +		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
>> +		return 0;
>> +	case BAT_TEMP_STATUS_COLD_SOFT_LIMIT_BIT:
>> +		*val = POWER_SUPPLY_HEALTH_COOL;
>> +		return 0;
>> +	case BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT:
>> +		*val = POWER_SUPPLY_HEALTH_WARM;
>> +		return 0;
>> +	default:
>> +		*val = POWER_SUPPLY_HEALTH_GOOD;
>> +		return 0;
>> +	}
>> +}
>> +
>> +static int smb2_get_property(struct power_supply *psy,
>> +			     enum power_supply_property psp,
>> +			     union power_supply_propval *val)
>> +{
>> +	struct smb2_chip *chip = power_supply_get_drvdata(psy);
>> +	int error = 0;
>> +
>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_MANUFACTURER:
>> +		val->strval = "Qualcomm";
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_MODEL_NAME:
>> +		val->strval = chip->name;
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
>> +		error = smb2_get_current_limit(chip, &val->intval);
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
>> +		error = smb2_get_iio_chan(chip, chip->usbin_i_chan,
>> +					  &val->intval);
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +		error = smb2_get_iio_chan(chip, chip->usbin_v_chan,
>> +					  &val->intval);
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_ONLINE:
>> +		error = smb2_get_prop_usb_online(chip, &val->intval);
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_STATUS:
>> +		error = smb2_get_prop_status(chip, &val->intval);
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_HEALTH:
>> +		error = smb2_get_prop_health(chip, &val->intval);
>> +		return 0;
>> +	case POWER_SUPPLY_PROP_USB_TYPE:
>> +		error = smb2_apsd_get_charger_type(chip, &val->intval);
>> +		return 0;
>> +	default:
>> +		dev_err(chip->dev, "invalid property: %d\n", psp);
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int smb2_set_property(struct power_supply *psy,
>> +			     enum power_supply_property psp,
>> +			     const union power_supply_propval *val)
>> +{
>> +	struct smb2_chip *chip = power_supply_get_drvdata(psy);
>> +	int error = 0;
>> +
>> +	dev_info(chip->dev, "Setting property: %d", psp);
> 
> drop
> 
>> +
>> +	mutex_lock(&chip->reg_lock);
>> +
>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
>> +		error = smb2_set_current_limit(chip, val->intval);
>> +		break;
>> +	default:
>> +		dev_err(chip->dev, "No setter for property: %d\n", psp);
>> +		error = -EINVAL;
>> +	}
>> +
>> +	mutex_unlock(&chip->reg_lock);
>> +
>> +	return error;
>> +}
>> +
>> +static int smb2_property_is_writable(struct power_supply *psy,
>> +				     enum power_supply_property psp)
>> +{
>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
>> +		return 1;
>> +	default:
>> +		return 0;
>> +	}
>> +}
>> +
>> +irqreturn_t smb2_handle_usb_plugin(int irq, void *data)
>> +{
>> +	struct smb2_chip *chip = data;
>> +	int rc;
>> +	unsigned int intrt_stat;
>> +
>> +	rc = regmap_read(chip->regmap, chip->base + INT_RT_STS, &intrt_stat);
>> +	if (rc < 0) {
>> +		dev_err(chip->dev,
>> +			"Couldn't read USB status from reg! ret=%d\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	chip->usb_present = (bool)(intrt_stat & USBIN_PLUGIN_RT_STS_BIT);
> 
> This variable is not read anywhere. I suppose you can just drop the
> regmap read from this place?
> 
>> +	power_supply_changed(chip->chg_psy);
>> +
>> +	schedule_delayed_work(&chip->status_change_work,
>> +			      msecs_to_jiffies(1000));
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static const struct power_supply_desc smb2_psy_desc = {
>> +	.name = "usb",
>> +	.type = POWER_SUPPLY_TYPE_USB,
>> +	.usb_types = smb2_usb_types,
>> +	.num_usb_types = ARRAY_SIZE(smb2_usb_types),
>> +	.properties = smb2_properties,
>> +	.num_properties = ARRAY_SIZE(smb2_properties),
>> +	.get_property = smb2_get_property,
>> +	.set_property = smb2_set_property,
>> +	.property_is_writeable = smb2_property_is_writable,
>> +};
>> +
>> +/* Init sequence derived from downstream driver */
>> +static struct smb2_register smb2_init_seq[] = {
> 
> static const?
> 
>> +	{ .addr = AICL_RERUN_TIME_CFG_REG,
>> +	  .mask = AICL_RERUN_TIME_MASK,
>> +	  .val = 0 },
>> +	{ .addr = USBIN_AICL_OPTIONS_CFG_REG,
>> +	  .mask = USBIN_AICL_START_AT_MAX_BIT | USBIN_AICL_ADC_EN_BIT,
>> +	  .val = 0 },
>> +	/*
>> +	 * By default configure us as an upstream facing port
>> +	 * FIXME: for OTG we should set UFP_EN_CMD_BIT and DFP_EN_CMD_BIT both to 0
>> +	 */
>> +	{ .addr = TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG,
>> +	  .mask = TYPEC_POWER_ROLE_CMD_MASK | VCONN_EN_SRC_BIT |
>> +		  VCONN_EN_VALUE_BIT,
>> +	  .val = VCONN_EN_SRC_BIT | UFP_EN_CMD_BIT },
>> +	/*
>> +	 * disable Type-C factory mode and stay in Attached.SRC state when VCONN
>> +	 * over-current happens
>> +	 */
>> +	{ .addr = TYPE_C_CFG_REG,
>> +	  .mask = FACTORY_MODE_DETECTION_EN_BIT | VCONN_OC_CFG_BIT,
>> +	  .val = 0 },
>> +	/* Configure VBUS for software control */
>> +	{ .addr = OTG_CFG_REG, .mask = OTG_EN_SRC_CFG_BIT, .val = 0 },
>> +	{ .addr = FG_UPDATE_CFG_2_SEL_REG,
>> +	  .mask = SOC_LT_CHG_RECHARGE_THRESH_SEL_BIT |
>> +		  VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT,
>> +	  .val = VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT },
>> +	/* Enable automatic input current limit */
>> +	{ .addr = USBIN_AICL_OPTIONS_CFG_REG,
>> +	  .mask = USBIN_AICL_EN_BIT,
>> +	  .val = 1 },
>> +	{ .addr = USBIN_OPTIONS_1_CFG_REG, .mask = HVDCP_EN_BIT, .val = 0 },
>> +	{ .addr = CHARGING_ENABLE_CMD_REG,
>> +	  .mask = CHARGING_ENABLE_CMD_BIT,
>> +	  .val = CHARGING_ENABLE_CMD_BIT },
>> +	{ .addr = USBIN_LOAD_CFG_REG,
>> +	  .mask = ICL_OVERRIDE_AFTER_APSD_BIT,
>> +	  .val = ICL_OVERRIDE_AFTER_APSD_BIT },
>> +	{ .addr = CHGR_CFG2_REG,
>> +	  .mask = CHG_EN_POLARITY_BIT | CHG_EN_SRC_BIT,
>> +	  .val = 0 },
>> +	{ .addr = TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG,
>> +	  .mask = UFP_EN_CMD_BIT,
>> +	  .val = UFP_EN_CMD_BIT },
>> +	/* Set the default SDP charger type to a 500ma USB 2.0 port
>> +	 * and apply ICL override? (OnePlus 6 hack??)
>> +	 */
>> +	{ .addr = USBIN_ICL_OPTIONS_REG,
>> +	  .mask = USB51_MODE_BIT | USBIN_MODE_CHG_BIT,
>> +	  .val = USB51_MODE_BIT }, //| USBIN_MODE_CHG_BIT },
>> +};
>> +
>> +static int smb2_init_hw(struct smb2_chip *chip)
>> +{
>> +	int rc, i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(smb2_init_seq); i++) {
>> +		dev_info(chip->dev, "%d: Writing 0x%02x to 0x%02x\n", i,
>> +			 smb2_init_seq[i].val, smb2_init_seq[i].addr);
> 
> dev_dbg
> 
>> +		rc = regmap_update_bits(chip->regmap,
>> +					chip->base + smb2_init_seq[i].addr,
>> +					smb2_init_seq[i].mask,
>> +					smb2_init_seq[i].val);
>> +		if (rc < 0) {
>> +			dev_err(chip->dev,
>> +				"%s: Failed to write 0x%02x to 0x%02x: %d\n",
>> +				__func__, smb2_init_seq[i].val,
>> +				smb2_init_seq[i].addr, rc);
>> +			return rc;
>> +		}
>> +	}
>> +
>> +	smb2_set_current_limit(chip, 1950 * 1000);
>> +	return rc;
>> +}
>> +
>> +static int smb2_probe(struct platform_device *pdev)
>> +{
>> +	struct power_supply_config supply_config = {};
>> +	struct smb2_chip *chip;
>> +	int rc = 0, irq;
>> +
>> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>> +	if (!chip)
>> +		return -ENOMEM;
>> +
>> +	chip->dev = &pdev->dev;
>> +	chip->name = pdev->name;
>> +	mutex_init(&chip->reg_lock);
>> +
>> +	chip->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +	if (!chip->regmap) {
>> +		dev_err(chip->dev, "failed to locate the regmap\n");
>> +		return -ENODEV;
> 
> dev_err_probe()
> 
>> +	}
>> +
>> +	rc = device_property_read_u32(chip->dev, "reg", &chip->base);
>> +	if (rc < 0) {
>> +		dev_err(chip->dev, "Couldn't read base address from dt: %d\n",
>> +			rc);
>> +		return rc;
> 
> dev_err_probe()
> 
>> +	}
>> +
>> +	irq = of_irq_get_byname(pdev->dev.of_node, "usb-plugin");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev, "Couldn't get irq usb-plugin byname\n");
>> +		return irq;
> 
> dev_err_probe()
> 
>> +	}
>> +
>> +	rc = devm_request_threaded_irq(chip->dev, irq, NULL,
>> +				       smb2_handle_usb_plugin, IRQF_ONESHOT,
>> +				       "usb-plugin", chip);
>> +	if (rc < 0) {
>> +		dev_err(chip->dev, "Couldn't request irq %d\n", irq);
>> +		return rc;
> 
> dev_err_probe()
> 
>> +	}
>> +
>> +	chip->usbin_v_chan = iio_channel_get(chip->dev, "usbin_v");
>> +	chip->usbin_i_chan = iio_channel_get(chip->dev, "usbin_i");
> 
> maybe add another _ and make this usb_in_v and usb_in_i, which is
> easier to read?
> 
>> +	/* RRADC should probe before us, but just in case it doesn't */
>> +	if (PTR_ERR(chip->usbin_v_chan) == -EPROBE_DEFER ||
>> +	    PTR_ERR(chip->usbin_i_chan) == -EPROBE_DEFER) {
>> +		return -EPROBE_DEFER;
> 
> return dev_err_probe(chip->dev, -EPROBE_DEFER, "Missing IIO device");
> 
> to get a proper info string in /sys/kernel/debug/devices_deferred
> for the charger when something goes wrong and IIO never probes.
> 
>> +	}
>> +
>> +	rc = smb2_init_hw(chip);
>> +	if (rc < 0) {
>> +		dev_err(chip->dev, "Couldn't init hw %d\n", rc);
>> +		return rc;
> 
> dev_err_probe
> 
>> +	}
>> +
>> +	supply_config.drv_data = chip;
>> +	supply_config.of_node = pdev->dev.of_node;
>> +
>> +	chip->chg_psy = devm_power_supply_register(chip->dev, &smb2_psy_desc,
>> +						   &supply_config);
>> +	if (IS_ERR(chip->chg_psy)) {
>> +		dev_err(&pdev->dev, "failed to register power supply\n");
>> +		return PTR_ERR(chip->chg_psy);
> 
> dev_err_probe
> 
>> +	}
>> +
>> +	rc = power_supply_get_battery_info(chip->chg_psy, &chip->batt_info);
>> +	if (rc) {
>> +		dev_err(&pdev->dev, "Failed to get battery info: %d\n", rc);
>> +		return rc;
> 
> dev_err_probe
> 
>> +	}
>> +
>> +	INIT_DELAYED_WORK(&chip->status_change_work, smb2_status_change_work);
> 
> #include <linux/devm-helpers.h>
> 
> devm_delayed_work_autocancel()
> 
> Also the delayed work must be registered before the irq, since it
> might trigger instantly during probe and or trigger the IRQ handler
> during driver removal resulting in queuing another delasyed work
> between the autocancel and the IRQ removal.
> 
>> +	rc = chip->batt_info->voltage_max_design_uv;
>> +	rc = regmap_update_bits(chip->regmap,
>> +				chip->base + FLOAT_VOLTAGE_CFG_REG,
>> +				FLOAT_VOLTAGE_SETTING_MASK, rc / 1000);
>> +	if (rc < 0)
>> +		dev_err(chip->dev, "Couldn't set vbat max rc = %d\n", rc);
>> +
>> +	platform_set_drvdata(pdev, chip);
>> +
>> +	/* trigger the IRQ to set up initial state */
>> +	smb2_handle_usb_plugin(irq, chip);
>> +
>> +	return 0;
>> +}
>> +
>> +static int smb2_remove(struct platform_device *pdev)
>> +{
>> +	struct smb2_chip *chip = platform_get_drvdata(pdev);
>> +
>> +	power_supply_put_battery_info(chip->chg_psy, chip->batt_info);
>> +	return 0;
>> +}
> 
> This can be removed, since power_supply_get_battery_info()
> now is using device managed memory allocation functions
> internally.
> 
>> +
>> +static const struct of_device_id fg_match_id_table[] = {
>> +	{ .compatible = "qcom,pmi8998-smb2" },
>> +	{ /* sentinal */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, fg_match_id_table);
>> +
>> +static struct platform_driver qcom_spmi_smb2 = {
>> +	.probe = smb2_probe,
>> +	.remove = smb2_remove,
>> +	.driver = {
>> +		.name = "qcom-spmi-smb2",
>> +		.of_match_table = fg_match_id_table,
>> +	},
>> +};
>> +
>> +module_platform_driver(qcom_spmi_smb2);
>> +
>> +MODULE_AUTHOR("Caleb Connolly <caleb.connolly@linaro.org>");
>> +MODULE_DESCRIPTION("Qualcomm SMB2 Charger Driver");
>> +MODULE_LICENSE("GPL");
> 
> -- Sebastian

-- 
Kind Regards,
Caleb (they/them)

Re: [PATCH 1/6] power: supply: add Qualcomm SMB2 driver

[Bjorn Andersson]

On Fri 01 Apr 13:26 PDT 2022, Caleb Connolly wrote:

> Add a driver for the SMB2 charger block found in the Qualcomm PMI8998
> and PM660.

Are there any differences between the two? Or would it be warranted to
add pm660 in the match table?

> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  drivers/power/supply/Kconfig     |   9 +
>  drivers/power/supply/Makefile    |   1 +
>  drivers/power/supply/qcom_smb2.c | 930 +++++++++++++++++++++++++++++++
>  3 files changed, 940 insertions(+)
>  create mode 100644 drivers/power/supply/qcom_smb2.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 1aa8323ad9f6..c72da3d75258 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -897,4 +897,13 @@ config BATTERY_UG3105
>  	  device is off or suspended, the functionality of this driver is
>  	  limited to reporting capacity only.
>  
> +config CHARGER_QCOM_SMB2
> +	tristate "Qualcomm PMIC charger driver"
> +	depends on MFD_SPMI_PMIC
> +	depends on IIO
> +	help
> +	  Say Y here to enable the Qualcomm PMIC Charger driver. This
> +	  adds support for the SMB2 switch mode battery charger found
> +	  in PMI8998 and related PMICs.
> +
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 7f02f36aea55..e5db9eda6a9b 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -108,3 +108,4 @@ obj-$(CONFIG_BATTERY_ACER_A500)	+= acer_a500_battery.o
>  obj-$(CONFIG_BATTERY_SURFACE)	+= surface_battery.o
>  obj-$(CONFIG_CHARGER_SURFACE)	+= surface_charger.o
>  obj-$(CONFIG_BATTERY_UG3105)	+= ug3105_battery.o
> +obj-$(CONFIG_CHARGER_QCOM_SMB2)	+= qcom_smb2.o
> diff --git a/drivers/power/supply/qcom_smb2.c b/drivers/power/supply/qcom_smb2.c
> new file mode 100644
> index 000000000000..63100cfc3300
> --- /dev/null
> +++ b/drivers/power/supply/qcom_smb2.c
> @@ -0,0 +1,930 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2016-2019 The Linux Foundation. All rights reserved.
> + * Copyright (c) 2021, Linaro Ltd.

2021-2022

> + *  Author: Caleb Connolly <caleb.connolly@linaro.org>

There's two spaces before Author...

> + *
> + * This driver is for the switch-mode battery charger and boost
> + * hardware found in pmi8998 and related PMICs.
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/kernel.h>
> +#include <linux/minmax.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/device.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +#include <linux/power_supply.h>
> +#include <linux/module.h>
> +#include <linux/math64.h>
> +#include <linux/of_irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/workqueue.h>
> +
> +#define BATTERY_CHARGER_STATUS_2_REG 0x07

It would be nice if the value of all these defines where indented, to
make these easier to read.

> +#define INPUT_CURRENT_LIMITED_BIT BIT(7)
> +#define CHARGER_ERROR_STATUS_SFT_EXPIRE_BIT BIT(6)
> +#define CHARGER_ERROR_STATUS_BAT_OV_BIT BIT(5)
> +#define CHARGER_ERROR_STATUS_BAT_TERM_MISSING_BIT BIT(4)
> +#define BAT_TEMP_STATUS_MASK GENMASK(3, 0)
> +#define BAT_TEMP_STATUS_SOFT_LIMIT_MASK GENMASK(3, 2)
> +#define BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT BIT(3)
> +#define BAT_TEMP_STATUS_COLD_SOFT_LIMIT_BIT BIT(2)
> +#define BAT_TEMP_STATUS_TOO_HOT_BIT BIT(1)
> +#define BAT_TEMP_STATUS_TOO_COLD_BIT BIT(0)
> +
> +#define CHARGING_ENABLE_CMD_REG 0x42
> +#define CHARGING_ENABLE_CMD_BIT BIT(0)
> +
> +#define CHGR_CFG2_REG 0x51
> +#define CHG_EN_SRC_BIT BIT(7)
> +#define CHG_EN_POLARITY_BIT BIT(6)
> +#define PRETOFAST_TRANSITION_CFG_BIT BIT(5)
> +#define BAT_OV_ECC_BIT BIT(4)
> +#define I_TERM_BIT BIT(3)
> +#define AUTO_RECHG_BIT BIT(2)
> +#define EN_ANALOG_DROP_IN_VBATT_BIT BIT(1)
> +#define CHARGER_INHIBIT_BIT BIT(0)
> +
> +#define FAST_CHARGE_CURRENT_CFG_REG 0x61
> +#define FAST_CHARGE_CURRENT_SETTING_MASK GENMASK(7, 0)
> +
> +#define FLOAT_VOLTAGE_CFG_REG 0x70
> +#define FLOAT_VOLTAGE_SETTING_MASK GENMASK(7, 0)
> +
> +#define FG_UPDATE_CFG_2_SEL_REG 0x7D
> +#define SOC_LT_OTG_THRESH_SEL_BIT BIT(3)
> +#define SOC_LT_CHG_RECHARGE_THRESH_SEL_BIT BIT(2)
> +#define VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT BIT(1)
> +#define IBT_LT_CHG_TERM_THRESH_SEL_BIT BIT(0)
> +
> +#define JEITA_EN_CFG_REG 0x90
> +#define JEITA_EN_HARDLIMIT_BIT BIT(4)
> +#define JEITA_EN_HOT_SL_FCV_BIT BIT(3)
> +#define JEITA_EN_COLD_SL_FCV_BIT BIT(2)
> +#define JEITA_EN_HOT_SL_CCC_BIT BIT(1)
> +#define JEITA_EN_COLD_SL_CCC_BIT BIT(0)
> +
> +#define INT_RT_STS 0x310
> +#define TYPE_C_CHANGE_RT_STS_BIT BIT(7)
> +#define USBIN_ICL_CHANGE_RT_STS_BIT BIT(6)
> +#define USBIN_SOURCE_CHANGE_RT_STS_BIT BIT(5)
> +#define USBIN_PLUGIN_RT_STS_BIT BIT(4)
> +#define USBIN_OV_RT_STS_BIT BIT(3)
> +#define USBIN_UV_RT_STS_BIT BIT(2)
> +#define USBIN_LT_3P6V_RT_STS_BIT BIT(1)
> +#define USBIN_COLLAPSE_RT_STS_BIT BIT(0)
> +
> +#define BATTERY_CHARGER_STATUS_1_REG 0x06
> +#define BVR_INITIAL_RAMP_BIT BIT(7)
> +#define CC_SOFT_TERMINATE_BIT BIT(6)
> +#define STEP_CHARGING_STATUS_SHIFT 3
> +#define STEP_CHARGING_STATUS_MASK GENMASK(5, 3)
> +#define BATTERY_CHARGER_STATUS_MASK GENMASK(2, 0)
> +
> +#define BATTERY_HEALTH_STATUS_REG 0x07
> +
> +#define OTG_CFG_REG 0x153
> +#define OTG_RESERVED_MASK GENMASK(7, 6)
> +#define DIS_OTG_ON_TLIM_BIT BIT(5)
> +#define QUICKSTART_OTG_FASTROLESWAP_BIT BIT(4)
> +#define INCREASE_DFP_TIME_BIT BIT(3)
> +#define ENABLE_OTG_IN_DEBUG_MODE_BIT BIT(2)
> +#define OTG_EN_SRC_CFG_BIT BIT(1)
> +#define CONCURRENT_MODE_CFG_BIT BIT(0)
> +
> +#define OTG_ENG_OTG_CFG_REG 0x1C0
> +#define ENG_BUCKBOOST_HALT1_8_MODE_BIT BIT(0)
> +
> +#define APSD_STATUS_REG 0x307
> +#define APSD_STATUS_7_BIT BIT(7)
> +#define HVDCP_CHECK_TIMEOUT_BIT BIT(6)
> +#define SLOW_PLUGIN_TIMEOUT_BIT BIT(5)
> +#define ENUMERATION_DONE_BIT BIT(4)
> +#define VADP_CHANGE_DONE_AFTER_AUTH_BIT BIT(3)
> +#define QC_AUTH_DONE_STATUS_BIT BIT(2)
> +#define QC_CHARGER_BIT BIT(1)
> +#define APSD_DTC_STATUS_DONE_BIT BIT(0)
> +
> +#define APSD_RESULT_STATUS_REG 0x308
> +#define ICL_OVERRIDE_LATCH_BIT BIT(7)
> +#define APSD_RESULT_STATUS_MASK GENMASK(6, 0)
> +#define QC_3P0_BIT BIT(6)
> +#define QC_2P0_BIT BIT(5)
> +#define FLOAT_CHARGER_BIT BIT(4)
> +#define DCP_CHARGER_BIT BIT(3)
> +#define CDP_CHARGER_BIT BIT(2)
> +#define OCP_CHARGER_BIT BIT(1)
> +#define SDP_CHARGER_BIT BIT(0)
> +
> +#define TYPE_C_STATUS_1_REG 0x30B
> +#define UFP_TYPEC_MASK GENMASK(7, 5)
> +#define UFP_TYPEC_RDSTD_BIT BIT(7)
> +#define UFP_TYPEC_RD1P5_BIT BIT(6)
> +#define UFP_TYPEC_RD3P0_BIT BIT(5)
> +#define UFP_TYPEC_FMB_255K_BIT BIT(4)
> +#define UFP_TYPEC_FMB_301K_BIT BIT(3)
> +#define UFP_TYPEC_FMB_523K_BIT BIT(2)
> +#define UFP_TYPEC_FMB_619K_BIT BIT(1)
> +#define UFP_TYPEC_OPEN_OPEN_BIT BIT(0)
> +
> +#define TYPE_C_STATUS_2_REG 0x30C
> +#define DFP_RA_OPEN_BIT BIT(7)
> +#define TIMER_STAGE_BIT BIT(6)
> +#define EXIT_UFP_MODE_BIT BIT(5)
> +#define EXIT_DFP_MODE_BIT BIT(4)
> +#define DFP_TYPEC_MASK GENMASK(3, 0)
> +#define DFP_RD_OPEN_BIT BIT(3)
> +#define DFP_RD_RA_VCONN_BIT BIT(2)
> +#define DFP_RD_RD_BIT BIT(1)
> +#define DFP_RA_RA_BIT BIT(0)
> +
> +#define TYPE_C_STATUS_3_REG 0x30D
> +#define ENABLE_BANDGAP_BIT BIT(7)
> +#define U_USB_GND_NOVBUS_BIT BIT(6)
> +#define U_USB_FLOAT_NOVBUS_BIT BIT(5)
> +#define U_USB_GND_BIT BIT(4)
> +#define U_USB_FMB1_BIT BIT(3)
> +#define U_USB_FLOAT1_BIT BIT(2)
> +#define U_USB_FMB2_BIT BIT(1)
> +#define U_USB_FLOAT2_BIT BIT(0)
> +
> +#define TYPE_C_STATUS_4_REG 0x30E
> +#define UFP_DFP_MODE_STATUS_BIT BIT(7)
> +#define TYPEC_VBUS_STATUS_BIT BIT(6)
> +#define TYPEC_VBUS_ERROR_STATUS_BIT BIT(5)
> +#define TYPEC_DEBOUNCE_DONE_STATUS_BIT BIT(4)
> +#define TYPEC_UFP_AUDIO_ADAPT_STATUS_BIT BIT(3)
> +#define TYPEC_VCONN_OVERCURR_STATUS_BIT BIT(2)
> +#define CC_ORIENTATION_BIT BIT(1)
> +#define CC_ATTACHED_BIT BIT(0)
> +
> +#define TYPE_C_STATUS_5_REG 0x30F
> +#define TRY_SOURCE_FAILED_BIT BIT(6)
> +#define TRY_SINK_FAILED_BIT BIT(5)
> +#define TIMER_STAGE_2_BIT BIT(4)
> +#define TYPEC_LEGACY_CABLE_STATUS_BIT BIT(3)
> +#define TYPEC_NONCOMP_LEGACY_CABLE_STATUS_BIT BIT(2)
> +#define TYPEC_TRYSOURCE_DETECT_STATUS_BIT BIT(1)
> +#define TYPEC_TRYSINK_DETECT_STATUS_BIT BIT(0)
> +
> +#define CMD_APSD_REG 0x341
> +#define ICL_OVERRIDE_BIT BIT(1)
> +#define APSD_RERUN_BIT BIT(0)
> +
> +#define TYPE_C_CFG_REG 0x358
> +#define APSD_START_ON_CC_BIT BIT(7)
> +#define WAIT_FOR_APSD_BIT BIT(6)
> +#define FACTORY_MODE_DETECTION_EN_BIT BIT(5)
> +#define FACTORY_MODE_ICL_3A_4A_BIT BIT(4)
> +#define FACTORY_MODE_DIS_CHGING_CFG_BIT BIT(3)
> +#define SUSPEND_NON_COMPLIANT_CFG_BIT BIT(2)
> +#define VCONN_OC_CFG_BIT BIT(1)
> +#define TYPE_C_OR_U_USB_BIT BIT(0)
> +
> +#define TYPE_C_CFG_2_REG 0x359
> +#define TYPE_C_DFP_CURRSRC_MODE_BIT BIT(7)
> +#define DFP_CC_1P4V_OR_1P6V_BIT BIT(6)
> +#define VCONN_SOFTSTART_CFG_MASK GENMASK(5, 4)
> +#define EN_TRY_SOURCE_MODE_BIT BIT(3)
> +#define USB_FACTORY_MODE_ENABLE_BIT BIT(2)
> +#define TYPE_C_UFP_MODE_BIT BIT(1)
> +#define EN_80UA_180UA_CUR_SOURCE_BIT BIT(0)
> +
> +#define TYPE_C_CFG_3_REG 0x35A
> +#define TVBUS_DEBOUNCE_BIT BIT(7)
> +#define TYPEC_LEGACY_CABLE_INT_EN_BIT BIT(6)
> +#define TYPEC_NONCOMPLIANT_LEGACY_CABLE_INT_EN_BIT BIT(5)
> +#define TYPEC_TRYSOURCE_DETECT_INT_EN_BIT BIT(4)
> +#define TYPEC_TRYSINK_DETECT_INT_EN_BIT BIT(3)
> +#define EN_TRYSINK_MODE_BIT BIT(2)
> +#define EN_LEGACY_CABLE_DETECTION_BIT BIT(1)
> +#define ALLOW_PD_DRING_UFP_TCCDB_BIT BIT(0)
> +
> +#define USBIN_OPTIONS_1_CFG_REG 0x362
> +#define CABLE_R_SEL_BIT BIT(7)
> +#define HVDCP_AUTH_ALG_EN_CFG_BIT BIT(6)
> +#define HVDCP_AUTONOMOUS_MODE_EN_CFG_BIT BIT(5)
> +#define INPUT_PRIORITY_BIT BIT(4)
> +#define AUTO_SRC_DETECT_BIT BIT(3)
> +#define HVDCP_EN_BIT BIT(2)
> +#define VADP_INCREMENT_VOLTAGE_LIMIT_BIT BIT(1)
> +#define VADP_TAPER_TIMER_EN_BIT BIT(0)
> +
> +#define USBIN_OPTIONS_2_CFG_REG 0x363
> +#define WIPWR_RST_EUD_CFG_BIT BIT(7)
> +#define SWITCHER_START_CFG_BIT BIT(6)
> +#define DCD_TIMEOUT_SEL_BIT BIT(5)
> +#define OCD_CURRENT_SEL_BIT BIT(4)
> +#define SLOW_PLUGIN_TIMER_EN_CFG_BIT BIT(3)
> +#define FLOAT_OPTIONS_MASK GENMASK(2, 0)
> +#define FLOAT_DIS_CHGING_CFG_BIT BIT(2)
> +#define SUSPEND_FLOAT_CFG_BIT BIT(1)
> +#define FORCE_FLOAT_SDP_CFG_BIT BIT(0)
> +
> +#define TAPER_TIMER_SEL_CFG_REG 0x364
> +#define TYPEC_SPARE_CFG_BIT BIT(7)
> +#define TYPEC_DRP_DFP_TIME_CFG_BIT BIT(5)
> +#define TAPER_TIMER_SEL_MASK GENMASK(1, 0)
> +
> +#define USBIN_LOAD_CFG_REG 0x365
> +#define USBIN_OV_CH_LOAD_OPTION_BIT BIT(7)
> +#define ICL_OVERRIDE_AFTER_APSD_BIT BIT(4)
> +
> +#define USBIN_ICL_OPTIONS_REG 0x366
> +#define CFG_USB3P0_SEL_BIT BIT(2)
> +#define USB51_MODE_BIT BIT(1)
> +#define USBIN_MODE_CHG_BIT BIT(0)
> +
> +#define TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG 0x368
> +#define EXIT_SNK_BASED_ON_CC_BIT BIT(7)
> +#define VCONN_EN_ORIENTATION_BIT BIT(6)
> +#define TYPEC_VCONN_OVERCURR_INT_EN_BIT BIT(5)
> +#define VCONN_EN_SRC_BIT BIT(4)
> +#define VCONN_EN_VALUE_BIT BIT(3)
> +#define TYPEC_POWER_ROLE_CMD_MASK GENMASK(2, 0)
> +#define UFP_EN_CMD_BIT BIT(2)
> +#define DFP_EN_CMD_BIT BIT(1)
> +#define TYPEC_DISABLE_CMD_BIT BIT(0)
> +
> +#define USBIN_CURRENT_LIMIT_CFG_REG 0x370
> +#define USBIN_CURRENT_LIMIT_MASK GENMASK(7, 0)
> +
> +#define USBIN_AICL_OPTIONS_CFG_REG 0x380
> +#define SUSPEND_ON_COLLAPSE_USBIN_BIT BIT(7)
> +#define USBIN_AICL_HDC_EN_BIT BIT(6)
> +#define USBIN_AICL_START_AT_MAX_BIT BIT(5)
> +#define USBIN_AICL_RERUN_EN_BIT BIT(4)
> +#define USBIN_AICL_ADC_EN_BIT BIT(3)
> +#define USBIN_AICL_EN_BIT BIT(2)
> +#define USBIN_HV_COLLAPSE_RESPONSE_BIT BIT(1)
> +#define USBIN_LV_COLLAPSE_RESPONSE_BIT BIT(0)
> +
> +#define DC_ENG_SSUPPLY_CFG2_REG 0x4C1
> +#define ENG_SSUPPLY_IVREF_OTG_SS_MASK GENMASK(2, 0)
> +#define OTG_SS_SLOW 0x3
> +
> +#define DCIN_AICL_REF_SEL_CFG_REG 0x481
> +#define DCIN_CONT_AICL_THRESHOLD_CFG_MASK GENMASK(5, 0)
> +
> +#define WI_PWR_OPTIONS_REG 0x495
> +#define CHG_OK_BIT BIT(7)
> +#define WIPWR_UVLO_IRQ_OPT_BIT BIT(6)
> +#define BUCK_HOLDOFF_ENABLE_BIT BIT(5)
> +#define CHG_OK_HW_SW_SELECT_BIT BIT(4)
> +#define WIPWR_RST_ENABLE_BIT BIT(3)
> +#define DCIN_WIPWR_IRQ_SELECT_BIT BIT(2)
> +#define AICL_SWITCH_ENABLE_BIT BIT(1)
> +#define ZIN_ICL_ENABLE_BIT BIT(0)
> +
> +#define ICL_STATUS_REG 0x607
> +#define INPUT_CURRENT_LIMIT_MASK GENMASK(7, 0)
> +
> +#define POWER_PATH_STATUS_REG 0x60B
> +#define P_PATH_INPUT_SS_DONE_BIT BIT(7)
> +#define P_PATH_USBIN_SUSPEND_STS_BIT BIT(6)
> +#define P_PATH_DCIN_SUSPEND_STS_BIT BIT(5)
> +#define P_PATH_USE_USBIN_BIT BIT(4)
> +#define P_PATH_USE_DCIN_BIT BIT(3)
> +#define P_PATH_POWER_PATH_MASK GENMASK(2, 1)
> +#define P_PATH_VALID_INPUT_POWER_SOURCE_STS_BIT BIT(0)
> +
> +#define WD_CFG_REG 0x651
> +#define WATCHDOG_TRIGGER_AFP_EN_BIT BIT(7)
> +#define BARK_WDOG_INT_EN_BIT BIT(6)
> +#define BITE_WDOG_INT_EN_BIT BIT(5)
> +#define SFT_AFTER_WDOG_IRQ_MASK GENMASK(4, 3)
> +#define WDOG_IRQ_SFT_BIT BIT(2)
> +#define WDOG_TIMER_EN_ON_PLUGIN_BIT BIT(1)
> +#define WDOG_TIMER_EN_BIT BIT(0)
> +
> +#define AICL_RERUN_TIME_CFG_REG 0x661
> +#define AICL_RERUN_TIME_MASK GENMASK(1, 0)
> +
> +#define SDP_CURRENT_UA 500000
> +#define CDP_CURRENT_UA 1500000
> +#define DCP_CURRENT_UA 1500000
> +
> +enum charger_status {
> +	TRICKLE_CHARGE = 0,
> +	PRE_CHARGE,
> +	FAST_CHARGE,
> +	FULLON_CHARGE,
> +	TAPER_CHARGE,
> +	TERMINATE_CHARGE,
> +	INHIBIT_CHARGE,
> +	DISABLE_CHARGE,
> +};
> +
> +struct smb2_register {
> +	u16 addr;
> +	u8 mask;
> +	u8 val;
> +};
> +
> +/**
> + * struct smb2_chip - smb2 chip structure
> + * @dev:		Device reference for power_supply

@name

> + * @base:		Base address for smb2 registers
> + * @regmap:		Register map
> + * @reg_lock:		Mutex for register access
> + * @batt_info:		Battery data from DT

status_change_work

> + * @usbin_i_chan:	USBIN current measurement channel
> + * @usbin_v_chan:	USBIN voltage measurement channel
> + * @chg_psy:		Charger power supply instance
> + * @usb_present:	True if a charger is attached
> + */
> +struct smb2_chip {
> +	struct device *dev;
> +	const char *name;
> +	unsigned int base;
> +	struct regmap *regmap;
> +	struct mutex reg_lock;
> +	struct power_supply_battery_info *batt_info;
> +
> +	struct delayed_work status_change_work;
> +
> +	struct iio_channel *usbin_i_chan;
> +	struct iio_channel *usbin_v_chan;
> +
> +	struct power_supply *chg_psy;
> +
> +	bool usb_present;
> +};
> +
> +static enum power_supply_property smb2_properties[] = {
> +	POWER_SUPPLY_PROP_MANUFACTURER, POWER_SUPPLY_PROP_MODEL_NAME,
> +	POWER_SUPPLY_PROP_CURRENT_MAX,	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_HEALTH,	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_USB_TYPE,
> +};
> +
> +static enum power_supply_usb_type smb2_usb_types[] = {
> +	POWER_SUPPLY_USB_TYPE_SDP, POWER_SUPPLY_USB_TYPE_DCP,
> +	POWER_SUPPLY_USB_TYPE_CDP, POWER_SUPPLY_USB_TYPE_C,
> +	POWER_SUPPLY_USB_TYPE_PD_DRP
> +};
> +
> +/*
> + * Qualcomm "automatic power source detection" aka APSD
> + * tells us what type of charger we're connected to.
> + */
> +static int smb2_apsd_get_charger_type(struct smb2_chip *chip, int *val)
> +{
> +	int rc;
> +	unsigned int apsd_stat, stat;
> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	rc = regmap_read(chip->regmap, chip->base + APSD_STATUS_REG,
> +			 &apsd_stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Failed to read apsd status, rc = %d", rc);
> +		return rc;
> +	}
> +	if (!(apsd_stat & APSD_DTC_STATUS_DONE_BIT)) {
> +		dev_err(chip->dev, "Apsd not ready");
> +		return -EAGAIN;
> +	}
> +
> +	rc = regmap_read(chip->regmap, chip->base + APSD_RESULT_STATUS_REG,
> +			 &stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Failed to read apsd result, rc = %d", rc);
> +		return rc;
> +	}
> +
> +	mutex_unlock(&chip->reg_lock);
> +
> +	stat &= APSD_RESULT_STATUS_MASK;
> +
> +	if (stat & CDP_CHARGER_BIT) {
> +		*val = POWER_SUPPLY_USB_TYPE_CDP;
> +	} else if (stat &
> +		   (DCP_CHARGER_BIT | OCP_CHARGER_BIT | FLOAT_CHARGER_BIT)) {
> +		*val = POWER_SUPPLY_USB_TYPE_DCP;
> +	} else { /* SDP_CHARGER_BIT (or others) */
> +		*val = POWER_SUPPLY_USB_TYPE_SDP;
> +	}
> +
> +	return 0;
> +}
> +
> +int smb2_get_prop_usb_online(struct smb2_chip *chip, int *val)
> +{
> +	unsigned int stat;
> +	int rc;
> +
> +	rc = regmap_read(chip->regmap, chip->base + POWER_PATH_STATUS_REG,
> +			 &stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't read POWER_PATH_STATUS! ret=%d\n",
> +			rc);
> +		return rc;
> +	}
> +
> +	*val = (stat & P_PATH_USE_USBIN_BIT) &&
> +	       (stat & P_PATH_VALID_INPUT_POWER_SOURCE_STS_BIT);
> +	return rc;

return 0;

> +}
> +
> +int smb2_get_prop_status(struct smb2_chip *chip, int *val)
> +{
> +	int usb_online_val;
> +	unsigned int stat;
> +	int rc;
> +	bool usb_online;
> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	rc = smb2_get_prop_usb_online(chip, &usb_online_val);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't get usb online property rc = %d\n",
> +			rc);
> +		return rc;
> +	}
> +	usb_online = (bool)usb_online_val;

No need to move this value over to a bool variable, just to check if
it's 0.

> +
> +	if (!usb_online) {
> +		*val = POWER_SUPPLY_STATUS_DISCHARGING;
> +		return rc;
> +	}
> +
> +	rc = regmap_read(chip->regmap,
> +			 chip->base + BATTERY_CHARGER_STATUS_1_REG, &stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev,
> +			"Failed to read charging status ret=%d\n", rc);
> +		return rc;
> +	}
> +
> +	mutex_unlock(&chip->reg_lock);
> +
> +	stat = stat & BATTERY_CHARGER_STATUS_MASK;
> +
> +	switch (stat) {
> +	case TRICKLE_CHARGE:
> +	case PRE_CHARGE:
> +	case FAST_CHARGE:
> +	case FULLON_CHARGE:
> +	case TAPER_CHARGE:
> +		*val = POWER_SUPPLY_STATUS_CHARGING;
> +		return rc;
> +	case DISABLE_CHARGE:
> +		*val = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		return rc;
> +	case TERMINATE_CHARGE:
> +		*val = POWER_SUPPLY_STATUS_FULL;
> +		return rc;
> +	case INHIBIT_CHARGE:
> +	default:
> +		*val = POWER_SUPPLY_STATUS_UNKNOWN;
> +		return rc;
> +	}
> +}
> +
> +static inline int smb2_get_current_limit(struct smb2_chip *chip,
> +					 unsigned int *val)
> +{
> +	int rc = regmap_read(chip->regmap, chip->base + ICL_STATUS_REG, val);
> +
> +	if (rc >= 0)
> +		*val *= 25000;
> +	return rc;
> +}
> +
> +static inline int smb2_set_current_limit(struct smb2_chip *chip,
> +					 unsigned int val)
> +{
> +	unsigned char val_raw;
> +
> +	if (val > 4800000) {
> +		dev_err(chip->dev,
> +			"Can't set current limit higher than 4800000uA");
> +		return -EINVAL;
> +	}
> +	val_raw = val / 25000;
> +	return regmap_write(chip->regmap,
> +			    chip->base + USBIN_CURRENT_LIMIT_CFG_REG, val_raw);
> +}
> +
> +void smb2_status_change_work(struct work_struct *work)
> +{
> +	struct smb2_chip *chip =
> +		container_of(work, struct smb2_chip, status_change_work.work);
> +	unsigned int charger_type, current_ua;
> +	int usb_online, count, rc;
> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	smb2_get_prop_usb_online(chip, &usb_online);
> +	if (usb_online == 0)

reg_lock is held here.

> +		return;
> +
> +	for (count = 0; count < 3; count++) {
> +		dev_info(chip->dev, "get charger type retry %d\n", count);
> +		rc = smb2_apsd_get_charger_type(chip, &charger_type);
> +		if (rc == 0)
> +			break;
> +		msleep(100);
> +	}
> +
> +	if (rc < 0) {
> +		rc = regmap_update_bits(chip->regmap, chip->base + CMD_APSD_REG,
> +					APSD_RERUN_BIT, APSD_RERUN_BIT);
> +		schedule_delayed_work(&chip->status_change_work,
> +				      msecs_to_jiffies(1500));

reg_lock is held here.

> +		return;
> +	}
> +
> +	switch (charger_type) {
> +	case POWER_SUPPLY_USB_TYPE_CDP:
> +		current_ua = CDP_CURRENT_UA;
> +		break;
> +	case POWER_SUPPLY_USB_TYPE_DCP:
> +		current_ua = DCP_CURRENT_UA;
> +		break;
> +	case POWER_SUPPLY_USB_TYPE_SDP:
> +	default:
> +		current_ua = SDP_CURRENT_UA;
> +		break;
> +	}
> +
> +	smb2_set_current_limit(chip, current_ua);
> +	mutex_unlock(&chip->reg_lock);
> +	power_supply_changed(chip->chg_psy);
> +}
> +
> +int smb2_get_iio_chan(struct smb2_chip *chip, struct iio_channel *chan,
> +		      int *val)
> +{
> +	int rc = 0;

First use of "rc" is an assignment, no need to initialize it here.

> +	union power_supply_propval status;
> +
> +	rc = power_supply_get_property(chip->chg_psy, POWER_SUPPLY_PROP_STATUS,
> +				       &status);
> +	if (rc < 0 || status.intval != POWER_SUPPLY_STATUS_CHARGING) {
> +		*val = 0;
> +		return 0;
> +	}
> +
> +	if (IS_ERR(chan)) {
> +		dev_err(chip->dev, "Failed to chan, err = %li", PTR_ERR(chan));
> +		return PTR_ERR(chan);
> +	}
> +
> +	rc = iio_read_channel_processed(chan, val);

return iio_read_channel_processed()

> +	return rc;
> +}
> +
> +int smb2_get_prop_health(struct smb2_chip *chip, int *val)
> +{
> +	int rc;
> +	unsigned int stat;
> +
> +	*val = POWER_SUPPLY_HEALTH_UNKNOWN;
> +
> +	rc = regmap_read(chip->regmap,
> +			 chip->base + BATTERY_CHARGER_STATUS_2_REG, &stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't read charger status rc=%d\n", rc);
> +		return rc;
> +	}
> +
> +	switch (stat & BATTERY_CHARGER_STATUS_MASK) {
> +	case CHARGER_ERROR_STATUS_BAT_OV_BIT:
> +		*val = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> +		return 0;
> +	case BAT_TEMP_STATUS_TOO_COLD_BIT:
> +		*val = POWER_SUPPLY_HEALTH_COLD;
> +		return 0;
> +	case BAT_TEMP_STATUS_TOO_HOT_BIT:
> +		*val = POWER_SUPPLY_HEALTH_OVERHEAT;
> +		return 0;
> +	case BAT_TEMP_STATUS_COLD_SOFT_LIMIT_BIT:
> +		*val = POWER_SUPPLY_HEALTH_COOL;
> +		return 0;
> +	case BAT_TEMP_STATUS_HOT_SOFT_LIMIT_BIT:
> +		*val = POWER_SUPPLY_HEALTH_WARM;
> +		return 0;
> +	default:
> +		*val = POWER_SUPPLY_HEALTH_GOOD;
> +		return 0;
> +	}
> +}
> +
> +static int smb2_get_property(struct power_supply *psy,
> +			     enum power_supply_property psp,
> +			     union power_supply_propval *val)
> +{
> +	struct smb2_chip *chip = power_supply_get_drvdata(psy);
> +	int error = 0;

error is only ever assigned, never used. Perhaps drop it?

> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_MANUFACTURER:
> +		val->strval = "Qualcomm";
> +		return 0;
> +	case POWER_SUPPLY_PROP_MODEL_NAME:
> +		val->strval = chip->name;
> +		return 0;
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +		error = smb2_get_current_limit(chip, &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +		error = smb2_get_iio_chan(chip, chip->usbin_i_chan,
> +					  &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		error = smb2_get_iio_chan(chip, chip->usbin_v_chan,
> +					  &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		error = smb2_get_prop_usb_online(chip, &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_STATUS:
> +		error = smb2_get_prop_status(chip, &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		error = smb2_get_prop_health(chip, &val->intval);
> +		return 0;
> +	case POWER_SUPPLY_PROP_USB_TYPE:
> +		error = smb2_apsd_get_charger_type(chip, &val->intval);
> +		return 0;
> +	default:
> +		dev_err(chip->dev, "invalid property: %d\n", psp);
> +		return -EINVAL;
> +	}
> +}
> +
> +static int smb2_set_property(struct power_supply *psy,
> +			     enum power_supply_property psp,
> +			     const union power_supply_propval *val)
> +{
> +	struct smb2_chip *chip = power_supply_get_drvdata(psy);
> +	int error = 0;

All code paths to return assigned error, no need to assign it here.

> +
> +	dev_info(chip->dev, "Setting property: %d", psp);
> +
> +	mutex_lock(&chip->reg_lock);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +		error = smb2_set_current_limit(chip, val->intval);
> +		break;
> +	default:
> +		dev_err(chip->dev, "No setter for property: %d\n", psp);
> +		error = -EINVAL;
> +	}
> +
> +	mutex_unlock(&chip->reg_lock);
> +
> +	return error;
> +}
> +
> +static int smb2_property_is_writable(struct power_supply *psy,
> +				     enum power_supply_property psp)
> +{
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +		return 1;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +irqreturn_t smb2_handle_usb_plugin(int irq, void *data)
> +{
> +	struct smb2_chip *chip = data;
> +	int rc;
> +	unsigned int intrt_stat;
> +
> +	rc = regmap_read(chip->regmap, chip->base + INT_RT_STS, &intrt_stat);
> +	if (rc < 0) {
> +		dev_err(chip->dev,
> +			"Couldn't read USB status from reg! ret=%d\n", rc);
> +		return rc;
> +	}
> +
> +	chip->usb_present = (bool)(intrt_stat & USBIN_PLUGIN_RT_STS_BIT);

No need to explicitly cast this to bool.

> +
> +	power_supply_changed(chip->chg_psy);
> +
> +	schedule_delayed_work(&chip->status_change_work,
> +			      msecs_to_jiffies(1000));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct power_supply_desc smb2_psy_desc = {
> +	.name = "usb",
> +	.type = POWER_SUPPLY_TYPE_USB,
> +	.usb_types = smb2_usb_types,
> +	.num_usb_types = ARRAY_SIZE(smb2_usb_types),
> +	.properties = smb2_properties,
> +	.num_properties = ARRAY_SIZE(smb2_properties),
> +	.get_property = smb2_get_property,
> +	.set_property = smb2_set_property,
> +	.property_is_writeable = smb2_property_is_writable,
> +};
> +
> +/* Init sequence derived from downstream driver */
> +static struct smb2_register smb2_init_seq[] = {
> +	{ .addr = AICL_RERUN_TIME_CFG_REG,
> +	  .mask = AICL_RERUN_TIME_MASK,
> +	  .val = 0 },
> +	{ .addr = USBIN_AICL_OPTIONS_CFG_REG,
> +	  .mask = USBIN_AICL_START_AT_MAX_BIT | USBIN_AICL_ADC_EN_BIT,
> +	  .val = 0 },
> +	/*
> +	 * By default configure us as an upstream facing port
> +	 * FIXME: for OTG we should set UFP_EN_CMD_BIT and DFP_EN_CMD_BIT both to 0
> +	 */
> +	{ .addr = TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG,
> +	  .mask = TYPEC_POWER_ROLE_CMD_MASK | VCONN_EN_SRC_BIT |
> +		  VCONN_EN_VALUE_BIT,
> +	  .val = VCONN_EN_SRC_BIT | UFP_EN_CMD_BIT },
> +	/*
> +	 * disable Type-C factory mode and stay in Attached.SRC state when VCONN
> +	 * over-current happens
> +	 */
> +	{ .addr = TYPE_C_CFG_REG,
> +	  .mask = FACTORY_MODE_DETECTION_EN_BIT | VCONN_OC_CFG_BIT,
> +	  .val = 0 },
> +	/* Configure VBUS for software control */
> +	{ .addr = OTG_CFG_REG, .mask = OTG_EN_SRC_CFG_BIT, .val = 0 },
> +	{ .addr = FG_UPDATE_CFG_2_SEL_REG,
> +	  .mask = SOC_LT_CHG_RECHARGE_THRESH_SEL_BIT |
> +		  VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT,
> +	  .val = VBT_LT_CHG_RECHARGE_THRESH_SEL_BIT },
> +	/* Enable automatic input current limit */
> +	{ .addr = USBIN_AICL_OPTIONS_CFG_REG,
> +	  .mask = USBIN_AICL_EN_BIT,
> +	  .val = 1 },
> +	{ .addr = USBIN_OPTIONS_1_CFG_REG, .mask = HVDCP_EN_BIT, .val = 0 },
> +	{ .addr = CHARGING_ENABLE_CMD_REG,
> +	  .mask = CHARGING_ENABLE_CMD_BIT,
> +	  .val = CHARGING_ENABLE_CMD_BIT },
> +	{ .addr = USBIN_LOAD_CFG_REG,
> +	  .mask = ICL_OVERRIDE_AFTER_APSD_BIT,
> +	  .val = ICL_OVERRIDE_AFTER_APSD_BIT },
> +	{ .addr = CHGR_CFG2_REG,
> +	  .mask = CHG_EN_POLARITY_BIT | CHG_EN_SRC_BIT,
> +	  .val = 0 },
> +	{ .addr = TYPE_C_INTRPT_ENB_SOFTWARE_CTRL_REG,
> +	  .mask = UFP_EN_CMD_BIT,
> +	  .val = UFP_EN_CMD_BIT },
> +	/* Set the default SDP charger type to a 500ma USB 2.0 port
> +	 * and apply ICL override? (OnePlus 6 hack??)
> +	 */
> +	{ .addr = USBIN_ICL_OPTIONS_REG,
> +	  .mask = USB51_MODE_BIT | USBIN_MODE_CHG_BIT,
> +	  .val = USB51_MODE_BIT }, //| USBIN_MODE_CHG_BIT },
> +};
> +
> +static int smb2_init_hw(struct smb2_chip *chip)
> +{
> +	int rc, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(smb2_init_seq); i++) {
> +		dev_info(chip->dev, "%d: Writing 0x%02x to 0x%02x\n", i,
> +			 smb2_init_seq[i].val, smb2_init_seq[i].addr);
> +		rc = regmap_update_bits(chip->regmap,
> +					chip->base + smb2_init_seq[i].addr,
> +					smb2_init_seq[i].mask,
> +					smb2_init_seq[i].val);
> +		if (rc < 0) {
> +			dev_err(chip->dev,
> +				"%s: Failed to write 0x%02x to 0x%02x: %d\n",
> +				__func__, smb2_init_seq[i].val,
> +				smb2_init_seq[i].addr, rc);
> +			return rc;
> +		}
> +	}
> +
> +	smb2_set_current_limit(chip, 1950 * 1000);
> +	return rc;

rc is 0, so return 0;

> +}
> +
> +static int smb2_probe(struct platform_device *pdev)
> +{
> +	struct power_supply_config supply_config = {};
> +	struct smb2_chip *chip;
> +	int rc = 0, irq;

First use of rc is assignment, no need to initialize it here.

> +
> +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->dev = &pdev->dev;
> +	chip->name = pdev->name;
> +	mutex_init(&chip->reg_lock);
> +
> +	chip->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!chip->regmap) {
> +		dev_err(chip->dev, "failed to locate the regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	rc = device_property_read_u32(chip->dev, "reg", &chip->base);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't read base address from dt: %d\n",
> +			rc);
> +		return rc;
> +	}
> +
> +	irq = of_irq_get_byname(pdev->dev.of_node, "usb-plugin");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Couldn't get irq usb-plugin byname\n");

Using dev_err_probe() instead is quite useful in that it leaves the
message in /sys/kernel/debug/devices_deferred, so please use that
whenever applicable.

> +		return irq;
> +	}
> +
> +	rc = devm_request_threaded_irq(chip->dev, irq, NULL,
> +				       smb2_handle_usb_plugin, IRQF_ONESHOT,
> +				       "usb-plugin", chip);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't request irq %d\n", irq);
> +		return rc;
> +	}
> +
> +	chip->usbin_v_chan = iio_channel_get(chip->dev, "usbin_v");
> +	chip->usbin_i_chan = iio_channel_get(chip->dev, "usbin_i");
> +
> +	/* RRADC should probe before us, but just in case it doesn't */
> +	if (PTR_ERR(chip->usbin_v_chan) == -EPROBE_DEFER ||
> +	    PTR_ERR(chip->usbin_i_chan) == -EPROBE_DEFER) {
> +		return -EPROBE_DEFER;

Don't you need to iio_channel_release(usbin_v_chan), in the case where
usbin_i_chan was the one deferring?

> +	}
> +
> +	rc = smb2_init_hw(chip);
> +	if (rc < 0) {
> +		dev_err(chip->dev, "Couldn't init hw %d\n", rc);

You already printed a more specific error in smb2_init_hw(), no need for
another.

> +		return rc;
> +	}
> +
> +	supply_config.drv_data = chip;
> +	supply_config.of_node = pdev->dev.of_node;
> +
> +	chip->chg_psy = devm_power_supply_register(chip->dev, &smb2_psy_desc,
> +						   &supply_config);
> +	if (IS_ERR(chip->chg_psy)) {
> +		dev_err(&pdev->dev, "failed to register power supply\n");
> +		return PTR_ERR(chip->chg_psy);
> +	}
> +
> +	rc = power_supply_get_battery_info(chip->chg_psy, &chip->batt_info);
> +	if (rc) {
> +		dev_err(&pdev->dev, "Failed to get battery info: %d\n", rc);
> +		return rc;
> +	}
> +
> +	INIT_DELAYED_WORK(&chip->status_change_work, smb2_status_change_work);
> +
> +	rc = chip->batt_info->voltage_max_design_uv;
> +	rc = regmap_update_bits(chip->regmap,
> +				chip->base + FLOAT_VOLTAGE_CFG_REG,
> +				FLOAT_VOLTAGE_SETTING_MASK, rc / 1000);
> +	if (rc < 0)
> +		dev_err(chip->dev, "Couldn't set vbat max rc = %d\n", rc);
> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	/* trigger the IRQ to set up initial state */
> +	smb2_handle_usb_plugin(irq, chip);
> +
> +	return 0;
> +}
> +
> +static int smb2_remove(struct platform_device *pdev)
> +{
> +	struct smb2_chip *chip = platform_get_drvdata(pdev);
> +
> +	power_supply_put_battery_info(chip->chg_psy, chip->batt_info);

power_supply_put_battery_info() only consists of devm_kfree() calls,
which in practice means that this shouldn't be needed. But neither the
name "power_supply_get_battery_info" or the (absent) kernel-doc
describes that.

So I would prefer that power_supply_get_battery_info() was improved and
this call to be dropped.

> +	return 0;
> +}
> +
> +static const struct of_device_id fg_match_id_table[] = {

Why is this the "fuel gauge" match table?

Regards,
Bjorn

> +	{ .compatible = "qcom,pmi8998-smb2" },
> +	{ /* sentinal */ }
> +};
> +MODULE_DEVICE_TABLE(of, fg_match_id_table);
> +
> +static struct platform_driver qcom_spmi_smb2 = {
> +	.probe = smb2_probe,
> +	.remove = smb2_remove,
> +	.driver = {
> +		.name = "qcom-spmi-smb2",
> +		.of_match_table = fg_match_id_table,
> +	},
> +};
> +
> +module_platform_driver(qcom_spmi_smb2);
> +
> +MODULE_AUTHOR("Caleb Connolly <caleb.connolly@linaro.org>");
> +MODULE_DESCRIPTION("Qualcomm SMB2 Charger Driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.35.1
> 

[PATCH 2/6] arm64: dts: qcom: pmi8998: add smb2 node

[Caleb Connolly]

Add a node for the smb2 charger hardware found on the pmi8998.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 arch/arm64/boot/dts/qcom/pmi8998.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pmi8998.dtsi b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
index da10668c361d..b98dad166a0b 100644
--- a/arch/arm64/boot/dts/qcom/pmi8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
@@ -9,6 +9,21 @@ pmi8998_lsid0: pmic@2 {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
+		pmi8998_smb2: smb2@1000 {
+			compatible = "qcom,pmi8998-smb2";
+			reg = <0x1000>;
+
+			interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>;
+			interrupt-names = "usb-plugin";
+
+			io-channels = <&pmi8998_rradc 3>,
+				      <&pmi8998_rradc 4>;
+			io-channel-names = "usbin_i",
+					   "usbin_v";
+
+			status = "disabled";
+		};
+
 		pmi8998_gpio: gpios@c000 {
 			compatible = "qcom,pmi8998-gpio", "qcom,spmi-gpio";
 			reg = <0xc000>;
-- 
2.35.1

Re: [PATCH 2/6] arm64: dts: qcom: pmi8998: add smb2 node

[Krzysztof Kozlowski]

On 01/04/2022 22:26, Caleb Connolly wrote:
> Add a node for the smb2 charger hardware found on the pmi8998.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/pmi8998.dtsi | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/pmi8998.dtsi b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
> index da10668c361d..b98dad166a0b 100644
> --- a/arch/arm64/boot/dts/qcom/pmi8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
> @@ -9,6 +9,21 @@ pmi8998_lsid0: pmic@2 {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
> +		pmi8998_smb2: smb2@1000 {

Generic node name please, so "charger". Have in mind that PMIC
(parent's) schema might require it.

If it exists (I did not check), it should reference also your charger's
schema.

Best regards,
Krzysztof

[PATCH 3/6] arm64: dts: sdm845-oneplus: enable smb2

[Caleb Connolly]

Enable the smb2 charger to expose usb attach/detach events, charging
stats etc.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi   | 4 ++++
 arch/arm64/boot/dts/qcom/sdm845-oneplus-enchilada.dts | 4 ++++
 arch/arm64/boot/dts/qcom/sdm845-oneplus-fajita.dts    | 4 ++++
 3 files changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
index 3f1b05603701..afc8b0c3aed7 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
@@ -461,6 +461,10 @@ pinconf {
 	};
 };
 
+&pmi8998_smb2 {
+	status = "okay";
+};
+
 &pmi8998_rradc {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-enchilada.dts b/arch/arm64/boot/dts/qcom/sdm845-oneplus-enchilada.dts
index bf2cf92e8976..b0fb2f8ca6d8 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-enchilada.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-enchilada.dts
@@ -32,3 +32,7 @@ &display_panel {
 &bq27441_fg {
 	monitored-battery = <&battery>;
 };
+
+&pmi8998_smb2 {
+	monitored-battery = <&battery>;
+};
diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-fajita.dts b/arch/arm64/boot/dts/qcom/sdm845-oneplus-fajita.dts
index 1b6b5bf368df..3e3c757fe6f2 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-fajita.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-fajita.dts
@@ -33,6 +33,10 @@ &bq27441_fg {
 	monitored-battery = <&battery>;
 };
 
+&pmi8998_smb2 {
+	monitored-battery = <&battery>;
+};
+
 &rmi4_f12 {
 	touchscreen-y-mm = <148>;
 };
-- 
2.35.1

[PATCH 4/6] arm64: dts: qcom: sdm845-db845c: enable smb2

[Caleb Connolly]

Enable the smb2 charger driver on the db845c and add a simple-battery
node to report the correct VBAT voltage.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
index 40a290b6d4f3..812bd7fd7941 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
@@ -240,6 +240,18 @@ vph_pwr: vph-pwr-regulator {
 
 		vin-supply = <&vbat_som>;
 	};
+
+	/*
+	 * The db845c doesn't actually have a battery, but the charger circuitry
+	 * is still wired up to support one, it needs to be programmed for nominal
+	 * vbat voltage. See the vbat-regulator above.
+	 */
+	battery: battery {
+		compatible = "simple-battery";
+
+		voltage-min-design-microvolt = <4200000>;
+		voltage-max-design-microvolt = <4200000>;
+	};
 };
 
 &adsp_pas {
@@ -603,6 +615,12 @@ &pmi8998_rradc {
 	status = "okay";
 };
 
+&pmi8998_smb2 {
+	status = "okay";
+
+	monitored-battery = <&battery>;
+};
+
 /* QUAT I2S Uses 4 I2S SD Lines for audio on LT9611 HDMI Bridge */
 &q6afedai {
 	qi2s@22 {
-- 
2.35.1

[PATCH 5/6] arm64: dts: qcom: sdm845-xiaomi-beryllium enable smb2

[Caleb Connolly]

Enable the smb2 charger driver and add a battery node to report the
battery stats correctly.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 .../boot/dts/qcom/sdm845-xiaomi-beryllium.dts      | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
index b3b6aa4e0fa3..e6170b05a73b 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
@@ -109,6 +109,14 @@ rmtfs_mem: memory@f6301000 {
 		};
 	};
 
+	battery: battery {
+		compatible = "simple-battery";
+
+		charge-full-design-microamp-hours = <4000000>;
+		voltage-min-design-microvolt = <3400000>;
+		voltage-max-design-microvolt = <4400000>;
+	};
+
 	vreg_s4a_1p8: vreg-s4a-1p8 {
 		compatible = "regulator-fixed";
 		regulator-name = "vreg_s4a_1p8";
@@ -302,6 +310,12 @@ vol_up_pin_a: vol-up-active {
 	};
 };
 
+
+&pmi8998_smb2 {
+	status = "okay";
+	monitored-battery = <&battery>;
+};
+
 &pm8998_pon {
 	resin {
 		compatible = "qcom,pm8941-resin";
-- 
2.35.1

[PATCH 6/6] dt-bindings: power: supply: qcom,smb2: add bindings for smb2 driver

[Caleb Connolly]

Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger
drivers.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 .../bindings/power/supply/qcom,smb2.yaml      | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml

diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
new file mode 100644
index 000000000000..1bea1fef78b8
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm PMI8998/PM660 Switch-Mode Battery Charger "2"
+
+maintainers:
+  - Caleb Connolly <caleb.connolly@linaro.org>
+
+properties:
+  compatible:
+    enum:
+      - qcom,pmi8998-smb2
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: usb plugin
+
+  interrupt-names:
+    items:
+      - const: usb-plugin
+
+  io-channels:
+    items:
+      - description: USB in current in uA
+      - description: USB in voltage in uV
+
+  io-channel-names:
+    items:
+      - const: usbin_i
+      - const: usbin_v
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - io-channels
+  - io-channel-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    pmic {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      #interrupt-cells = <4>;
+
+      smb2@1000 {
+        compatible = "qcom,pmi8998-smb2";
+        reg = <0x1000>;
+
+        interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>;
+        interrupt-names = "usb-plugin";
+
+        io-channels = <&pmi8998_rradc 3>,
+                      <&pmi8998_rradc 4>;
+        io-channel-names = "usbin_i",
+                           "usbin_v";
+      };
+    };
-- 
2.35.1

Re: [PATCH 6/6] dt-bindings: power: supply: qcom,smb2: add bindings for smb2 driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2467 bytes --]

Hi,

On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote:
> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger
> drivers.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  .../bindings/power/supply/qcom,smb2.yaml      | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
> new file mode 100644
> index 000000000000..1bea1fef78b8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm PMI8998/PM660 Switch-Mode Battery Charger "2"
> +
> +maintainers:
> +  - Caleb Connolly <caleb.connolly@linaro.org>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,pmi8998-smb2
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: usb plugin
> +
> +  interrupt-names:
> +    items:
> +      - const: usb-plugin
> +
> +  io-channels:
> +    items:
> +      - description: USB in current in uA
> +      - description: USB in voltage in uV
> +
> +  io-channel-names:
> +    items:
> +      - const: usbin_i
> +      - const: usbin_v

Is there a good reason to use usbin_ instead of usb_in_?

-- Sebastian

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - io-channels
> +  - io-channel-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    pmic {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      #interrupt-cells = <4>;
> +
> +      smb2@1000 {
> +        compatible = "qcom,pmi8998-smb2";
> +        reg = <0x1000>;
> +
> +        interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>;
> +        interrupt-names = "usb-plugin";
> +
> +        io-channels = <&pmi8998_rradc 3>,
> +                      <&pmi8998_rradc 4>;
> +        io-channel-names = "usbin_i",
> +                           "usbin_v";
> +      };
> +    };
> -- 
> 2.35.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 6/6] dt-bindings: power: supply: qcom,smb2: add bindings for smb2 driver

[Caleb Connolly]

On 02/04/2022 00:03, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote:
>> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger
>> drivers.
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>>   .../bindings/power/supply/qcom,smb2.yaml      | 68 +++++++++++++++++++
>>   1 file changed, 68 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
>> new file mode 100644
>> index 000000000000..1bea1fef78b8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
>> @@ -0,0 +1,68 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm PMI8998/PM660 Switch-Mode Battery Charger "2"
>> +
>> +maintainers:
>> +  - Caleb Connolly <caleb.connolly@linaro.org>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,pmi8998-smb2
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    items:
>> +      - description: usb plugin
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: usb-plugin
>> +
>> +  io-channels:
>> +    items:
>> +      - description: USB in current in uA
>> +      - description: USB in voltage in uV
>> +
>> +  io-channel-names:
>> +    items:
>> +      - const: usbin_i
>> +      - const: usbin_v
> 
> Is there a good reason to use usbin_ instead of usb_in_?
These are the channel names exposed by the RRADC driver, so they would have to 
be changed there.
> 
> -- Sebastian
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - interrupt-names
>> +  - io-channels
>> +  - io-channel-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    pmic {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +      #interrupt-cells = <4>;
>> +
>> +      smb2@1000 {
>> +        compatible = "qcom,pmi8998-smb2";
>> +        reg = <0x1000>;
>> +
>> +        interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>;
>> +        interrupt-names = "usb-plugin";
>> +
>> +        io-channels = <&pmi8998_rradc 3>,
>> +                      <&pmi8998_rradc 4>;
>> +        io-channel-names = "usbin_i",
>> +                           "usbin_v";
>> +      };
>> +    };
>> -- 
>> 2.35.1
>>

-- 
Kind Regards,
Caleb (they/them)

Re: [PATCH 6/6] dt-bindings: power: supply: qcom,smb2: add bindings for smb2 driver

[Krzysztof Kozlowski]

On 01/04/2022 22:26, Caleb Connolly wrote:
> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger
> drivers.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  .../bindings/power/supply/qcom,smb2.yaml      | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
> new file mode 100644
> index 000000000000..1bea1fef78b8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml#

Hi,

Are you sure "smb2" is a real Qualcomm versioning? IOW, is there going
to be smb3 in the future? If not, better to just name the file according
to model, so like compatible and like other existing schemas from Qualcomm.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm PMI8998/PM660 Switch-Mode Battery Charger "2"
> +
> +maintainers:
> +  - Caleb Connolly <caleb.connolly@linaro.org>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,pmi8998-smb2
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: usb plugin

Just maxItems:1 (description is obvious and matches names).

> +
> +  interrupt-names:
> +    items:
> +      - const: usb-plugin
> +
> +  io-channels:
> +    items:
> +      - description: USB in current in uA
> +      - description: USB in voltage in uV
> +
> +  io-channel-names:
> +    items:
> +      - const: usbin_i
> +      - const: usbin_v
> +

What about monitored-battery? How do you configure the battery
characteristics?

> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - io-channels
> +  - io-channel-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    pmic {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      #interrupt-cells = <4>;
> +
> +      smb2@1000 {

Generic node name please, so "charger".



Best regards,
Krzysztof

Re: [PATCH 6/6] dt-bindings: power: supply: qcom,smb2: add bindings for smb2 driver

[Caleb Connolly]

On 02/04/2022 15:22, Krzysztof Kozlowski wrote:
> On 01/04/2022 22:26, Caleb Connolly wrote:
>> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger
>> drivers.
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>>   .../bindings/power/supply/qcom,smb2.yaml      | 68 +++++++++++++++++++
>>   1 file changed, 68 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
>> new file mode 100644
>> index 000000000000..1bea1fef78b8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
>> @@ -0,0 +1,68 @@
>> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml#
> 
> Hi,
Hi Krzysztof,
> 
> Are you sure "smb2" is a real Qualcomm versioning? IOW, is there going
> to be smb3 in the future? If not, better to just name the file according
> to model, so like compatible and like other existing schemas from Qualcomm.
Qualcomm versioning is a complete mystery to me 😅, downstream kernels have a 
"pmi8998_charger" which uses the qpnp-smb2 driver, there is also an "smb5" 
driver presumably for newer PMICs. This driver is used for the charger block 
found on the PMI8998 and PM660 at least, a name like "pmi8998_charger" might be 
more suitable.

> 
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm PMI8998/PM660 Switch-Mode Battery Charger "2"
>> +
>> +maintainers:
>> +  - Caleb Connolly <caleb.connolly@linaro.org>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,pmi8998-smb2
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    items:
>> +      - description: usb plugin
> 
> Just maxItems:1 (description is obvious and matches names).
> 
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: usb-plugin
>> +
>> +  io-channels:
>> +    items:
>> +      - description: USB in current in uA
>> +      - description: USB in voltage in uV
>> +
>> +  io-channel-names:
>> +    items:
>> +      - const: usbin_i
>> +      - const: usbin_v
>> +
> 
> What about monitored-battery? How do you configure the battery
> characteristics?
> 
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - interrupt-names
>> +  - io-channels
>> +  - io-channel-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    pmic {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +      #interrupt-cells = <4>;
>> +
>> +      smb2@1000 {
> 
> Generic node name please, so "charger".
> 
> 
> 
> Best regards,
> Krzysztof

-- 
Kind Regards,
Caleb (they/them)

Re: [PATCH 6/6] dt-bindings: power: supply: qcom,smb2: add bindings for smb2 driver

[Kuldeep Singh]

On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote:
> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger
> drivers.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  .../bindings/power/supply/qcom,smb2.yaml      | 68 +++++++++++++++++++
>  1 file changed, 68 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
> new file mode 100644
> index 000000000000..1bea1fef78b8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml
> @@ -0,0 +1,68 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm PMI8998/PM660 Switch-Mode Battery Charger "2"
> +
> +maintainers:
> +  - Caleb Connolly <caleb.connolly@linaro.org>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,pmi8998-smb2

Since there's only 1 entry, please use const.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    items:
> +      - description: usb plugin
> +
> +  interrupt-names:
> +    items:
> +      - const: usb-plugin
> +
> +  io-channels:
> +    items:
> +      - description: USB in current in uA
> +      - description: USB in voltage in uV
> +
> +  io-channel-names:
> +    items:
> +      - const: usbin_i
> +      - const: usbin_v
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - io-channels
> +  - io-channel-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>

New line here. Looks nice.

> +    pmic {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      #interrupt-cells = <4>;
> +
> +      smb2@1000 {
> +        compatible = "qcom,pmi8998-smb2";
> +        reg = <0x1000>;
> +
> +        interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>;
> +        interrupt-names = "usb-plugin";
> +

In-between new lines may not be required.
And DTs use tabs instead of 2 spaces, we can follow that here also.

> +        io-channels = <&pmi8998_rradc 3>,
> +                      <&pmi8998_rradc 4>;
> +        io-channel-names = "usbin_i",
> +                           "usbin_v";

Channel-names can be written in one line.

> +      };
> +    };
> -- 
> 2.35.1
> 

Re: [PATCH 6/6] dt-bindings: power: supply: qcom,smb2: add bindings for smb2 driver

[Krzysztof Kozlowski]

On 03/04/2022 09:14, Kuldeep Singh wrote:
> On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote:
>> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger
>> drivers.
>>

(...)

> 
>> +    pmic {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +      #interrupt-cells = <4>;
>> +
>> +      smb2@1000 {
>> +        compatible = "qcom,pmi8998-smb2";
>> +        reg = <0x1000>;
>> +
>> +        interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>;
>> +        interrupt-names = "usb-plugin";
>> +
> 
> In-between new lines may not be required.
> And DTs use tabs instead of 2 spaces, we can follow that here also.

The DT examples in bindings use spaces. Either two (like YAML) or four
(for easier reading).

> 
>> +        io-channels = <&pmi8998_rradc 3>,
>> +                      <&pmi8998_rradc 4>;
>> +        io-channel-names = "usbin_i",
>> +                           "usbin_v";
> 
> Channel-names can be written in one line.

They match the format of io-channels, so this is quite readable.



Best regards,
Krzysztof

Re: [PATCH 6/6] dt-bindings: power: supply: qcom,smb2: add bindings for smb2 driver

[Kuldeep Singh]

On Sun, Apr 03, 2022 at 09:56:25AM +0200, Krzysztof Kozlowski wrote:
> On 03/04/2022 09:14, Kuldeep Singh wrote:
> > On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote:
> >> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger
> >> drivers.
> >>
> 
> (...)
> 
> > 
> >> +    pmic {
> >> +      #address-cells = <1>;
> >> +      #size-cells = <0>;
> >> +      #interrupt-cells = <4>;
> >> +
> >> +      smb2@1000 {
> >> +        compatible = "qcom,pmi8998-smb2";
> >> +        reg = <0x1000>;
> >> +
> >> +        interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>;
> >> +        interrupt-names = "usb-plugin";
> >> +
> > 
> > In-between new lines may not be required.
> > And DTs use tabs instead of 2 spaces, we can follow that here also.
> 
> The DT examples in bindings use spaces. Either two (like YAML) or four
> (for easier reading).

ok, since example snippet is taken from DT that's why I said four
spaces(tab) as it will be closest to actual env.

> 
> > 
> >> +        io-channels = <&pmi8998_rradc 3>,
> >> +                      <&pmi8998_rradc 4>;
> >> +        io-channel-names = "usbin_i",
> >> +                           "usbin_v";
> > 
> > Channel-names can be written in one line.
> 
> They match the format of io-channels, so this is quite readable.

io-channels doesn't exceed max characters in line(i.e 75) even after
being clubbed. Won't be better if kept in one line?
This might be personal perspective but I thought it's worth mentioning.

Re: [PATCH 6/6] dt-bindings: power: supply: qcom,smb2: add bindings for smb2 driver

[Krzysztof Kozlowski]

On 03/04/2022 15:31, Kuldeep Singh wrote:
> On Sun, Apr 03, 2022 at 09:56:25AM +0200, Krzysztof Kozlowski wrote:
>> On 03/04/2022 09:14, Kuldeep Singh wrote:
>>> On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote:
>>>> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger
>>>> drivers.
>>>>
>>
>> (...)
>>
>>>
>>>> +    pmic {
>>>> +      #address-cells = <1>;
>>>> +      #size-cells = <0>;
>>>> +      #interrupt-cells = <4>;
>>>> +
>>>> +      smb2@1000 {
>>>> +        compatible = "qcom,pmi8998-smb2";
>>>> +        reg = <0x1000>;
>>>> +
>>>> +        interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>;
>>>> +        interrupt-names = "usb-plugin";
>>>> +
>>>
>>> In-between new lines may not be required.
>>> And DTs use tabs instead of 2 spaces, we can follow that here also.
>>
>> The DT examples in bindings use spaces. Either two (like YAML) or four
>> (for easier reading).
> 
> ok, since example snippet is taken from DT that's why I said four
> spaces(tab) as it will be closest to actual env.

You said "use tabs", which is 8 spaces in Linux. So to clarify - we do
not use tabs here, so do not use tabs.

>>>> +        io-channels = <&pmi8998_rradc 3>,
>>>> +                      <&pmi8998_rradc 4>;
>>>> +        io-channel-names = "usbin_i",
>>>> +                           "usbin_v";
>>>
>>> Channel-names can be written in one line.
>>
>> They match the format of io-channels, so this is quite readable.
> 
> io-channels doesn't exceed max characters in line(i.e 75) even after
> being clubbed. Won't be better if kept in one line?
> This might be personal perspective but I thought it's worth mentioning.

I find current code readable. The other option would be fine as well,
kind of does not matter to me much.

Best regards,
Krzysztof