| Message ID | 20180227071134.28063-7-a.hajda@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 2018년 02월 27일 16:11, Andrzej Hajda wrote: > From: Maciej Purski <m.purski@samsung.com> > > Currently MHL chip must be turned on permanently to detect MHL cable. It > duplicates micro-USB controller's (MUIC) functionality and consumes > unnecessary power. Lets use extcon attached to MUIC to enable MHL chip > only if it detects MHL cable. > > Signed-off-by: Maciej Purski <m.purski@samsung.com> > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > v5: updated extcon API > > This is rework of the patch by Maciej with following changes: > - use micro-USB port bindings to get extcon, instead of extcon property, > - fixed remove sequence, > - fixed extcon get state logic. > > Code finding extcon node is hacky IMO, I guess ultimately it should be done > via some framework (maybe even extcon), or at least via helper, I hope it > can stay as is until the proper solution will be merged. > > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> > --- > drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 94 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c > index 9e785b8e0ea2..62b0adabcac2 100644 > --- a/drivers/gpu/drm/bridge/sil-sii8620.c > +++ b/drivers/gpu/drm/bridge/sil-sii8620.c > @@ -17,6 +17,7 @@ > > #include <linux/clk.h> > #include <linux/delay.h> > +#include <linux/extcon.h> > #include <linux/gpio/consumer.h> > #include <linux/i2c.h> > #include <linux/interrupt.h> > @@ -25,6 +26,7 @@ > #include <linux/list.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/of_graph.h> > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > > @@ -81,6 +83,10 @@ struct sii8620 { > struct edid *edid; > unsigned int gen2_write_burst:1; > enum sii8620_mt_state mt_state; > + struct extcon_dev *extcon; > + struct notifier_block extcon_nb; > + struct work_struct extcon_wq; > + int cable_state; > struct list_head mt_queue; > struct { > int r_size; > @@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) > ctx->rc_dev = rc_dev; > } > > +static void sii8620_cable_out(struct sii8620 *ctx) > +{ > + disable_irq(to_i2c_client(ctx->dev)->irq); > + sii8620_hw_off(ctx); > +} > + > +static void sii8620_extcon_work(struct work_struct *work) > +{ > + struct sii8620 *ctx = > + container_of(work, struct sii8620, extcon_wq); > + int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL); > + > + if (state == ctx->cable_state) > + return; > + > + ctx->cable_state = state; > + > + if (state > 0) > + sii8620_cable_in(ctx); > + else > + sii8620_cable_out(ctx); > +} > + > +static int sii8620_extcon_notifier(struct notifier_block *self, > + unsigned long event, void *ptr) > +{ > + struct sii8620 *ctx = > + container_of(self, struct sii8620, extcon_nb); > + > + schedule_work(&ctx->extcon_wq); > + > + return NOTIFY_DONE; > +} > + > +static int sii8620_extcon_init(struct sii8620 *ctx) > +{ > + struct extcon_dev *edev; > + struct device_node *musb, *muic; > + int ret; > + > + /* get micro-USB connector node */ > + musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1); > + /* next get micro-USB Interface Controller node */ > + muic = of_get_next_parent(musb); > + > + if (!muic) { > + dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n"); > + return 0; > + } > + > + edev = extcon_find_edev_by_node(muic); > + of_node_put(muic); > + if (IS_ERR(edev)) { > + if (PTR_ERR(edev) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + dev_err(ctx->dev, "Invalid or missing extcon\n"); > + return PTR_ERR(edev); > + } > + > + ctx->extcon = edev; > + ctx->extcon_nb.notifier_call = sii8620_extcon_notifier; > + INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work); > + ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb); You better to use devm_extcon_register_notifier(). > + if (ret) { > + dev_err(ctx->dev, "failed to register notifier for MHL\n"); > + return ret; > + } > + > + return 0; > +} > + > static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) > { > return container_of(bridge, struct sii8620, bridge); > @@ -2307,13 +2384,20 @@ static int sii8620_probe(struct i2c_client *client, > if (ret) > return ret; > > + ret = sii8620_extcon_init(ctx); > + if (ret < 0) { > + dev_err(ctx->dev, "failed to initialize EXTCON\n"); > + return ret; > + } > + > i2c_set_clientdata(client, ctx); > > ctx->bridge.funcs = &sii8620_bridge_funcs; > ctx->bridge.of_node = dev->of_node; > drm_bridge_add(&ctx->bridge); > > - sii8620_cable_in(ctx); > + if (!ctx->extcon) > + sii8620_cable_in(ctx); > > return 0; > } > @@ -2322,8 +2406,15 @@ static int sii8620_remove(struct i2c_client *client) > { > struct sii8620 *ctx = i2c_get_clientdata(client); > > - disable_irq(to_i2c_client(ctx->dev)->irq); > - sii8620_hw_off(ctx); > + if (ctx->extcon) { > + extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL, > + &ctx->extcon_nb); Don't need to unregister the notifier if using devm_extcon_register_notifier(). > + flush_work(&ctx->extcon_wq); > + if (ctx->cable_state > 0) > + sii8620_cable_out(ctx); > + } else { > + sii8620_cable_out(ctx); > + } > drm_bridge_remove(&ctx->bridge); > > return 0; > If you use the resource managed function (devm_extcon_register_notifier), Looks good to me. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
On 27.02.2018 12:08, Chanwoo Choi wrote: > Hi, > > On 2018년 02월 27일 16:11, Andrzej Hajda wrote: >> From: Maciej Purski <m.purski@samsung.com> >> >> Currently MHL chip must be turned on permanently to detect MHL cable. It >> duplicates micro-USB controller's (MUIC) functionality and consumes >> unnecessary power. Lets use extcon attached to MUIC to enable MHL chip >> only if it detects MHL cable. >> >> Signed-off-by: Maciej Purski <m.purski@samsung.com> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >> --- >> v5: updated extcon API >> >> This is rework of the patch by Maciej with following changes: >> - use micro-USB port bindings to get extcon, instead of extcon property, >> - fixed remove sequence, >> - fixed extcon get state logic. >> >> Code finding extcon node is hacky IMO, I guess ultimately it should be done >> via some framework (maybe even extcon), or at least via helper, I hope it >> can stay as is until the proper solution will be merged. >> >> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >> --- >> drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- >> 1 file changed, 94 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c >> index 9e785b8e0ea2..62b0adabcac2 100644 >> --- a/drivers/gpu/drm/bridge/sil-sii8620.c >> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c >> @@ -17,6 +17,7 @@ >> >> #include <linux/clk.h> >> #include <linux/delay.h> >> +#include <linux/extcon.h> >> #include <linux/gpio/consumer.h> >> #include <linux/i2c.h> >> #include <linux/interrupt.h> >> @@ -25,6 +26,7 @@ >> #include <linux/list.h> >> #include <linux/module.h> >> #include <linux/mutex.h> >> +#include <linux/of_graph.h> >> #include <linux/regulator/consumer.h> >> #include <linux/slab.h> >> >> @@ -81,6 +83,10 @@ struct sii8620 { >> struct edid *edid; >> unsigned int gen2_write_burst:1; >> enum sii8620_mt_state mt_state; >> + struct extcon_dev *extcon; >> + struct notifier_block extcon_nb; >> + struct work_struct extcon_wq; >> + int cable_state; >> struct list_head mt_queue; >> struct { >> int r_size; >> @@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) >> ctx->rc_dev = rc_dev; >> } >> >> +static void sii8620_cable_out(struct sii8620 *ctx) >> +{ >> + disable_irq(to_i2c_client(ctx->dev)->irq); >> + sii8620_hw_off(ctx); >> +} >> + >> +static void sii8620_extcon_work(struct work_struct *work) >> +{ >> + struct sii8620 *ctx = >> + container_of(work, struct sii8620, extcon_wq); >> + int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL); >> + >> + if (state == ctx->cable_state) >> + return; >> + >> + ctx->cable_state = state; >> + >> + if (state > 0) >> + sii8620_cable_in(ctx); >> + else >> + sii8620_cable_out(ctx); >> +} >> + >> +static int sii8620_extcon_notifier(struct notifier_block *self, >> + unsigned long event, void *ptr) >> +{ >> + struct sii8620 *ctx = >> + container_of(self, struct sii8620, extcon_nb); >> + >> + schedule_work(&ctx->extcon_wq); >> + >> + return NOTIFY_DONE; >> +} >> + >> +static int sii8620_extcon_init(struct sii8620 *ctx) >> +{ >> + struct extcon_dev *edev; >> + struct device_node *musb, *muic; >> + int ret; >> + >> + /* get micro-USB connector node */ >> + musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1); >> + /* next get micro-USB Interface Controller node */ >> + muic = of_get_next_parent(musb); >> + >> + if (!muic) { >> + dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n"); >> + return 0; >> + } >> + >> + edev = extcon_find_edev_by_node(muic); >> + of_node_put(muic); >> + if (IS_ERR(edev)) { >> + if (PTR_ERR(edev) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> + dev_err(ctx->dev, "Invalid or missing extcon\n"); >> + return PTR_ERR(edev); >> + } >> + >> + ctx->extcon = edev; >> + ctx->extcon_nb.notifier_call = sii8620_extcon_notifier; >> + INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work); >> + ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb); > You better to use devm_extcon_register_notifier(). With devm version I risk that in case of device unbind notification will be called after .remove callback, it seems to me quite dangerous. Or am I missing something? Regards Andrzej > >> + if (ret) { >> + dev_err(ctx->dev, "failed to register notifier for MHL\n"); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) >> { >> return container_of(bridge, struct sii8620, bridge); >> @@ -2307,13 +2384,20 @@ static int sii8620_probe(struct i2c_client *client, >> if (ret) >> return ret; >> >> + ret = sii8620_extcon_init(ctx); >> + if (ret < 0) { >> + dev_err(ctx->dev, "failed to initialize EXTCON\n"); >> + return ret; >> + } >> + >> i2c_set_clientdata(client, ctx); >> >> ctx->bridge.funcs = &sii8620_bridge_funcs; >> ctx->bridge.of_node = dev->of_node; >> drm_bridge_add(&ctx->bridge); >> >> - sii8620_cable_in(ctx); >> + if (!ctx->extcon) >> + sii8620_cable_in(ctx); >> >> return 0; >> } >> @@ -2322,8 +2406,15 @@ static int sii8620_remove(struct i2c_client *client) >> { >> struct sii8620 *ctx = i2c_get_clientdata(client); >> >> - disable_irq(to_i2c_client(ctx->dev)->irq); >> - sii8620_hw_off(ctx); >> + if (ctx->extcon) { >> + extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL, >> + &ctx->extcon_nb); > Don't need to unregister the notifier if using devm_extcon_register_notifier(). > >> + flush_work(&ctx->extcon_wq); >> + if (ctx->cable_state > 0) >> + sii8620_cable_out(ctx); >> + } else { >> + sii8620_cable_out(ctx); >> + } >> drm_bridge_remove(&ctx->bridge); >> >> return 0; >> > If you use the resource managed function (devm_extcon_register_notifier), Looks good to me. > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> >
Hi, On 2018년 02월 27일 21:05, Andrzej Hajda wrote: > On 27.02.2018 12:08, Chanwoo Choi wrote: >> Hi, >> >> On 2018년 02월 27일 16:11, Andrzej Hajda wrote: >>> From: Maciej Purski <m.purski@samsung.com> >>> >>> Currently MHL chip must be turned on permanently to detect MHL cable. It >>> duplicates micro-USB controller's (MUIC) functionality and consumes >>> unnecessary power. Lets use extcon attached to MUIC to enable MHL chip >>> only if it detects MHL cable. >>> >>> Signed-off-by: Maciej Purski <m.purski@samsung.com> >>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>> --- >>> v5: updated extcon API >>> >>> This is rework of the patch by Maciej with following changes: >>> - use micro-USB port bindings to get extcon, instead of extcon property, >>> - fixed remove sequence, >>> - fixed extcon get state logic. >>> >>> Code finding extcon node is hacky IMO, I guess ultimately it should be done >>> via some framework (maybe even extcon), or at least via helper, I hope it >>> can stay as is until the proper solution will be merged. >>> >>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>> --- >>> drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 94 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c >>> index 9e785b8e0ea2..62b0adabcac2 100644 >>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c >>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c >>> @@ -17,6 +17,7 @@ >>> >>> #include <linux/clk.h> >>> #include <linux/delay.h> >>> +#include <linux/extcon.h> >>> #include <linux/gpio/consumer.h> >>> #include <linux/i2c.h> >>> #include <linux/interrupt.h> >>> @@ -25,6 +26,7 @@ >>> #include <linux/list.h> >>> #include <linux/module.h> >>> #include <linux/mutex.h> >>> +#include <linux/of_graph.h> >>> #include <linux/regulator/consumer.h> >>> #include <linux/slab.h> >>> >>> @@ -81,6 +83,10 @@ struct sii8620 { >>> struct edid *edid; >>> unsigned int gen2_write_burst:1; >>> enum sii8620_mt_state mt_state; >>> + struct extcon_dev *extcon; >>> + struct notifier_block extcon_nb; >>> + struct work_struct extcon_wq; >>> + int cable_state; >>> struct list_head mt_queue; >>> struct { >>> int r_size; >>> @@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) >>> ctx->rc_dev = rc_dev; >>> } >>> >>> +static void sii8620_cable_out(struct sii8620 *ctx) >>> +{ >>> + disable_irq(to_i2c_client(ctx->dev)->irq); >>> + sii8620_hw_off(ctx); >>> +} >>> + >>> +static void sii8620_extcon_work(struct work_struct *work) >>> +{ >>> + struct sii8620 *ctx = >>> + container_of(work, struct sii8620, extcon_wq); >>> + int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL); >>> + >>> + if (state == ctx->cable_state) >>> + return; >>> + >>> + ctx->cable_state = state; >>> + >>> + if (state > 0) >>> + sii8620_cable_in(ctx); >>> + else >>> + sii8620_cable_out(ctx); >>> +} >>> + >>> +static int sii8620_extcon_notifier(struct notifier_block *self, >>> + unsigned long event, void *ptr) >>> +{ >>> + struct sii8620 *ctx = >>> + container_of(self, struct sii8620, extcon_nb); >>> + >>> + schedule_work(&ctx->extcon_wq); >>> + >>> + return NOTIFY_DONE; >>> +} >>> + >>> +static int sii8620_extcon_init(struct sii8620 *ctx) >>> +{ >>> + struct extcon_dev *edev; >>> + struct device_node *musb, *muic; >>> + int ret; >>> + >>> + /* get micro-USB connector node */ >>> + musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1); >>> + /* next get micro-USB Interface Controller node */ >>> + muic = of_get_next_parent(musb); >>> + >>> + if (!muic) { >>> + dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n"); >>> + return 0; >>> + } >>> + >>> + edev = extcon_find_edev_by_node(muic); >>> + of_node_put(muic); >>> + if (IS_ERR(edev)) { >>> + if (PTR_ERR(edev) == -EPROBE_DEFER) >>> + return -EPROBE_DEFER; >>> + dev_err(ctx->dev, "Invalid or missing extcon\n"); >>> + return PTR_ERR(edev); >>> + } >>> + >>> + ctx->extcon = edev; >>> + ctx->extcon_nb.notifier_call = sii8620_extcon_notifier; >>> + INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work); >>> + ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb); >> You better to use devm_extcon_register_notifier(). > > With devm version I risk that in case of device unbind notification will > be called after .remove callback, it seems to me quite dangerous. Or am > I missing something? If you use the cancel_work_sync() in remove() instead of flush_work(), you can use the 'devm_extcon_*'. > > Regards > Andrzej > >> >>> + if (ret) { >>> + dev_err(ctx->dev, "failed to register notifier for MHL\n"); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) >>> { >>> return container_of(bridge, struct sii8620, bridge); >>> @@ -2307,13 +2384,20 @@ static int sii8620_probe(struct i2c_client *client, >>> if (ret) >>> return ret; >>> >>> + ret = sii8620_extcon_init(ctx); >>> + if (ret < 0) { >>> + dev_err(ctx->dev, "failed to initialize EXTCON\n"); >>> + return ret; >>> + } >>> + >>> i2c_set_clientdata(client, ctx); >>> >>> ctx->bridge.funcs = &sii8620_bridge_funcs; >>> ctx->bridge.of_node = dev->of_node; >>> drm_bridge_add(&ctx->bridge); >>> >>> - sii8620_cable_in(ctx); >>> + if (!ctx->extcon) >>> + sii8620_cable_in(ctx); >>> >>> return 0; >>> } >>> @@ -2322,8 +2406,15 @@ static int sii8620_remove(struct i2c_client *client) >>> { >>> struct sii8620 *ctx = i2c_get_clientdata(client); >>> >>> - disable_irq(to_i2c_client(ctx->dev)->irq); >>> - sii8620_hw_off(ctx); >>> + if (ctx->extcon) { >>> + extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL, >>> + &ctx->extcon_nb); >> Don't need to unregister the notifier if using devm_extcon_register_notifier(). >> >>> + flush_work(&ctx->extcon_wq); >>> + if (ctx->cable_state > 0) >>> + sii8620_cable_out(ctx); >>> + } else { >>> + sii8620_cable_out(ctx); >>> + } >>> drm_bridge_remove(&ctx->bridge); >>> >>> return 0; >>> >> If you use the resource managed function (devm_extcon_register_notifier), Looks good to me. >> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
On 27.02.2018 23:26, Chanwoo Choi wrote: > Hi, > > On 2018년 02월 27일 21:05, Andrzej Hajda wrote: >> On 27.02.2018 12:08, Chanwoo Choi wrote: >>> Hi, >>> >>> On 2018년 02월 27일 16:11, Andrzej Hajda wrote: >>>> From: Maciej Purski <m.purski@samsung.com> >>>> >>>> Currently MHL chip must be turned on permanently to detect MHL cable. It >>>> duplicates micro-USB controller's (MUIC) functionality and consumes >>>> unnecessary power. Lets use extcon attached to MUIC to enable MHL chip >>>> only if it detects MHL cable. >>>> >>>> Signed-off-by: Maciej Purski <m.purski@samsung.com> >>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>>> --- >>>> v5: updated extcon API >>>> >>>> This is rework of the patch by Maciej with following changes: >>>> - use micro-USB port bindings to get extcon, instead of extcon property, >>>> - fixed remove sequence, >>>> - fixed extcon get state logic. >>>> >>>> Code finding extcon node is hacky IMO, I guess ultimately it should be done >>>> via some framework (maybe even extcon), or at least via helper, I hope it >>>> can stay as is until the proper solution will be merged. >>>> >>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>>> --- >>>> drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 94 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c >>>> index 9e785b8e0ea2..62b0adabcac2 100644 >>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c >>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c >>>> @@ -17,6 +17,7 @@ >>>> >>>> #include <linux/clk.h> >>>> #include <linux/delay.h> >>>> +#include <linux/extcon.h> >>>> #include <linux/gpio/consumer.h> >>>> #include <linux/i2c.h> >>>> #include <linux/interrupt.h> >>>> @@ -25,6 +26,7 @@ >>>> #include <linux/list.h> >>>> #include <linux/module.h> >>>> #include <linux/mutex.h> >>>> +#include <linux/of_graph.h> >>>> #include <linux/regulator/consumer.h> >>>> #include <linux/slab.h> >>>> >>>> @@ -81,6 +83,10 @@ struct sii8620 { >>>> struct edid *edid; >>>> unsigned int gen2_write_burst:1; >>>> enum sii8620_mt_state mt_state; >>>> + struct extcon_dev *extcon; >>>> + struct notifier_block extcon_nb; >>>> + struct work_struct extcon_wq; >>>> + int cable_state; >>>> struct list_head mt_queue; >>>> struct { >>>> int r_size; >>>> @@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) >>>> ctx->rc_dev = rc_dev; >>>> } >>>> >>>> +static void sii8620_cable_out(struct sii8620 *ctx) >>>> +{ >>>> + disable_irq(to_i2c_client(ctx->dev)->irq); >>>> + sii8620_hw_off(ctx); >>>> +} >>>> + >>>> +static void sii8620_extcon_work(struct work_struct *work) >>>> +{ >>>> + struct sii8620 *ctx = >>>> + container_of(work, struct sii8620, extcon_wq); >>>> + int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL); >>>> + >>>> + if (state == ctx->cable_state) >>>> + return; >>>> + >>>> + ctx->cable_state = state; >>>> + >>>> + if (state > 0) >>>> + sii8620_cable_in(ctx); >>>> + else >>>> + sii8620_cable_out(ctx); >>>> +} >>>> + >>>> +static int sii8620_extcon_notifier(struct notifier_block *self, >>>> + unsigned long event, void *ptr) >>>> +{ >>>> + struct sii8620 *ctx = >>>> + container_of(self, struct sii8620, extcon_nb); >>>> + >>>> + schedule_work(&ctx->extcon_wq); >>>> + >>>> + return NOTIFY_DONE; >>>> +} >>>> + >>>> +static int sii8620_extcon_init(struct sii8620 *ctx) >>>> +{ >>>> + struct extcon_dev *edev; >>>> + struct device_node *musb, *muic; >>>> + int ret; >>>> + >>>> + /* get micro-USB connector node */ >>>> + musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1); >>>> + /* next get micro-USB Interface Controller node */ >>>> + muic = of_get_next_parent(musb); >>>> + >>>> + if (!muic) { >>>> + dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n"); >>>> + return 0; >>>> + } >>>> + >>>> + edev = extcon_find_edev_by_node(muic); >>>> + of_node_put(muic); >>>> + if (IS_ERR(edev)) { >>>> + if (PTR_ERR(edev) == -EPROBE_DEFER) >>>> + return -EPROBE_DEFER; >>>> + dev_err(ctx->dev, "Invalid or missing extcon\n"); >>>> + return PTR_ERR(edev); >>>> + } >>>> + >>>> + ctx->extcon = edev; >>>> + ctx->extcon_nb.notifier_call = sii8620_extcon_notifier; >>>> + INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work); >>>> + ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb); >>> You better to use devm_extcon_register_notifier(). >> With devm version I risk that in case of device unbind notification will >> be called after .remove callback, it seems to me quite dangerous. Or am >> I missing something? > If you use the cancel_work_sync() in remove() instead of flush_work(), > you can use the 'devm_extcon_*'. cancel_work_sync() does not prevent works scheduled later from execution [1] and this scenario is possible with devm_extcon_register_notifier() and cancel_work_sync(). So we end up with: sii8620_remove() calls cancel_work_sync() ... notifier(called asynchronously) schedules sii8620_extcon_work() ... notifier is removed by devm framework sii8620 context is destroyed by devm framework ... sii8620_extcon_work is executed on destroyed context !!! BUG !!! For me it seems that devm_extcon_register_notifier is not safe in this case. [1]: Since documentation was not clear I have performed live test confirming my suspicions. Regards Andrzej > >> Regards >> Andrzej >> >>>> + if (ret) { >>>> + dev_err(ctx->dev, "failed to register notifier for MHL\n"); >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) >>>> { >>>> return container_of(bridge, struct sii8620, bridge); >>>> @@ -2307,13 +2384,20 @@ static int sii8620_probe(struct i2c_client *client, >>>> if (ret) >>>> return ret; >>>> >>>> + ret = sii8620_extcon_init(ctx); >>>> + if (ret < 0) { >>>> + dev_err(ctx->dev, "failed to initialize EXTCON\n"); >>>> + return ret; >>>> + } >>>> + >>>> i2c_set_clientdata(client, ctx); >>>> >>>> ctx->bridge.funcs = &sii8620_bridge_funcs; >>>> ctx->bridge.of_node = dev->of_node; >>>> drm_bridge_add(&ctx->bridge); >>>> >>>> - sii8620_cable_in(ctx); >>>> + if (!ctx->extcon) >>>> + sii8620_cable_in(ctx); >>>> >>>> return 0; >>>> } >>>> @@ -2322,8 +2406,15 @@ static int sii8620_remove(struct i2c_client *client) >>>> { >>>> struct sii8620 *ctx = i2c_get_clientdata(client); >>>> >>>> - disable_irq(to_i2c_client(ctx->dev)->irq); >>>> - sii8620_hw_off(ctx); >>>> + if (ctx->extcon) { >>>> + extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL, >>>> + &ctx->extcon_nb); >>> Don't need to unregister the notifier if using devm_extcon_register_notifier(). >>> >>>> + flush_work(&ctx->extcon_wq); >>>> + if (ctx->cable_state > 0) >>>> + sii8620_cable_out(ctx); >>>> + } else { >>>> + sii8620_cable_out(ctx); >>>> + } >>>> drm_bridge_remove(&ctx->bridge); >>>> >>>> return 0; >>>> >>> If you use the resource managed function (devm_extcon_register_notifier), Looks good to me. >>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >
On 2018년 02월 28일 22:44, Andrzej Hajda wrote: > On 27.02.2018 23:26, Chanwoo Choi wrote: >> Hi, >> >> On 2018년 02월 27일 21:05, Andrzej Hajda wrote: >>> On 27.02.2018 12:08, Chanwoo Choi wrote: >>>> Hi, >>>> >>>> On 2018년 02월 27일 16:11, Andrzej Hajda wrote: >>>>> From: Maciej Purski <m.purski@samsung.com> >>>>> >>>>> Currently MHL chip must be turned on permanently to detect MHL cable. It >>>>> duplicates micro-USB controller's (MUIC) functionality and consumes >>>>> unnecessary power. Lets use extcon attached to MUIC to enable MHL chip >>>>> only if it detects MHL cable. >>>>> >>>>> Signed-off-by: Maciej Purski <m.purski@samsung.com> >>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>>>> --- >>>>> v5: updated extcon API >>>>> >>>>> This is rework of the patch by Maciej with following changes: >>>>> - use micro-USB port bindings to get extcon, instead of extcon property, >>>>> - fixed remove sequence, >>>>> - fixed extcon get state logic. >>>>> >>>>> Code finding extcon node is hacky IMO, I guess ultimately it should be done >>>>> via some framework (maybe even extcon), or at least via helper, I hope it >>>>> can stay as is until the proper solution will be merged. >>>>> >>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com> >>>>> --- >>>>> drivers/gpu/drm/bridge/sil-sii8620.c | 97 ++++++++++++++++++++++++++++++++++-- >>>>> 1 file changed, 94 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c >>>>> index 9e785b8e0ea2..62b0adabcac2 100644 >>>>> --- a/drivers/gpu/drm/bridge/sil-sii8620.c >>>>> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c >>>>> @@ -17,6 +17,7 @@ >>>>> >>>>> #include <linux/clk.h> >>>>> #include <linux/delay.h> >>>>> +#include <linux/extcon.h> >>>>> #include <linux/gpio/consumer.h> >>>>> #include <linux/i2c.h> >>>>> #include <linux/interrupt.h> >>>>> @@ -25,6 +26,7 @@ >>>>> #include <linux/list.h> >>>>> #include <linux/module.h> >>>>> #include <linux/mutex.h> >>>>> +#include <linux/of_graph.h> >>>>> #include <linux/regulator/consumer.h> >>>>> #include <linux/slab.h> >>>>> >>>>> @@ -81,6 +83,10 @@ struct sii8620 { >>>>> struct edid *edid; >>>>> unsigned int gen2_write_burst:1; >>>>> enum sii8620_mt_state mt_state; >>>>> + struct extcon_dev *extcon; >>>>> + struct notifier_block extcon_nb; >>>>> + struct work_struct extcon_wq; >>>>> + int cable_state; >>>>> struct list_head mt_queue; >>>>> struct { >>>>> int r_size; >>>>> @@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) >>>>> ctx->rc_dev = rc_dev; >>>>> } >>>>> >>>>> +static void sii8620_cable_out(struct sii8620 *ctx) >>>>> +{ >>>>> + disable_irq(to_i2c_client(ctx->dev)->irq); >>>>> + sii8620_hw_off(ctx); >>>>> +} >>>>> + >>>>> +static void sii8620_extcon_work(struct work_struct *work) >>>>> +{ >>>>> + struct sii8620 *ctx = >>>>> + container_of(work, struct sii8620, extcon_wq); >>>>> + int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL); >>>>> + >>>>> + if (state == ctx->cable_state) >>>>> + return; >>>>> + >>>>> + ctx->cable_state = state; >>>>> + >>>>> + if (state > 0) >>>>> + sii8620_cable_in(ctx); >>>>> + else >>>>> + sii8620_cable_out(ctx); >>>>> +} >>>>> + >>>>> +static int sii8620_extcon_notifier(struct notifier_block *self, >>>>> + unsigned long event, void *ptr) >>>>> +{ >>>>> + struct sii8620 *ctx = >>>>> + container_of(self, struct sii8620, extcon_nb); >>>>> + >>>>> + schedule_work(&ctx->extcon_wq); >>>>> + >>>>> + return NOTIFY_DONE; >>>>> +} >>>>> + >>>>> +static int sii8620_extcon_init(struct sii8620 *ctx) >>>>> +{ >>>>> + struct extcon_dev *edev; >>>>> + struct device_node *musb, *muic; >>>>> + int ret; >>>>> + >>>>> + /* get micro-USB connector node */ >>>>> + musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1); >>>>> + /* next get micro-USB Interface Controller node */ >>>>> + muic = of_get_next_parent(musb); >>>>> + >>>>> + if (!muic) { >>>>> + dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n"); >>>>> + return 0; >>>>> + } >>>>> + >>>>> + edev = extcon_find_edev_by_node(muic); >>>>> + of_node_put(muic); >>>>> + if (IS_ERR(edev)) { >>>>> + if (PTR_ERR(edev) == -EPROBE_DEFER) >>>>> + return -EPROBE_DEFER; >>>>> + dev_err(ctx->dev, "Invalid or missing extcon\n"); >>>>> + return PTR_ERR(edev); >>>>> + } >>>>> + >>>>> + ctx->extcon = edev; >>>>> + ctx->extcon_nb.notifier_call = sii8620_extcon_notifier; >>>>> + INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work); >>>>> + ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb); >>>> You better to use devm_extcon_register_notifier(). >>> With devm version I risk that in case of device unbind notification will >>> be called after .remove callback, it seems to me quite dangerous. Or am >>> I missing something? >> If you use the cancel_work_sync() in remove() instead of flush_work(), >> you can use the 'devm_extcon_*'. > > cancel_work_sync() does not prevent works scheduled later from execution > [1] and this scenario is possible with devm_extcon_register_notifier() > and cancel_work_sync(). > So we end up with: > sii8620_remove() calls cancel_work_sync() > ... > notifier(called asynchronously) schedules sii8620_extcon_work() > ... > notifier is removed by devm framework > sii8620 context is destroyed by devm framework > ... > sii8620_extcon_work is executed on destroyed context !!! BUG !!! > > For me it seems that devm_extcon_register_notifier is not safe in this case. > > [1]: Since documentation was not clear I have performed live test > confirming my suspicions. You're right. Sorry for confusion. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> [snip]
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c index 9e785b8e0ea2..62b0adabcac2 100644 --- a/drivers/gpu/drm/bridge/sil-sii8620.c +++ b/drivers/gpu/drm/bridge/sil-sii8620.c @@ -17,6 +17,7 @@ #include <linux/clk.h> #include <linux/delay.h> +#include <linux/extcon.h> #include <linux/gpio/consumer.h> #include <linux/i2c.h> #include <linux/interrupt.h> @@ -25,6 +26,7 @@ #include <linux/list.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/of_graph.h> #include <linux/regulator/consumer.h> #include <linux/slab.h> @@ -81,6 +83,10 @@ struct sii8620 { struct edid *edid; unsigned int gen2_write_burst:1; enum sii8620_mt_state mt_state; + struct extcon_dev *extcon; + struct notifier_block extcon_nb; + struct work_struct extcon_wq; + int cable_state; struct list_head mt_queue; struct { int r_size; @@ -2175,6 +2181,77 @@ static void sii8620_init_rcp_input_dev(struct sii8620 *ctx) ctx->rc_dev = rc_dev; } +static void sii8620_cable_out(struct sii8620 *ctx) +{ + disable_irq(to_i2c_client(ctx->dev)->irq); + sii8620_hw_off(ctx); +} + +static void sii8620_extcon_work(struct work_struct *work) +{ + struct sii8620 *ctx = + container_of(work, struct sii8620, extcon_wq); + int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL); + + if (state == ctx->cable_state) + return; + + ctx->cable_state = state; + + if (state > 0) + sii8620_cable_in(ctx); + else + sii8620_cable_out(ctx); +} + +static int sii8620_extcon_notifier(struct notifier_block *self, + unsigned long event, void *ptr) +{ + struct sii8620 *ctx = + container_of(self, struct sii8620, extcon_nb); + + schedule_work(&ctx->extcon_wq); + + return NOTIFY_DONE; +} + +static int sii8620_extcon_init(struct sii8620 *ctx) +{ + struct extcon_dev *edev; + struct device_node *musb, *muic; + int ret; + + /* get micro-USB connector node */ + musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1); + /* next get micro-USB Interface Controller node */ + muic = of_get_next_parent(musb); + + if (!muic) { + dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n"); + return 0; + } + + edev = extcon_find_edev_by_node(muic); + of_node_put(muic); + if (IS_ERR(edev)) { + if (PTR_ERR(edev) == -EPROBE_DEFER) + return -EPROBE_DEFER; + dev_err(ctx->dev, "Invalid or missing extcon\n"); + return PTR_ERR(edev); + } + + ctx->extcon = edev; + ctx->extcon_nb.notifier_call = sii8620_extcon_notifier; + INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work); + ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb); + if (ret) { + dev_err(ctx->dev, "failed to register notifier for MHL\n"); + return ret; + } + + return 0; +} + static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge) { return container_of(bridge, struct sii8620, bridge); @@ -2307,13 +2384,20 @@ static int sii8620_probe(struct i2c_client *client, if (ret) return ret; + ret = sii8620_extcon_init(ctx); + if (ret < 0) { + dev_err(ctx->dev, "failed to initialize EXTCON\n"); + return ret; + } + i2c_set_clientdata(client, ctx); ctx->bridge.funcs = &sii8620_bridge_funcs; ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge); - sii8620_cable_in(ctx); + if (!ctx->extcon) + sii8620_cable_in(ctx); return 0; } @@ -2322,8 +2406,15 @@ static int sii8620_remove(struct i2c_client *client) { struct sii8620 *ctx = i2c_get_clientdata(client); - disable_irq(to_i2c_client(ctx->dev)->irq); - sii8620_hw_off(ctx); + if (ctx->extcon) { + extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL, + &ctx->extcon_nb); + flush_work(&ctx->extcon_wq); + if (ctx->cable_state > 0) + sii8620_cable_out(ctx); + } else { + sii8620_cable_out(ctx); + } drm_bridge_remove(&ctx->bridge); return 0;