On Thu, Mar 07, 2013 at 01:50:24PM +0800, Peter Chen wrote:
> On Wed, Mar 06, 2013 at 07:09:34PM +0200, Alexander Shishkin wrote:
> > > - start/stop API are used at otg id switch and probe routine
> > >
> > > - Defer some gadget operations at ci's delayed work queue
> >
> > When I asked you to reorder patches, I didn't mean squash them all
> > into one. Now you have a patch that does 5 different things and none
> > of them properly, because you still need to amend these changes later
> > in the patchset, like the patch 7, where you remove the delayed work,
> > which is added in this patch. At the same time, you have bits in this
> > patch that should be moved to other patches. See comments below.
> >
>
> I am sorry to let you review difficult, as I changed/added patch according
> to function change, but not updated previous patches.
>
> I will re-organize all patches according to functionalities.
> Besides, can you help review all patches during next time, in that case,
> I can know which patch is ok.
>
> >
> > You remove this function in patch 7. Why add it in the first place?
> >
>
> As it is needed at patch 7 to let the function work
>
> > > +
> > > +static inline void ci_role_destroy(struct ci13xxx *ci)
> > > +{
> > > + ci_hdrc_gadget_destroy(ci);
> > > + ci_hdrc_host_destroy(ci);
> > > +}
> >
> > You shouldn't use "inline" here. And the name of the function is also
> > ambiguous, since it destroys all roles. It would be better to just
> > call both _destroys(), like I suggested in the diff I gave you some
> > time ago.
>
> Will change.
>
> >
> > > +
> > > static ssize_t show_role(struct device *dev, struct device_attribute *attr,
> > > char *buf)
> > > {
> > > @@ -352,25 +468,50 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr,
> > >
> > > static DEVICE_ATTR(role, S_IRUSR | S_IWUSR, show_role, store_role);
> > >
> > > +static bool ci_is_otg_capable(struct ci13xxx *ci)
> > > +{
> > > + return hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC | DCCPARAMS_HC)
> > > + == (DCCPARAMS_DC | DCCPARAMS_HC);
> > > +}
> >
> > Can't we
> > a) remember this value so that we don't have to read the register at
> > every interrupt
>
> Yes.
>
> > b) also take into account the fact that DCCPARAMS doesn't read
> > correctly on all chipideas? (see dr_mode patches)
> >
>
> We have discussed it, but you have not given me exactly answer
> "Which situation we can read OTGSC"?
>
> 1. We Can't read OTGSC if DCCPARAMS shows it is only device-capable
> 2. DCCPARAMS doesn't correctly for some chips
> 3. If we depend on dr_mode, OTGSC should be read when dr_mode = "peripheral",
> in that case, the condition is ci->roles[CI_ROLE_GADGET].
>
> If the dr_mode is not set by DT, the 1 and 3 is conflict.
>
>
> > >
> > > - if (ci->is_otg)
> > > + if (ci_is_otg_capable(ci))
> >
> > ci->is_otg is actually the right thing to use, we should instead make
> > sure that it's set properly
> >
>
> Yes, what is ci->is_otg stands for in your opinion?
> In my patchset, it means it supportes partial OTG (id switch) function.
>
> > > if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
> > > + dev_dbg(dev, "support otg\n");
> > > ci->is_otg = true;
> > > + /* if otg is supported, create struct usb_otg */
> > > + ci_hdrc_otg_init(ci);
> > > /* ID pin needs 1ms debouce time, we delay 2ms for safe */
> > > mdelay(2);
> > > ci->role = ci_otg_role(ci);
> >
> > I'm thinking that this should be based on dr_mode patches and not the
> > other way around.
> >
>
> No, ci->role means which role is currently.
> And it is otg mode already as
> (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]).
>
> > >
> > > static int ci_otg_set_peripheral(struct usb_otg *otg,
> > > struct usb_gadget *periph)
> > > @@ -55,6 +62,7 @@ int ci_hdrc_otg_init(struct ci13xxx *ci)
> > > ci->otg.set_host = ci_otg_set_host;
> > > if (!IS_ERR_OR_NULL(ci->transceiver))
> > > ci->transceiver->otg = &ci->otg;
> > > + ci_enable_otg_interrupt(ci, OTGSC_IDIE);
> >
> > There should be a counter-action to ci_hdrc_otg_init().
> > Btw, why can't clear_otg_interrupt() be called from here as well
> > instead of hw_device_init()?
>
> What the criterion for reading OTGSC?
> What means otg capable and how do we know it is otg capable?
>
> > This function should only be called for
> > otg capable devices, so it should be safe. And while at it, I think
> > this whole otg.c/otg.h part of this patch should be moved to patch 2.
> >
>
> OK.
>
> > >
> > > +static int udc_id_switch_for_device(struct ci13xxx *ci)
> >
> > I would use _to_ instead of _for_, for clarity.
> >
> > > +{
> > > + ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
> > > + ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void udc_id_switch_for_host(struct ci13xxx *ci)
> >
Hi Alan,
Would you like to answer my questions, esp for
"the situation to read OTGSC", we discussed it lots of times,
but seems has no conclusion.
Thanks.
--
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
