On Fri, Aug 06, 2010 at 05:51:38PM +0800, Wu, Hao wrote:
> Hi, Greg
>
> Thanks a lot for code review.
>
> >> + * the Intel MID (Langwell/Penwell) otg transceiver driver needs to interact
> >> + * with device and host drivers to implement the USB OTG related feature. More
> >> + * function members are added based on otg_transceiver data structure for this
> >> + * purpose.
> >> + */
> >> +struct intel_mid_otg_xceiv {
> >> + struct otg_transceiver otg;
> >> + struct otg_hsm hsm;
> >> +
> >> + /* base address */
> >> + void __iomem *base;
> >> +
> >> + /* ops to access ulpi */
> >> + struct iotg_ulpi_access_ops ulpi_ops;
> >> +
> >> + /* atomic notifier for interrupt context */
> >> + struct atomic_notifier_head iotg_notifier;
> >> +
> >> + /* start/stop USB Host function */
> >> + int (*start_host)(struct intel_mid_otg_xceiv *iotg);
> >> + int (*stop_host)(struct intel_mid_otg_xceiv *iotg);
> >> +
> >> + /* start/stop USB Peripheral function */
> >> + int (*start_peripheral)(struct intel_mid_otg_xceiv *iotg);
> >> + int (*stop_peripheral)(struct intel_mid_otg_xceiv *iotg);
> >> +
> >> + /* start/stop ADP sense/probe function */
> >> + int (*set_adp_probe)(struct intel_mid_otg_xceiv *iotg,
> >> + bool enabled, int dev);
> >> + int (*set_adp_sense)(struct intel_mid_otg_xceiv *iotg,
> >> + bool enabled);
> >> +
> >> +#ifdef CONFIG_PM
> >> + /* suspend/resume USB host function */
> >> + int (*suspend_host)(struct intel_mid_otg_xceiv *iotg,
> >> + pm_message_t message);
> >> + int (*resume_host)(struct intel_mid_otg_xceiv *iotg);
> >> +
> >> + int (*suspend_peripheral)(struct intel_mid_otg_xceiv *iotg,
> >> + pm_message_t message);
> >> + int (*resume_peripheral)(struct intel_mid_otg_xceiv *iotg);
> >> +#endif
> >> +
> >> +};
> >
> >Why not just extend the internal structure so that not every OTG
> >controller has to define this type of structure? I think they should
> >all be the same, right?
>
> This data structure is only used for Intel MID Platform USB OTG controllers.
> So I did not extend the struct otg_transceiver for all OTG controllers.
> But I also think check if any member can be moved into the internal structure
> for common use will be one thing we can do in future development/improvement.
Why not do that now?
> >> +static inline int
> >> +intel_mid_otg_register_notifier(struct intel_mid_otg_xceiv *iotg,
> >> + struct notifier_block *nb)
> >> +{
> >> + return atomic_notifier_chain_register(&iotg->iotg_notifier, nb);
> >> +}
> >> +
> >> +static inline void
> >> +intel_mid_otg_unregister_notifier(struct intel_mid_otg_xceiv *iotg,
> >> + struct notifier_block *nb)
> >> +{
> >> + atomic_notifier_chain_unregister(&iotg->iotg_notifier, nb);
> >> +}
> >
> >Are these really needed?
>
> I think it will make code a little bit more readable if such wrapped inline function
> used.
No, it is obvious with the call to the atomic_notifier_chain_* function
what you are doing, don't wrap up things that don't need to be please.
thanks,
greg k-h
--
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
