* [PATCH v3 0/3] extcon: sm5502: Add support for SM5504
@ 2021-06-01 20:00 Stephan Gerhold
2021-06-01 20:00 ` [PATCH v3 1/3] dt-bindings: extcon: sm5502: Document siliconmitus,sm5504-muic Stephan Gerhold
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Stephan Gerhold @ 2021-06-01 20:00 UTC (permalink / raw)
To: Chanwoo Choi, MyungJoo Ham
Cc: Rob Herring, devicetree, linux-kernel, Nikita Travkin,
~postmarketos/upstreaming, 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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v3 1/3] dt-bindings: extcon: sm5502: Document siliconmitus,sm5504-muic 2021-06-01 20:00 [PATCH v3 0/3] extcon: sm5502: Add support for SM5504 Stephan Gerhold @ 2021-06-01 20:00 ` Stephan Gerhold 2021-06-02 19:37 ` Rob Herring 2021-06-01 20:00 ` [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct Stephan Gerhold 2021-06-01 20:00 ` [PATCH v3 3/3] extcon: sm5502: Add support for SM5504 Stephan Gerhold 2 siblings, 1 reply; 11+ messages in thread From: Stephan Gerhold @ 2021-06-01 20:00 UTC (permalink / raw) To: Chanwoo Choi, MyungJoo Ham Cc: Rob Herring, devicetree, linux-kernel, Nikita Travkin, ~postmarketos/upstreaming, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] dt-bindings: extcon: sm5502: Document siliconmitus,sm5504-muic 2021-06-01 20:00 ` [PATCH v3 1/3] dt-bindings: extcon: sm5502: Document siliconmitus,sm5504-muic Stephan Gerhold @ 2021-06-02 19:37 ` Rob Herring 0 siblings, 0 replies; 11+ messages in thread From: Rob Herring @ 2021-06-02 19:37 UTC (permalink / raw) To: Stephan Gerhold Cc: MyungJoo Ham, linux-kernel, Nikita Travkin, Chanwoo Choi, Rob Herring, devicetree, ~postmarketos/upstreaming 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct 2021-06-01 20:00 [PATCH v3 0/3] extcon: sm5502: Add support for SM5504 Stephan Gerhold 2021-06-01 20:00 ` [PATCH v3 1/3] dt-bindings: extcon: sm5502: Document siliconmitus,sm5504-muic Stephan Gerhold @ 2021-06-01 20:00 ` Stephan Gerhold 2021-06-02 15:13 ` Chanwoo Choi 2021-06-01 20:00 ` [PATCH v3 3/3] extcon: sm5502: Add support for SM5504 Stephan Gerhold 2 siblings, 1 reply; 11+ messages in thread From: Stephan Gerhold @ 2021-06-01 20:00 UTC (permalink / raw) To: Chanwoo Choi, MyungJoo Ham Cc: Rob Herring, devicetree, linux-kernel, Nikita Travkin, ~postmarketos/upstreaming, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct 2021-06-01 20:00 ` [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct Stephan Gerhold @ 2021-06-02 15:13 ` Chanwoo Choi 2021-06-02 15:20 ` Stephan Gerhold 0 siblings, 1 reply; 11+ messages in thread From: Chanwoo Choi @ 2021-06-02 15:13 UTC (permalink / raw) To: Stephan Gerhold, Chanwoo Choi, MyungJoo Ham Cc: Rob Herring, devicetree, linux-kernel, Nikita Travkin, ~postmarketos/upstreaming 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct 2021-06-02 15:13 ` Chanwoo Choi @ 2021-06-02 15:20 ` Stephan Gerhold 2021-06-02 15:30 ` Chanwoo Choi 0 siblings, 1 reply; 11+ messages in thread From: Stephan Gerhold @ 2021-06-02 15:20 UTC (permalink / raw) To: Chanwoo Choi Cc: Chanwoo Choi, MyungJoo Ham, Rob Herring, devicetree, linux-kernel, Nikita Travkin, ~postmarketos/upstreaming 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct 2021-06-02 15:20 ` Stephan Gerhold @ 2021-06-02 15:30 ` Chanwoo Choi 2021-06-02 15:35 ` Chanwoo Choi 0 siblings, 1 reply; 11+ messages in thread From: Chanwoo Choi @ 2021-06-02 15:30 UTC (permalink / raw) To: Stephan Gerhold Cc: Chanwoo Choi, MyungJoo Ham, Rob Herring, devicetree, linux-kernel, Nikita Travkin, ~postmarketos/upstreaming 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct 2021-06-02 15:30 ` Chanwoo Choi @ 2021-06-02 15:35 ` Chanwoo Choi 2021-06-02 15:42 ` Stephan Gerhold 0 siblings, 1 reply; 11+ messages in thread From: Chanwoo Choi @ 2021-06-02 15:35 UTC (permalink / raw) To: Stephan Gerhold Cc: Chanwoo Choi, MyungJoo Ham, Rob Herring, devicetree, linux-kernel, Nikita Travkin, ~postmarketos/upstreaming 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct 2021-06-02 15:35 ` Chanwoo Choi @ 2021-06-02 15:42 ` Stephan Gerhold 2021-06-03 3:05 ` Chanwoo Choi 0 siblings, 1 reply; 11+ messages in thread From: Stephan Gerhold @ 2021-06-02 15:42 UTC (permalink / raw) To: Chanwoo Choi Cc: Chanwoo Choi, MyungJoo Ham, Rob Herring, devicetree, linux-kernel, Nikita Travkin, ~postmarketos/upstreaming 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct 2021-06-02 15:42 ` Stephan Gerhold @ 2021-06-03 3:05 ` Chanwoo Choi 0 siblings, 0 replies; 11+ messages in thread From: Chanwoo Choi @ 2021-06-03 3:05 UTC (permalink / raw) To: Stephan Gerhold, Chanwoo Choi Cc: MyungJoo Ham, Rob Herring, devicetree, linux-kernel, Nikita Travkin, ~postmarketos/upstreaming 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 3/3] extcon: sm5502: Add support for SM5504 2021-06-01 20:00 [PATCH v3 0/3] extcon: sm5502: Add support for SM5504 Stephan Gerhold 2021-06-01 20:00 ` [PATCH v3 1/3] dt-bindings: extcon: sm5502: Document siliconmitus,sm5504-muic Stephan Gerhold 2021-06-01 20:00 ` [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct Stephan Gerhold @ 2021-06-01 20:00 ` Stephan Gerhold 2 siblings, 0 replies; 11+ messages in thread From: Stephan Gerhold @ 2021-06-01 20:00 UTC (permalink / raw) To: Chanwoo Choi, MyungJoo Ham Cc: Rob Herring, devicetree, linux-kernel, Nikita Travkin, ~postmarketos/upstreaming, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-06-03 2:46 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-01 20:00 [PATCH v3 0/3] extcon: sm5502: Add support for SM5504 Stephan Gerhold 2021-06-01 20:00 ` [PATCH v3 1/3] dt-bindings: extcon: sm5502: Document siliconmitus,sm5504-muic Stephan Gerhold 2021-06-02 19:37 ` Rob Herring 2021-06-01 20:00 ` [PATCH v3 2/3] extcon: sm5502: Refactor driver to use chip-specific struct Stephan Gerhold 2021-06-02 15:13 ` Chanwoo Choi 2021-06-02 15:20 ` Stephan Gerhold 2021-06-02 15:30 ` Chanwoo Choi 2021-06-02 15:35 ` Chanwoo Choi 2021-06-02 15:42 ` Stephan Gerhold 2021-06-03 3:05 ` Chanwoo Choi 2021-06-01 20:00 ` [PATCH v3 3/3] extcon: sm5502: Add support for SM5504 Stephan Gerhold
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).