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 <b47624@xxxxxxxxxxxxx>
> >
> > Use a more general way to read and write otgsc register.
> >
> > Signed-off-by: Li Jun <b47624@xxxxxxxxxxxxx>
> > ---
> > 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 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);
> }
For read, I will add mask.
>
> void hw_write_otgsc(struct ci_hdrc *ci, u32 mask, u32 bits)
> {
> hw_write(ci, OP_OTGSC, mask | OTGSC_INT_STATUS_BITS, bits);
> }
>
For write,
I had that kind of idea in my initial implementation, which is not
straightforward when using it, user need take care mask anyway.
One API for write will make it close to hw_write(), just skip
one para:OP_OTGSC
Li Jun
> > +/**
> > * 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, OTGSC_INT_EN_BITS);
> > + /* Clear all OTG irq status */
> > + hw_set_otgsc_bits(ci, OTGSC_INT_STATUS_BITS);
> > }
> > diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h
> > index 449bee0..024d1e8 100644
> > --- a/drivers/usb/chipidea/otg.h
> > +++ b/drivers/usb/chipidea/otg.h
> > @@ -11,22 +11,9 @@
> > #ifndef __DRIVERS_USB_CHIPIDEA_OTG_H
> > #define __DRIVERS_USB_CHIPIDEA_OTG_H
> >
> > -static inline void ci_clear_otg_interrupt(struct ci_hdrc *ci, u32 bits)
> > -{
> > - /* Only clear request bits */
> > - hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, bits);
> > -}
> > -
> > -static inline void ci_enable_otg_interrupt(struct ci_hdrc *ci, u32 bits)
> > -{
> > - hw_write(ci, OP_OTGSC, bits | OTGSC_INT_STATUS_BITS, bits);
> > -}
> > -
> > -static inline void ci_disable_otg_interrupt(struct ci_hdrc *ci, u32 bits)
> > -{
> > - hw_write(ci, OP_OTGSC, bits | OTGSC_INT_STATUS_BITS, 0);
> > -}
> > -
> > +u32 hw_read_otgsc(struct ci_hdrc *ci);
> > +void hw_set_otgsc_bits(struct ci_hdrc *ci, u32 bits);
> > +void hw_clear_otgsc_bits(struct ci_hdrc *ci, u32 bits);
> > int ci_hdrc_otg_init(struct ci_hdrc *ci);
> > void ci_hdrc_otg_destroy(struct ci_hdrc *ci);
> > enum ci_role ci_otg_role(struct ci_hdrc *ci);
> > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > index 7739c64..6a847c2 100644
> > --- a/drivers/usb/chipidea/udc.c
> > +++ b/drivers/usb/chipidea/udc.c
> > @@ -1844,8 +1844,10 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
> > static int udc_id_switch_for_device(struct ci_hdrc *ci)
> > {
> > if (ci->is_otg) {
> > - ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
> > - ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
> > + /* Clear BSV irq */
> > + hw_set_otgsc_bits(ci, OTGSC_BSVIS);
> > + /* Enable BSV irq */
> > + hw_set_otgsc_bits(ci, OTGSC_BSVIE);
> > }
> >
> > return 0;
> > @@ -1855,8 +1857,9 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci)
> > {
> > if (ci->is_otg) {
> > /* host doesn't care B_SESSION_VALID event */
> > - ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
> > - ci_disable_otg_interrupt(ci, OTGSC_BSVIE);
> > + hw_set_otgsc_bits(ci, OTGSC_BSVIS);
> > + /* Disable BSV irq */
> > + hw_clear_otgsc_bits(ci, OTGSC_BSVIE);
> > }
> > }
> >
> > --
> > 1.7.9.5
> >
> >
>
> --
>
> Best Regards,
> Peter Chen
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
