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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
