[PATCH v3 0/3] extcon: sm5502: Add support for SM5504

[PATCH v3 0/3] extcon: sm5502: Add support for SM5504

[Stephan Gerhold]

This patch series adds support for SM5504 to the existing extcon-sm5502
driver. SM5502 and SM5504 are fairly similar so support for SM5504 can
be added with a few simple if statements in the code.

I also put a few cleanup patches in front and convert the device tree
bindings to DT schema.

I tested this patch series on both SM5502 (Samsung Galaxy A5 2015)
and SM5504 (Samsung Galaxy S4 Mini Value Edition) and it seems to work
just fine for both.

---
Changes in v3:
  - Drop patch 1-4 (already applied)
  - Avoid if (type == TYPE_SM5504) everywhere in the code, instead
    introduce a struct sm5502_type that encodes chip-specific information.

v2: https://lore.kernel.org/lkml/20210531133438.3511-1-stephan@gerhold.net/
Changes in v2: Fix compile warning in last patch
v1: https://lore.kernel.org/lkml/20210520112334.129556-1-stephan@gerhold.net/

Stephan Gerhold (3):
  dt-bindings: extcon: sm5502: Document siliconmitus,sm5504-muic
  extcon: sm5502: Refactor driver to use chip-specific struct
  extcon: sm5502: Add support for SM5504

 .../extcon/siliconmitus,sm5502-muic.yaml      |   6 +-
 drivers/extcon/Kconfig                        |   2 +-
 drivers/extcon/extcon-sm5502.c                | 196 +++++++++++++++---
 drivers/extcon/extcon-sm5502.h                |  82 +++++++-
 4 files changed, 248 insertions(+), 38 deletions(-)

-- 
2.31.1

[PATCH v3 1/3] dt-bindings: extcon: sm5502: Document siliconmitus,sm5504-muic

[Stephan Gerhold]

Document support for SM5504 with the new siliconmitus,sm5504-muic
compatible.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 .../bindings/extcon/siliconmitus,sm5502-muic.yaml           | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/extcon/siliconmitus,sm5502-muic.yaml b/Documentation/devicetree/bindings/extcon/siliconmitus,sm5502-muic.yaml
index 0432b0502e0b..fd2e55088888 100644
--- a/Documentation/devicetree/bindings/extcon/siliconmitus,sm5502-muic.yaml
+++ b/Documentation/devicetree/bindings/extcon/siliconmitus,sm5502-muic.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/extcon/siliconmitus,sm5502-muic.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: SM5502 MUIC (Micro-USB Interface Controller) device
+title: SM5502/SM5504 MUIC (Micro-USB Interface Controller) device
 
 maintainers:
   - Chanwoo Choi <cw00.choi@samsung.com>
@@ -19,10 +19,12 @@ properties:
   compatible:
     enum:
       - siliconmitus,sm5502-muic
+      - siliconmitus,sm5504-muic
 
   reg:
     maxItems: 1
-    description: I2C slave address of the device. Usually 0x25 for SM5502.
+    description: I2C slave address of the device. Usually 0x25 for SM5502,
+      0x14 for SM5504.
 
   interrupts:
     maxItems: 1
-- 
2.31.1

[PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct

[Stephan Gerhold]

Prepare for supporting SM5504 in the extcon-sm5502 driver by replacing
enum sm5504_types with a struct sm5504_type that stores the chip-specific
definitions. This struct can then be defined separately for SM5504
without having to add if (type == TYPE_SM5504) everywhere in the code.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Changes in v3: New patch to simplify diff on next patch
---
 drivers/extcon/extcon-sm5502.c | 64 +++++++++++++++++++++-------------
 drivers/extcon/extcon-sm5502.h |  4 ---
 2 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
index 9f40bb9f1f81..951f6ca4c479 100644
--- a/drivers/extcon/extcon-sm5502.c
+++ b/drivers/extcon/extcon-sm5502.c
@@ -40,17 +40,13 @@ struct sm5502_muic_info {
 	struct i2c_client *i2c;
 	struct regmap *regmap;
 
+	const struct sm5502_type *type;
 	struct regmap_irq_chip_data *irq_data;
-	struct muic_irq *muic_irqs;
-	unsigned int num_muic_irqs;
 	int irq;
 	bool irq_attach;
 	bool irq_detach;
 	struct work_struct irq_work;
 
-	struct reg_data *reg_data;
-	unsigned int num_reg_data;
-
 	struct mutex mutex;
 
 	/*
@@ -62,6 +58,17 @@ struct sm5502_muic_info {
 	struct delayed_work wq_detcable;
 };
 
+struct sm5502_type {
+	struct muic_irq *muic_irqs;
+	unsigned int num_muic_irqs;
+	const struct regmap_irq_chip *irq_chip;
+
+	struct reg_data *reg_data;
+	unsigned int num_reg_data;
+
+	int (*parse_irq)(struct sm5502_muic_info *info, int irq_type);
+};
+
 /* Default value of SM5502 register to bring up MUIC device. */
 static struct reg_data sm5502_reg_data[] = {
 	{
@@ -502,11 +509,11 @@ static irqreturn_t sm5502_muic_irq_handler(int irq, void *data)
 	struct sm5502_muic_info *info = data;
 	int i, irq_type = -1, ret;
 
-	for (i = 0; i < info->num_muic_irqs; i++)
-		if (irq == info->muic_irqs[i].virq)
-			irq_type = info->muic_irqs[i].irq;
+	for (i = 0; i < info->type->num_muic_irqs; i++)
+		if (irq == info->type->muic_irqs[i].virq)
+			irq_type = info->type->muic_irqs[i].irq;
 
-	ret = sm5502_parse_irq(info, irq_type);
+	ret = info->type->parse_irq(info, irq_type);
 	if (ret < 0) {
 		dev_warn(info->dev, "cannot handle is interrupt:%d\n",
 				    irq_type);
@@ -551,14 +558,14 @@ static void sm5502_init_dev_type(struct sm5502_muic_info *info)
 			    version_id, vendor_id);
 
 	/* Initiazle the register of SM5502 device to bring-up */
-	for (i = 0; i < info->num_reg_data; i++) {
+	for (i = 0; i < info->type->num_reg_data; i++) {
 		unsigned int val = 0;
 
-		if (!info->reg_data[i].invert)
-			val |= ~info->reg_data[i].val;
+		if (!info->type->reg_data[i].invert)
+			val |= ~info->type->reg_data[i].val;
 		else
-			val = info->reg_data[i].val;
-		regmap_write(info->regmap, info->reg_data[i].reg, val);
+			val = info->type->reg_data[i].val;
+		regmap_write(info->regmap, info->type->reg_data[i].reg, val);
 	}
 }
 
@@ -579,10 +586,9 @@ static int sm5022_muic_i2c_probe(struct i2c_client *i2c)
 	info->dev = &i2c->dev;
 	info->i2c = i2c;
 	info->irq = i2c->irq;
-	info->muic_irqs = sm5502_muic_irqs;
-	info->num_muic_irqs = ARRAY_SIZE(sm5502_muic_irqs);
-	info->reg_data = sm5502_reg_data;
-	info->num_reg_data = ARRAY_SIZE(sm5502_reg_data);
+	info->type = device_get_match_data(info->dev);
+	if (!info->type)
+		return -EINVAL;
 
 	mutex_init(&info->mutex);
 
@@ -598,16 +604,17 @@ static int sm5022_muic_i2c_probe(struct i2c_client *i2c)
 
 	/* Support irq domain for SM5502 MUIC device */
 	irq_flags = IRQF_TRIGGER_FALLING | IRQF_ONESHOT | IRQF_SHARED;
-	ret = devm_regmap_add_irq_chip(info->dev, info->regmap, info->irq, irq_flags,
-				       0, &sm5502_muic_irq_chip, &info->irq_data);
+	ret = devm_regmap_add_irq_chip(info->dev, info->regmap, info->irq,
+				       irq_flags, 0, info->type->irq_chip,
+				       &info->irq_data);
 	if (ret != 0) {
 		dev_err(info->dev, "failed to request IRQ %d: %d\n",
 				    info->irq, ret);
 		return ret;
 	}
 
-	for (i = 0; i < info->num_muic_irqs; i++) {
-		struct muic_irq *muic_irq = &info->muic_irqs[i];
+	for (i = 0; i < info->type->num_muic_irqs; i++) {
+		struct muic_irq *muic_irq = &info->type->muic_irqs[i];
 		int virq = 0;
 
 		virq = regmap_irq_get_virq(info->irq_data, muic_irq->irq);
@@ -659,8 +666,17 @@ static int sm5022_muic_i2c_probe(struct i2c_client *i2c)
 	return 0;
 }
 
+static const struct sm5502_type sm5502_data = {
+	.muic_irqs = sm5502_muic_irqs,
+	.num_muic_irqs = ARRAY_SIZE(sm5502_muic_irqs),
+	.irq_chip = &sm5502_muic_irq_chip,
+	.reg_data = sm5502_reg_data,
+	.num_reg_data = ARRAY_SIZE(sm5502_reg_data),
+	.parse_irq = sm5502_parse_irq,
+};
+
 static const struct of_device_id sm5502_dt_match[] = {
-	{ .compatible = "siliconmitus,sm5502-muic" },
+	{ .compatible = "siliconmitus,sm5502-muic", .data = &sm5502_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, sm5502_dt_match);
@@ -691,7 +707,7 @@ static SIMPLE_DEV_PM_OPS(sm5502_muic_pm_ops,
 			 sm5502_muic_suspend, sm5502_muic_resume);
 
 static const struct i2c_device_id sm5502_i2c_id[] = {
-	{ "sm5502", TYPE_SM5502 },
+	{ "sm5502", (kernel_ulong_t)&sm5502_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, sm5502_i2c_id);
diff --git a/drivers/extcon/extcon-sm5502.h b/drivers/extcon/extcon-sm5502.h
index ce1f1ec310c4..d187205df7b3 100644
--- a/drivers/extcon/extcon-sm5502.h
+++ b/drivers/extcon/extcon-sm5502.h
@@ -8,10 +8,6 @@
 #ifndef __LINUX_EXTCON_SM5502_H
 #define __LINUX_EXTCON_SM5502_H
 
-enum sm5502_types {
-	TYPE_SM5502,
-};
-
 /* SM5502 registers */
 enum sm5502_reg {
 	SM5502_REG_DEVICE_ID = 0x01,
-- 
2.31.1

[PATCH v3 3/3] extcon: sm5502: Add support for SM5504

[Stephan Gerhold]

SM5504 is another MUIC from Silicon Mitus that is fairly similar
to SM5502. They seem to use the same register set, but:

  - SM5504 has some additional bits in SM5502_REG_CONTROL
  - SM5504 has a quite different set of interrupts
  - SM5504 reports USB OTG as dev_type1 = BIT(0) instead of BIT(7)

Overall it's minor and we can support this by defining a separate
struct sm5502_type for SM5504.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Changes in v3: Avoid if statements, use struct sm5502_type indirection

Changes in v2: Fix warning: cast to smaller integer type 'enum sm5502_types'
               from 'const void *' [-Wvoid-pointer-to-enum-cast]
               reported by kernel test robot
v2: https://lore.kernel.org/lkml/20210531133438.3511-7-stephan@gerhold.net/

Interestingly enough the register set (and especially the interrupts)
also look *very* similar to Richtek RT8973A (extcon-rt8973a) but
I didn't investigate this further.

Note that the changes in this patch are mostly based on guesswork
based on a SM5504 driver from Nitin Chaudhary [1] used in some Samsung
vendor kernels, since I was not able to find a public datasheet for SM5504.

[1]: https://github.com/NitinChaudharyUSC/MSM8x16_8x26/blob/master/drivers/misc/sm5504.c
---
 drivers/extcon/Kconfig         |   2 +-
 drivers/extcon/extcon-sm5502.c | 132 +++++++++++++++++++++++++++++++--
 drivers/extcon/extcon-sm5502.h |  78 +++++++++++++++++++
 3 files changed, 204 insertions(+), 8 deletions(-)

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index e3db936becfd..c69d40ae5619 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -154,7 +154,7 @@ config EXTCON_RT8973A
 	  from abnormal high input voltage (up to 28V).
 
 config EXTCON_SM5502
-	tristate "Silicon Mitus SM5502 EXTCON support"
+	tristate "Silicon Mitus SM5502/SM5504 EXTCON support"
 	depends on I2C
 	select IRQ_DOMAIN
 	select REGMAP_I2C
diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
index 951f6ca4c479..af44c1e2f368 100644
--- a/drivers/extcon/extcon-sm5502.c
+++ b/drivers/extcon/extcon-sm5502.c
@@ -66,6 +66,7 @@ struct sm5502_type {
 	struct reg_data *reg_data;
 	unsigned int num_reg_data;
 
+	unsigned int otg_dev_type1;
 	int (*parse_irq)(struct sm5502_muic_info *info, int irq_type);
 };
 
@@ -97,6 +98,33 @@ static struct reg_data sm5502_reg_data[] = {
 	},
 };
 
+/* Default value of SM5504 register to bring up MUIC device. */
+static struct reg_data sm5504_reg_data[] = {
+	{
+		.reg = SM5502_REG_RESET,
+		.val = SM5502_REG_RESET_MASK,
+		.invert = true,
+	}, {
+		.reg = SM5502_REG_INTMASK1,
+		.val = SM5504_REG_INTM1_ATTACH_MASK
+			| SM5504_REG_INTM1_DETACH_MASK,
+		.invert = false,
+	}, {
+		.reg = SM5502_REG_INTMASK2,
+		.val = SM5504_REG_INTM2_RID_CHG_MASK
+			| SM5504_REG_INTM2_UVLO_MASK
+			| SM5504_REG_INTM2_POR_MASK,
+		.invert = true,
+	}, {
+		.reg = SM5502_REG_CONTROL,
+		.val = SM5502_REG_CONTROL_MANUAL_SW_MASK
+			| SM5504_REG_CONTROL_CHGTYP_MASK
+			| SM5504_REG_CONTROL_USBCHDEN_MASK
+			| SM5504_REG_CONTROL_ADC_EN_MASK,
+		.invert = true,
+	},
+};
+
 /* List of detectable cables */
 static const unsigned int sm5502_extcon_cable[] = {
 	EXTCON_USB,
@@ -205,6 +233,55 @@ static const struct regmap_irq_chip sm5502_muic_irq_chip = {
 	.num_irqs		= ARRAY_SIZE(sm5502_irqs),
 };
 
+/* List of supported interrupt for SM5504 */
+static struct muic_irq sm5504_muic_irqs[] = {
+	{ SM5504_IRQ_INT1_ATTACH,	"muic-attach" },
+	{ SM5504_IRQ_INT1_DETACH,	"muic-detach" },
+	{ SM5504_IRQ_INT1_CHG_DET,	"muic-chg-det" },
+	{ SM5504_IRQ_INT1_DCD_OUT,	"muic-dcd-out" },
+	{ SM5504_IRQ_INT1_OVP_EVENT,	"muic-ovp-event" },
+	{ SM5504_IRQ_INT1_CONNECT,	"muic-connect" },
+	{ SM5504_IRQ_INT1_ADC_CHG,	"muic-adc-chg" },
+	{ SM5504_IRQ_INT2_RID_CHG,	"muic-rid-chg" },
+	{ SM5504_IRQ_INT2_UVLO,		"muic-uvlo" },
+	{ SM5504_IRQ_INT2_POR,		"muic-por" },
+	{ SM5504_IRQ_INT2_OVP_FET,	"muic-ovp-fet" },
+	{ SM5504_IRQ_INT2_OCP_LATCH,	"muic-ocp-latch" },
+	{ SM5504_IRQ_INT2_OCP_EVENT,	"muic-ocp-event" },
+	{ SM5504_IRQ_INT2_OVP_OCP_EVENT, "muic-ovp-ocp-event" },
+};
+
+/* Define interrupt list of SM5504 to register regmap_irq */
+static const struct regmap_irq sm5504_irqs[] = {
+	/* INT1 interrupts */
+	{ .reg_offset = 0, .mask = SM5504_IRQ_INT1_ATTACH_MASK, },
+	{ .reg_offset = 0, .mask = SM5504_IRQ_INT1_DETACH_MASK, },
+	{ .reg_offset = 0, .mask = SM5504_IRQ_INT1_CHG_DET_MASK, },
+	{ .reg_offset = 0, .mask = SM5504_IRQ_INT1_DCD_OUT_MASK, },
+	{ .reg_offset = 0, .mask = SM5504_IRQ_INT1_OVP_MASK, },
+	{ .reg_offset = 0, .mask = SM5504_IRQ_INT1_CONNECT_MASK, },
+	{ .reg_offset = 0, .mask = SM5504_IRQ_INT1_ADC_CHG_MASK, },
+
+	/* INT2 interrupts */
+	{ .reg_offset = 1, .mask = SM5504_IRQ_INT2_RID_CHG_MASK,},
+	{ .reg_offset = 1, .mask = SM5504_IRQ_INT2_UVLO_MASK, },
+	{ .reg_offset = 1, .mask = SM5504_IRQ_INT2_POR_MASK, },
+	{ .reg_offset = 1, .mask = SM5504_IRQ_INT2_OVP_FET_MASK, },
+	{ .reg_offset = 1, .mask = SM5504_IRQ_INT2_OCP_LATCH_MASK, },
+	{ .reg_offset = 1, .mask = SM5504_IRQ_INT2_OCP_EVENT_MASK, },
+	{ .reg_offset = 1, .mask = SM5504_IRQ_INT2_OVP_OCP_EVENT_MASK, },
+};
+
+static const struct regmap_irq_chip sm5504_muic_irq_chip = {
+	.name			= "sm5504",
+	.status_base		= SM5502_REG_INT1,
+	.mask_base		= SM5502_REG_INTMASK1,
+	.mask_invert		= false,
+	.num_regs		= 2,
+	.irqs			= sm5504_irqs,
+	.num_irqs		= ARRAY_SIZE(sm5504_irqs),
+};
+
 /* Define regmap configuration of SM5502 for I2C communication  */
 static bool sm5502_muic_volatile_reg(struct device *dev, unsigned int reg)
 {
@@ -308,11 +385,9 @@ static unsigned int sm5502_muic_get_cable_type(struct sm5502_muic_info *info)
 			return ret;
 		}
 
-		switch (dev_type1) {
-		case SM5502_REG_DEV_TYPE1_USB_OTG_MASK:
+		if (dev_type1 == info->type->otg_dev_type1) {
 			cable_type = SM5502_MUIC_ADC_GROUND_USB_OTG;
-			break;
-		default:
+		} else {
 			dev_dbg(info->dev,
 				"cannot identify the cable type: adc(0x%x), dev_type1(0x%x)\n",
 				adc, dev_type1);
@@ -365,6 +440,11 @@ static unsigned int sm5502_muic_get_cable_type(struct sm5502_muic_info *info)
 			return ret;
 		}
 
+		if (dev_type1 == info->type->otg_dev_type1) {
+			cable_type = SM5502_MUIC_ADC_OPEN_USB_OTG;
+			break;
+		}
+
 		switch (dev_type1) {
 		case SM5502_REG_DEV_TYPE1_USB_SDP_MASK:
 			cable_type = SM5502_MUIC_ADC_OPEN_USB;
@@ -372,9 +452,6 @@ static unsigned int sm5502_muic_get_cable_type(struct sm5502_muic_info *info)
 		case SM5502_REG_DEV_TYPE1_DEDICATED_CHG_MASK:
 			cable_type = SM5502_MUIC_ADC_OPEN_TA;
 			break;
-		case SM5502_REG_DEV_TYPE1_USB_OTG_MASK:
-			cable_type = SM5502_MUIC_ADC_OPEN_USB_OTG;
-			break;
 		default:
 			dev_dbg(info->dev,
 				"cannot identify the cable type: adc(0x%x)\n",
@@ -504,6 +581,34 @@ static int sm5502_parse_irq(struct sm5502_muic_info *info, int irq_type)
 	return 0;
 }
 
+static int sm5504_parse_irq(struct sm5502_muic_info *info, int irq_type)
+{
+	switch (irq_type) {
+	case SM5504_IRQ_INT1_ATTACH:
+		info->irq_attach = true;
+		break;
+	case SM5504_IRQ_INT1_DETACH:
+		info->irq_detach = true;
+		break;
+	case SM5504_IRQ_INT1_CHG_DET:
+	case SM5504_IRQ_INT1_DCD_OUT:
+	case SM5504_IRQ_INT1_OVP_EVENT:
+	case SM5504_IRQ_INT1_CONNECT:
+	case SM5504_IRQ_INT1_ADC_CHG:
+	case SM5504_IRQ_INT2_RID_CHG:
+	case SM5504_IRQ_INT2_UVLO:
+	case SM5504_IRQ_INT2_POR:
+	case SM5504_IRQ_INT2_OVP_FET:
+	case SM5504_IRQ_INT2_OCP_LATCH:
+	case SM5504_IRQ_INT2_OCP_EVENT:
+	case SM5504_IRQ_INT2_OVP_OCP_EVENT:
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static irqreturn_t sm5502_muic_irq_handler(int irq, void *data)
 {
 	struct sm5502_muic_info *info = data;
@@ -672,11 +777,23 @@ static const struct sm5502_type sm5502_data = {
 	.irq_chip = &sm5502_muic_irq_chip,
 	.reg_data = sm5502_reg_data,
 	.num_reg_data = ARRAY_SIZE(sm5502_reg_data),
+	.otg_dev_type1 = SM5502_REG_DEV_TYPE1_USB_OTG_MASK,
 	.parse_irq = sm5502_parse_irq,
 };
 
+static const struct sm5502_type sm5504_data = {
+	.muic_irqs = sm5504_muic_irqs,
+	.num_muic_irqs = ARRAY_SIZE(sm5504_muic_irqs),
+	.irq_chip = &sm5504_muic_irq_chip,
+	.reg_data = sm5504_reg_data,
+	.num_reg_data = ARRAY_SIZE(sm5504_reg_data),
+	.otg_dev_type1 = SM5504_REG_DEV_TYPE1_USB_OTG_MASK,
+	.parse_irq = sm5504_parse_irq,
+};
+
 static const struct of_device_id sm5502_dt_match[] = {
 	{ .compatible = "siliconmitus,sm5502-muic", .data = &sm5502_data },
+	{ .compatible = "siliconmitus,sm5504-muic", .data = &sm5504_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, sm5502_dt_match);
@@ -708,6 +825,7 @@ static SIMPLE_DEV_PM_OPS(sm5502_muic_pm_ops,
 
 static const struct i2c_device_id sm5502_i2c_id[] = {
 	{ "sm5502", (kernel_ulong_t)&sm5502_data },
+	{ "sm5504", (kernel_ulong_t)&sm5504_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, sm5502_i2c_id);
diff --git a/drivers/extcon/extcon-sm5502.h b/drivers/extcon/extcon-sm5502.h
index d187205df7b3..9c04315d48e2 100644
--- a/drivers/extcon/extcon-sm5502.h
+++ b/drivers/extcon/extcon-sm5502.h
@@ -89,6 +89,13 @@ enum sm5502_reg {
 #define SM5502_REG_CONTROL_RAW_DATA_MASK	(0x1 << SM5502_REG_CONTROL_RAW_DATA_SHIFT)
 #define SM5502_REG_CONTROL_SW_OPEN_MASK		(0x1 << SM5502_REG_CONTROL_SW_OPEN_SHIFT)
 
+#define SM5504_REG_CONTROL_CHGTYP_SHIFT		5
+#define SM5504_REG_CONTROL_USBCHDEN_SHIFT	6
+#define SM5504_REG_CONTROL_ADC_EN_SHIFT		7
+#define SM5504_REG_CONTROL_CHGTYP_MASK		(0x1 << SM5504_REG_CONTROL_CHGTYP_SHIFT)
+#define SM5504_REG_CONTROL_USBCHDEN_MASK	(0x1 << SM5504_REG_CONTROL_USBCHDEN_SHIFT)
+#define SM5504_REG_CONTROL_ADC_EN_MASK		(0x1 << SM5504_REG_CONTROL_ADC_EN_SHIFT)
+
 #define SM5502_REG_INTM1_ATTACH_SHIFT		0
 #define SM5502_REG_INTM1_DETACH_SHIFT		1
 #define SM5502_REG_INTM1_KP_SHIFT		2
@@ -119,6 +126,36 @@ enum sm5502_reg {
 #define SM5502_REG_INTM2_STUCK_KEY_RCV_MASK	(0x1 << SM5502_REG_INTM2_STUCK_KEY_RCV_SHIFT)
 #define SM5502_REG_INTM2_MHL_MASK		(0x1 << SM5502_REG_INTM2_MHL_SHIFT)
 
+#define SM5504_REG_INTM1_ATTACH_SHIFT		0
+#define SM5504_REG_INTM1_DETACH_SHIFT		1
+#define SM5504_REG_INTM1_CHG_DET_SHIFT		2
+#define SM5504_REG_INTM1_DCD_OUT_SHIFT		3
+#define SM5504_REG_INTM1_OVP_EVENT_SHIFT	4
+#define SM5504_REG_INTM1_CONNECT_SHIFT		5
+#define SM5504_REG_INTM1_ADC_CHG_SHIFT		6
+#define SM5504_REG_INTM1_ATTACH_MASK		(0x1 << SM5504_REG_INTM1_ATTACH_SHIFT)
+#define SM5504_REG_INTM1_DETACH_MASK		(0x1 << SM5504_REG_INTM1_DETACH_SHIFT)
+#define SM5504_REG_INTM1_CHG_DET_MASK		(0x1 << SM5504_REG_INTM1_CHG_DET_SHIFT)
+#define SM5504_REG_INTM1_DCD_OUT_MASK		(0x1 << SM5504_REG_INTM1_DCD_OUT_SHIFT)
+#define SM5504_REG_INTM1_OVP_EVENT_MASK		(0x1 << SM5504_REG_INTM1_OVP_EVENT_SHIFT)
+#define SM5504_REG_INTM1_CONNECT_MASK		(0x1 << SM5504_REG_INTM1_CONNECT_SHIFT)
+#define SM5504_REG_INTM1_ADC_CHG_MASK		(0x1 << SM5504_REG_INTM1_ADC_CHG_SHIFT)
+
+#define SM5504_REG_INTM2_RID_CHG_SHIFT		0
+#define SM5504_REG_INTM2_UVLO_SHIFT		1
+#define SM5504_REG_INTM2_POR_SHIFT		2
+#define SM5504_REG_INTM2_OVP_FET_SHIFT		4
+#define SM5504_REG_INTM2_OCP_LATCH_SHIFT	5
+#define SM5504_REG_INTM2_OCP_EVENT_SHIFT	6
+#define SM5504_REG_INTM2_OVP_OCP_EVENT_SHIFT	7
+#define SM5504_REG_INTM2_RID_CHG_MASK		(0x1 << SM5504_REG_INTM2_RID_CHG_SHIFT)
+#define SM5504_REG_INTM2_UVLO_MASK		(0x1 << SM5504_REG_INTM2_UVLO_SHIFT)
+#define SM5504_REG_INTM2_POR_MASK		(0x1 << SM5504_REG_INTM2_POR_SHIFT)
+#define SM5504_REG_INTM2_OVP_FET_MASK		(0x1 << SM5504_REG_INTM2_OVP_FET_SHIFT)
+#define SM5504_REG_INTM2_OCP_LATCH_MASK		(0x1 << SM5504_REG_INTM2_OCP_LATCH_SHIFT)
+#define SM5504_REG_INTM2_OCP_EVENT_MASK		(0x1 << SM5504_REG_INTM2_OCP_EVENT_SHIFT)
+#define SM5504_REG_INTM2_OVP_OCP_EVENT_MASK	(0x1 << SM5504_REG_INTM2_OVP_OCP_EVENT_SHIFT)
+
 #define SM5502_REG_ADC_SHIFT			0
 #define SM5502_REG_ADC_MASK			(0x1f << SM5502_REG_ADC_SHIFT)
 
@@ -195,6 +232,9 @@ enum sm5502_reg {
 #define SM5502_REG_DEV_TYPE1_DEDICATED_CHG_MASK		(0x1 << SM5502_REG_DEV_TYPE1_DEDICATED_CHG_SHIFT)
 #define SM5502_REG_DEV_TYPE1_USB_OTG_MASK		(0x1 << SM5502_REG_DEV_TYPE1_USB_OTG_SHIFT)
 
+#define SM5504_REG_DEV_TYPE1_USB_OTG_SHIFT		0
+#define SM5504_REG_DEV_TYPE1_USB_OTG_MASK		(0x1 << SM5504_REG_DEV_TYPE1_USB_OTG_SHIFT)
+
 #define SM5502_REG_DEV_TYPE2_JIG_USB_ON_SHIFT		0
 #define SM5502_REG_DEV_TYPE2_JIG_USB_OFF_SHIFT		1
 #define SM5502_REG_DEV_TYPE2_JIG_UART_ON_SHIFT		2
@@ -273,4 +313,42 @@ enum sm5502_irq {
 #define SM5502_IRQ_INT2_STUCK_KEY_RCV_MASK	BIT(4)
 #define SM5502_IRQ_INT2_MHL_MASK		BIT(5)
 
+/* SM5504 Interrupts */
+enum sm5504_irq {
+	/* INT1 */
+	SM5504_IRQ_INT1_ATTACH,
+	SM5504_IRQ_INT1_DETACH,
+	SM5504_IRQ_INT1_CHG_DET,
+	SM5504_IRQ_INT1_DCD_OUT,
+	SM5504_IRQ_INT1_OVP_EVENT,
+	SM5504_IRQ_INT1_CONNECT,
+	SM5504_IRQ_INT1_ADC_CHG,
+
+	/* INT2 */
+	SM5504_IRQ_INT2_RID_CHG,
+	SM5504_IRQ_INT2_UVLO,
+	SM5504_IRQ_INT2_POR,
+	SM5504_IRQ_INT2_OVP_FET,
+	SM5504_IRQ_INT2_OCP_LATCH,
+	SM5504_IRQ_INT2_OCP_EVENT,
+	SM5504_IRQ_INT2_OVP_OCP_EVENT,
+
+	SM5504_IRQ_NUM,
+};
+
+#define SM5504_IRQ_INT1_ATTACH_MASK		BIT(0)
+#define SM5504_IRQ_INT1_DETACH_MASK		BIT(1)
+#define SM5504_IRQ_INT1_CHG_DET_MASK		BIT(2)
+#define SM5504_IRQ_INT1_DCD_OUT_MASK		BIT(3)
+#define SM5504_IRQ_INT1_OVP_MASK		BIT(4)
+#define SM5504_IRQ_INT1_CONNECT_MASK		BIT(5)
+#define SM5504_IRQ_INT1_ADC_CHG_MASK		BIT(6)
+#define SM5504_IRQ_INT2_RID_CHG_MASK		BIT(0)
+#define SM5504_IRQ_INT2_UVLO_MASK		BIT(1)
+#define SM5504_IRQ_INT2_POR_MASK		BIT(2)
+#define SM5504_IRQ_INT2_OVP_FET_MASK		BIT(4)
+#define SM5504_IRQ_INT2_OCP_LATCH_MASK		BIT(5)
+#define SM5504_IRQ_INT2_OCP_EVENT_MASK		BIT(6)
+#define SM5504_IRQ_INT2_OVP_OCP_EVENT_MASK	BIT(7)
+
 #endif /*  __LINUX_EXTCON_SM5502_H */
-- 
2.31.1

Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct

[Chanwoo Choi]

On 21. 6. 2. 오전 5:00, Stephan Gerhold wrote:
> Prepare for supporting SM5504 in the extcon-sm5502 driver by replacing
> enum sm5504_types with a struct sm5504_type that stores the chip-specific
> definitions. This struct can then be defined separately for SM5504
> without having to add if (type == TYPE_SM5504) everywhere in the code.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> Changes in v3: New patch to simplify diff on next patch
> ---
>   drivers/extcon/extcon-sm5502.c | 64 +++++++++++++++++++++-------------
>   drivers/extcon/extcon-sm5502.h |  4 ---
>   2 files changed, 40 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
> index 9f40bb9f1f81..951f6ca4c479 100644
> --- a/drivers/extcon/extcon-sm5502.c
> +++ b/drivers/extcon/extcon-sm5502.c
> @@ -40,17 +40,13 @@ struct sm5502_muic_info {
>   	struct i2c_client *i2c;
>   	struct regmap *regmap;
>   
> +	const struct sm5502_type *type;
>   	struct regmap_irq_chip_data *irq_data;
> -	struct muic_irq *muic_irqs;
> -	unsigned int num_muic_irqs;
>   	int irq;
>   	bool irq_attach;
>   	bool irq_detach;
>   	struct work_struct irq_work;
>   
> -	struct reg_data *reg_data;
> -	unsigned int num_reg_data;
> -
>   	struct mutex mutex;
>   
>   	/*
> @@ -62,6 +58,17 @@ struct sm5502_muic_info {
>   	struct delayed_work wq_detcable;
>   };
>   
> +struct sm5502_type {
> +	struct muic_irq *muic_irqs;
> +	unsigned int num_muic_irqs;
> +	const struct regmap_irq_chip *irq_chip;
> +
> +	struct reg_data *reg_data;
> +	unsigned int num_reg_data;
> +
> +	int (*parse_irq)(struct sm5502_muic_info *info, int irq_type);
> +};
> +
>   /* Default value of SM5502 register to bring up MUIC device. */
>   static struct reg_data sm5502_reg_data[] = {
>   	{
> @@ -502,11 +509,11 @@ static irqreturn_t sm5502_muic_irq_handler(int irq, void *data)
>   	struct sm5502_muic_info *info = data;
>   	int i, irq_type = -1, ret;
>   
> -	for (i = 0; i < info->num_muic_irqs; i++)
> -		if (irq == info->muic_irqs[i].virq)
> -			irq_type = info->muic_irqs[i].irq;
> +	for (i = 0; i < info->type->num_muic_irqs; i++)
> +		if (irq == info->type->muic_irqs[i].virq)
> +			irq_type = info->type->muic_irqs[i].irq;
>   
> -	ret = sm5502_parse_irq(info, irq_type);
> +	ret = info->type->parse_irq(info, irq_type);

Looks good to me. But there is only one comment.
Need to check the 'parse_irq' as following:

If you agree this suggestion, I'll apply with following changes by myself:

	if (!info->type->parse_irq) {
		dev_err(info->dev, "failed to handle irq due to parse_irq\n",
		return IRQ_NONE;
	}


(snip)

-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct

[Stephan Gerhold]

On Thu, Jun 03, 2021 at 12:13:18AM +0900, Chanwoo Choi wrote:
> On 21. 6. 2. 오전 5:00, Stephan Gerhold wrote:
> > Prepare for supporting SM5504 in the extcon-sm5502 driver by replacing
> > enum sm5504_types with a struct sm5504_type that stores the chip-specific
> > definitions. This struct can then be defined separately for SM5504
> > without having to add if (type == TYPE_SM5504) everywhere in the code.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > Changes in v3: New patch to simplify diff on next patch
> > ---
> >   drivers/extcon/extcon-sm5502.c | 64 +++++++++++++++++++++-------------
> >   drivers/extcon/extcon-sm5502.h |  4 ---
> >   2 files changed, 40 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
> > index 9f40bb9f1f81..951f6ca4c479 100644
> > --- a/drivers/extcon/extcon-sm5502.c
> > +++ b/drivers/extcon/extcon-sm5502.c
> > @@ -40,17 +40,13 @@ struct sm5502_muic_info {
> >   	struct i2c_client *i2c;
> >   	struct regmap *regmap;
> > +	const struct sm5502_type *type;
> >   	struct regmap_irq_chip_data *irq_data;
> > -	struct muic_irq *muic_irqs;
> > -	unsigned int num_muic_irqs;
> >   	int irq;
> >   	bool irq_attach;
> >   	bool irq_detach;
> >   	struct work_struct irq_work;
> > -	struct reg_data *reg_data;
> > -	unsigned int num_reg_data;
> > -
> >   	struct mutex mutex;
> >   	/*
> > @@ -62,6 +58,17 @@ struct sm5502_muic_info {
> >   	struct delayed_work wq_detcable;
> >   };
> > +struct sm5502_type {
> > +	struct muic_irq *muic_irqs;
> > +	unsigned int num_muic_irqs;
> > +	const struct regmap_irq_chip *irq_chip;
> > +
> > +	struct reg_data *reg_data;
> > +	unsigned int num_reg_data;
> > +
> > +	int (*parse_irq)(struct sm5502_muic_info *info, int irq_type);
> > +};
> > +
> >   /* Default value of SM5502 register to bring up MUIC device. */
> >   static struct reg_data sm5502_reg_data[] = {
> >   	{
> > @@ -502,11 +509,11 @@ static irqreturn_t sm5502_muic_irq_handler(int irq, void *data)
> >   	struct sm5502_muic_info *info = data;
> >   	int i, irq_type = -1, ret;
> > -	for (i = 0; i < info->num_muic_irqs; i++)
> > -		if (irq == info->muic_irqs[i].virq)
> > -			irq_type = info->muic_irqs[i].irq;
> > +	for (i = 0; i < info->type->num_muic_irqs; i++)
> > +		if (irq == info->type->muic_irqs[i].virq)
> > +			irq_type = info->type->muic_irqs[i].irq;
> > -	ret = sm5502_parse_irq(info, irq_type);
> > +	ret = info->type->parse_irq(info, irq_type);
> 
> Looks good to me. But there is only one comment.
> Need to check the 'parse_irq' as following:
> 
> If you agree this suggestion, I'll apply with following changes by myself:
> 
> 	if (!info->type->parse_irq) {
> 		dev_err(info->dev, "failed to handle irq due to parse_irq\n",
> 		return IRQ_NONE;
> 	}
> 
> 

This condition should be impossible, since .parse_irq is set for both
SM5502 and SM5504:

static const struct sm5502_type sm5502_data = {
	/* ... */
	.parse_irq = sm5502_parse_irq,
};

static const struct sm5502_type sm5504_data = {
	/* ... */
	.parse_irq = sm5504_parse_irq,
};

Which failure case are you trying to handle with that if statement?

Thanks!
Stephan

Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct

[Chanwoo Choi]

On 21. 6. 3. 오전 12:20, Stephan Gerhold wrote:
> On Thu, Jun 03, 2021 at 12:13:18AM +0900, Chanwoo Choi wrote:
>> On 21. 6. 2. 오전 5:00, Stephan Gerhold wrote:
>>> Prepare for supporting SM5504 in the extcon-sm5502 driver by replacing
>>> enum sm5504_types with a struct sm5504_type that stores the chip-specific
>>> definitions. This struct can then be defined separately for SM5504
>>> without having to add if (type == TYPE_SM5504) everywhere in the code.
>>>
>>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>>> ---
>>> Changes in v3: New patch to simplify diff on next patch
>>> ---
>>>    drivers/extcon/extcon-sm5502.c | 64 +++++++++++++++++++++-------------
>>>    drivers/extcon/extcon-sm5502.h |  4 ---
>>>    2 files changed, 40 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
>>> index 9f40bb9f1f81..951f6ca4c479 100644
>>> --- a/drivers/extcon/extcon-sm5502.c
>>> +++ b/drivers/extcon/extcon-sm5502.c
>>> @@ -40,17 +40,13 @@ struct sm5502_muic_info {
>>>    	struct i2c_client *i2c;
>>>    	struct regmap *regmap;
>>> +	const struct sm5502_type *type;
>>>    	struct regmap_irq_chip_data *irq_data;
>>> -	struct muic_irq *muic_irqs;
>>> -	unsigned int num_muic_irqs;
>>>    	int irq;
>>>    	bool irq_attach;
>>>    	bool irq_detach;
>>>    	struct work_struct irq_work;
>>> -	struct reg_data *reg_data;
>>> -	unsigned int num_reg_data;
>>> -
>>>    	struct mutex mutex;
>>>    	/*
>>> @@ -62,6 +58,17 @@ struct sm5502_muic_info {
>>>    	struct delayed_work wq_detcable;
>>>    };
>>> +struct sm5502_type {
>>> +	struct muic_irq *muic_irqs;
>>> +	unsigned int num_muic_irqs;
>>> +	const struct regmap_irq_chip *irq_chip;
>>> +
>>> +	struct reg_data *reg_data;
>>> +	unsigned int num_reg_data;
>>> +
>>> +	int (*parse_irq)(struct sm5502_muic_info *info, int irq_type);
>>> +};
>>> +
>>>    /* Default value of SM5502 register to bring up MUIC device. */
>>>    static struct reg_data sm5502_reg_data[] = {
>>>    	{
>>> @@ -502,11 +509,11 @@ static irqreturn_t sm5502_muic_irq_handler(int irq, void *data)
>>>    	struct sm5502_muic_info *info = data;
>>>    	int i, irq_type = -1, ret;
>>> -	for (i = 0; i < info->num_muic_irqs; i++)
>>> -		if (irq == info->muic_irqs[i].virq)
>>> -			irq_type = info->muic_irqs[i].irq;
>>> +	for (i = 0; i < info->type->num_muic_irqs; i++)
>>> +		if (irq == info->type->muic_irqs[i].virq)
>>> +			irq_type = info->type->muic_irqs[i].irq;
>>> -	ret = sm5502_parse_irq(info, irq_type);
>>> +	ret = info->type->parse_irq(info, irq_type);
>>
>> Looks good to me. But there is only one comment.
>> Need to check the 'parse_irq' as following:
>>
>> If you agree this suggestion, I'll apply with following changes by myself:
>>
>> 	if (!info->type->parse_irq) {
>> 		dev_err(info->dev, "failed to handle irq due to parse_irq\n",
>> 		return IRQ_NONE;
>> 	}
>>
>>
> 
> This condition should be impossible, since .parse_irq is set for both
> SM5502 and SM5504:
> 
> static const struct sm5502_type sm5502_data = {
> 	/* ... */
> 	.parse_irq = sm5502_parse_irq,
> };
> 
> static const struct sm5502_type sm5504_data = {
> 	/* ... */
> 	.parse_irq = sm5504_parse_irq,
> };
> 
> Which failure case are you trying to handle with that if statement?

There is not failure case of this patchset. But, this refactoring 
suggestion has the potential problem without checking whether mandatory 
function pointer is NULL or not. When adding new chip by using this 
driver, the author might have the human error without parse_irq 
initialization even if the mandatory.

> 
> Thanks!
> Stephan
> 


-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct

[Chanwoo Choi]

On 21. 6. 3. 오전 12:30, Chanwoo Choi wrote:
> On 21. 6. 3. 오전 12:20, Stephan Gerhold wrote:
>> On Thu, Jun 03, 2021 at 12:13:18AM +0900, Chanwoo Choi wrote:
>>> On 21. 6. 2. 오전 5:00, Stephan Gerhold wrote:
>>>> Prepare for supporting SM5504 in the extcon-sm5502 driver by replacing
>>>> enum sm5504_types with a struct sm5504_type that stores the 
>>>> chip-specific
>>>> definitions. This struct can then be defined separately for SM5504
>>>> without having to add if (type == TYPE_SM5504) everywhere in the code.
>>>>
>>>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>>>> ---
>>>> Changes in v3: New patch to simplify diff on next patch
>>>> ---
>>>>    drivers/extcon/extcon-sm5502.c | 64 
>>>> +++++++++++++++++++++-------------
>>>>    drivers/extcon/extcon-sm5502.h |  4 ---
>>>>    2 files changed, 40 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/extcon/extcon-sm5502.c 
>>>> b/drivers/extcon/extcon-sm5502.c
>>>> index 9f40bb9f1f81..951f6ca4c479 100644
>>>> --- a/drivers/extcon/extcon-sm5502.c
>>>> +++ b/drivers/extcon/extcon-sm5502.c
>>>> @@ -40,17 +40,13 @@ struct sm5502_muic_info {
>>>>        struct i2c_client *i2c;
>>>>        struct regmap *regmap;
>>>> +    const struct sm5502_type *type;
>>>>        struct regmap_irq_chip_data *irq_data;
>>>> -    struct muic_irq *muic_irqs;
>>>> -    unsigned int num_muic_irqs;
>>>>        int irq;
>>>>        bool irq_attach;
>>>>        bool irq_detach;
>>>>        struct work_struct irq_work;
>>>> -    struct reg_data *reg_data;
>>>> -    unsigned int num_reg_data;
>>>> -
>>>>        struct mutex mutex;
>>>>        /*
>>>> @@ -62,6 +58,17 @@ struct sm5502_muic_info {
>>>>        struct delayed_work wq_detcable;
>>>>    };
>>>> +struct sm5502_type {
>>>> +    struct muic_irq *muic_irqs;
>>>> +    unsigned int num_muic_irqs;
>>>> +    const struct regmap_irq_chip *irq_chip;
>>>> +
>>>> +    struct reg_data *reg_data;
>>>> +    unsigned int num_reg_data;
>>>> +
>>>> +    int (*parse_irq)(struct sm5502_muic_info *info, int irq_type);
>>>> +};
>>>> +
>>>>    /* Default value of SM5502 register to bring up MUIC device. */
>>>>    static struct reg_data sm5502_reg_data[] = {
>>>>        {
>>>> @@ -502,11 +509,11 @@ static irqreturn_t sm5502_muic_irq_handler(int 
>>>> irq, void *data)
>>>>        struct sm5502_muic_info *info = data;
>>>>        int i, irq_type = -1, ret;
>>>> -    for (i = 0; i < info->num_muic_irqs; i++)
>>>> -        if (irq == info->muic_irqs[i].virq)
>>>> -            irq_type = info->muic_irqs[i].irq;
>>>> +    for (i = 0; i < info->type->num_muic_irqs; i++)
>>>> +        if (irq == info->type->muic_irqs[i].virq)
>>>> +            irq_type = info->type->muic_irqs[i].irq;
>>>> -    ret = sm5502_parse_irq(info, irq_type);
>>>> +    ret = info->type->parse_irq(info, irq_type);
>>>
>>> Looks good to me. But there is only one comment.
>>> Need to check the 'parse_irq' as following:
>>>
>>> If you agree this suggestion, I'll apply with following changes by 
>>> myself:
>>>
>>>     if (!info->type->parse_irq) {
>>>         dev_err(info->dev, "failed to handle irq due to parse_irq\n",
>>>         return IRQ_NONE;
>>>     }
>>>
>>>
>>
>> This condition should be impossible, since .parse_irq is set for both
>> SM5502 and SM5504:
>>
>> static const struct sm5502_type sm5502_data = {
>>     /* ... */
>>     .parse_irq = sm5502_parse_irq,
>> };
>>
>> static const struct sm5502_type sm5504_data = {
>>     /* ... */
>>     .parse_irq = sm5504_parse_irq,
>> };
>>
>> Which failure case are you trying to handle with that if statement?
> 
> There is not failure case of this patchset. But, this refactoring 
> suggestion has the potential problem without checking whether mandatory 
> function pointer is NULL or not. When adding new chip by using this 
> driver, the author might have the human error without parse_irq 
> initialization even if the mandatory.
> 

Instead, it is better to check whether parser_irq is NULL or not
on probe function in order to reduce the unnecessary repetitive checking.

>>
>> Thanks!
>> Stephan
>>
> 
> 


-- 
Best Regards,
Samsung Electronics
Chanwoo Choi

Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct

[Stephan Gerhold]

On Thu, Jun 03, 2021 at 12:35:58AM +0900, Chanwoo Choi wrote:
> On 21. 6. 3. 오전 12:30, Chanwoo Choi wrote:
> > On 21. 6. 3. 오전 12:20, Stephan Gerhold wrote:
> > > On Thu, Jun 03, 2021 at 12:13:18AM +0900, Chanwoo Choi wrote:
> > > > On 21. 6. 2. 오전 5:00, Stephan Gerhold wrote:
> > > > > Prepare for supporting SM5504 in the extcon-sm5502 driver by replacing
> > > > > enum sm5504_types with a struct sm5504_type that stores the
> > > > > chip-specific
> > > > > definitions. This struct can then be defined separately for SM5504
> > > > > without having to add if (type == TYPE_SM5504) everywhere in the code.
> > > > > 
> > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > > > ---
> > > > > Changes in v3: New patch to simplify diff on next patch
> > > > > ---
> > > > >    drivers/extcon/extcon-sm5502.c | 64
> > > > > +++++++++++++++++++++-------------
> > > > >    drivers/extcon/extcon-sm5502.h |  4 ---
> > > > >    2 files changed, 40 insertions(+), 28 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/extcon/extcon-sm5502.c
> > > > > b/drivers/extcon/extcon-sm5502.c
> > > > > index 9f40bb9f1f81..951f6ca4c479 100644
> > > > > --- a/drivers/extcon/extcon-sm5502.c
> > > > > +++ b/drivers/extcon/extcon-sm5502.c
> > > > > @@ -40,17 +40,13 @@ struct sm5502_muic_info {
> > > > >        struct i2c_client *i2c;
> > > > >        struct regmap *regmap;
> > > > > +    const struct sm5502_type *type;
> > > > >        struct regmap_irq_chip_data *irq_data;
> > > > > -    struct muic_irq *muic_irqs;
> > > > > -    unsigned int num_muic_irqs;
> > > > >        int irq;
> > > > >        bool irq_attach;
> > > > >        bool irq_detach;
> > > > >        struct work_struct irq_work;
> > > > > -    struct reg_data *reg_data;
> > > > > -    unsigned int num_reg_data;
> > > > > -
> > > > >        struct mutex mutex;
> > > > >        /*
> > > > > @@ -62,6 +58,17 @@ struct sm5502_muic_info {
> > > > >        struct delayed_work wq_detcable;
> > > > >    };
> > > > > +struct sm5502_type {
> > > > > +    struct muic_irq *muic_irqs;
> > > > > +    unsigned int num_muic_irqs;
> > > > > +    const struct regmap_irq_chip *irq_chip;
> > > > > +
> > > > > +    struct reg_data *reg_data;
> > > > > +    unsigned int num_reg_data;
> > > > > +
> > > > > +    int (*parse_irq)(struct sm5502_muic_info *info, int irq_type);
> > > > > +};
> > > > > +
> > > > >    /* Default value of SM5502 register to bring up MUIC device. */
> > > > >    static struct reg_data sm5502_reg_data[] = {
> > > > >        {
> > > > > @@ -502,11 +509,11 @@ static irqreturn_t
> > > > > sm5502_muic_irq_handler(int irq, void *data)
> > > > >        struct sm5502_muic_info *info = data;
> > > > >        int i, irq_type = -1, ret;
> > > > > -    for (i = 0; i < info->num_muic_irqs; i++)
> > > > > -        if (irq == info->muic_irqs[i].virq)
> > > > > -            irq_type = info->muic_irqs[i].irq;
> > > > > +    for (i = 0; i < info->type->num_muic_irqs; i++)
> > > > > +        if (irq == info->type->muic_irqs[i].virq)
> > > > > +            irq_type = info->type->muic_irqs[i].irq;
> > > > > -    ret = sm5502_parse_irq(info, irq_type);
> > > > > +    ret = info->type->parse_irq(info, irq_type);
> > > > 
> > > > Looks good to me. But there is only one comment.
> > > > Need to check the 'parse_irq' as following:
> > > > 
> > > > If you agree this suggestion, I'll apply with following changes
> > > > by myself:
> > > > 
> > > >     if (!info->type->parse_irq) {
> > > >         dev_err(info->dev, "failed to handle irq due to parse_irq\n",
> > > >         return IRQ_NONE;
> > > >     }
> > > > 
> > > > 
> > > 
> > > This condition should be impossible, since .parse_irq is set for both
> > > SM5502 and SM5504:
> > > 
> > > static const struct sm5502_type sm5502_data = {
> > >     /* ... */
> > >     .parse_irq = sm5502_parse_irq,
> > > };
> > > 
> > > static const struct sm5502_type sm5504_data = {
> > >     /* ... */
> > >     .parse_irq = sm5504_parse_irq,
> > > };
> > > 
> > > Which failure case are you trying to handle with that if statement?
> > 
> > There is not failure case of this patchset. But, this refactoring
> > suggestion has the potential problem without checking whether mandatory
> > function pointer is NULL or not. When adding new chip by using this
> > driver, the author might have the human error without parse_irq
> > initialization even if the mandatory.
> > 
> 
> Instead, it is better to check whether parser_irq is NULL or not
> on probe function in order to reduce the unnecessary repetitive checking.
> 

Thanks for the explanation. This suggestion sounds better to me.
(Although I consider it unlikely that someone would forget to define
 .parse_irq when adding a new chip...)

Feel free to add something like the below when applying.
Or let me know if I should re-send with this change:

diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
index af44c1e2f368..93da2d8379b1 100644
--- a/drivers/extcon/extcon-sm5502.c
+++ b/drivers/extcon/extcon-sm5502.c
@@ -694,6 +694,10 @@ static int sm5022_muic_i2c_probe(struct i2c_client *i2c)
 	info->type = device_get_match_data(info->dev);
 	if (!info->type)
 		return -EINVAL;
+	if (!info->type->parse_irq) {
+		dev_err(info->dev, "parse_irq missing in struct sm5502_type\n");
+		return -EINVAL;
+	}
 
 	mutex_init(&info->mutex);
 

Thanks for your review!
Stephan

Re: [PATCH v3 1/3] dt-bindings: extcon: sm5502: Document siliconmitus,sm5504-muic

[Rob Herring]

On Tue, 01 Jun 2021 22:00:05 +0200, Stephan Gerhold wrote:
> Document support for SM5504 with the new siliconmitus,sm5504-muic
> compatible.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  .../bindings/extcon/siliconmitus,sm5502-muic.yaml           | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct

[Chanwoo Choi]

On 6/3/21 12:42 AM, Stephan Gerhold wrote:
> On Thu, Jun 03, 2021 at 12:35:58AM +0900, Chanwoo Choi wrote:
>> On 21. 6. 3. 오전 12:30, Chanwoo Choi wrote:
>>> On 21. 6. 3. 오전 12:20, Stephan Gerhold wrote:
>>>> On Thu, Jun 03, 2021 at 12:13:18AM +0900, Chanwoo Choi wrote:
>>>>> On 21. 6. 2. 오전 5:00, Stephan Gerhold wrote:
>>>>>> Prepare for supporting SM5504 in the extcon-sm5502 driver by replacing
>>>>>> enum sm5504_types with a struct sm5504_type that stores the
>>>>>> chip-specific
>>>>>> definitions. This struct can then be defined separately for SM5504
>>>>>> without having to add if (type == TYPE_SM5504) everywhere in the code.
>>>>>>
>>>>>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>>>>>> ---
>>>>>> Changes in v3: New patch to simplify diff on next patch
>>>>>> ---
>>>>>>    drivers/extcon/extcon-sm5502.c | 64
>>>>>> +++++++++++++++++++++-------------
>>>>>>    drivers/extcon/extcon-sm5502.h |  4 ---
>>>>>>    2 files changed, 40 insertions(+), 28 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/extcon/extcon-sm5502.c
>>>>>> b/drivers/extcon/extcon-sm5502.c
>>>>>> index 9f40bb9f1f81..951f6ca4c479 100644
>>>>>> --- a/drivers/extcon/extcon-sm5502.c
>>>>>> +++ b/drivers/extcon/extcon-sm5502.c
>>>>>> @@ -40,17 +40,13 @@ struct sm5502_muic_info {
>>>>>>        struct i2c_client *i2c;
>>>>>>        struct regmap *regmap;
>>>>>> +    const struct sm5502_type *type;
>>>>>>        struct regmap_irq_chip_data *irq_data;
>>>>>> -    struct muic_irq *muic_irqs;
>>>>>> -    unsigned int num_muic_irqs;
>>>>>>        int irq;
>>>>>>        bool irq_attach;
>>>>>>        bool irq_detach;
>>>>>>        struct work_struct irq_work;
>>>>>> -    struct reg_data *reg_data;
>>>>>> -    unsigned int num_reg_data;
>>>>>> -
>>>>>>        struct mutex mutex;
>>>>>>        /*
>>>>>> @@ -62,6 +58,17 @@ struct sm5502_muic_info {
>>>>>>        struct delayed_work wq_detcable;
>>>>>>    };
>>>>>> +struct sm5502_type {
>>>>>> +    struct muic_irq *muic_irqs;
>>>>>> +    unsigned int num_muic_irqs;
>>>>>> +    const struct regmap_irq_chip *irq_chip;
>>>>>> +
>>>>>> +    struct reg_data *reg_data;
>>>>>> +    unsigned int num_reg_data;
>>>>>> +
>>>>>> +    int (*parse_irq)(struct sm5502_muic_info *info, int irq_type);
>>>>>> +};
>>>>>> +
>>>>>>    /* Default value of SM5502 register to bring up MUIC device. */
>>>>>>    static struct reg_data sm5502_reg_data[] = {
>>>>>>        {
>>>>>> @@ -502,11 +509,11 @@ static irqreturn_t
>>>>>> sm5502_muic_irq_handler(int irq, void *data)
>>>>>>        struct sm5502_muic_info *info = data;
>>>>>>        int i, irq_type = -1, ret;
>>>>>> -    for (i = 0; i < info->num_muic_irqs; i++)
>>>>>> -        if (irq == info->muic_irqs[i].virq)
>>>>>> -            irq_type = info->muic_irqs[i].irq;
>>>>>> +    for (i = 0; i < info->type->num_muic_irqs; i++)
>>>>>> +        if (irq == info->type->muic_irqs[i].virq)
>>>>>> +            irq_type = info->type->muic_irqs[i].irq;
>>>>>> -    ret = sm5502_parse_irq(info, irq_type);
>>>>>> +    ret = info->type->parse_irq(info, irq_type);
>>>>>
>>>>> Looks good to me. But there is only one comment.
>>>>> Need to check the 'parse_irq' as following:
>>>>>
>>>>> If you agree this suggestion, I'll apply with following changes
>>>>> by myself:
>>>>>
>>>>>     if (!info->type->parse_irq) {
>>>>>         dev_err(info->dev, "failed to handle irq due to parse_irq\n",
>>>>>         return IRQ_NONE;
>>>>>     }
>>>>>
>>>>>
>>>>
>>>> This condition should be impossible, since .parse_irq is set for both
>>>> SM5502 and SM5504:
>>>>
>>>> static const struct sm5502_type sm5502_data = {
>>>>     /* ... */
>>>>     .parse_irq = sm5502_parse_irq,
>>>> };
>>>>
>>>> static const struct sm5502_type sm5504_data = {
>>>>     /* ... */
>>>>     .parse_irq = sm5504_parse_irq,
>>>> };
>>>>
>>>> Which failure case are you trying to handle with that if statement?
>>>
>>> There is not failure case of this patchset. But, this refactoring
>>> suggestion has the potential problem without checking whether mandatory
>>> function pointer is NULL or not. When adding new chip by using this
>>> driver, the author might have the human error without parse_irq
>>> initialization even if the mandatory.
>>>
>>
>> Instead, it is better to check whether parser_irq is NULL or not
>> on probe function in order to reduce the unnecessary repetitive checking.
>>
> 
> Thanks for the explanation. This suggestion sounds better to me.
> (Although I consider it unlikely that someone would forget to define
>  .parse_irq when adding a new chip...)
> 
> Feel free to add something like the below when applying.
> Or let me know if I should re-send with this change:

Please resend them. Thanks.

> 
> diff --git a/drivers/extcon/extcon-sm5502.c b/drivers/extcon/extcon-sm5502.c
> index af44c1e2f368..93da2d8379b1 100644
> --- a/drivers/extcon/extcon-sm5502.c
> +++ b/drivers/extcon/extcon-sm5502.c
> @@ -694,6 +694,10 @@ static int sm5022_muic_i2c_probe(struct i2c_client *i2c)
>  	info->type = device_get_match_data(info->dev);
>  	if (!info->type)
>  		return -EINVAL;
> +	if (!info->type->parse_irq) {
> +		dev_err(info->dev, "parse_irq missing in struct sm5502_type\n");
> +		return -EINVAL;
> +	}
>  
>  	mutex_init(&info->mutex);
>  
> 
> Thanks for your review!
> Stephan
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics