From: Mike F. <vap...@gm...> - 2009-09-25 20:24:43
|
On Thu, Sep 24, 2009 at 19:32, Andrew Morton wrote: > On Thu, 17 Sep 2009 17:37:06 -0400 Mike Frysinger wrote: >> +config FB_BFIN_LQ035Q1 >> + tristate "SHARP LQ035Q1DH02 TFT LCD" >> + depends on FB && BLACKFIN >> + select FB_CFB_FILLRECT >> + select FB_CFB_COPYAREA >> + select FB_CFB_IMAGEBLIT >> + select BFIN_GPTIMERS >> + select SPI > > Are we sure about the `select SPI'? There's only one other place in > the kernel which does this, and `select' often makes things explode. I > fear that you're either selecting the wrong thing or you're selecting > something which won't work well. is it there on purpose and is it not just a mistaken typo ? yes. do we really need it and will we cry if it changes to a "depends" ? no. it's just confusing to have a device driver disappear if SPI is disabled, but considering SPI is enabled by default now, it's not a big deal. >> +#define DRIVER_NAME "bfin-lq035q1" >> +static char driver_name[] = DRIVER_NAME; > > Will the compielr magically put this string into read-only storage for > us, or should we do that manually with `const'? is this question a rhetorical one ? oh no, infinite loop ... i'll fix it up in v3 >> +static int lq035q1_control(unsigned char reg, unsigned short value) >> +{ >> + int ret; >> + u8 regs[3] = {LQ035_INDEX, 0, 0}; >> + u8 dat[3] = {LQ035_DATA, 0, 0}; >> + >> + if (spi_control.spidev) { >> + regs[2] = reg; >> + dat[1] = value >> 8; >> + dat[2] = value & 0xFF; >> + >> + ret = spi_write(spi_control.spidev, regs, ARRAY_SIZE(regs)); >> + ret |= spi_write(spi_control.spidev, dat, ARRAY_SIZE(dat)); >> + } else >> + return -ENODEV; >> + >> + return ret; >> +} > > I am suspecting that this function (and the similar ones below) rely > upon state within the hardware and will hence misbehave if two > instances are run concurrently. > > Is that correct> If so, is there locking to prevent this from occurring? if by "instances" you mean "users" as in "multiple programs reading/writing the framebuffer concurrently", then probably. rather than handle the locking ourselves, it can be pushed to the SPI bus by having the regs/dat be transfers in a single message. -mike |