Re: [PATCH 1/3] usb: chipidea: operate on otgsc register in a general way
On Wed, Mar 12, 2014 at 07:49:52PM +0800, Li Jun wrote: > On Wed, Mar 12, 2014 at 04:14:31PM +0800, Peter Chen wrote: > > On Wed, Mar 12, 2014 at 02:32:39PM +0800, Li Jun wrote: > > > From: Li Jun > > > > > > Use a more general way to read and write otgsc register. > > > > > > Signed-off-by: Li Jun > > > --- > > > drivers/usb/chipidea/core.c | 19 + > > > drivers/usb/chipidea/otg.c | 48 > > > +++ > > > drivers/usb/chipidea/otg.h | 19 +++-- > > > drivers/usb/chipidea/udc.c | 11 ++ > > > 4 files changed, 65 insertions(+), 32 deletions(-) > > > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > > index ca6831c..20be020 100644 > > > --- a/drivers/usb/chipidea/core.c > > > +++ b/drivers/usb/chipidea/core.c > > > @@ -359,16 +359,15 @@ static irqreturn_t ci_irq(int irq, void *data) > > > irqreturn_t ret = IRQ_NONE; > > > u32 otgsc = 0; > > > > > > - if (ci->is_otg) > > > - otgsc = hw_read(ci, OP_OTGSC, ~0); > > > - > > > + otgsc = hw_read_otgsc(ci); > > > /* > > >* Handle id change interrupt, it indicates device/host function > > >* switch. > > >*/ > > > if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) { > > > ci->id_event = true; > > > - ci_clear_otg_interrupt(ci, OTGSC_IDIS); > > > + /* Clear ID change irq status */ > > > + hw_set_otgsc_bits(ci, OTGSC_IDIS); > > > disable_irq_nosync(ci->irq); > > > queue_work(ci->wq, &ci->work); > > > return IRQ_HANDLED; > > > @@ -380,7 +379,8 @@ static irqreturn_t ci_irq(int irq, void *data) > > >*/ > > > if (ci->is_otg && (otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) { > > > ci->b_sess_valid_event = true; > > > - ci_clear_otg_interrupt(ci, OTGSC_BSVIS); > > > + /* Clear BSV irq */ > > > + hw_set_otgsc_bits(ci, OTGSC_BSVIS); > > > disable_irq_nosync(ci->irq); > > > queue_work(ci->wq, &ci->work); > > > return IRQ_HANDLED; > > > @@ -502,8 +502,10 @@ static void ci_get_otg_capable(struct ci_hdrc *ci) > > > == (DCCPARAMS_DC | DCCPARAMS_HC)); > > > if (ci->is_otg) { > > > dev_dbg(ci->dev, "It is OTG capable controller\n"); > > > - ci_disable_otg_interrupt(ci, OTGSC_INT_EN_BITS); > > > - ci_clear_otg_interrupt(ci, OTGSC_INT_STATUS_BITS); > > > + /* Disable all OTG irq */ > > > + hw_clear_otgsc_bits(ci, OTGSC_INT_EN_BITS); > > > + /* Clear all OTG irq status */ > > > + hw_set_otgsc_bits(ci, OTGSC_INT_STATUS_BITS); > > > } > > > } > > > > > > @@ -617,7 +619,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > >*/ > > > mdelay(2); > > > ci->role = ci_otg_role(ci); > > > - ci_enable_otg_interrupt(ci, OTGSC_IDIE); > > > + /* Enable ID change irq */ > > > + hw_set_otgsc_bits(ci, OTGSC_IDIE); > > > } else { > > > /* > > >* If the controller is not OTG capable, but support > > > diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c > > > index 39bd7ec..f214ade 100644 > > > --- a/drivers/usb/chipidea/otg.c > > > +++ b/drivers/usb/chipidea/otg.c > > > @@ -24,12 +24,50 @@ > > > #include "otg.h" > > > > > > /** > > > + * hw_read_otgsc: returns otgsc register > > > + * > > > + * This function returns register data > > > + */ > > > > register contents, I copied from drivers/usb/chipidea/ci.h :). > > > will update. > > > > +u32 hw_read_otgsc(struct ci_hdrc *ci) > > > +{ > > > + if (ci->is_otg) > > > + return hw_read(ci, OP_OTGSC, ~0); > > > + else > > > + return -ENOTSUPP; > > > +} > > > + > > > +/** > > > + * hw_set_otgsc_bits > > > + * > > > + * This function sets target bits of OTGSC register, > > > + * any bits within OTGSC_INT_STATUS_BITS will be cleared, > > > + * so use this func to clear irq status instead of hw_clear_otgsc_bits. > > > + */ > > > +void hw_set_otgsc_bits(struct ci_hdrc *ci, u32 bits) > > > +{ > > > + if (ci->is_otg) > > > + hw_write(ci, OP_OTGSC, bits | OTGSC_INT_STATUS_BITS, bits); > > > +} > > > + > > > +/** > > > + * hw_clear_otgsc_bits > > > + * > > > + * This function clear target bits of OTGSC register, > > > + * Note:any bits within OTGSC_INT_STATUS_BITS will not be cleared. > > > + */ > > > +void hw_clear_otgsc_bits(struct ci_hdrc *ci, u32 bits) > > > +{ > > > + if (ci->is_otg) > > > + hw_write(ci, OP_OTGSC, bits | OTGSC_INT_STATUS_BITS, 0); > > > +} > > > + > > > > - The caller should make sure the otgsc access under the condition of > > ci->is_otg > > so do not need to add ci->is_otg at your APIs. > > > In that way, have to add if(ci->is_otg) everywhere access to otgsc register, > I should remove all "if(ci->is_otg)" at calling place in current driv
Re: [PATCH 1/3] usb: chipidea: operate on otgsc register in a general way
On Wed, Mar 12, 2014 at 04:14:31PM +0800, Peter Chen wrote: > On Wed, Mar 12, 2014 at 02:32:39PM +0800, Li Jun wrote: > > From: Li Jun > > > > Use a more general way to read and write otgsc register. > > > > Signed-off-by: Li Jun > > --- > > drivers/usb/chipidea/core.c | 19 + > > drivers/usb/chipidea/otg.c | 48 > > +++ > > drivers/usb/chipidea/otg.h | 19 +++-- > > drivers/usb/chipidea/udc.c | 11 ++ > > 4 files changed, 65 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > index ca6831c..20be020 100644 > > --- a/drivers/usb/chipidea/core.c > > +++ b/drivers/usb/chipidea/core.c > > @@ -359,16 +359,15 @@ static irqreturn_t ci_irq(int irq, void *data) > > irqreturn_t ret = IRQ_NONE; > > u32 otgsc = 0; > > > > - if (ci->is_otg) > > - otgsc = hw_read(ci, OP_OTGSC, ~0); > > - > > + otgsc = hw_read_otgsc(ci); > > /* > > * Handle id change interrupt, it indicates device/host function > > * switch. > > */ > > if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) { > > ci->id_event = true; > > - ci_clear_otg_interrupt(ci, OTGSC_IDIS); > > + /* Clear ID change irq status */ > > + hw_set_otgsc_bits(ci, OTGSC_IDIS); > > disable_irq_nosync(ci->irq); > > queue_work(ci->wq, &ci->work); > > return IRQ_HANDLED; > > @@ -380,7 +379,8 @@ static irqreturn_t ci_irq(int irq, void *data) > > */ > > if (ci->is_otg && (otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) { > > ci->b_sess_valid_event = true; > > - ci_clear_otg_interrupt(ci, OTGSC_BSVIS); > > + /* Clear BSV irq */ > > + hw_set_otgsc_bits(ci, OTGSC_BSVIS); > > disable_irq_nosync(ci->irq); > > queue_work(ci->wq, &ci->work); > > return IRQ_HANDLED; > > @@ -502,8 +502,10 @@ static void ci_get_otg_capable(struct ci_hdrc *ci) > > == (DCCPARAMS_DC | DCCPARAMS_HC)); > > if (ci->is_otg) { > > dev_dbg(ci->dev, "It is OTG capable controller\n"); > > - ci_disable_otg_interrupt(ci, OTGSC_INT_EN_BITS); > > - ci_clear_otg_interrupt(ci, OTGSC_INT_STATUS_BITS); > > + /* Disable all OTG irq */ > > + hw_clear_otgsc_bits(ci, OTGSC_INT_EN_BITS); > > + /* Clear all OTG irq status */ > > + hw_set_otgsc_bits(ci, OTGSC_INT_STATUS_BITS); > > } > > } > > > > @@ -617,7 +619,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > */ > > mdelay(2); > > ci->role = ci_otg_role(ci); > > - ci_enable_otg_interrupt(ci, OTGSC_IDIE); > > + /* Enable ID change irq */ > > + hw_set_otgsc_bits(ci, OTGSC_IDIE); > > } else { > > /* > > * If the controller is not OTG capable, but support > > diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c > > index 39bd7ec..f214ade 100644 > > --- a/drivers/usb/chipidea/otg.c > > +++ b/drivers/usb/chipidea/otg.c > > @@ -24,12 +24,50 @@ > > #include "otg.h" > > > > /** > > + * hw_read_otgsc: returns otgsc register > > + * > > + * This function returns register data > > + */ > > register contents, I copied from drivers/usb/chipidea/ci.h :). > will update. > > +u32 hw_read_otgsc(struct ci_hdrc *ci) > > +{ > > + if (ci->is_otg) > > + return hw_read(ci, OP_OTGSC, ~0); > > + else > > + return -ENOTSUPP; > > +} > > + > > +/** > > + * hw_set_otgsc_bits > > + * > > + * This function sets target bits of OTGSC register, > > + * any bits within OTGSC_INT_STATUS_BITS will be cleared, > > + * so use this func to clear irq status instead of hw_clear_otgsc_bits. > > + */ > > +void hw_set_otgsc_bits(struct ci_hdrc *ci, u32 bits) > > +{ > > + if (ci->is_otg) > > + hw_write(ci, OP_OTGSC, bits | OTGSC_INT_STATUS_BITS, bits); > > +} > > + > > +/** > > + * hw_clear_otgsc_bits > > + * > > + * This function clear target bits of OTGSC register, > > + * Note:any bits within OTGSC_INT_STATUS_BITS will not be cleared. > > + */ > > +void hw_clear_otgsc_bits(struct ci_hdrc *ci, u32 bits) > > +{ > > + if (ci->is_otg) > > + hw_write(ci, OP_OTGSC, bits | OTGSC_INT_STATUS_BITS, 0); > > +} > > + > > - The caller should make sure the otgsc access under the condition of > ci->is_otg > so do not need to add ci->is_otg at your APIs. > In that way, have to add if(ci->is_otg) everywhere access to otgsc register, I should remove all "if(ci->is_otg)" at calling place in current driver with this patch. Which way you prefer, move out to this API like I do or keep them? Li Jun > - It may confuse the user that there are two APIs for writing otgsc, and > he must use hw_set_otg_bits to clea
Re: [PATCH 1/3] usb: chipidea: operate on otgsc register in a general way
On Wed, Mar 12, 2014 at 02:32:39PM +0800, Li Jun wrote: > From: Li Jun > > Use a more general way to read and write otgsc register. > > Signed-off-by: Li Jun > --- > drivers/usb/chipidea/core.c | 19 + > drivers/usb/chipidea/otg.c | 48 > +++ > drivers/usb/chipidea/otg.h | 19 +++-- > drivers/usb/chipidea/udc.c | 11 ++ > 4 files changed, 65 insertions(+), 32 deletions(-) > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index ca6831c..20be020 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -359,16 +359,15 @@ static irqreturn_t ci_irq(int irq, void *data) > irqreturn_t ret = IRQ_NONE; > u32 otgsc = 0; > > - if (ci->is_otg) > - otgsc = hw_read(ci, OP_OTGSC, ~0); > - > + otgsc = hw_read_otgsc(ci); > /* >* Handle id change interrupt, it indicates device/host function >* switch. >*/ > if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) { > ci->id_event = true; > - ci_clear_otg_interrupt(ci, OTGSC_IDIS); > + /* Clear ID change irq status */ > + hw_set_otgsc_bits(ci, OTGSC_IDIS); > disable_irq_nosync(ci->irq); > queue_work(ci->wq, &ci->work); > return IRQ_HANDLED; > @@ -380,7 +379,8 @@ static irqreturn_t ci_irq(int irq, void *data) >*/ > if (ci->is_otg && (otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) { > ci->b_sess_valid_event = true; > - ci_clear_otg_interrupt(ci, OTGSC_BSVIS); > + /* Clear BSV irq */ > + hw_set_otgsc_bits(ci, OTGSC_BSVIS); > disable_irq_nosync(ci->irq); > queue_work(ci->wq, &ci->work); > return IRQ_HANDLED; > @@ -502,8 +502,10 @@ static void ci_get_otg_capable(struct ci_hdrc *ci) > == (DCCPARAMS_DC | DCCPARAMS_HC)); > if (ci->is_otg) { > dev_dbg(ci->dev, "It is OTG capable controller\n"); > - ci_disable_otg_interrupt(ci, OTGSC_INT_EN_BITS); > - ci_clear_otg_interrupt(ci, OTGSC_INT_STATUS_BITS); > + /* Disable all OTG irq */ > + hw_clear_otgsc_bits(ci, OTGSC_INT_EN_BITS); > + /* Clear all OTG irq status */ > + hw_set_otgsc_bits(ci, OTGSC_INT_STATUS_BITS); > } > } > > @@ -617,7 +619,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) >*/ > mdelay(2); > ci->role = ci_otg_role(ci); > - ci_enable_otg_interrupt(ci, OTGSC_IDIE); > + /* Enable ID change irq */ > + hw_set_otgsc_bits(ci, OTGSC_IDIE); > } else { > /* >* If the controller is not OTG capable, but support > diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c > index 39bd7ec..f214ade 100644 > --- a/drivers/usb/chipidea/otg.c > +++ b/drivers/usb/chipidea/otg.c > @@ -24,12 +24,50 @@ > #include "otg.h" > > /** > + * hw_read_otgsc: returns otgsc register > + * > + * This function returns register data > + */ register contents, I copied from drivers/usb/chipidea/ci.h :). > +u32 hw_read_otgsc(struct ci_hdrc *ci) > +{ > + if (ci->is_otg) > + return hw_read(ci, OP_OTGSC, ~0); > + else > + return -ENOTSUPP; > +} > + > +/** > + * hw_set_otgsc_bits > + * > + * This function sets target bits of OTGSC register, > + * any bits within OTGSC_INT_STATUS_BITS will be cleared, > + * so use this func to clear irq status instead of hw_clear_otgsc_bits. > + */ > +void hw_set_otgsc_bits(struct ci_hdrc *ci, u32 bits) > +{ > + if (ci->is_otg) > + hw_write(ci, OP_OTGSC, bits | OTGSC_INT_STATUS_BITS, bits); > +} > + > +/** > + * hw_clear_otgsc_bits > + * > + * This function clear target bits of OTGSC register, > + * Note:any bits within OTGSC_INT_STATUS_BITS will not be cleared. > + */ > +void hw_clear_otgsc_bits(struct ci_hdrc *ci, u32 bits) > +{ > + if (ci->is_otg) > + hw_write(ci, OP_OTGSC, bits | OTGSC_INT_STATUS_BITS, 0); > +} > + - The caller should make sure the otgsc access under the condition of ci->is_otg so do not need to add ci->is_otg at your APIs. - It may confuse the user that there are two APIs for writing otgsc, and he must use hw_set_otg_bits to clear interrupt, how about using below two APIs, it more likes current register usage. u32 hw_read_otgsc(struct ci_hdrc *ci, u32 mask) { return hw_read(ci, OP_OTGSC, mask); } void hw_write_otgsc(struct ci_hdrc *ci, u32 mask, u32 bits) { hw_write(ci, OP_OTGSC, mask | OTGSC_INT_STATUS_BITS, bits); } > +/** > * ci_otg_role - pick role based on ID pin state > * @ci: the controller > */ > enum ci_role ci_otg_role(struct ci_hdrc *ci) > { > -
[PATCH 1/3] usb: chipidea: operate on otgsc register in a general way
From: Li Jun Use a more general way to read and write otgsc register. Signed-off-by: Li Jun --- drivers/usb/chipidea/core.c | 19 + drivers/usb/chipidea/otg.c | 48 +++ drivers/usb/chipidea/otg.h | 19 +++-- drivers/usb/chipidea/udc.c | 11 ++ 4 files changed, 65 insertions(+), 32 deletions(-) diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index ca6831c..20be020 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -359,16 +359,15 @@ static irqreturn_t ci_irq(int irq, void *data) irqreturn_t ret = IRQ_NONE; u32 otgsc = 0; - if (ci->is_otg) - otgsc = hw_read(ci, OP_OTGSC, ~0); - + otgsc = hw_read_otgsc(ci); /* * Handle id change interrupt, it indicates device/host function * switch. */ if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) { ci->id_event = true; - ci_clear_otg_interrupt(ci, OTGSC_IDIS); + /* Clear ID change irq status */ + hw_set_otgsc_bits(ci, OTGSC_IDIS); disable_irq_nosync(ci->irq); queue_work(ci->wq, &ci->work); return IRQ_HANDLED; @@ -380,7 +379,8 @@ static irqreturn_t ci_irq(int irq, void *data) */ if (ci->is_otg && (otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) { ci->b_sess_valid_event = true; - ci_clear_otg_interrupt(ci, OTGSC_BSVIS); + /* Clear BSV irq */ + hw_set_otgsc_bits(ci, OTGSC_BSVIS); disable_irq_nosync(ci->irq); queue_work(ci->wq, &ci->work); return IRQ_HANDLED; @@ -502,8 +502,10 @@ static void ci_get_otg_capable(struct ci_hdrc *ci) == (DCCPARAMS_DC | DCCPARAMS_HC)); if (ci->is_otg) { dev_dbg(ci->dev, "It is OTG capable controller\n"); - ci_disable_otg_interrupt(ci, OTGSC_INT_EN_BITS); - ci_clear_otg_interrupt(ci, OTGSC_INT_STATUS_BITS); + /* Disable all OTG irq */ + hw_clear_otgsc_bits(ci, OTGSC_INT_EN_BITS); + /* Clear all OTG irq status */ + hw_set_otgsc_bits(ci, OTGSC_INT_STATUS_BITS); } } @@ -617,7 +619,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) */ mdelay(2); ci->role = ci_otg_role(ci); - ci_enable_otg_interrupt(ci, OTGSC_IDIE); + /* Enable ID change irq */ + hw_set_otgsc_bits(ci, OTGSC_IDIE); } else { /* * If the controller is not OTG capable, but support diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c index 39bd7ec..f214ade 100644 --- a/drivers/usb/chipidea/otg.c +++ b/drivers/usb/chipidea/otg.c @@ -24,12 +24,50 @@ #include "otg.h" /** + * hw_read_otgsc: returns otgsc register + * + * This function returns register data + */ +u32 hw_read_otgsc(struct ci_hdrc *ci) +{ + if (ci->is_otg) + return hw_read(ci, OP_OTGSC, ~0); + else + return -ENOTSUPP; +} + +/** + * hw_set_otgsc_bits + * + * This function sets target bits of OTGSC register, + * any bits within OTGSC_INT_STATUS_BITS will be cleared, + * so use this func to clear irq status instead of hw_clear_otgsc_bits. + */ +void hw_set_otgsc_bits(struct ci_hdrc *ci, u32 bits) +{ + if (ci->is_otg) + hw_write(ci, OP_OTGSC, bits | OTGSC_INT_STATUS_BITS, bits); +} + +/** + * hw_clear_otgsc_bits + * + * This function clear target bits of OTGSC register, + * Note:any bits within OTGSC_INT_STATUS_BITS will not be cleared. + */ +void hw_clear_otgsc_bits(struct ci_hdrc *ci, u32 bits) +{ + if (ci->is_otg) + hw_write(ci, OP_OTGSC, bits | OTGSC_INT_STATUS_BITS, 0); +} + +/** * ci_otg_role - pick role based on ID pin state * @ci: the controller */ enum ci_role ci_otg_role(struct ci_hdrc *ci) { - u32 sts = hw_read(ci, OP_OTGSC, ~0); + u32 sts = hw_read_otgsc(ci); enum ci_role role = sts & OTGSC_ID ? CI_ROLE_GADGET : CI_ROLE_HOST; @@ -44,7 +82,7 @@ void ci_handle_vbus_change(struct ci_hdrc *ci) if (!ci->is_otg) return; - otgsc = hw_read(ci, OP_OTGSC, ~0); + otgsc = hw_read_otgsc(ci); if (otgsc & OTGSC_BSV) usb_gadget_vbus_connect(&ci->gadget); @@ -115,6 +153,8 @@ void ci_hdrc_otg_destroy(struct ci_hdrc *ci) flush_workqueue(ci->wq); destroy_workqueue(ci->wq); } - ci_disable_otg_interrupt(ci, OTGSC_INT_EN_BITS); - ci_clear_otg_interrupt(ci, OTGSC_INT_STATUS_BITS); + /* Disable all OTG irq */ + hw_clear_otgsc_bits(ci, OT