lkml.org 
[lkml]   [2011]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/4] Intel PTI implementaiton of MIPI 1149.7.
    On Fri, 22 Apr 2011, james_p_freyensee@linux.intel.com wrote:

    > From: J Freyensee <james_p_freyensee@linux.intel.com>
    >
    > The PTI (Parallel Trace Interface) driver directs
    > trace data routed from various parts in the system out
    > through an Intel Penwell PTI port and out of the mobile
    > device for analysis with a debugging tool (Lauterbach or Fido).
    > Though n_tracesink and n_tracerouter line discipline drivers
    > are used to extract modem tracing data to the PTI driver
    > and other parts of an Intel mobile solution, the PTI driver
    > can be used independent of n_tracesink and n_tracerouter.
    >
    > You should select this driver if the target kernel is meant for
    > an Intel Atom (non-netbook) mobile device containing a MIPI
    > P1149.7 standard implementation.
    >
    > Signed-off-by: J Freyensee <james_p_freyensee@linux.intel.com>

    A few comments below.

    ...
    > +#define DRIVERNAME "pti"
    > +#define PCINAME "pciPTI"
    > +#define TTYNAME "ttyPTI"
    > +#define CHARNAME "pti"
    > +#define PTITTY_MINOR_START 0
    > +#define PTITTY_MINOR_NUM 2
    > +#define MAX_APP_IDS 16 /* 128 channel ids / u8 bit size */
    > +#define MAX_OS_IDS 16 /* 128 channel ids / u8 bit size */
    > +#define MAX_MODEM_IDS 16 /* 128 channel ids / u8 bit size */
    > +#define MODEM_BASE_ID 71 /* modem master ID address */
    ...

    Would be nice if the values of these defines would line up nicely.


    ...
    > +static struct pci_device_id pci_ids[] __devinitconst = {
    > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x82B) },
    > + {0}
    ...

    Why are there spaces after the opening { and before the closing } for the
    first entry, but not the second. Looks like you need to pick a
    consistent style.


    > + * regroup the appropriate message segments together reconstituting each
    > + * message.
    > + */
    > +static void pti_write_to_aperture(struct pti_masterchannel *mc,
    > + u8 *buf,
    > + int len)
    > +{
    > + int dwordcnt, final, i;
    > + u32 ptiword;
    > + u8 *p;
    > + u32 __iomem *aperture;
    > +
    > + p = buf;
    ...

    Perhaps save a few lines by doing

    static void pti_write_to_aperture(struct pti_masterchannel *mc,
    u8 *buf,
    int len)
    {
    int dwordcnt, final, i;
    u32 ptiword;
    u32 __iomem *aperture;
    u8 *p = buf;


    ...
    > +void pti_writedata(struct pti_masterchannel *mc, u8 *buf, int count)
    > +{
    > + /*
    > + * since this function is exported, this is treated like an
    > + * API function, thus, all parameters should
    > + * be checked for validity.
    > + */
    > + if ((mc != NULL) && (buf != NULL) && (count > 0))
    > + pti_write_to_aperture(mc, buf, count);
    > + return;
    ...

    Pointless return; statement.


    ...
    > +static void __devexit pti_pci_remove(struct pci_dev *pdev)
    > +{
    > + struct pti_dev *drv_data;
    > +
    > + drv_data = pci_get_drvdata(pdev);
    > + if (drv_data != NULL) {

    Perhaps

    static void __devexit pti_pci_remove(struct pci_dev *pdev)
    {
    struct pti_dev *drv_data = pci_get_drvdata(pdev);

    if (drv_data) {


    ...
    > +static int pti_tty_driver_open(struct tty_struct *tty, struct file *filp)
    > +{
    > + int ret = 0;
    > +
    > + /*
    > + * we actually want to allocate a new channel per open, per
    > + * system arch. HW gives more than plenty channels for a single
    > + * system task to have its own channel to write trace data. This
    > + * also removes a locking requirement for the actual write
    > + * procedure.
    > + */
    > + ret = tty_port_open(&drv_data->port, tty, filp);
    > +
    > + return ret;
    > +}
    ...

    Why not get rid of the pointless 'ret' variable and simplify this down to

    static int pti_tty_driver_open(struct tty_struct *tty, struct file *filp)
    {
    /*
    * we actually want to allocate a new channel per open, per
    * system arch. HW gives more than plenty channels for a single
    * system task to have its own channel to write trace data. This
    * also removes a locking requirement for the actual write
    * procedure.
    */
    return tty_port_open(&drv_data->port, tty, filp);
    }

    ??


    ...
    > +static void pti_tty_driver_close(struct tty_struct *tty, struct file *filp)
    > +{
    > + tty_port_close(&drv_data->port, tty, filp);
    > +
    > + return;
    > +}

    Just kill that superfluous return statement.


    ...
    > +static void pti_tty_cleanup(struct tty_struct *tty)
    > +{
    > + struct pti_tty *pti_tty_data;
    > + struct pti_masterchannel *mc;
    > +
    > + pti_tty_data = tty->driver_data;
    > +
    > + if (pti_tty_data != NULL) {
    > + mc = pti_tty_data->mc;
    > + pti_release_masterchannel(mc);
    > + pti_tty_data->mc = NULL;
    > + }
    > +
    > + if (pti_tty_data != NULL)
    > + kfree(pti_tty_data);
    > +
    > + tty->driver_data = NULL;
    > +}

    How about this instead?

    static void pti_tty_cleanup(struct tty_struct *tty)
    {
    if (!tty->driver_data)
    return;
    pti_release_masterchannel(tty->driver_data->mc);
    kfree(tty->driver_data);
    }

    ...
    > +static int pti_tty_driver_write(struct tty_struct *tty,
    > + const unsigned char *buf, int len)
    > +{
    > + struct pti_masterchannel *mc;
    > + struct pti_tty *pti_tty_data;
    > +
    > + pti_tty_data = tty->driver_data;
    > + mc = pti_tty_data->mc;
    > + pti_write_to_aperture(mc, (u8 *)buf, len);
    > +
    > + return len;
    > +}

    I'd like to suggest this as an alternative:

    static int pti_tty_driver_write(struct tty_struct *tty,
    const unsigned char *buf, int len)
    {
    pti_write_to_aperture(tty->driver_data->mc, (u8 *)buf, len);
    return len;
    }


    ..
    > +static int pti_char_open(struct inode *inode, struct file *filp)
    > +{
    > + struct pti_masterchannel *mc;
    > +
    > + mc = pti_request_masterchannel(0);
    > + if (mc == NULL)
    > + return -ENOMEM;
    > + filp->private_data = mc;
    > + return 0;
    > +}

    Ok, so I admit that I haven't looked to check if it's actually important
    that filp->private_data does not get overwritten if
    pti_request_masterchannel() returns NULL, but if we assume that it is not
    important, then this would be an improvement IMHO:

    static int pti_char_open(struct inode *inode, struct file *filp)
    {
    filp->private_data = pti_request_masterchannel(0);
    if (!filp->private_data)
    return -ENOMEM;
    return 0;
    }


    ...
    > +
    > +/**
    > + * pti_char_release()- Close a char channel to the PTI device. Part
    > + * of the misc device implementation.
    > + *
    > + * @inode: Not used in this implementaiton.
    > + * @filp: Contains private_data that contains the master, channel
    > + * ID to be released by the PTI device.
    > + *
    > + * Returns:
    > + * always 0

    Why not void then?


    > + pti_release_masterchannel(filp->private_data);
    > + return 0;
    > +}
    > +
    > +/**
    > + * pti_char_write()- Write trace debugging data through the char
    > + * interface to the PTI HW. Part of the misc device implementation.
    > + *
    > + * @filp: Contains private data which is used to obtain
    > + * master, channel write ID.
    > + * @data: trace data to be written.
    > + * @len: # of byte to write.
    > + * @ppose: Not used in this function implementation.
    > + *
    > + * Returns:
    > + * int, # of bytes written
    > + * otherwise, error value
    > + *
    > + * Notes: From side discussions with Alan Cox and experimenting
    > + * with PTI debug HW like Nokia's Fido box and Lauterbach
    > + * devices, 8192 byte write buffer used by USER_COPY_SIZE was
    > + * deemed an appropriate size for this type of usage with
    > + * debugging HW.
    > + */
    > +static ssize_t pti_char_write(struct file *filp, const char __user *data,
    > + size_t len, loff_t *ppose)
    > +{
    > + struct pti_masterchannel *mc;
    > + void *kbuf;
    > + const char __user *tmp;
    > + size_t size = USER_COPY_SIZE, n = 0;

    It would be nice to declare these two variables on two sepperate lines
    IMO.

    > +
    > + tmp = data;
    > + mc = filp->private_data;
    > +
    > + kbuf = kmalloc(size, GFP_KERNEL);
    > + if (kbuf == NULL) {
    > + pr_err("%s(%d): buf allocation failed\n",
    > + __func__, __LINE__);
    > + return 0;

    Shouldn't you be returning -ENOMEM here?

    > + }
    > +
    > + do {
    > + if (len - n > USER_COPY_SIZE)
    > + size = USER_COPY_SIZE;
    > + else
    > + size = len - n;
    > +
    > + if (copy_from_user(kbuf, tmp, size)) {
    > + kfree(kbuf);
    > + return n ? n : -EFAULT;
    > + }
    > +
    > + pti_write_to_aperture(mc, kbuf, size);
    > + n += size;
    > + tmp += size;
    > +
    > + } while (len > n);
    > +
    > + kfree(kbuf);
    > + kbuf = NULL;
    > +

    kbuff is a local variable. What's the point in assigning NULL to it just
    before you return? Just get rid of that silly assignment.


    ...
    > + * pti_char_release()- Close a char channel to the PTI device. Part
    > + * of the misc device implementation.
    > + *
    > + * @inode: Not used in this implementaiton.
    > + * @filp: Contains private_data that contains the master, channel
    > + * ID to be released by the PTI device.
    > + *
    > + * Returns:
    > + * always 0

    So why not void?

    ...
    > + * pti_console_setup()- Initialize console variables used by the driver.
    > + *
    > + * @c: Not used.
    > + * @opts: Not used.
    > + *
    > + * Returns:
    > + * always 0.

    Why not void?


    ...
    > + * pti_port_activate()- Used to start/initialize any items upon
    > + * first opening of tty_port().
    > + *
    > + * @port- The tty port number of the PTI device.
    > + * @tty- The tty struct associated with this device.
    > + *
    > + * Returns:
    > + * always returns 0

    Shouldn't it just return void then?


    --
    Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
    Don't top-post http://www.catb.org/jargon/html/T/top-post.html
    Plain text mails only, please.



    \
     
     \ /
      Last update: 2011-04-24 03:05    [W:0.039 / U:90.908 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site