Re: [PATCH/RFC v3 08/19] video: display: Add MIPI DBI bus support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  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