On Thu, Feb 14, 2013 at 02:37:42PM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen@xxxxxxxxxxxxx> writes:
>
> > @@ -130,6 +130,7 @@ struct hw_bank {
> > * @transceiver: pointer to USB PHY, if any
> > * @hcd: pointer to usb_hcd for ehci host driver
> > * @otg: for otg support
> > + * @events: events for otg, and handled at ci_role_work
>
> Should be id_event and b_sess_valid_event.
Will change
>
> If you reordered the patchset so that the role api patch comes before
> this one, this function would have been much shorter from the start. Now
> it looks like you're adding lots of code in one patch and then replace
> and reshuffle it in subsequent patches, which makes it really difficult
> to review.
I will merge the 3/8 and 4/8 to one patch.
> > +static void ci_delayed_work(struct work_struct *work)
> > +{
> > + struct delayed_work *dwork = to_delayed_work(work);
> > + struct ci13xxx *ci = container_of(dwork, struct ci13xxx, dwork);
> > +
> > + otg_set_vbus(&ci->otg, true);
> > +
> > +}
>
> That's one newline too many.
Will change
>
> > +
> > static ssize_t show_role(struct device *dev, struct device_attribute *attr,
> > char *buf)
> > {
> > @@ -352,25 +469,49 @@ 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_supports_gadget(struct ci13xxx *ci)
> > +{
> > + return (ci->roles[CI_ROLE_GADGET]) ? true : false;
>
I prefer to keep it
> > +}
> > +
> > static irqreturn_t ci_irq(int irq, void *data)
> > {
> > struct ci13xxx *ci = data;
> > irqreturn_t ret = IRQ_NONE;
> > u32 otgsc = 0;
> >
> > - if (ci->is_otg)
> > + if (ci_supports_gadget(ci))
>
> We can't do this since there are indeed devices out there that support
> gadget and don't support otg to such an extend that OTGSC accesses are
> discouraged. We should really make sure that we're only touching it on
> otg capable devices.
>
OK, for such devices they need external gpio to judge connection
and disconnection.
I will replace ci_supports_gadget(ci) to ci_supports_otg(ci), it
indicates the controller supports both host and devices. Currently,
the ci->is_otg indicates the driver has both host and device funcitons.
--
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
