On Fri, Apr 25, 2008 at 02:56:37PM -0400, Justin Waters wrote:
> The atmel tsadcc is a touchscreen/adc controller found on the AT91SAM9RL
> SoC. This driver provides basic support for this controller for use as a
> touchscreen. Some future improvements include suspend/resume capabilities
> and debugfs support.
>...
> +config TOUCHSCREEN_ATMEL_TSADCC
> + tristate "Atmel Touchscreen Interface"
No dependencies. So does this mean this'll build and link on an
x86 platform?
> +/* These values are dependent upon the performance of the ADC. Ideally,
> + * These should be as close to the startup time and debounce time as
> + * possible (without going over). These assume an optimally setup ADC as
> + * per the electrical characteristics of the AT91SAM9RL tsadcc module
> + */
> +#define TS_POLL_DELAY (1 * 1000000) /* ns delay before the first sample */
> +#define TS_POLL_PERIOD (5 * 1000000) /* ns delay between samples */
So 1ms and 5ms... Not particularly high resolution...
> +static enum hrtimer_restart atmel_tsadcc_timer(struct hrtimer *handle)
So this uses the highres timer infrastructure. Does this build if
CONFIG_HIGH_RES_TIMERS is not set?
> +static int __devinit atmel_tsadcc_probe(struct platform_device *pdev)
> +{
> + struct atmel_tsadcc *ts;
> + struct input_dev *input_dev;
> + struct atmel_tsadcc_platform_data *pdata = pdev->dev.platform_data;
> + int err;
> + struct resource *res;
> + unsigned int regbuf;
This looks haphazard.
> + /* Touchscreen Information */
> + ts->pdev = pdev;
> + ts->input = input_dev;
Weird indentation.
> + /* Remap Register Memory */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + ts->reg_start = res->start;
> + ts->reg_length = res->end - res->start +1;
So if no memory resources were passed, this oopses.
> + /* Setup Clock */
> + ts->tsc_clk = clk_get(&pdev->dev,"tsc_clk");
> + if (IS_ERR(ts->tsc_clk)) {
Great, nice to see a driver using the clock API properly.
> + dev_err(&pdev->dev, "unable to get clock\n");
> + err = PTR_ERR(ts->tsc_clk);
> + goto err_iounmap;
> + }
Odd indentation on the last line above.
> + clk_enable(ts->tsc_clk);
> +
> + regbuf = clk_get_rate(ts->tsc_clk);
> + dev_dbg (&pdev->dev,"Master clock is set at: %d Hz\n",regbuf);
> +
> + /* Setup IRQ */
> + ts->irq = platform_get_irq(pdev, 0);
> + if (ts->irq < 0) {
> + dev_err(&pdev->dev, "unable to get IRQ\n");
> + err = -EBUSY;
> + goto err_clk_put;
> + }
Ditto.
> +
> + /* Fill in initial Register Data */
> + ATMEL_TSADCC_RESET;
> + regbuf = (0x01 & 0x3) | /* TSAMOD */
> + ((0x0 & 0x1) << 5) | /* SLEEP */
(0 & 1) << 5 ? How about using a #define for bit 5 and simply
omitting it?
> + ((0x1 & 0x1) << 6) | /* PENDET */
And a #define for bit 6 as well?
> + ((pdata->prescaler & 0x3F) << 8) | /* PRESCAL */
> + ((pdata->startup & 0x7F) << 16) | /* STARTUP */
> + ((pdata->debounce & 0x0F) << 28); /* PENDBC */
> + atmel_tsadcc_writereg(ATMEL_TSADCC_MR,regbuf);
> +
> + regbuf = (0x4 & 0x7) | /* TRGMOD */
> + ((0xFFFF & 0xFFFF) << 16); /* TRGPER */
> + atmel_tsadcc_writereg(ATMEL_TSADCC_TRGR,regbuf);
> +
> + regbuf = ((pdata->tsshtim & 0xF) << 24); /* TSSHTIM */
> + atmel_tsadcc_writereg(ATMEL_TSADCC_TSR,regbuf);
> +
> + regbuf = atmel_tsadcc_readreg(ATMEL_TSADCC_IMR);
> + dev_dbg (&pdev->dev, "Initial IMR = %X\n", regbuf);
> +
> + /* Register and enable IRQ */
> + if (request_irq(ts->irq, atmel_tsadcc_irq, IRQF_TRIGGER_LOW,
> + pdev->dev.driver->name, ts)) {
> + dev_dbg(&pdev->dev, "IRQ %d busy!\n", ts->irq);
> + err = -EBUSY;
> + goto err_iounmap;
> + }
Ditto.
> + regbuf = ((0x1 & 0x1) << 20) | /* PENCNT */
> + ((0x1 & 0x1) << 21); /* NOCNT */
#defines... even more so then the comments become entirely unnecessary.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
