Re: [PATCH][resubmit] HP iLO driver

[Pekka Enberg]

Hi David,

Some minor nitpicks follow.

On Mon, Jun 23, 2008 at 7:00 PM, David Altobelli <david.altobelli@xxxxxx> wrote:
> +
> +/*
> + * FIFO queues, shared with hardware.
> + *
> + * If a queue has empty slots, an entry is added to the queue tail,
> + * and that entry is marked as occupied.
> + * Entries can be dequeued from the head of the list, when the device
> + * has marked the entry as consumed.
> + *
> + * Returns true on successful queue/dequeue, false on failure.
> + */
> +static int fifo_enqueue(struct ilo_hwinfo *hw, char *fifobar, int entry)
> +{
> +       struct fifo *Q = FIFOBARTOHANDLE(fifobar);

The upper case Q is not a proper local variable name (appears
elsewhere as well).


> diff -urpN linux-2.6.26-rc7.orig/drivers/misc/hpilo.h linux-2.6.26-rc7/drivers/misc/hpilo.h
> --- linux-2.6.26-rc7.orig/drivers/misc/hpilo.h  1969-12-31 18:00:00.000000000 -0600
> +++ linux-2.6.26-rc7/drivers/misc/hpilo.h       2008-06-23 08:52:25.000000000 -0500

> +#define IS_DEVICE_RESET(hw) (ioread32(&(hw)->mmio_vaddr[DB_OUT]) & (1 << 26))
> +/* clear the device (reset bits, pending channel entries) */
> +#define CLEAR_DEVICE(hw)    (iowrite32(-1, &(hw)->mmio_vaddr[DB_OUT]))

> +#define DESC_MEM_SZ(_descs) ((_descs) << L2_QENTRY_SZ)

> +#define CTRL_SET(_c, _l, _f, _d, _a, _g)        \
> +       ((_c) =                                  \
> +        (((_l) << CTRL_BITPOS_L2SZ)            |\
> +         ((_f) << CTRL_BITPOS_FIFOINDEXMASK)   |\
> +         ((_d) << CTRL_BITPOS_DESCLIMIT)       |\
> +         ((_a) << CTRL_BITPOS_A)               |\
> +         ((_g) << CTRL_BITPOS_G)))
> +

> +#define DOORBELL_SET(_ccb)     (iowrite8(1, (_ccb)->ccb_u5.db_base))
> +#define DOORBELL_CLR(_ccb)     (iowrite8(2, (_ccb)->ccb_u5.db_base))

> +#define FIFOBARTOHANDLE(_fifo) \
> +       ((struct fifo *)(((char *)(_fifo)) - FIFOHANDLESIZE))
> +
> +/* set a flag indicating this channel needs a reset */
> +#define SET_CHANNEL_RESET(_ccb) \
> +       (FIFOBARTOHANDLE((_ccb)->ccb_u1.send_fifobar)->reset = 1)
> +
> +/* check for this particular channel needing a reset */
> +#define IS_CHANNEL_RESET(_ccb) \
> +       (FIFOBARTOHANDLE((_ccb)->ccb_u1.send_fifobar)->reset)
> +
> +/* overall size of a fifo is determined by the number of entries it contains */
> +#define FIFO_SZ(_num) (((_num)*sizeof(u64)) + FIFOHANDLESIZE)

> +#define GETQWORDS(_e) (((_e) & ENTRY_MASK_QWORDS) >> ENTRY_BITPOS_QWORDS)
> +#define GETDESC(_e) (((_e) & ENTRY_MASK_DESCRIPTOR) >> ENTRY_BITPOS_DESCRIPTOR)
> +
> +#define QWORDS(_B) (((_B) & 7) ? (((_B) >> 3) + 1) : ((_B) >> 3))

Static inline functions are preferred over function-like macros.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/