| Message ID | 1345164583-18924-4-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote: > +/* ----------------------------------------------------------------------------- > + * Bus operations > + */ > + > +void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned long cmd) > +{ > + dev->bus->ops->write_command(dev->bus, cmd); > +} > +EXPORT_SYMBOL_GPL(panel_dbi_write_command); > + > +void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long data) > +{ > + dev->bus->ops->write_data(dev->bus, data); > +} > +EXPORT_SYMBOL_GPL(panel_dbi_write_data); > + > +unsigned long panel_dbi_read_data(struct panel_dbi_device *dev) > +{ > + return dev->bus->ops->read_data(dev->bus); > +} > +EXPORT_SYMBOL_GPL(panel_dbi_read_data); I'm not that familiar with how to implement bus drivers, can you describe in pseudo code how the SoC's DBI driver would register these? I think write/read data functions are a bit limited. Shouldn't they be something like write_data(const u8 *buf, int size) and read_data(u8 *buf, int len)? Something that's totally missing is configuring the DBI bus. There are a bunch of timing related values that need to be configured. See include/video/omapdss.h struct rfbi_timings. While the struct is OMAP specific, if I recall right most of the values match to the MIPI DBI spec. And this makes me wonder, you use DBI bus for SYS-80 panel. The busses may look similar in operation, but are they really so similar when you take into account the timings (and perhaps something else, it's been years since I read the MIPI DBI spec)? Then there's the start_transfer. This is something I'm not sure what is the best way to handle it, but the same problems that I mentioned in the previous post related to enable apply here also. For example, what if the panel needs to be update in two parts? This is done in Nokia N9. From panel's perspective, it'd be best to handle it somewhat like this (although asynchronously, probably): write_update_area(0, 0, xres, yres / 2); write_memory_start() start_pixel_transfer(); wait_transfer_done(); write_update_area(0, yres / 2, xres, yres / 2); write_memory_start() start_pixel_transfer(); Why I said I'm not sure about this is that it does complicate things, as the actual pixel data often comes from the display subsystem hardware, which should probably be controlled by the display driver. I think there also needs to be some kind of transfer_done notifier, for both the display driver and the panel driver. Although if the display driver handles starting the actual pixel transfer, then it'll get the transfer_done via whatever interrupt the SoC has. Also as food for thought, videomode timings does not really make sense for DBI panels, at least when you just consider the DBI side. There's really just the resolution of the display, and then the DBI timings. No pck, syncs, etc. Of course in the end there's the actual panel, which does have these video timings. But often they cannot be configured, and often you don't even know them as the specs don't tell them. Tomi
Hi Tomi, Thank you for the review. On Friday 17 August 2012 12:03:02 Tomi Valkeinen wrote: > On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote: > > +/* > > ------------------------------------------------------------------------- > > ---- + * Bus operations > > + */ > > + > > +void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned long > > cmd) +{ > > + dev->bus->ops->write_command(dev->bus, cmd); > > +} > > +EXPORT_SYMBOL_GPL(panel_dbi_write_command); > > + > > +void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long > > data) +{ > > + dev->bus->ops->write_data(dev->bus, data); > > +} > > +EXPORT_SYMBOL_GPL(panel_dbi_write_data); > > + > > +unsigned long panel_dbi_read_data(struct panel_dbi_device *dev) > > +{ > > + return dev->bus->ops->read_data(dev->bus); > > +} > > +EXPORT_SYMBOL_GPL(panel_dbi_read_data); > > I'm not that familiar with how to implement bus drivers, can you > describe in pseudo code how the SoC's DBI driver would register these? Sure. The DBI bus driver first needs to create a panel_dbi_bus_ops instance: static const struct panel_dbi_bus_ops sh_mobile_lcdc_dbi_bus_ops = { .write_command = lcdc_dbi_write_command, .write_data = lcdc_dbi_write_data, .read_data = lcdc_dbi_read_data, }; and a panel_dbi_bus instance, usually embedded in its private driver data structure, and initialize it by setting its dev and ops fields: ch->dbi_bus.dev = ch->lcdc->dev; ch->dbi_bus.ops = &sh_mobile_lcdc_dbi_bus_ops; In my current implementation, the panel_dbi_device is created in board code: static struct panel_dbi_device migor_panel_device = { .name = "r61505", .id = 0, .dev = { .platform_data = &migor_panel_info, }, }; A pointer to that structure is passed to the DBI master driver, which then registers the device: panel_dbi_device_register(dbi, &ch->dbi_bus); With a DT-based solution the DBI core will expose a function to register DBI devices from OF nodes. The bus itself is currently not registered with the DBI code because there was no need to. > I think write/read data functions are a bit limited. Shouldn't they be > something like write_data(const u8 *buf, int size) and read_data(u8 > *buf, int len)? Good point. My hardware doesn't support multi-byte read/write operations directly so I haven't thought about adding those. Can your hardware group command + data writes in a single operation ? If so we should expose that at the API level as well. Is DBI limited to 8-bit data transfers for commands ? Pixels can be transferred 16-bit at a time, commands might as well. While DCS only specifies 8-bit command/data, DBI panels that are not DCS compliant can use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel, does so). > Something that's totally missing is configuring the DBI bus. There are a > bunch of timing related values that need to be configured. See > include/video/omapdss.h struct rfbi_timings. While the struct is OMAP > specific, if I recall right most of the values match to the MIPI DBI > spec. I've left that out currently, and thought about passing that information as platform data to the DBI bus driver. That's the easiest solution, but I agree that it's a hack. Panel should expose their timing requirements to the DBI host. API wise that wouldn't be difficult (we only need to add timing information to the panel platform data and add a function to the DBI API to retrieve it), one of challenges might be to express it in a way that's both universal enough and easy to use for DBI bus drivers. > And this makes me wonder, you use DBI bus for SYS-80 panel. The busses > may look similar in operation, but are they really so similar when you > take into account the timings (and perhaps something else, it's been > years since I read the MIPI DBI spec)? I'll have to check all the details. SYS-80 is similar to DBI-B, but supports a wider bus width of 18 bits. I think the interfaces are similar enough to use a single bus implementation, possibly with quirks and/or options (see SCCB support in I2C for instance, with flags to ignore acks, force a stop bit generation, ...). We would duplicate lots of code if we had two different implementations, and would prevent a DBI panel to be attached to a SYS-80 host and vice-versa (the format is known to work). > Then there's the start_transfer. This is something I'm not sure what is > the best way to handle it, but the same problems that I mentioned in the > previous post related to enable apply here also. For example, what if > the panel needs to be update in two parts? This is done in Nokia N9. > From panel's perspective, it'd be best to handle it somewhat like this > (although asynchronously, probably): > > write_update_area(0, 0, xres, yres / 2); > write_memory_start() > start_pixel_transfer(); > > wait_transfer_done(); > > write_update_area(0, yres / 2, xres, yres / 2); > write_memory_start() > start_pixel_transfer(); > > Why I said I'm not sure about this is that it does complicate things, as > the actual pixel data often comes from the display subsystem hardware, > which should probably be controlled by the display driver. I have no solution for this at the moment. That's an advanced (but definitely required) feature, I've tried to concentrate on the basics first. > I think there also needs to be some kind of transfer_done notifier, for > both the display driver and the panel driver. Although if the display > driver handles starting the actual pixel transfer, then it'll get the > transfer_done via whatever interrupt the SoC has. > > Also as food for thought, videomode timings does not really make sense > for DBI panels, at least when you just consider the DBI side. There's > really just the resolution of the display, and then the DBI timings. No > pck, syncs, etc. Of course in the end there's the actual panel, which > does have these video timings. But often they cannot be configured, and > often you don't even know them as the specs don't tell them. We might just need to provide fake timings. Video mode timings are at the core of display support in all drivers so we can't just get rid of them. The h/v front/back porch and sync won't be used by display drivers for DBI/DSI panels anyway.
On Fri, 2012-08-17 at 12:02 +0200, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the review. > > On Friday 17 August 2012 12:03:02 Tomi Valkeinen wrote: > > On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote: > > > +/* > > > ------------------------------------------------------------------------- > > > ---- + * Bus operations > > > + */ > > > + > > > +void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned long > > > cmd) +{ > > > + dev->bus->ops->write_command(dev->bus, cmd); > > > +} > > > +EXPORT_SYMBOL_GPL(panel_dbi_write_command); > > > + > > > +void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long > > > data) +{ > > > + dev->bus->ops->write_data(dev->bus, data); > > > +} > > > +EXPORT_SYMBOL_GPL(panel_dbi_write_data); > > > + > > > +unsigned long panel_dbi_read_data(struct panel_dbi_device *dev) > > > +{ > > > + return dev->bus->ops->read_data(dev->bus); > > > +} > > > +EXPORT_SYMBOL_GPL(panel_dbi_read_data); > > > > I'm not that familiar with how to implement bus drivers, can you > > describe in pseudo code how the SoC's DBI driver would register these? > > Sure. > > The DBI bus driver first needs to create a panel_dbi_bus_ops instance: > > static const struct panel_dbi_bus_ops sh_mobile_lcdc_dbi_bus_ops = { > .write_command = lcdc_dbi_write_command, > .write_data = lcdc_dbi_write_data, > .read_data = lcdc_dbi_read_data, > }; Thanks for the example, I think it cleared up things a bit. As I mentioned earlier, I really think "panel" is not right here. While the whole framework may be called panel framework, the bus drivers are not panels, and we should support external chips also, which are not panels either. > > I think write/read data functions are a bit limited. Shouldn't they be > > something like write_data(const u8 *buf, int size) and read_data(u8 > > *buf, int len)? > > Good point. My hardware doesn't support multi-byte read/write operations > directly so I haven't thought about adding those. OMAP HW doesn't support it either. Well, not quite true, as OMAP's system DMA could be used to write a buffer to the DBI output. But that's really the same as doing the write with a a loop with CPU. But first, the data type should be byte, not unsigned long. How would you write 8 bits or 16 bits with your API? And second, if the function takes just u8, you'd need lots of calls to do simple writes. > Can your hardware group command + data writes in a single operation ? If so we > should expose that at the API level as well. No it can't. But with DCS that is a common operation, so we could have some helpers to send command + data with one call. Then again, I'd hope to have DCS somehow as a separate library, which would then use DBI/DSI/whatnot to actually send the data. I'm not quite sure how easy that is because of the differences between the busses. > Is DBI limited to 8-bit data transfers for commands ? Pixels can be > transferred 16-bit at a time, commands might as well. While DCS only specifies > 8-bit command/data, DBI panels that are not DCS compliant can use 16-bit > command/data (the R61505 panel, albeit a SYS-80 panel, does so). I have to say I don't remember much about DBI =). Looking at OMAP's driver, which was made for omap2 and hasn't been much updated since, I see that there are 4 modes, 8/9/12/16 bits. I think that defines how many of the parallel data lines are used. However, I don't think that matters for the panel driver when it wants to send data. The panel driver should just call dbi_write(buf, buf_len), and the dbi driver would send the data in the buffer according to the bus width. Also note that some chips need to change the bus width on the fly. The chip used on N800 wants configuration to be done with 8-bits, and pixel transfers with 16-bits. Who knows why... So I think this, and generally most of the configuration, should be somewhat dynamic, so that the panel driver can change them when it needs. > > Something that's totally missing is configuring the DBI bus. There are a > > bunch of timing related values that need to be configured. See > > include/video/omapdss.h struct rfbi_timings. While the struct is OMAP > > specific, if I recall right most of the values match to the MIPI DBI > > spec. > > I've left that out currently, and thought about passing that information as > platform data to the DBI bus driver. That's the easiest solution, but I agree > that it's a hack. Panel should expose their timing requirements to the DBI > host. API wise that wouldn't be difficult (we only need to add timing > information to the panel platform data and add a function to the DBI API to > retrieve it), one of challenges might be to express it in a way that's both > universal enough and easy to use for DBI bus drivers. As I pointed above, I think the panel driver shouldn't expose it, but the panel driver should somehow set it. Or at least allowed to change it in some manner. This is actually again, the same problem as with enable and transfer: who controls what's going on. How I think it should work is something like: mipi_dbi_set_timings(dbi_dev, mytimings); mipi_dbi_set_bus_width(dbi_dev, 8); mipi_dbi_write(dbi_dev, ...); mipi_dbi_set_bus_width(dbi_dev, 16); start_frame_transfer(dbi_dev, ...); > > And this makes me wonder, you use DBI bus for SYS-80 panel. The busses > > may look similar in operation, but are they really so similar when you > > take into account the timings (and perhaps something else, it's been > > years since I read the MIPI DBI spec)? > > I'll have to check all the details. SYS-80 is similar to DBI-B, but supports a > wider bus width of 18 bits. I think the interfaces are similar enough to use a > single bus implementation, possibly with quirks and/or options (see SCCB > support in I2C for instance, with flags to ignore acks, force a stop bit > generation, ...). We would duplicate lots of code if we had two different > implementations, and would prevent a DBI panel to be attached to a SYS-80 host > and vice-versa (the format is known to work). Ah ok, if a DBI panel can be connected to SYS-80 output and vice versa, then I agree they are similar enough. > We might just need to provide fake timings. Video mode timings are at the core > of display support in all drivers so we can't just get rid of them. The h/v > front/back porch and sync won't be used by display drivers for DBI/DSI panels > anyway. Right. But we should probably think if we can, at the panel level, easily separate conventional panels and smart panels. Then this framework wouldn't need to fake the timings, and it'd be up to the higher level to decide if and how to fake them. Then again, this is no biggie. Just thought that at the lowest level it'd be nice to be "correct" and leave faking to upper layers =). Tomi
Hi Tomi, On Friday 17 August 2012 13:51:49 Tomi Valkeinen wrote: > On Fri, 2012-08-17 at 12:02 +0200, Laurent Pinchart wrote: > > On Friday 17 August 2012 12:03:02 Tomi Valkeinen wrote: > > > On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote: > > > > +/* > > > > ---------------------------------------------------------------------- > > > > --- > > > > ---- + * Bus operations > > > > + */ > > > > + > > > > +void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned > > > > long > > > > cmd) +{ > > > > + dev->bus->ops->write_command(dev->bus, cmd); > > > > +} > > > > +EXPORT_SYMBOL_GPL(panel_dbi_write_command); > > > > + > > > > +void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long > > > > data) +{ > > > > + dev->bus->ops->write_data(dev->bus, data); > > > > +} > > > > +EXPORT_SYMBOL_GPL(panel_dbi_write_data); > > > > + > > > > +unsigned long panel_dbi_read_data(struct panel_dbi_device *dev) > > > > +{ > > > > + return dev->bus->ops->read_data(dev->bus); > > > > +} > > > > +EXPORT_SYMBOL_GPL(panel_dbi_read_data); > > > > > > I'm not that familiar with how to implement bus drivers, can you > > > describe in pseudo code how the SoC's DBI driver would register these? > > > > Sure. > > > > The DBI bus driver first needs to create a panel_dbi_bus_ops instance: > > > > static const struct panel_dbi_bus_ops sh_mobile_lcdc_dbi_bus_ops = { > > > > .write_command = lcdc_dbi_write_command, > > .write_data = lcdc_dbi_write_data, > > .read_data = lcdc_dbi_read_data, > > > > }; > > Thanks for the example, I think it cleared up things a bit. > > As I mentioned earlier, I really think "panel" is not right here. While > the whole framework may be called panel framework, the bus drivers are > not panels, and we should support external chips also, which are not > panels either. I agree. I've renamed panel_dbi_* to mipi_dbi_*. > > > I think write/read data functions are a bit limited. Shouldn't they be > > > something like write_data(const u8 *buf, int size) and read_data(u8 > > > *buf, int len)? > > > > Good point. My hardware doesn't support multi-byte read/write operations > > directly so I haven't thought about adding those. > > OMAP HW doesn't support it either. Well, not quite true, as OMAP's > system DMA could be used to write a buffer to the DBI output. But that's > really the same as doing the write with a a loop with CPU. > > But first, the data type should be byte, not unsigned long. How would > you write 8 bits or 16 bits with your API? u8 and u16 both fit in an unsigned long :-) Please see below. > And second, if the function takes just u8, you'd need lots of calls to do > simple writes. I agree, an array write function is a good idea. > > Can your hardware group command + data writes in a single operation ? If > > so we should expose that at the API level as well. > > No it can't. But with DCS that is a common operation, so we could have > some helpers to send command + data with one call. Agreed. > Then again, I'd hope to have DCS somehow as a separate library, which would > then use DBI/DSI/whatnot to actually send the data. > > I'm not quite sure how easy that is because of the differences between > the busses. > > > Is DBI limited to 8-bit data transfers for commands ? Pixels can be > > transferred 16-bit at a time, commands might as well. While DCS only > > specifies 8-bit command/data, DBI panels that are not DCS compliant can > > use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel, does > > so). > > I have to say I don't remember much about DBI =). Looking at OMAP's > driver, which was made for omap2 and hasn't been much updated since, I > see that there are 4 modes, 8/9/12/16 bits. I think that defines how > many of the parallel data lines are used. SYS-80 also has an 18-bits mode, where bits 0 and 9 are always ignored when transferring instructions and data other than pixels (for pixels the 18-bits bus width can be used to transfer RGB666 in a single clock cycle). See page 87 of http://read.pudn.com/downloads91/sourcecode/others/348230/e61505_103a.pdf. > However, I don't think that matters for the panel driver when it wants > to send data. The panel driver should just call dbi_write(buf, buf_len), > and the dbi driver would send the data in the buffer according to the > bus width. According to the DCS specification, commands and parameters are transferred using 8-bit data. Non-DCS panels can however use wider commands and parameters (all commands and parameters are 16-bits wide for the R61505 for instance). We can add an API to switch the DBI bus width on the fly. For Renesas hardware this would "just" require shifting bits around to output the 8-bit or 16-bit commands on the right data lines (the R61505 uses D17-D9 in 8-bit mode, while the DCS specification mentions D7-D0) based on how the panel is connected and on which lines the panel expects data. As commands can be expressed on either 8 or 16 bits I would use a 16 type for them. For parameters, we can either express everything as u8 * in the DBI bus operations, or use a union similar to i2c_smbus_data union i2c_smbus_data { __u8 byte; __u16 word; __u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */ /* and one more for user-space compatibility */ }; Helper functions would be available to perform 8-bit, 16-bit or n*8 bits transfers. Would that work for your use cases ? > Also note that some chips need to change the bus width on the fly. The > chip used on N800 wants configuration to be done with 8-bits, and pixel > transfers with 16-bits. Who knows why... On which data lines is configuration performed ? D7-D0 ? > So I think this, and generally most of the configuration, should be > somewhat dynamic, so that the panel driver can change them when it > needs. > > > > Something that's totally missing is configuring the DBI bus. There are a > > > bunch of timing related values that need to be configured. See > > > include/video/omapdss.h struct rfbi_timings. While the struct is OMAP > > > specific, if I recall right most of the values match to the MIPI DBI > > > spec. > > > > I've left that out currently, and thought about passing that information > > as platform data to the DBI bus driver. That's the easiest solution, but I > > agree that it's a hack. Panel should expose their timing requirements to > > the DBI host. API wise that wouldn't be difficult (we only need to add > > timing information to the panel platform data and add a function to the > > DBI API to retrieve it), one of challenges might be to express it in a > > way that's both universal enough and easy to use for DBI bus drivers. > > As I pointed above, I think the panel driver shouldn't expose it, but > the panel driver should somehow set it. Or at least allowed to change it > in some manner. This is actually again, the same problem as with enable > and transfer: who controls what's going on. > > How I think it should work is something like: > > mipi_dbi_set_timings(dbi_dev, mytimings); > mipi_dbi_set_bus_width(dbi_dev, 8); > mipi_dbi_write(dbi_dev, ...); > mipi_dbi_set_bus_width(dbi_dev, 16); > start_frame_transfer(dbi_dev, ...); I'll first implement bus width setting. > > > And this makes me wonder, you use DBI bus for SYS-80 panel. The busses > > > may look similar in operation, but are they really so similar when you > > > take into account the timings (and perhaps something else, it's been > > > years since I read the MIPI DBI spec)? > > > > I'll have to check all the details. SYS-80 is similar to DBI-B, but > > supports a wider bus width of 18 bits. I think the interfaces are similar > > enough to use a single bus implementation, possibly with quirks and/or > > options (see SCCB support in I2C for instance, with flags to ignore acks, > > force a stop bit generation, ...). We would duplicate lots of code if we > > had two different implementations, and would prevent a DBI panel to be > > attached to a SYS-80 host and vice-versa (the format is known to work). > > Ah ok, if a DBI panel can be connected to SYS-80 output and vice versa, > then I agree they are similar enough. Not all combination will work (a SYS panel that requires 16-bit transfers won't work with a DBI host that only supports 8-bit), but some do. > > We might just need to provide fake timings. Video mode timings are at the > > core of display support in all drivers so we can't just get rid of them. > > The h/v front/back porch and sync won't be used by display drivers for > > DBI/DSI panels anyway. > > Right. But we should probably think if we can, at the panel level, easily > separate conventional panels and smart panels. Then this framework wouldn't > need to fake the timings, and it'd be up to the higher level to decide if > and how to fake them. Then again, this is no biggie. Just thought that at > the lowest level it'd be nice to be "correct" and leave faking to upper > layers =). But we would then have two different APIs at the lower level depending on the panel type. I'm not sure that's a good thing.
On Fri, 2012-08-17 at 14:33 +0200, Laurent Pinchart wrote: > > But first, the data type should be byte, not unsigned long. How would > > you write 8 bits or 16 bits with your API? > > u8 and u16 both fit in an unsigned long :-) Please see below. Ah, I see, so the driver would just discard 24 bits or 16 bits from the ulong. I somehow thought that if you have 8 bit bus, and you call the write with ulong, 4 bytes will be written. > > Then again, I'd hope to have DCS somehow as a separate library, which would > > then use DBI/DSI/whatnot to actually send the data. > > > > I'm not quite sure how easy that is because of the differences between > > the busses. > > > > > Is DBI limited to 8-bit data transfers for commands ? Pixels can be > > > transferred 16-bit at a time, commands might as well. While DCS only > > > specifies 8-bit command/data, DBI panels that are not DCS compliant can > > > use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel, does > > > so). > > > > I have to say I don't remember much about DBI =). Looking at OMAP's > > driver, which was made for omap2 and hasn't been much updated since, I > > see that there are 4 modes, 8/9/12/16 bits. I think that defines how > > many of the parallel data lines are used. > > SYS-80 also has an 18-bits mode, where bits 0 and 9 are always ignored when > transferring instructions and data other than pixels (for pixels the 18-bits > bus width can be used to transfer RGB666 in a single clock cycle). > > See page 87 of > http://read.pudn.com/downloads91/sourcecode/others/348230/e61505_103a.pdf. > > > However, I don't think that matters for the panel driver when it wants > > to send data. The panel driver should just call dbi_write(buf, buf_len), > > and the dbi driver would send the data in the buffer according to the > > bus width. > > According to the DCS specification, commands and parameters are transferred > using 8-bit data. Non-DCS panels can however use wider commands and parameters > (all commands and parameters are 16-bits wide for the R61505 for instance). > > We can add an API to switch the DBI bus width on the fly. For Renesas hardware > this would "just" require shifting bits around to output the 8-bit or 16-bit > commands on the right data lines (the R61505 uses D17-D9 in 8-bit mode, while > the DCS specification mentions D7-D0) based on how the panel is connected and > on which lines the panel expects data. > > As commands can be expressed on either 8 or 16 bits I would use a 16 type for > them. I won't put my head on the block, but I don't think DBI has any restriction on the size of the command. A "command" just means a data transfer while keeping the D/CX line low, and "data" when the line is high. Similar transfers for n bytes can be done in both modes. > For parameters, we can either express everything as u8 * in the DBI bus > operations, or use a union similar to i2c_smbus_data > > union i2c_smbus_data { > __u8 byte; > __u16 word; > __u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */ > /* and one more for user-space compatibility */ > }; There's no DBI_BLOCK_MAX, so at least identical union won't work. I think it's simplest to have u8 * function as a base, and then a few helpers to write the most common datatypes. So we could have on the lowest level something like: dbi_write_command(u8 *buf, size_t size); dbi_write_data(u8 *buf, size_t size); And possible helpers: dbi_write_data(u8 *cmd_buf, size_t cmd_size, u8 *data_buf, size_t data_size); dbi_write_dcs(u8 cmd, u8 *data, size_t size); And variations: dbi_write_dcs_0(u8 cmd); dbi_write_dcs_1(u8 cmd, u8 data); etc. So a simple helper to send 16 bits would be: dbi_write_data(u16 data) { // or are the bytes the other way around... u8 buf[2] = { data & 0xff, (data >> 8) & 0xff }; return dbi_write_data(buf, 2); } > Helper functions would be available to perform 8-bit, 16-bit or n*8 bits > transfers. > > Would that work for your use cases ? > > > Also note that some chips need to change the bus width on the fly. The > > chip used on N800 wants configuration to be done with 8-bits, and pixel > > transfers with 16-bits. Who knows why... > > On which data lines is configuration performed ? D7-D0 ? I guess so, but does it matter? All the bus driver needs to know is how to send 8/16/.. bit data. On OMAP we just write the data to a 32 bit register, and the HW takes the lowest n bits. Do the bits represent the data lines directly on Renesans? The omap driver actually only implements 8 and 16 bit modes, not the 9 and 12 bit modes. I'm not sure what kind of shifting is needed for those. > > > We might just need to provide fake timings. Video mode timings are at the > > > core of display support in all drivers so we can't just get rid of them. > > > The h/v front/back porch and sync won't be used by display drivers for > > > DBI/DSI panels anyway. > > > > Right. But we should probably think if we can, at the panel level, easily > > separate conventional panels and smart panels. Then this framework wouldn't > > need to fake the timings, and it'd be up to the higher level to decide if > > and how to fake them. Then again, this is no biggie. Just thought that at > > the lowest level it'd be nice to be "correct" and leave faking to upper > > layers =). > > But we would then have two different APIs at the lower level depending on the > panel type. I'm not sure that's a good thing. Different API for what? Why anyway need panel type specific functions. In the panel struct we could just have an union of the different types of parameters for different types of panels. But if this complicates things, it's not a biggie. Just something that has been in my mind when dealing with smart panels and assigning dummy video timings for them =). Tomi
Hi Tomi, On Friday 17 August 2012 16:06:30 Tomi Valkeinen wrote: > On Fri, 2012-08-17 at 14:33 +0200, Laurent Pinchart wrote: > > > But first, the data type should be byte, not unsigned long. How would > > > you write 8 bits or 16 bits with your API? > > > > u8 and u16 both fit in an unsigned long :-) Please see below. > > Ah, I see, so the driver would just discard 24 bits or 16 bits from the > ulong. That's right. > I somehow thought that if you have 8 bit bus, and you call the write with > ulong, 4 bytes will be written. > > > > Then again, I'd hope to have DCS somehow as a separate library, which > > > would then use DBI/DSI/whatnot to actually send the data. > > > > > > I'm not quite sure how easy that is because of the differences between > > > the busses. > > > > > > > Is DBI limited to 8-bit data transfers for commands ? Pixels can be > > > > transferred 16-bit at a time, commands might as well. While DCS only > > > > specifies 8-bit command/data, DBI panels that are not DCS compliant > > > > can use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel, > > > > does so). > > > > > > I have to say I don't remember much about DBI =). Looking at OMAP's > > > driver, which was made for omap2 and hasn't been much updated since, I > > > see that there are 4 modes, 8/9/12/16 bits. I think that defines how > > > many of the parallel data lines are used. > > > > SYS-80 also has an 18-bits mode, where bits 0 and 9 are always ignored > > when transferring instructions and data other than pixels (for pixels the > > 18-bits bus width can be used to transfer RGB666 in a single clock cycle). > > > > See page 87 of > > http://read.pudn.com/downloads91/sourcecode/others/348230/e61505_103a.pdf. > > > > > However, I don't think that matters for the panel driver when it wants > > > to send data. The panel driver should just call dbi_write(buf, buf_len), > > > and the dbi driver would send the data in the buffer according to the > > > bus width. > > > > According to the DCS specification, commands and parameters are > > transferred using 8-bit data. Non-DCS panels can however use wider > > commands and parameters (all commands and parameters are 16-bits wide for > > the R61505 for instance). > > > > We can add an API to switch the DBI bus width on the fly. For Renesas > > hardware this would "just" require shifting bits around to output the > > 8-bit or 16-bit commands on the right data lines (the R61505 uses D17-D9 > > in 8-bit mode, while the DCS specification mentions D7-D0) based on how > > the panel is connected and on which lines the panel expects data. > > > > As commands can be expressed on either 8 or 16 bits I would use a 16 type > > for them. > > I won't put my head on the block, but I don't think DBI has any > restriction on the size of the command. A "command" just means a data > transfer while keeping the D/CX line low, and "data" when the line is > high. Similar transfers for n bytes can be done in both modes. Right. I'll see if the API could be simplified by having a single write callback function with a data/command parameter. > > For parameters, we can either express everything as u8 * in the DBI bus > > operations, or use a union similar to i2c_smbus_data > > > > union i2c_smbus_data { > > > > __u8 byte; > > __u16 word; > > __u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for > > length */ > > > > /* and one more for user-space > > compatibility */ > > > > }; > > There's no DBI_BLOCK_MAX, so at least identical union won't work. I > think it's simplest to have u8 * function as a base, and then a few > helpers to write the most common datatypes. OK, that sounds good to me. > So we could have on the lowest level something like: > > dbi_write_command(u8 *buf, size_t size); > dbi_write_data(u8 *buf, size_t size); > > And possible helpers: > > dbi_write_data(u8 *cmd_buf, size_t cmd_size, u8 *data_buf, size_t > data_size); > > dbi_write_dcs(u8 cmd, u8 *data, size_t size); > > And variations: > > dbi_write_dcs_0(u8 cmd); > dbi_write_dcs_1(u8 cmd, u8 data); > > etc. So a simple helper to send 16 bits would be: > > dbi_write_data(u16 data) > { > // or are the bytes the other way around... > u8 buf[2] = { data & 0xff, (data >> 8) & 0xff }; > return dbi_write_data(buf, 2); > } > > > Helper functions would be available to perform 8-bit, 16-bit or n*8 bits > > transfers. > > > > Would that work for your use cases ? > > > > > Also note that some chips need to change the bus width on the fly. The > > > chip used on N800 wants configuration to be done with 8-bits, and pixel > > > transfers with 16-bits. Who knows why... > > > > On which data lines is configuration performed ? D7-D0 ? > > I guess so, but does it matter? All the bus driver needs to know is how > to send 8/16/.. bit data. On OMAP we just write the data to a 32 bit > register, and the HW takes the lowest n bits. Do the bits represent the > data lines directly on Renesans? Yes they do. For a SYS-80 panel configured in 18-bits mode, I'll have to write ((data & 0xff00) << 2) | ((data & 0x00ff) << 1) (d15:8 -> D17:10, d7:0 -> D8:1 where d is the word to be written, and D the physical lines) to the hardware data register and trigger the write. In 8 bits mode, there would be two write operations with (data & 0xff00) << 2 (data & 0x00ff) << 10 (d15:8 -> D17:10, d7:0 -> D17:10) However, when writing a 8-bit command to a DBI panel in either 16- or 8-bits mode, there would be a single write with d7:0 -> D7:0 How to shift the data thus depends both on the bus width and on which data lines the panel expects data to be present. I wrote drivers for two DBI panels based on existing board code, the R61505 and the R61517. The R61505 datasheet is available online, the panel is a SYS-80 panel that supports 8-, 9-, 16- and 18-bits bus widths. It aligns data towards the MSB when using a bus width lower than 18. The R61517 datasheet doesn't seem to be freely available. The panel seems to be DBI-compliant as it uses a subset of the DCS commands and a wide range of panel-specific commands. The panel is connected using a 16-bit bus, all commands and parameters are 8-bits wide and aligned towards the LSB. To properly transfer commands and parameters, the DBI host will need to know on how many bits to perform transfers, and how to align data on the bus. For the former, your mipi_dbi_set_bus_width() function could be used, although probably not out of the box. The R61505 panel would call mipi_dbi_set_bus_width() to set the bus width to 16 (as commands and parameters are 16-bits wide), but if the panel is connected using only 8 or 9 data lines, the host would need to split the 16-bits writes into two 8-bits writes. Should that be done transparently ? mipi_dbi_set_bus_width() could possibly act as a mipi_dbi_set_max_bus_width(), but that might be a bit too hackish. I'd like to hide as much of the complexity as possible in mipi-dbi-bus.c but I don't know whether that's possible. > The omap driver actually only implements 8 and 16 bit modes, not the 9 and > 12 bit modes. I'm not sure what kind of shifting is needed for those. There's no 12-bits mode in DBI-2 as far as I can tell. We will need to support 8-, 9- and 16-bits modes for DBI-2, and additionally 18-bits mode for SYS-80. > > > > We might just need to provide fake timings. Video mode timings are at > > > > the core of display support in all drivers so we can't just get rid of > > > > them. The h/v front/back porch and sync won't be used by display > > > > drivers for DBI/DSI panels anyway. > > > > > > Right. But we should probably think if we can, at the panel level, > > > easily separate conventional panels and smart panels. Then this > > > framework wouldn't need to fake the timings, and it'd be up to the > > > higher level to decide if and how to fake them. Then again, this is no > > > biggie. Just thought that at the lowest level it'd be nice to be > > > "correct" and leave faking to upper layers =). > > > > But we would then have two different APIs at the lower level depending on > > the panel type. I'm not sure that's a good thing. > > Different API for what? Why anyway need panel type specific functions. > In the panel struct we could just have an union of the different types > of parameters for different types of panels. > > But if this complicates things, it's not a biggie. Just something that > has been in my mind when dealing with smart panels and assigning dummy > video timings for them =). Please feel free to make a proposal for this when I'll post v2. A patch would be nice :-)
diff --git a/drivers/video/panel/Kconfig b/drivers/video/panel/Kconfig index 36fb9ca..fd0b3cf 100644 --- a/drivers/video/panel/Kconfig +++ b/drivers/video/panel/Kconfig @@ -12,4 +12,8 @@ config DISPLAY_PANEL_DUMMY If you are in doubt, say N. +config DISPLAY_PANEL_DBI + tristate + default n + endif # DISPLAY_PANEL diff --git a/drivers/video/panel/Makefile b/drivers/video/panel/Makefile index 9fc05c2..2ab0520 100644 --- a/drivers/video/panel/Makefile +++ b/drivers/video/panel/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_DISPLAY_PANEL) += panel.o obj-$(CONFIG_DISPLAY_PANEL_DUMMY) += panel-dummy.o +obj-$(CONFIG_DISPLAY_PANEL_DBI) += panel-dbi.o diff --git a/drivers/video/panel/panel-dbi.c b/drivers/video/panel/panel-dbi.c new file mode 100644 index 0000000..0511997 --- /dev/null +++ b/drivers/video/panel/panel-dbi.c @@ -0,0 +1,217 @@ +/* + * MIPI DBI Bus + * + * Copyright (C) 2012 Renesas Solutions Corp. + * + * Contacts: Laurent Pinchart <laurent.pinchart@ideasonboard.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/device.h> +#include <linux/export.h> +#include <linux/kernel.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/pm.h> +#include <linux/pm_runtime.h> + +#include <video/panel-dbi.h> + +/* ----------------------------------------------------------------------------- + * Bus operations + */ + +void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned long cmd) +{ + dev->bus->ops->write_command(dev->bus, cmd); +} +EXPORT_SYMBOL_GPL(panel_dbi_write_command); + +void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long data) +{ + dev->bus->ops->write_data(dev->bus, data); +} +EXPORT_SYMBOL_GPL(panel_dbi_write_data); + +unsigned long panel_dbi_read_data(struct panel_dbi_device *dev) +{ + return dev->bus->ops->read_data(dev->bus); +} +EXPORT_SYMBOL_GPL(panel_dbi_read_data); + +/* ----------------------------------------------------------------------------- + * Bus type + */ + +static const struct panel_dbi_device_id * +panel_dbi_match_id(const struct panel_dbi_device_id *id, + struct panel_dbi_device *dev) +{ + while (id->name[0]) { + if (strcmp(dev->name, id->name) == 0) { + dev->id_entry = id; + return id; + } + id++; + } + return NULL; +} + +static int panel_dbi_match(struct device *_dev, struct device_driver *_drv) +{ + struct panel_dbi_device *dev = to_panel_dbi_device(_dev); + struct panel_dbi_driver *drv = to_panel_dbi_driver(_drv); + + if (drv->id_table) + return panel_dbi_match_id(drv->id_table, dev) != NULL; + + return (strcmp(dev->name, _drv->name) == 0); +} + +static ssize_t modalias_show(struct device *_dev, struct device_attribute *a, + char *buf) +{ + struct panel_dbi_device *dev = to_panel_dbi_device(_dev); + int len = snprintf(buf, PAGE_SIZE, PANEL_DBI_MODULE_PREFIX "%s\n", + dev->name); + + return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len; +} + +static struct device_attribute panel_dbi_dev_attrs[] = { + __ATTR_RO(modalias), + __ATTR_NULL, +}; + +static int panel_dbi_uevent(struct device *_dev, struct kobj_uevent_env *env) +{ + struct panel_dbi_device *dev = to_panel_dbi_device(_dev); + + add_uevent_var(env, "MODALIAS=%s%s", PANEL_DBI_MODULE_PREFIX, + dev->name); + return 0; +} + +static const struct dev_pm_ops panel_dbi_dev_pm_ops = { + .runtime_suspend = pm_generic_runtime_suspend, + .runtime_resume = pm_generic_runtime_resume, + .runtime_idle = pm_generic_runtime_idle, + .suspend = pm_generic_suspend, + .resume = pm_generic_resume, + .freeze = pm_generic_freeze, + .thaw = pm_generic_thaw, + .poweroff = pm_generic_poweroff, + .restore = pm_generic_restore, +}; + +static struct bus_type panel_dbi_bus_type = { + .name = "mipi-dbi", + .dev_attrs = panel_dbi_dev_attrs, + .match = panel_dbi_match, + .uevent = panel_dbi_uevent, + .pm = &panel_dbi_dev_pm_ops, +}; + +/* ----------------------------------------------------------------------------- + * Device and driver (un)registration + */ + +/** + * panel_dbi_device_register - register a DBI device + * @dev: DBI device we're registering + */ +int panel_dbi_device_register(struct panel_dbi_device *dev, + struct panel_dbi_bus *bus) +{ + device_initialize(&dev->dev); + + dev->bus = bus; + dev->dev.bus = &panel_dbi_bus_type; + dev->dev.parent = bus->dev; + + if (dev->id != -1) + dev_set_name(&dev->dev, "%s.%d", dev->name, dev->id); + else + dev_set_name(&dev->dev, "%s", dev->name); + + return device_add(&dev->dev); +} +EXPORT_SYMBOL_GPL(panel_dbi_device_register); + +/** + * panel_dbi_device_unregister - unregister a DBI device + * @dev: DBI device we're unregistering + */ +void panel_dbi_device_unregister(struct panel_dbi_device *dev) +{ + device_del(&dev->dev); + put_device(&dev->dev); +} +EXPORT_SYMBOL_GPL(panel_dbi_device_unregister); + +static int panel_dbi_drv_probe(struct device *_dev) +{ + struct panel_dbi_driver *drv = to_panel_dbi_driver(_dev->driver); + struct panel_dbi_device *dev = to_panel_dbi_device(_dev); + + return drv->probe(dev); +} + +static int panel_dbi_drv_remove(struct device *_dev) +{ + struct panel_dbi_driver *drv = to_panel_dbi_driver(_dev->driver); + struct panel_dbi_device *dev = to_panel_dbi_device(_dev); + + return drv->remove(dev); +} + +/** + * panel_dbi_driver_register - register a driver for DBI devices + * @drv: DBI driver structure + */ +int panel_dbi_driver_register(struct panel_dbi_driver *drv) +{ + drv->driver.bus = &panel_dbi_bus_type; + if (drv->probe) + drv->driver.probe = panel_dbi_drv_probe; + if (drv->remove) + drv->driver.remove = panel_dbi_drv_remove; + + return driver_register(&drv->driver); +} +EXPORT_SYMBOL_GPL(panel_dbi_driver_register); + +/** + * panel_dbi_driver_unregister - unregister a driver for DBI devices + * @drv: DBI driver structure + */ +void panel_dbi_driver_unregister(struct panel_dbi_driver *drv) +{ + driver_unregister(&drv->driver); +} +EXPORT_SYMBOL_GPL(panel_dbi_driver_unregister); + +/* ----------------------------------------------------------------------------- + * Init/exit + */ + +static int __init panel_dbi_init(void) +{ + return bus_register(&panel_dbi_bus_type); +} + +static void __exit panel_dbi_exit(void) +{ + bus_unregister(&panel_dbi_bus_type); +} + +module_init(panel_dbi_init); +module_exit(panel_dbi_exit) + +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>"); +MODULE_DESCRIPTION("MIPI DBI Bus"); +MODULE_LICENSE("GPL"); diff --git a/include/video/panel-dbi.h b/include/video/panel-dbi.h new file mode 100644 index 0000000..799ac41 --- /dev/null +++ b/include/video/panel-dbi.h @@ -0,0 +1,92 @@ +/* + * MIPI DBI Bus + * + * Copyright (C) 2012 Renesas Solutions Corp. + * + * Contacts: Laurent Pinchart <laurent.pinchart@ideasonboard.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __PANEL_DBI_H__ +#define __PANEL_DBI_H__ + +#include <linux/device.h> +#include <video/panel.h> + +struct panel_dbi_bus; + +struct panel_dbi_bus_ops { + void (*write_command)(struct panel_dbi_bus *bus, unsigned long cmd); + void (*write_data)(struct panel_dbi_bus *bus, unsigned long data); + unsigned long (*read_data)(struct panel_dbi_bus *bus); +}; + +struct panel_dbi_bus { + struct device *dev; + const struct panel_dbi_bus_ops *ops; +}; + +#define PANEL_DBI_MODULE_PREFIX "mipi-dbi:" +#define PANEL_DBI_NAME_SIZE 32 + +struct panel_dbi_device_id { + char name[PANEL_DBI_NAME_SIZE]; + kernel_ulong_t driver_data /* Data private to the driver */ + __aligned(sizeof(kernel_ulong_t)); +}; + +struct panel_dbi_device { + const char *name; + int id; + struct device dev; + + const struct panel_dbi_device_id *id_entry; + struct panel_dbi_bus *bus; +}; + +#define to_panel_dbi_device(d) container_of(d, struct panel_dbi_device, dev) + +int panel_dbi_device_register(struct panel_dbi_device *dev, + struct panel_dbi_bus *bus); +void panel_dbi_device_unregister(struct panel_dbi_device *dev); + +struct panel_dbi_driver { + int(*probe)(struct panel_dbi_device *); + int(*remove)(struct panel_dbi_device *); + struct device_driver driver; + const struct panel_dbi_device_id *id_table; +}; + +#define to_panel_dbi_driver(d) container_of(d, struct panel_dbi_driver, driver) + +int panel_dbi_driver_register(struct panel_dbi_driver *drv); +void panel_dbi_driver_unregister(struct panel_dbi_driver *drv); + +static inline void *panel_dbi_get_drvdata(const struct panel_dbi_device *dev) +{ + return dev_get_drvdata(&dev->dev); +} + +static inline void panel_dbi_set_drvdata(struct panel_dbi_device *dev, + void *data) +{ + dev_set_drvdata(&dev->dev, data); +} + +/* module_panel_dbi_driver() - Helper macro for drivers that don't do + * anything special in module init/exit. This eliminates a lot of + * boilerplate. Each module may only use this macro once, and + * calling it replaces module_init() and module_exit() + */ +#define module_panel_dbi_driver(__panel_dbi_driver) \ + module_driver(__panel_dbi_driver, panel_dbi_driver_register, \ + panel_dbi_driver_unregister) + +void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned long cmd); +void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long data); +unsigned long panel_dbi_read_data(struct panel_dbi_device *dev); + +#endif /* __PANEL_DBI__ */
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/video/panel/Kconfig | 4 + drivers/video/panel/Makefile | 1 + drivers/video/panel/panel-dbi.c | 217 +++++++++++++++++++++++++++++++++++++++ include/video/panel-dbi.h | 92 +++++++++++++++++ 4 files changed, 314 insertions(+), 0 deletions(-) create mode 100644 drivers/video/panel/panel-dbi.c create mode 100644 include/video/panel-dbi.h