lkml.org 
[lkml]   [2011]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC 2/5] gpio: add support for Technologic Systems TS-5500 GPIOs
    > + * The TS-5500 board has 38 GPIOs referred to as DIOs in the product's
    > + * litterature.

    literature

    > +#define MODULE_NAME "ts5500-gpio"

    see dev_dbg etc and the fmt support in them - they will do all this for
    you nicely and the various printks can become dev_* which will tidy it up
    and give more info

    > +
    > +#define PORT_BIT_SET(addr, bit) \
    > + do { \
    > + u8 var; \
    > + var = inb(addr); \
    > + var |= (1 << bit); \
    > + outb(var, addr); \
    > + } while (0)
    > +
    > +#define PORT_BIT_CLEAR(addr, bit) \
    > + do { \
    > + u8 var; \
    > + var = inb(addr); \
    > + var &= ~(1 << bit); \
    > + outb(var, addr); \
    > + } while (0)

    These shouldn't be magic macros especially if creating variables and give
    the multiple references to addr will break on things like ++ - use a
    function so it is typechecked and variables behave as espected.


    > + /* Request DIO1 */
    > + if (!request_region(0x7A, 3, "ts5500-gpio-DIO1")) {
    > + dev_err(&pdev->dev, "Cannot request I/O port 0x7A-7C\n");
    > + goto err_req_dio1;
    > + }
    > +
    > + /* Request DIO2 */
    > + if (!request_region(0x7D, 3, "ts5500-gpio-DIO2")) {
    > + dev_err(&pdev->dev, "Cannot request I/O port 0x7D-7F\n");
    > + goto err_req_dio2;
    > + }
    > +
    > + /* Request LCDIO if wanted */
    > + if (use_lcdio && !request_region(0x72, 2, "ts5500-gpio-LCD")) {
    > + dev_err(&pdev->dev, "Cannot request I/O port 0x72-73\n");
    > + goto err_req_lcdio;
    > + }

    I'd have expected these to be resources of the platform device but I
    guess that if they are entirely fixed it doesn't really matter much.

    > + /* Enable IRQ generation */
    > + mutex_lock(&drvdata->gpio_lock);
    > + PORT_BIT_SET(0x7A, 7); /* DIO1_13 on IRQ7 */
    > + PORT_BIT_SET(0x7D, 7); /* DIO2_13 on IRQ6 */
    > + if (use_lcdio) {
    > + PORT_BIT_CLEAR(0x7D, 4); /* Enable LCD header usage as DIO */
    > + PORT_BIT_SET(0x7D, 6); /* LCD_RS on IRQ1 */
    > + }

    What happens if an IRQ occurs at this point, you have no handler for it ?

    > +err_gpiochip_add:
    > + printk(KERN_ERR "gpio: Failed registering gpio chip.\n");
    > + kfree(drvdata);

    pr_err/dev_err etc - in general


    > +
    > +static int __init ts5500_gpio_init(void)
    > +{
    > + int ret;
    > +
    > + ret = platform_driver_register(&ts5500_gpio_driver);
    > + if (ret)
    > + return ret;
    > +
    > + ret = platform_device_register(&gpio_device);
    > + if (ret) {
    > + printk(MODULE_NAME ": Failed to register platform device\n");
    > + platform_driver_unregister(&ts5500_gpio_driver);
    > + return ret;
    > + }
    > +
    > + printk(MODULE_NAME ": GPIO/DIO driver loaded.\n");

    prinkt isn't needed

    Also you've done no checks to see if it's being loaded on the right
    board. I'd actually have expected a single chunk of code in
    arch/x86/platform/ts5500 or similar to do the board check and create the
    platform devices, and this code to just register them.



    \
     
     \ /
      Last update: 2011-04-30 12:17    [W:4.062 / U:0.216 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site