On Saturday 11 August 2007, Sergei Shtylyov wrote: > Hello. > > Bartlomiej Zolnierkiewicz wrote: > > >>>>Index: linux-2.6/drivers/ide/pci/hpt366.c > >>>>=================================================================== > >>>>--- linux-2.6.orig/drivers/ide/pci/hpt366.c > >>>>+++ linux-2.6/drivers/ide/pci/hpt366.c > > [...] > >>>IMO all HPT*_ALLOW_ATA* defines should just go away... > > >> I think it's still worth to keep 'em alive for the possible blacklist > >>additions. > > > No strong feelings about these defines but I think that they actually make > > the code less readable and also more complex because they control _both_ > > DPLL used (through controlling max_ultra) and the maximum UDMA mask. > > That's because the maximum UDMA mask depends on the DPLL frequency... > > > Moreover they are _compile_ time options so for testing purposes we may > > as well ask user to change UDMA mask etc. > > ... and UltraDMA/100 is *not* reachable with 66 MHz clock (it will have to > use the same timings as UltraDMA/66 -- so changing the mask only is just not > enough. > > Now you can hopefully see that these #define's as they are now exist for a > good reason... :-) Yes but I still believe that there should be some cleaner solution... ;) > >>>Also now that ->udma_filter is always present the initial hwif->ultra_mask > >>>doesn't matter so as well we may set it to ATA_UDMA6 (0x7f) and cleanup > >>>struct hpt_info (by removing max_ultra after fixing init_chipset_hpt366() > >>>to use info->chip_type >= HPT374 check instead), > > >> It's all interesting but you've missed one aspect -- this will make the > >>kernel larger while the current code keeps all this logic in the init.text > >>section. > > > We won't be adding a single line of new code: > > > - the current ->udma_filter implementation does everything needed already > > Not really. It will return 0x7f for chipset not supporting it Which is 100% correct given current values of HPT*_ALLOW_ATA133_6 defines. > > - in init_chipset_hpt366() we simply would replace > > > if (info->max_ultra > 6) > > Actually,( info->max_ultra == 6) Yes. > > with > > > if (info->chip_type >= HPT374) > > This is just wrong -- HPT374 does not tolarate 66 MHz clock. You probably > meant HPT372 (or >)? Yes, ">". > > (this change depends on the current HPT3xx enums order > > and on removal HPT*_ALLOW_ATA* defines) > > Heh, how about doing this (pardon for the bad... er, sed language): > > default: > return s/0x71/drive->hwif->ultra_mask/; > > without all any changes that you've proposed and being done with that fix? :-) I hope that you meant: default: return s/0x7f/drive->hwif->ultra_mask/; Yep, I'm fine with it. > >>>Uh, the only real change here consists of the three lines above, the rest > >>>is just a noise caused by removal of one tab. > > >>>Such changes are really not worth it - in this case it caused rejects in > >>>two patches from IDE quilt tree which I had to fix manually. > > >> I hope now that you've fixed it, I may leave this part intact? ;-) > > > Iff you base the new patch on top of IDE quilt tree otherwise I'll have > > to fix it _again_. ;-) > > I hope you haven't forgotten the basic rule: "the fixes come first"? :-) The basic rule is to keep the process steady. :-) "The fixes come first" is the 2-nd rule and becomes a bit hard to keep up when there is almost hundred patches in the series. > And why fix it again, if I'm not going to drop that part? Indeed, there is no need to fix again... Thanks, Bart - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
