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

From: Vikas Sajjan <vikas.sajjan@linaro.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-fbdev@vger.kernel.org, Sebastien Guiriec <s-guiriec@ti.com>,
	DRI mailing list <dri-devel@lists.freedesktop.org>,
	Jesse Barnes <jesse.barnes@intel.com>,
	Benjamin Gaignard <benjamin.gaignard@linaro.org>,
	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
	Tom Gall <tom.gall@linaro.org>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	linux-media@vger.kernel.org,
	Stephen Warren <swarren@wwwdotorg.org>,
	Mark Zhang <markz@nvidia.com>,
	Alexandre Courbot <acourbot@nvidia.com>,
	Ragesh Radhakrishnan <Ragesh.R@linaro.org>,
	Thomas Petazzoni <thomas.petazzoni@free-electrons.com>,
	Sunil Joshi <joshi@samsung.com>,
	Maxime Ripard <maxime.ripard@free-electrons.com>,
	Marcus Lorentzon <marcus.lorentzon@huawei.com>
Subject: Re: [PATCH/RFC v3 08/19] video: display: Add MIPI DBI bus support
Date: Wed, 18 Sep 2013 16:29:47 +0530	[thread overview]
Message-ID: <CAD025yQ4qaxW7sm-aAg4JqoYhtThXKN5bqwdq2gq4XVzJa3-iQ@mail.gmail.com> (raw)
In-Reply-To: <2207987.VxcRbivMXz@avalon>

Hi Laurent,

On 6 September 2013 20:07, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> 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 ?
>

Sure, will send the patch to add MIPI DBI/DSI device via DT way.

>> > +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) ?

Yeah, with the above method, it should be possible.

>
>> 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
>

-- 
Thanks and Regards
 Vikas Sajjan