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

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

Hi Rob,

On Tuesday 13 August 2013 20:52:15 Rob Clark wrote:
> On Fri, Aug 9, 2013 at 1:14 PM, 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/include/video/mipi-dbi-bus.h b/include/video/mipi-dbi-bus.h
> > new file mode 100644
> > index 0000000..876b69d
> > --- /dev/null
> > +++ b/include/video/mipi-dbi-bus.h

[snip]

> > +struct mipi_dbi_device {
> > +       const char *name;
> > +       int id;
> > +       struct device dev;
> 
> just fwiw, we need the "framework" to be agnostic of the association between
> devices and entities in the framework. Some things that are multiple
> entities may actually map to a single device in hw

I haven't written down that requirement, but I've kept it in mind while 
designing CDF. We have similar use cases in V4L2.

> (and possibly visa versa?).

I don't think that would be needed, but if you can think of a use case I'm all 
ears.

> Otherwise we end up having to create fake devices in some cases. Really the
> entities, or objects, or whatever you call 'em should just extend some
> kref'd base class. Sort of like how existing kms objects extend
> drm_mode_object.

struct display_entity {
        struct list_head list;
        struct device *dev;
        struct module *owner;
        struct kref ref;

        char name[32];
        struct media_entity entity;

        const struct display_entity_ops *ops;

        void(*release)(struct display_entity *ent);
};

(from patch 02/19)

> So any 'struct device' moves down into the derived class, not in the base
> class. Configuration data is passed in separate, not grabbed out of dev->
> platform_data, etc. Probably there is room for some helpers to pull stuff
> out of DT/ACPI/whatever.

struct mipi_dbi_device is not a display entity, it's a physical device plugged 
on a DBI bus. It's thus similar in purpose to struct pci_device or struct 
platform_device. The DBI device driver will then create one or more display 
entities depending on its needs.

> > +       const struct mipi_dbi_device_id *id_entry;
> > +       struct mipi_dbi_bus *bus;
> > +
> > +       unsigned int flags;
> > +       unsigned int bus_width;
> > +       unsigned int data_width;
> > +};

-- 
Regards,

Laurent Pinchart