[parisc-linux-cvs] 64-bit cleanup for the sim700 driver.

[Richard Hirst]

On Tue, Mar 20, 2001 at 09:20:19PM -0700, Ryan Bradetich wrote:
> > ...
> > >  	targdata->din_script[MAX_SG*2+1] =
> > > -		cpu_to_ncr32((u32)(targdata->ba->script + Ent_end_data_trans/4)
> > >   );
> > > +		cpu_to_ncr32(low32(targdata->ba->script + Ent_end_data_trans/4)
> > >   );
> > 
> > Some code uses sizeof(u32) and other '4'.
> > I can't say "sizeof(u32)" is more "correct" than hardcoding '4'.
> > My preference is just one of those forms be used though unless there
> > is a sematic difference (which doesn't seem to be the case).
> 
> Richard, does it make sense to replace the '4's with sizeof(32)'s?  If it
> makes sense, I would be glad to make the changes and submit the patch.
> If it does not make sense, can we use a define to replace the magic number
> for better readability?

It isn't always immediately obvious which is more logical, consider:

            u32 offset = (dsp - (u32)targdata->ba->script)/4;
            if (offset == Ent_reselect/4 + 4) {

In the first line I might use sizeof(u32), because we are manipulating
script[] which is an array of u32.  In the second line we are
manipulating Ent_reselect, which is a define spit out of the script
compiler, as a byte offset up the script.  Each word of the script
is 4 bytes long, so I'd /4.

Perhaps we should have a 'typedef u32 ncr_word', make script[] an
array of ncr_word, and use sizeof(ncr_word).  But then the script
compiler spits out 'static u32 SCRIPT[]', and I don't really want
to change that.

Most of the /4 in my local source are Ent_xxx/4, where all Ent_xxx
come from the script compiler output.  In fact, in my source there
are only two occurances of /sizeof(u32).  Personally I'd make those
/4 and leave it at that.  If you'd rather have a #define that's ok
with me, but I'm not convinced it helps much - probably because
I know the code too well.

> > ...
> > > @@ -888,7 +887,7 @@ handle_phase_mismatch (struct Scsi_Host 
> > >      unsigned char sbcl;
> > >  
> > >      sbcl = NCR_read8(SBCL_REG) & SBCL_PHASE_MASK;
> > > -    index = (u32)((u32 *)(NCR_read32(DSP_REG)) - targdata->ba->script);
> > > +    index = low32((u32 *)(unsigned long)NCR_read32(DSP_REG) - targdata->ba->
> > >   script);
> > 
> > I'm abit confused by the casts. Is this meant?
> > 	index = NCR_read32(DSP_REG) - low32(targdata->ba->script);
> 
> I thought the same thing at first .... cost me several hours of debugging time too :(
> Here is the error message I was seeing under the 32-bit kernel when I reduced the
> casting to your (and my previous) suggestion.

targdata->ba->script is an array of u32, DSP_REG points to an element
of that array.  I am trying to set index to the index in to that
array.  So, Grants proposed change is wrong because it doesn't do a
/sizeof(u32).  The original code has an implied /sizeof(u32) because
it is taking the difference of two (u32 *).

Try the following if you like:

index = (NCR_read32(DSP_REG) - low32(targdata->ba->script))/sizeof(u32);

> > > @@ -1243,8 +1242,8 @@ sim700_intr_handle(int irq, void *dev_id
> > >  	    printk("scsi%d: Int %d, istat %02x, sstat0 %02x "
> > >  		"dstat %02x, dsp [%04x], target %x, dsps %08x\n",
> > >  		host->host_no, sim700_intrs, istat, sstat0, dstat,
> > > -		(u32 *)(NCR_read32(DSP_REG)) - targdata->ba->script,
> > > -		target, dsps);
> > > +		low32((u32 *)(unsigned long)NCR_read32(DSP_REG) - 
> > > +	  	(unsigned long)targdata->ba->script), target, dsps);
> > 
> > Again, similar construct with casts.

Again, I am diff-ing two (u32 *) to get an array index rather than
a byte index.

> > ...
> > > @@ -1255,7 +1254,7 @@ sim700_intr_handle(int irq, void *dev_id
> > >  	    resume_offset = Ent_reselect;
> > >  	}
> > >  	else if (hostdata->chip == 700 && (dstat & DSTAT_ABRT)) {
> > > -	    u32 offset = (dsp - (u32)targdata->ba->script)/4;
> > > +	    u32 offset = (dsp - low32(targdata->ba->script)/4);
> > 
> > This looks wrong to me. Is it? (parenthesis moved)
> 
> it looks correct to me ... but let me macro-expand it to make sure we are not
> missing anything:

It's wrong.  (a - b)/4  is not the same as  (a - b/4)  ;-)
The original code is correct, making offset an index in to an array
of u32.  The correct change is to

	u32 offset = (dsp - low32(targdata->ba->script))/4;


> Thanks for the review Grant!  Hopefully this email will hit the list ... playing
> with mutt now instead of my normal mail client :)

Yes, nice work Grant - obviously got too much time to kill down
there on the beach ;)

Richard