[parisc-linux] pdc_add_valid

[parisc-linux] pdc_add_valid

[Matthew Wilcox]

The pdc_add_valid function really annoys me.  Or maybe it's just how
it's used:

        if (!pdc_add_valid( (void *)(port+4))) {

Which says to me `if this address is not valid' -- the opposite of what it
does.  It could be rewritten as:

	if (pdc_add_valid((void *)(port+4)) == PDC_OK) {

but this takes more effort.  In my tree, I've added valid_io_addr which
does what it says on the tin:

	if (valid_io_addr(port+4)) {

For the moment, I've put it in <asm/io.h>, since it is for I/O addresses.

+int valid_io_addr(unsigned long addr)
+{
+       return (pdc_add_valid(addr) == PDC_OK);
+}

But maybe we should replace pdc_add_valid with valid_io_addr?  Or a
different function?  Suggestions appreciated.

-- 
Revolutions do not require corporate support.

Re: [parisc-linux] pdc_add_valid

[Stan Sieler]

Re:

>         if (!pdc_add_valid( (void *)(port+4))) {
> 
> Which says to me `if this address is not valid' -- the opposite of what it
> does.  It could be rewritten as:

Very good point.

Code should be readable first, and efficient second.

Routines that work the opposite of their name are just problems
waiting to happen.
 
> 	if (valid_io_addr (port+4)) {
vs:
> +       return (pdc_add_valid (addr) == PDC_OK);

The latter has the advantage that the reader knows it's a PDC
routine being called, not some kernel routine that could be done
who knows what.

Thus, I'd go for the latter, or change your "valid_io_addr" to
"pdc_valid_io_addr", to (a) be readable; and (b) convey the PDC info.

Stan (still can't believe HP-UX uses non-compatible "sprintf" in the kernel) Sieler

Re: [parisc-linux] pdc_add_valid

[Matthew Wilcox]

On Fri, Oct 05, 2001 at 02:58:13PM -0700, Stan Sieler wrote:
> > 	if (valid_io_addr (port+4)) {
> vs:
> > +       return (pdc_add_valid (addr) == PDC_OK);
> 
> The latter has the advantage that the reader knows it's a PDC
> routine being called, not some kernel routine that could be done
> who knows what.
> 
> Thus, I'd go for the latter, or change your "valid_io_addr" to
> "pdc_valid_io_addr", to (a) be readable; and (b) convey the PDC info.

I should have explained this in my original mail... the reason I don't
want to make this a pdc_* call is that I believe we have an expectation
that pdc_* calls return PDC_ error codes.  Perhaps this isn't an
expectation that people have, in which case I don't have an objection
to simply renaming pdc_add_valid to pdc_addr_valid, and changing the
sense of the return value.

-- 
Revolutions do not require corporate support.

Re: [parisc-linux] pdc_add_valid

[Grant Grundler]

Matthew Wilcox wrote:
> 
> The pdc_add_valid function really annoys me.

So you want a sane wrapper around it.

> For the moment, I've put it in <asm/io.h>, since it is for I/O addresses.
> 
> +int valid_io_addr(unsigned long addr)
> +{
> +       return (pdc_add_valid(addr) == PDC_OK);
> +}

I prefer asm/io.h not depend on pdc.h.
Why not just add whatever wrapper to pdc.h?

> But maybe we should replace pdc_add_valid with valid_io_addr?
> Or a different function?  Suggestions appreciated.

sorry - no better ideas.

grant