> From: Matthijs Kooijman [mailto:matthijs@xxxxxxxx] > Sent: Monday, August 05, 2013 7:51 AM > > 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). OK, I'm kind of on the fence about this one, so if you prefer it this way I guess it's OK with me. Acked-by: Paul Zimmerman <paulz@xxxxxxxxxxxx> What platforms have you tested this on, BTW? You said you have a Raspberry Pi, so I'd like you to test these patches there before resubmitting them. And I'll test them on my PCI-based platform. -- Paul -- 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
