lkml.org 
[lkml]   [2010]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv3] drivers/misc: Altera active serial implementation
    On 11/15/2010 09:23 AM, Baruch Siach wrote:
    > --- /dev/null
    > +++ b/drivers/misc/altera_as.c
    > @@ -0,0 +1,349 @@
    ...
    > +struct altera_as_device {
    > + unsigned id;
    > + struct device *dev;
    > + struct miscdevice miscdev;
    > + struct mutex open_lock;
    > + struct gpio gpios[AS_GPIO_NUM];
    > +};
    > +
    > +/*
    > + * The only functions updating this array are .probe/.remove, which are
    > + * serialized. Hence, no locking is needed here.
    > + */
    > +static struct {
    > + int minor;

    Why you need minor here? It's in drvdata->miscdev.minor.

    > + struct altera_as_device *drvdata;
    > +} altera_as_devs[AS_MAX_DEVS];

    You don't need the struct at all.
    static struct altera_as_device *drvdata[AS_MAX_DEVS];
    should be enough.

    > +static int altera_as_open(struct inode *inode, struct file *file)
    > +{
    > + int ret;
    > + struct altera_as_device *drvdata = get_as_dev(iminor(inode));
    > +
    > + if (drvdata == NULL)
    > + return -ENODEV;
    > + file->private_data = drvdata;
    > +
    > + ret = mutex_trylock(&drvdata->open_lock);
    > + if (ret == 0)
    > + return -EBUSY;
    > +
    > + ret = gpio_request_array(drvdata->gpios, ARRAY_SIZE(drvdata->gpios));
    > + if (ret < 0) {
    > + mutex_unlock(&drvdata->open_lock);
    > + return ret;
    > + }

    So leaving to userspace with mutex held. This is *wrong*. use
    test_and_set_bit or alike instead.

    > + return 0;
    > +}
    > +
    > +static ssize_t altera_as_write(struct file *file, const char __user *buf,
    > + size_t count, loff_t *ppos)
    > +{
    ...
    > + while ((count - written) > 0) {
    > + err = copy_from_user(page_buf, buf+written, AS_PAGE_SIZE);
    > + if (err < 0) {
    > + err = -EFAULT;
    > + goto out;
    > + }
    > +
    > + xmit_cmd(drvdata->gpios, AS_WRITE_ENABLE);
    > +
    > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 0);
    > + /* op code */
    > + xmit_byte(drvdata->gpios, AS_PAGE_PROGRAM);
    > + /* address */
    > + xmit_byte(drvdata->gpios, (*ppos >> 16) & 0xff);
    > + xmit_byte(drvdata->gpios, (*ppos >> 8) & 0xff);
    > + xmit_byte(drvdata->gpios, *ppos & 0xff);
    > + /* page data (LSB first) */
    > + for (i = 0; i < AS_PAGE_SIZE; i++)
    > + xmit_byte(drvdata->gpios, bitrev8(page_buf[i]));
    > + gpio_set_value(drvdata->gpios[ASIO_NCS].gpio, 1);
    > + mdelay(7);

    So if I write 100 pages, I will _spin_ for 700 ms. You should rather
    (m)sleep here.

    > +
    > + *ppos += AS_PAGE_SIZE;
    > + written += AS_PAGE_SIZE;
    > + }
    > +
    > +out:
    > + kfree(page_buf);
    > + return err ?: written;
    > +}
    ...
    > +static int __init altera_as_probe(struct platform_device *pdev)
    > +{
    ...
    > + drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
    > + drvdata->miscdev.name = kasprintf(GFP_KERNEL, "altera_as%d",
    > + pdata->id);
    > + if (drvdata->miscdev.name == NULL)
    > + return -ENOMEM;
    > + drvdata->miscdev.fops = &altera_as_fops;
    > + if (misc_register(&drvdata->miscdev) < 0) {

    Hmm, how many devices can be in the system? This doesn't look like the
    right way.

    > + kfree(drvdata->miscdev.name);
    > + return -ENODEV;
    > + }
    > + altera_as_devs[pdata->id].minor = drvdata->miscdev.minor;
    > + altera_as_devs[pdata->id].drvdata = drvdata;

    So now the device is usable without the mutex and gpios inited?

    > + mutex_init(&drvdata->open_lock);
    > +
    > + drvdata->id = pdata->id;
    > +
    > + drvdata->gpios[ASIO_DATA].gpio = pdata->data;
    > + drvdata->gpios[ASIO_DATA].flags = GPIOF_IN;
    > + drvdata->gpios[ASIO_DATA].label = "as_data";
    ...
    > + drvdata->gpios[ASIO_NCE].gpio = pdata->nce;
    > + drvdata->gpios[ASIO_NCE].flags = GPIOF_OUT_INIT_HIGH;
    > + drvdata->gpios[ASIO_NCE].label = "as_nce";
    > +
    > + dev_info(drvdata->dev, "Altera Cyclone Active Serial driver\n");
    > +
    > + return 0;
    > +}

    regards,
    --
    js
    suse labs


    \
     
     \ /
      Last update: 2010-11-15 10:37    [W:0.032 / U:30.280 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site