Re: [PATCH 20/25] From dbe448efe7c4459a1cb6a25ab0a6ef41272757cf Mon Sep 17 00:00:00 2001

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux