Re: [PATCH 1/2] regmap: Add DSI bus support

[Jeffrey Hugo <jeffrey.l.hugo@gmail.com>]

On Fri, Jul 5, 2019 at 7:06 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jul 03, 2019 at 02:45:12PM -0700, Jeffrey Hugo wrote:
> > Add basic support with a simple implementation that utilizes the generic
> > read/write commands to allow device registers to be configured.
>
> This looks good to me but I really don't know anything about DSI,
> I'd appreciate some review from other people who do.  I take it
> there's some spec thing in DSI that says registers and bytes must
> both be 8 bit?

DSI appears to reside under DRM, and the DRM maintainers are copied on
this thread, so hopefully they will chime in.

Context on DSI:
The MIPI (Mobile Industry Processor Interface) Alliance DSI (Display
Serial Interface) spec defines an interface between host processors
and displays for embedded applications (smartphones and the like).
The spec itself is private to MIPI members, although I suspect if you
run some queries on your preferred search engine, you may find some
accessible copies of it floating around somewhere.

The spec defines some specific messages that run over the DSI link.
Most of those are grouped into the purposes of sending pixel data over
to the display, or configuring gamma, etc.  As far as I can tell, DSI
does not require these operations be backed by registers, however the
several implementations I've seen do it that way.  The spec does
mandate that to configure something like gamma, one needs to send a
message with a specific address, and payload.

The addresses for these spec defined messages are 8-bit wide, so 256
valid "destinations".  However, the payload is variable.  Most of the
defined operations take an 8-bit payload, but there are a few that I
see with 16-bit payloads.

The DSI spec defines two mechanisms for implementation specific
configuration (what I'm attempting to do with this series).  You can
use a spec defined message to select a different set of registers
(called a different page), after which point, the addresses of the
messages target implementation specific functionality.  I've seen this
used a lot on the specific panels which can be directly connected to
DSI.  The second mechanism is to use the generic read/write messages,
which the spec says are implementation defined - essentially the spec
defines the message type but the contents of the message are not spec
defined.  This is the mechanism the TI bridge uses.

As the contents of the generic read/write messages are implementation
defined, the answer to your question seems to be no - the spec does
not define that the registers are 8-bit addressable, and 8-bit wide.

In running this series more, I actually found a bug with it.  It turns
out that the TI bridge requires 16-bit addressing (LSB ordering), with
the upper 8-bit reserved for future use, but only on reads.  Writes
are 8-bit addressing.  This is part of that implementation specific
details.

I think perhaps the discussion needs to step back a bit, and decide
how flexible do we want this regmap over DSI to be?  I think its
usefulness comes from when a device can be configured via multiple
interfaces, so I don't expect it to be useful for every DSI interface.
It seems like the DSI panels use DSI directly to craft their
configuration.  As a result, we are probably looking at just devices
which use the generic read/write commands, but sadly the format for
those is not universal per the spec.  From the implementations I've
seen, I suspect 8-bit addressing of 8-bit wide registers to be the
most common, but apparently there is an exception to that already in
the one device that I care about.

Do we want to go forward with this regmap support just for the one TI
device, and see what other usecases come out of it, and attempt to
solve those as we go?

>
> A couple of minor comments, no need to resend just for these:
>
> > +       payload[0] = (char)reg;
> > +       payload[1] = (char)val;
>
> Do you need the casts?

Apparently not.  I was assuming the compiler would complain about
implicit truncation.

>
> > +     ret = mipi_dsi_generic_write(dsi, payload, 2);
> > +     return ret < 0 ? ret : 0;
>
> Please just write an if statement, it helps with legibility.

Uhh, sure.  There appear to be several instances of the trinary
operator in drivers/base/regmap/ but if an explicit if statement here
makes you happy, then I'll do it.

>
> > +struct regmap *__regmap_init_dsi(struct mipi_dsi_device *dsi,
> > +                              const struct regmap_config *config,
> > +                              struct lock_class_key *lock_key,
> > +                              const char *lock_name)
> > +{
> > +     return __regmap_init(&dsi->dev, &dsi_bus, &dsi->dev, config,
> > +                          lock_key, lock_name);
> > +}
> > +EXPORT_SYMBOL_GPL(__regmap_init_dsi);
>
> Perhaps validate that the config is OK (mainly the register/value
> sizes)?  Though I'm not sure it's worth it so perhaps not - up to
> you.

Probably.  Based on the above discussion, should I be making use of
reg_read/reg_write in the config?