Hi Paul, On Wed, Jul 17, 2013 at 06:52:56PM +0000, Paul Zimmerman wrote: > > From: Matthijs Kooijman [mailto:matthijs@xxxxxxxx] > > Sent: Wednesday, July 17, 2013 8:10 AM > > > > Various register fields wider than one bit have constants defined for > > their value. Previously, these registers would define the values as they > > appear in the register, so shifted to the right to the position the > > value appears in the register. > > > > This commit changes those constants to their natural values (e.g, 0, 1, > > 2, etc.), as they are after shifting the register value to the right. > > This also changes all relevant code to shift the values before comparing > > them with constants. > > > > This has the advantage that the values can be stored in smaller > > variables (now they always require a u32) and makes the handling of > > these values more consistent with other register fields that represent > > natural numbers instead of enumerations (e.g., number of host channels). > > Hi Matthijs, > > Are you sure the advantages outweigh the drawbacks and the amount > of code churn here? There are a lot of changes similar to this: > > > - u32 hs_phy_type = hsotg->hwcfg2 & GHWCFG2_HS_PHY_TYPE_MASK; > > - u32 fs_phy_type = hsotg->hwcfg2 & GHWCFG2_FS_PHY_TYPE_MASK; > > + u32 hs_phy_type = (hsotg->hwcfg2 & GHWCFG2_HS_PHY_TYPE_MASK) >> > > + GHWCFG2_HS_PHY_TYPE_SHIFT; > > + u32 fs_phy_type = (hsotg->hwcfg2 & GHWCFG2_FS_PHY_TYPE_MASK) >> > > + GHWCFG2_FS_PHY_TYPE_SHIFT; > > I.e we went from two lines of code to four. Other than saving a tiny > amount of RAM by being able to store the config values in a smaller > variable, are there any other advantages? I'd consider the RAM gain just a small bonus. For me, the real gain is that the code makes more sense. My view is that a register is a collection of fields, placed at different offsets into the register. With this patch, you're properly extracting the field value from the register value right at the spot where you read the register and after that can just work with a (zero-based) field value as documented, without caring where in the register it was stored. For natural number type fields (e.g., number of host channels), this handling makes the most sense, since otherwise you'd have to do the shifting later when you actually need to interpret the value. For boolean fields, the shifting is done implicitely by using the !! operator. For enumeration type fields (e.g., the hs_phy_type above), the gain isn't so clear, since the comparisons will almost always be done against constants. Having all types of fields be handled in the same way, seems like a gain in itself to me. For me, the gains outweigh the extra lines of code (especially since a lot of them are moved into a single dwc2_get_hw_params function in a later patch). If I haven't convinced you yet, I'll drop this patch from the series and resubmit. This would require quite some (textual, not functional) changes to the later patches. Can I still add your acked-by to them, or do you want to see the changed patches first? Gr. Matthijs -- 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
