> + * 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.
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86"
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
