Re: [PATCH 1/9] ps3: AV Settings Driver

[Christoph Hellwig <hch@lst.de>]

On Thu, Jan 25, 2007 at 06:48:04PM +0100, Geert Uytterhoeven wrote:
> +EXPORT_SYMBOL(ps3av_set_audio_mode);

not _GPL?  This module seems like a very deeply internal helper for
other ps3 driver and not a general kernel API.

Btw, the Kconfig and/or top of file comment really needs a better
explanation what this "driver" is for.  I also wonder why it's
selectable in Kconfig at all given it has no visible user interface.
Shouldn't the users of the exported symbols simply pull it in or the
code made part of the unconditional build bits in the ps3 platform
directory?

> +static int ps3av_at_exit(struct notifier_block *self,
> +			 unsigned long state, void *data)
> +{
> +	/* fin avsetting modules */
> +	ps3av_cmd_fin();
> +
> +	if (!ps3av.available)
> +		return NOTIFY_OK;
> +
> +	ps3av.available = 0;
> +
> +	return NOTIFY_OK;
> +}
> +
> +static struct notifier_block ps3av_reboot_nb = {
> +	.notifier_call = ps3av_at_exit
> +};

Please implement this as ->shutdown method of the driver instead.
In case ps3_vuart_port_driver doesn't have that method yet please
add it as a forwarder for the generic driver model ->shutdown method.

> +#ifdef __KERNEL__

Why would you want to export the rest of this header to userspace?
(And in case you really want that you also have to add it to the Kbuild
file to actually be exported..)



and btw, please beat up the folks who designed this awfully stupid API..