From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH/RFC v3 08/19] video: display: Add MIPI DBI bus support Date: Fri, 06 Sep 2013 16:37:48 +0200 Message-ID: <2207987.VxcRbivMXz@avalon> References: <1376068510-30363-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com> <1376068510-30363-9-git-send-email-laurent.pinchart+renesas@ideasonboard.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [95.142.166.194]) by gabe.freedesktop.org (Postfix) with ESMTP id 02D2FE6402 for ; Fri, 6 Sep 2013 07:37:45 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Vikas Sajjan Cc: linux-fbdev@vger.kernel.org, Sebastien Guiriec , DRI mailing list , Jesse Barnes , Benjamin Gaignard , Laurent Pinchart , Tom Gall , Kyungmin Park , Tomi Valkeinen , linux-media@vger.kernel.org, Stephen Warren , Mark Zhang , Alexandre Courbot , Ragesh Radhakrishnan , Thomas Petazzoni , Sunil Joshi , Maxime Ripard , Marcus Lorentzon List-Id: dri-devel@lists.freedesktop.org Hi Vikas, On Wednesday 04 September 2013 16:20:59 Vikas Sajjan wrote: > On 9 August 2013 22:44, Laurent Pinchart wrote: > > MIPI DBI is a configurable-width parallel display bus that transmits > > commands and data. > > > > Add a new DBI Linux bus type that implements the usual bus > > infrastructure (including devices and drivers (un)registration and > > matching, and bus configuration and access functions). > > > > Signed-off-by: Laurent Pinchart > > --- > > > > drivers/video/display/Kconfig | 8 ++ > > drivers/video/display/Makefile | 1 + > > drivers/video/display/mipi-dbi-bus.c | 234 ++++++++++++++++++++++++++++++ > > include/video/display.h | 4 + > > include/video/mipi-dbi-bus.h | 125 +++++++++++++++++++ > > 5 files changed, 372 insertions(+) > > create mode 100644 drivers/video/display/mipi-dbi-bus.c > > create mode 100644 include/video/mipi-dbi-bus.h [snip] > > diff --git a/drivers/video/display/mipi-dbi-bus.c > > b/drivers/video/display/mipi-dbi-bus.c new file mode 100644 > > index 0000000..791fb4d > > --- /dev/null > > +++ b/drivers/video/display/mipi-dbi-bus.c [snip] > > +/* ---------------------------------------------------------------------- > > + * Device and driver (un)registration > > + */ > > + > > +/** > > + * mipi_dbi_device_register - register a DBI device > > + * @dev: DBI device we're registering > > + */ > > +int mipi_dbi_device_register(struct mipi_dbi_device *dev, > > + struct mipi_dbi_bus *bus) > > +{ > > + device_initialize(&dev->dev); > > + > > + dev->bus = bus; > > + dev->dev.bus = &mipi_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); > > +} > > The function looks very much specific to NON-DT case where you will be > calling mipi_dbi_device_register() in the machine file. You're absolutely right. > I was actually trying to migrate to CDFv3 and adding MIPI DSI support > for exynos5250, > but in my case where exynos5250 is fully DT based, in which case we > need something like ./drivers/of/platform.c for MIPI DBI and MIPI DSI > to add the MIPI DBI/DSI device via DT way, ./drivers/of/mipi_dbi.c and > ./drivers/of/mipi_dsi.c > > may look like below, > > int of_mipi_dbi_device_register(struct device_node *np, > const char *bus_id, > struct device *parent) > { > struct mipi_dbi_device *dev; > dev = of_device_alloc(np, bus_id, parent); > > if (!dev) > return NULL; > device_initialize(dev); > > dev->bus = &mipi_dbi_bus_type; > dev->parent = parent; > > return of_device_add(dev); > } > > Correct me if I am wrong. You're correct, but the implementation will need to be a little bit more complex than that. From an API point of view, something like of_i2c_register_devices() (drivers/of/of_i2c.c) would probably make sense. the function should iterate over child nodes, and call of_mipi_dbi_device_register() (we could maybe rename that to of_mipi_dbi_device_create() to mimic the platform device code) for each child. In your above code, you should replace of_device_alloc() with of_mipi_dbi_device_alloc(), as of_device_alloc() allocates a struct platform_device. You should also call mipi_dsi_device_put() on the device if of_device_add() returns a failure. Would you like to send a patch on top of 08/19 to implement this ? > > +EXPORT_SYMBOL_GPL(mipi_dbi_device_register); > > + > > +/** > > + * mipi_dbi_device_unregister - unregister a DBI device > > + * @dev: DBI device we're unregistering > > + */ > > +void mipi_dbi_device_unregister(struct mipi_dbi_device *dev) > > +{ > > + device_del(&dev->dev); > > + put_device(&dev->dev); > > +} > > +EXPORT_SYMBOL_GPL(mipi_dbi_device_unregister); > > + > > +static int mipi_dbi_drv_probe(struct device *_dev) > > +{ > > + struct mipi_dbi_driver *drv = to_mipi_dbi_driver(_dev->driver); > > + struct mipi_dbi_device *dev = to_mipi_dbi_device(_dev); > > Here we are assuming that mipi_dbi_device can be obtained by using > _dev pointer, which may NOT be true in DT case, i think. Why wouldn't it be true (if we create the devices as explained above) ? > let me know if i am missing something. > > if you can give me a example for DT case, that would be helpful. I'm afraid I don't have any, as the DBI drivers I wrote are used by a platform that doesn't support DT. > > + > > + return drv->probe(dev); > > +} -- Regards, Laurent Pinchart