[PATCH 02/13] powerpc/5200: LocalPlus driver: use SCLPC register structure

[Grant Likely]

On Mon, Dec 21, 2009 at 11:59 PM, Roman Fietze
<roman.fietze at telemotive.de> wrote:

No patch description?  Very few patches are sufficiently described by
the subject line alone.  Tell me what the problem is, what the patch
changes, and why.

> Signed-off-by: Roman Fietze <roman.fietze at telemotive.de>
> ---
>  arch/powerpc/include/asm/mpc52xx.h            |   24 ++++++++
>  arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c |   79 +++++++++++--------------
>  2 files changed, 59 insertions(+), 44 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mpc52xx.h b/arch/powerpc/include/asm/mpc52xx.h
> index b664ce7..57f8335 100644
> --- a/arch/powerpc/include/asm/mpc52xx.h
> +++ b/arch/powerpc/include/asm/mpc52xx.h
> @@ -193,6 +193,30 @@ struct mpc52xx_xlb {
>  #define MPC52xx_XLB_CFG_PLDIS          (1 << 31)
>  #define MPC52xx_XLB_CFG_SNOOP          (1 << 15)
>
> +/* SCLPC */
> +struct mpc52xx_sclpc {
> +       union {
> +               u8 restart;     /* 0x00 restart bit */
> +               u32 packet_size; /* 0x00 packet size register */
> +       } packet_size;
> +       u32 start_address;      /* 0x04 start Address register */
> +       u32 control;            /* 0x08 control register */
> +       u32 enable;             /* 0x0C enable register */
> +       u32 unused0;            /* 0x10 */
> +       union {
> +               u8 status;      /* 0x14 status register bits */
> +               u32 bytes_done; /* 0x14 bytes done register bits, read only */
> +       } bytes_done_status;
> +
> +       u32 reserved1[(0x40-0x18) / sizeof(u32)];       /* 0x18 .. 0x3c */
> +
> +       u32 fifo_data;          /* 0x40 FIFO data word register */
> +       u32 fifo_status;        /* 0x44 FIFO status register */
> +       u8 fifo_control;        /* 0x48 FIFO control register */
> +       u8 reserved2[3];
> +       u32 fifo_alarm;         /* 0x4C FIFO alarm register */
> +};

Please don't.  I know that a lot of other 5200 code uses register map
structures in this way, but I consider it bad practice.  I coded this
driver without a structure for a reason.  The reason I haven't removed
the other 5200 register map structures is the code impact would be
huge, it would probably cause breakage, and it would break all
out-of-tree patches touching the same code for no measurable
advantage.

For this code, there is no advantage to changing the register access
method, and it generates more work in other areas.  Even if I
preferred register map structures I would resist applying this patch.

Please drop from your series.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.