Message ID | 1568043974-1236-2-git-send-email-external.veeraiyan.c@de.adit-jv.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | [v4,1/3] usb: renesas_usbhs: simplify usbhs_status_get_device_state() | expand |
Hi Veeraiyan, Thank you for the patch! > From: Veeraiyan Chidambaram, Sent: Tuesday, September 10, 2019 12:46 AM <snip> > Commit [1] enabled the possibility of checking the DVST (Device State > Transition) bit of INTSTS0 (Interrupt Status Register 0) and calling > the irq_dev_state() handler if the DVST bit is set. But neither > commit [1] nor commit [2] actually enabled the DVSE (Device State > Transition Interrupt Enable) bit in the INTENB0 (Interrupt Enable > Register 0). As a consequence, irq_dev_state() handler is getting > called as a side effect of other (non-DVSE) interrupts being fired, > which definitely can't be relied upon, if DVST notifications are of > any value. > > Why this doesn't hurt is because usbhsg_irq_dev_state() currently > doesn't do much except of a dev_dbg(). Once more work is added to > the handler (e.g. detecting device "Suspended" state and notifying > other USB gadget components about it), enabling DVSE becomes a hard > requirement. Do it in a standalone commit for better visibility and > clear explanation. > > [1] f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code") > [2] 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget") I'm afraid I should have realized but, according to checkpatch.pl, these formats cause ERROR like below. So, they should be fixed. ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")' #90: [1] f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code") ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget")' #91: [2] 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget") Best regards, Yoshihiro Shimoda
Hello shimoda-san, Thanks for point out checkpatch warning. After resolving checkpatch warning, below checkpatch warning is seen. WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #23: [1] commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code") is this warning fine for you? . Regards, veeraiyan chidambaram. On Tue, Sep 10, 2019 at 04:45:29AM +0000, Yoshihiro Shimoda wrote: > Hi Veeraiyan, > > Thank you for the patch! > > > From: Veeraiyan Chidambaram, Sent: Tuesday, September 10, 2019 12:46 AM > <snip> > > Commit [1] enabled the possibility of checking the DVST (Device State > > Transition) bit of INTSTS0 (Interrupt Status Register 0) and calling > > the irq_dev_state() handler if the DVST bit is set. But neither > > commit [1] nor commit [2] actually enabled the DVSE (Device State > > Transition Interrupt Enable) bit in the INTENB0 (Interrupt Enable > > Register 0). As a consequence, irq_dev_state() handler is getting > > called as a side effect of other (non-DVSE) interrupts being fired, > > which definitely can't be relied upon, if DVST notifications are of > > any value. > > > > Why this doesn't hurt is because usbhsg_irq_dev_state() currently > > doesn't do much except of a dev_dbg(). Once more work is added to > > the handler (e.g. detecting device "Suspended" state and notifying > > other USB gadget components about it), enabling DVSE becomes a hard > > requirement. Do it in a standalone commit for better visibility and > > clear explanation. > > > > [1] f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code") > > [2] 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget") > > I'm afraid I should have realized but, according to checkpatch.pl, > these formats cause ERROR like below. So, they should be fixed. > > ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")' > #90: > [1] f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code") > > ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget")' > #91: > [2] 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget") > > Best regards, > Yoshihiro Shimoda >
Hello Veeraiyan-san, > From: veeraiyan chidambaram, Sent: Tuesday, September 10, 2019 6:31 PM > > Hello shimoda-san, > > Thanks for point out checkpatch warning. After resolving checkpatch warning, > below checkpatch warning is seen. > > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) > #23: > [1] commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code") I checked other commit log, and it seems adding a new line is better like below: [1] commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code") JFYI (out-of-topic though), checkpatch.pl doesn't warn if a "Fixes:" tag with more than 75 chars like following commit [2]. [2] --- commit d950fd992ef89f39ff8908f389ed6cbd2fdc0513 Author: Niklas Söderlund <***> Date: Wed Feb 13 17:07:54 2019 -0500 media: rcar-vin: Fix lockdep warning at stream on Changes to v4l2-fwnode in commit [1] triggered a lockdep warning in rcar-vin. The first attempt to solve this warning in the rcar-vin driver was incomplete and only pushed the warning to happen at stream on time instead of at probe time. This change reverts the incomplete fix and properly fixes the warning by removing the need to hold the rcar-vin specific group lock when calling v4l2_async_notifier_parse_fwnode_endpoints_by_port(). And instead takes it in the callback where it's really needed. [1] commit eae2aed1eab9bf08 ("media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev") Fixes: 6458afc8c49148f0 ("media: rcar-vin: remove unneeded locking in async callbacks") <snip> --- Best regards, Yoshihiro Shimoda
Hello shimoda-san, Thanks a lot for those hints. On Wed, Sep 11, 2019 at 02:45:52AM +0000, Yoshihiro Shimoda wrote: > Hello Veeraiyan-san, > > > From: veeraiyan chidambaram, Sent: Tuesday, September 10, 2019 6:31 PM > > > > Hello shimoda-san, > > > > Thanks for point out checkpatch warning. After resolving checkpatch warning, > > below checkpatch warning is seen. > > > > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) > > #23: > > [1] commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code") > > I checked other commit log, and it seems adding a new line is better like below: > > [1] commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common > code") I have fixed and submitted v5 patch https://patchwork.kernel.org/patch/11141085/. Best Regards, Veeraiyan Chidambaram
diff --git a/drivers/usb/renesas_usbhs/mod.c b/drivers/usb/renesas_usbhs/mod.c index 4fbb1d538b82..3384308169f5 100644 --- a/drivers/usb/renesas_usbhs/mod.c +++ b/drivers/usb/renesas_usbhs/mod.c @@ -339,10 +339,6 @@ void usbhs_irq_callback_update(struct usbhs_priv *priv, struct usbhs_mod *mod) * usbhs_interrupt */ - /* - * it don't enable DVSE (intenb0) here - * but "mod->irq_dev_state" will be called. - */ if (info->irq_vbus) intenb0 |= VBSE; @@ -353,6 +349,9 @@ void usbhs_irq_callback_update(struct usbhs_priv *priv, struct usbhs_mod *mod) if (mod->irq_ctrl_stage) intenb0 |= CTRE; + if (mod->irq_dev_state) + intenb0 |= DVSE; + if (mod->irq_empty && mod->irq_bempsts) { usbhs_write(priv, BEMPENB, mod->irq_bempsts); intenb0 |= BEMPE;