Re: [PATCH] s390: export ipl device parameters

[Greg KH <gregkh@suse.de>]

On Mon, Sep 26, 2005 at 11:15:37AM +0200, Heiko Carstens wrote:
> Hi,
> 
> > > +#ifdef CONFIG_SYSFS
> > Does anyone build a s390 kernel without sysfs?  You can probably just
> > drop this ifdef.
> 
> Yes, you're right.
> 
> > > +DEFINE_IPL_ATTR(lun, "0x%016llx\n", (unsigned long long)
> > > +DEFINE_IPL_ATTR(bootprog, "%lld\n", (unsigned long long)
> > Why have a format field, if you only use the same format?
> 
> I use two different formats (hexadecimal and decimal).

Sorry, I was referring to the (unsigned long long) duplication there,
not the format field.

> > > +	__ATTR(device, S_IRUGO, ipl_device_show, NULL);
> > Why not use __ATTR_RO() like you did above?
> 
> The name of the attribute is supposed to be 'device'. If I would use
> __ATTR_RO it stringifies the first parameter and the result would be
> 'ipl_device' because of the function name I use.
> Otherwise I would have to rename my function, which is something I
> don't want to do. Somehow __ATTR_RO doesn't fit.

Ok, fair enough.

> > > +#define IPL_PARMBLOCK_ORIGIN	0x2000
> > You are just directly addressing memory with this address, right?
> 
> Yes.
> 
> > Shouldn't you iomap it or something first?
> 
> No, we don't have memory mapped IO on S390.

Neither do we on x86, yet we force everyone to use the proper function
calls, for when that code is then ported to other arches.  No reason
s390 should be alone with getting away from that :)

> How about this:
> 
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> Remove unnecessary ifdef + unused variable.

<snip>

Looks fine to me.

thanks,

greg k-h