Messages in this thread | | | From | Mike Frysinger <> | Date | Fri, 25 Sep 2009 16:24:10 -0400 | Subject | Re: [PATCH v2] fbdev: bfin-lq035q1-fb: new Blackfin Landscape LCD EZ-Extender driver |
| |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |