Message-ID: <570B35C0.9040205@au1.ibm.com>
Date:	Mon, 11 Apr 2016 15:27:28 +1000
From:	Andrew Donnellan <andrew.donnellan@....ibm.com>
To:	Suraj Jitindar Singh <sjitindarsingh@...il.com>, arnd@...db.de
Cc:	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	paulus@...ba.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH 2/2] powerpc/drivers: Add driver for operator panel on FSP
 machines

On 11/04/16 11:41, Suraj Jitindar Singh wrote:
> Implement new character device driver to allow access from user space
> to the 2x16 character operator panel display present on powernv machines.

Specifically, on IBM Power Systems machines with FSPs (see comments below).

> This will allow status information to be presented on the display which
> is visible to a user.
>
> The driver implements a 32 character buffer which a user can read/write
> by accessing the device (/dev/oppanel). This buffer is then displayed on
> the operator panel display. Any attempt to write past the 32nd position
> will have no effect and attempts to write more than 32 characters will be
> truncated. Valid characters are ascii: '.', '/', ':', '0-9', 'a-z',
> 'A-Z'. All other characters are considered invalid and will be replaced
> with '.'.

For reference, the ASCII character whitelist is enforced by skiboot, not 
by the driver (see 
https://github.com/open-power/skiboot/blob/master/hw/fsp/fsp-op-panel.c#L217). 
It's been included ever since the first public release of skiboot, so 
this statement is true for all machines at present, though theoretically 
might not be true in future skiboots or alternative OPAL implementations 
(should someone be crazy enough to write one).

> A write call past the 32nd character will return zero characters
> written. A write call will not clear the display and it is up to the
> user to put spaces (' ') where blank space is required. The device may
> only be accessed by a single process at a time.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@...il.com>

I reviewed an earlier version of this patch internally and Suraj has 
fixed a bunch of issues which I raised. I'm not hugely experienced with 
this, but all the obvious things I noticed have gone, so...

Reviewed-by: Andrew Donnellan <andrew.donnellan@....ibm.com>

A couple of minor nitpicks below.

> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 3ec0766..8c91edf 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -178,6 +178,20 @@ config IBM_BSR
>   	  of threads across a large system which avoids bouncing a cacheline
>   	  between several cores on a system
>
> +config IBM_OP_PANEL
> +	tristate "IBM POWER Operator Panel Display support"
> +	depends on PPC_POWERNV
> +	default m
> +	help
> +	  If you say Y here, a special character device node /dev/oppanel will

Add commas: "node, /dev/oppanel, will"

> diff --git a/drivers/char/op-panel-powernv.c b/drivers/char/op-panel-powernv.c
> new file mode 100644
> index 0000000..cc72c5d
> --- /dev/null
> +++ b/drivers/char/op-panel-powernv.c
[...]
> +/*
> + * This driver creates a character device (/dev/oppanel) which exposes the
> + * operator panel display (2x16 character display) on IBM pSeries machines.

I'd prefer "IBM Power Systems machines with FSPs" so as to avoid 
confusion with the Linux pseries platform, to be in line with current 
IBM branding, and to emphasise that it's only FSP machines (the Power 
Systems LC models are not).

Hmm, perhaps also mention that in the Kconfig description too?

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@....ibm.com  IBM Australia Limited

Powered by blists - more mailing lists