Re: [PATCH 0/2] [libata] sata_mv: Add support for Marvell's integrated SATA controller
saeed bishara wrote: I really think that a lot of the new variable/macro/enum names are overly long, making the code a bit harder to read. The patch is all about "System On Chip (SOC)" support, yet the names all say "INTEGRATED". well, many socs have SATA or Ethernet controllers as pci controller, but in this case, the sata controller has been integrated into the soc by cutting the pci interface and connecting the sata core directly to the soc's bus. so I though that the "integrated" well show this fact. How about changing "INTEGRATED" to "SOC", and "integrated" to "soc" everywhere ? The mv_integrated_reset_hc_port() (or mv_soc_reset_hc_port()) function seems to be a duplicate of the existing mv5_reset_hc_port() function. except the line that writes to the EDMA_CFG_OFS register (101f vs 11f). also , I think that in the future the the integrated/soc variant will have more register that does not exist in mv5 to reset. .. The names are too long as-is. BTW, the mv5_sht and mv6_sht are identical, are there any plans to modify any or them? .. Good idea. Please send a separate patch for that one, thanks. The new fields in the mv_host_priv struct are __iomem pointers rather than offsets as was done for similar fields in the past (offsets take up less space on 64-bit machines). We should be consistent there, one way or the other. well, as those registers get access in the main flow (data commands), preparing the final address will save some runtime calculations. so, it's the speed vs memory tradeoff. I think speed should be preferred. agree? .. Yes, speed is important. Memory is considerably slower than CPUs, so change them to offsets. Cheers - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] [libata] sata_mv: Add support for Marvell's integrated SATA controller
> > I really think that a lot of the new variable/macro/enum names are overly > long, > making the code a bit harder to read. > The patch is all about "System On Chip (SOC)" support, > yet the names all say "INTEGRATED". well, many socs have SATA or Ethernet controllers as pci controller, but in this case, the sata controller has been integrated into the soc by cutting the pci interface and connecting the sata core directly to the soc's bus. so I though that the "integrated" well show this fact. > > How about changing "INTEGRATED" to "SOC", and "integrated" to "soc" > everywhere ? > > The mv_integrated_reset_hc_port() (or mv_soc_reset_hc_port()) function > seems to be a duplicate of the existing mv5_reset_hc_port() function. except the line that writes to the EDMA_CFG_OFS register (101f vs 11f). also , I think that in the future the the integrated/soc variant will have more register that does not exist in mv5 to reset. BTW, the mv5_sht and mv6_sht are identical, are there any plans to modify any or them? > The new fields in the mv_host_priv struct are __iomem pointers > rather than offsets as was done for similar fields in the past > (offsets take up less space on 64-bit machines). > We should be consistent there, one way or the other. well, as those registers get access in the main flow (data commands), preparing the final address will save some runtime calculations. so, it's the speed vs memory tradeoff. I think speed should be preferred. agree? > > Cheers > > Mark - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
