[1/2] regmap: Add DSI bus support
diff mbox series

Message ID 20190703214512.41319-1-jeffrey.l.hugo@gmail.com
State New
Headers show
Series
  • ti-sn65dsi86 DSI configuration support
Related show

Commit Message

Jeffrey Hugo July 3, 2019, 9:45 p.m. UTC
Add basic support with a simple implementation that utilizes the generic
read/write commands to allow device registers to be configured.

Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
---
 drivers/base/regmap/Kconfig      |  6 +++-
 drivers/base/regmap/Makefile     |  1 +
 drivers/base/regmap/regmap-dsi.c | 62 ++++++++++++++++++++++++++++++++
 include/linux/regmap.h           | 37 +++++++++++++++++++
 4 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-dsi.c

Comments

Mark Brown July 6, 2019, 1:06 a.m. UTC | #1
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?

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?

> +	ret = mipi_dsi_generic_write(dsi, payload, 2);
> +	return ret < 0 ? ret : 0;

Please just write an if statement, it helps with legibility.

> +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.
Jeffrey Hugo July 10, 2019, 6:08 p.m. UTC | #2
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?
Andrzej Hajda July 11, 2019, 1:11 p.m. UTC | #3
On 06.07.2019 03:06, Mark Brown 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?


I am little bit confused about regmap usage here. On the one hand it
nicely fits to this specific driver, probably because it already uses
regmap_i2c.

On the other it will be unusable for almost all current DSI drivers and
probably for most new drivers. Why?

1. DSI protocol defines actually more than 30 types of transactions[1],
but this patchset implements only few of them (dsi generic write/read
family). Is it possible to implement multiple types of transactions in
regmap?

2. There is already some set of helpers which uses dsi bus, rewriting it
on regmap is possible or driver could use of regmap and direct access
together, the question is if it is really necessary.

3. DSI devices are no MFDs so regmap abstraction has no big value added
(correct me, if there are other significant benefits).


[1]:
https://elixir.bootlin.com/linux/latest/source/include/video/mipi_display.h#L15


Regards

Andrzej


>
> 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?
>
>> +	ret = mipi_dsi_generic_write(dsi, payload, 2);
>> +	return ret < 0 ? ret : 0;
> Please just write an if statement, it helps with legibility.
>
>> +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.
Rob Clark July 11, 2019, 1:56 p.m. UTC | #4
On Thu, Jul 11, 2019 at 6:11 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 06.07.2019 03:06, Mark Brown 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?
>
>
> I am little bit confused about regmap usage here. On the one hand it
> nicely fits to this specific driver, probably because it already uses
> regmap_i2c.
>
> On the other it will be unusable for almost all current DSI drivers and
> probably for most new drivers. Why?
>
> 1. DSI protocol defines actually more than 30 types of transactions[1],
> but this patchset implements only few of them (dsi generic write/read
> family). Is it possible to implement multiple types of transactions in
> regmap?
>
> 2. There is already some set of helpers which uses dsi bus, rewriting it
> on regmap is possible or driver could use of regmap and direct access
> together, the question is if it is really necessary.
>
> 3. DSI devices are no MFDs so regmap abstraction has no big value added
> (correct me, if there are other significant benefits).
>

I assume it is not *just* this one bridge that can be programmed over
either i2c or dsi, depending on how things are wired up on the board.
It certainly would be nice for regmap to support this case, so we
don't have to write two different bridge drivers for the same bridge.
I wouldn't expect a panel that is only programmed via dsi to use this.

BR,
-R

>
> [1]:
> https://elixir.bootlin.com/linux/latest/source/include/video/mipi_display.h#L15
>
>
> Regards
>
> Andrzej
>
>
> >
> > 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?
> >
> >> +    ret = mipi_dsi_generic_write(dsi, payload, 2);
> >> +    return ret < 0 ? ret : 0;
> > Please just write an if statement, it helps with legibility.
> >
> >> +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.
>
>
Mark Brown July 11, 2019, 2:41 p.m. UTC | #5
On Wed, Jul 10, 2019 at 12:08:34PM -0600, Jeffrey Hugo wrote:
> On Fri, Jul 5, 2019 at 7:06 PM Mark Brown <broonie@kernel.org> wrote:

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

Oh, good, variable register sizes, what a market leading idea :(
That basically doesn't work with regmap, you need to either
define one regmap per register size and attach them to the device
or use reg_read() and reg_write() and hide the complexity in
there.

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

The code definitely ought to at least be more flexible then.
Right now it's very hard coded.

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

It's relatively easy to add a bunch of special cases in - look at
how the I2C code handles it, keying off a combination of the
register configuration and the capabilities of the host
controller.  I guess for this it'd mainly be the register
configuration.  You might find the reg_read()/reg_write()
interface better than the raw buffer one for some of the formats,
it does let 

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

I have no strong opinions here, it looks fine from a framework
point of view though it's unclear to me if viewing it as a
register map meshes well with how the hardware is designed or not
- it seems plausible though.
Mark Brown July 11, 2019, 2:50 p.m. UTC | #6
On Thu, Jul 11, 2019 at 03:11:56PM +0200, Andrzej Hajda wrote:

> 1. DSI protocol defines actually more than 30 types of transactions[1],
> but this patchset implements only few of them (dsi generic write/read
> family). Is it possible to implement multiple types of transactions in
> regmap?

You can, there's a couple of different ways depending on how
exactly things are done.

> 3. DSI devices are no MFDs so regmap abstraction has no big value added
> (correct me, if there are other significant benefits).

There's a few extra bits even if you're not using the marshalling
code to get things onto the bus - the main ones are the register
cache support (which people often use for simpler suspend/resume
support) and the debug and trace facilities (things like
tracepoints and debugfs for dumping the register map).  There's
no real connection to MFDs, I'd say the majority of users are not
MFDs.
Andrzej Hajda July 12, 2019, 1:01 p.m. UTC | #7
On 11.07.2019 15:56, Rob Clark wrote:
> On Thu, Jul 11, 2019 at 6:11 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 06.07.2019 03:06, Mark Brown 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?
>>
>> I am little bit confused about regmap usage here. On the one hand it
>> nicely fits to this specific driver, probably because it already uses
>> regmap_i2c.
>>
>> On the other it will be unusable for almost all current DSI drivers and
>> probably for most new drivers. Why?
>>
>> 1. DSI protocol defines actually more than 30 types of transactions[1],
>> but this patchset implements only few of them (dsi generic write/read
>> family). Is it possible to implement multiple types of transactions in
>> regmap?
>>
>> 2. There is already some set of helpers which uses dsi bus, rewriting it
>> on regmap is possible or driver could use of regmap and direct access
>> together, the question is if it is really necessary.
>>
>> 3. DSI devices are no MFDs so regmap abstraction has no big value added
>> (correct me, if there are other significant benefits).
>>
> I assume it is not *just* this one bridge that can be programmed over
> either i2c or dsi, depending on how things are wired up on the board.
> It certainly would be nice for regmap to support this case, so we
> don't have to write two different bridge drivers for the same bridge.
> I wouldn't expect a panel that is only programmed via dsi to use this.


On the other side supporting DSI and I2C in one driver is simply matter
of writing proper accesors.


Regards

Andrzej


>
> BR,
> -R
>
>> [1]:
>> https://elixir.bootlin.com/linux/latest/source/include/video/mipi_display.h#L15
>>
>>
>> Regards
>>
>> Andrzej
>>
>>
>>> 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?
>>>
>>>> +    ret = mipi_dsi_generic_write(dsi, payload, 2);
>>>> +    return ret < 0 ? ret : 0;
>>> Please just write an if statement, it helps with legibility.
>>>
>>>> +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.
>>
Jeffrey Hugo July 12, 2019, 2:22 p.m. UTC | #8
On Fri, Jul 12, 2019 at 7:01 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 11.07.2019 15:56, Rob Clark wrote:
> > On Thu, Jul 11, 2019 at 6:11 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >> On 06.07.2019 03:06, Mark Brown 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?
> >>
> >> I am little bit confused about regmap usage here. On the one hand it
> >> nicely fits to this specific driver, probably because it already uses
> >> regmap_i2c.
> >>
> >> On the other it will be unusable for almost all current DSI drivers and
> >> probably for most new drivers. Why?
> >>
> >> 1. DSI protocol defines actually more than 30 types of transactions[1],
> >> but this patchset implements only few of them (dsi generic write/read
> >> family). Is it possible to implement multiple types of transactions in
> >> regmap?
> >>
> >> 2. There is already some set of helpers which uses dsi bus, rewriting it
> >> on regmap is possible or driver could use of regmap and direct access
> >> together, the question is if it is really necessary.
> >>
> >> 3. DSI devices are no MFDs so regmap abstraction has no big value added
> >> (correct me, if there are other significant benefits).
> >>
> > I assume it is not *just* this one bridge that can be programmed over
> > either i2c or dsi, depending on how things are wired up on the board.
> > It certainly would be nice for regmap to support this case, so we
> > don't have to write two different bridge drivers for the same bridge.
> > I wouldn't expect a panel that is only programmed via dsi to use this.
>
>
> On the other side supporting DSI and I2C in one driver is simply matter
> of writing proper accesors.

To me, this reads like your counter argument to not using regmap, is
to reinvent regmap.  Maybe I don't understand what you are proposing
here, but it sounds like remove the regmap support, define sn65
specific accessors that just before sending the write to the bus does
a check if the access needs to go over i2c or DSI.  Feels like a
clunky version of regmap to me.  Why not use the existing "generic"
framework?

To your point that DSI defines over 30 message types, yes it does, but
that seems to be outside of the scope.  How many of those are actually
for doing register access?  I'm thinking just 4 (technically a hair
more than that because of the multiple version of the same message) -
generic read, generic write, dcs read, dcs write.  I don't view regmap
as a generic abstraction layer over a particular mechanism, and thus
needs to support everything that mechanism does.  Sending sync
commands, or pixel data over DSI is outside the scope of regmap to me.
Rob Clark July 13, 2019, 12:49 a.m. UTC | #9
On Fri, Jul 12, 2019 at 7:22 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:
>
> On Fri, Jul 12, 2019 at 7:01 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> >
> > On 11.07.2019 15:56, Rob Clark wrote:
> > > On Thu, Jul 11, 2019 at 6:11 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
> > >> On 06.07.2019 03:06, Mark Brown 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?
> > >>
> > >> I am little bit confused about regmap usage here. On the one hand it
> > >> nicely fits to this specific driver, probably because it already uses
> > >> regmap_i2c.
> > >>
> > >> On the other it will be unusable for almost all current DSI drivers and
> > >> probably for most new drivers. Why?
> > >>
> > >> 1. DSI protocol defines actually more than 30 types of transactions[1],
> > >> but this patchset implements only few of them (dsi generic write/read
> > >> family). Is it possible to implement multiple types of transactions in
> > >> regmap?
> > >>
> > >> 2. There is already some set of helpers which uses dsi bus, rewriting it
> > >> on regmap is possible or driver could use of regmap and direct access
> > >> together, the question is if it is really necessary.
> > >>
> > >> 3. DSI devices are no MFDs so regmap abstraction has no big value added
> > >> (correct me, if there are other significant benefits).
> > >>
> > > I assume it is not *just* this one bridge that can be programmed over
> > > either i2c or dsi, depending on how things are wired up on the board.
> > > It certainly would be nice for regmap to support this case, so we
> > > don't have to write two different bridge drivers for the same bridge.
> > > I wouldn't expect a panel that is only programmed via dsi to use this.
> >
> >
> > On the other side supporting DSI and I2C in one driver is simply matter
> > of writing proper accesors.
>
> To me, this reads like your counter argument to not using regmap, is
> to reinvent regmap.  Maybe I don't understand what you are proposing
> here, but it sounds like remove the regmap support, define sn65
> specific accessors that just before sending the write to the bus does
> a check if the access needs to go over i2c or DSI.  Feels like a
> clunky version of regmap to me.  Why not use the existing "generic"
> framework?
>
> To your point that DSI defines over 30 message types, yes it does, but
> that seems to be outside of the scope.  How many of those are actually
> for doing register access?  I'm thinking just 4 (technically a hair
> more than that because of the multiple version of the same message) -
> generic read, generic write, dcs read, dcs write.  I don't view regmap
> as a generic abstraction layer over a particular mechanism, and thus
> needs to support everything that mechanism does.  Sending sync
> commands, or pixel data over DSI is outside the scope of regmap to me.

I'm w/ jhugo on this one.. if you are working w/ a device that can be
programmed via i2c or dsi, regmap and limiting yourself to the small
subset of dsi cmds which map to i2c reads/writes, makes a ton of
sense.  (And I'm almost certain this bridge isn't the only such
device.)

That isn't to say you should use regmap for devices that are only
programmed over dsi.  That would be silly.  The original argument
about this not being usable by most DSI devices is really a
non-sequitur.

BR,
-R
Andrzej Hajda July 15, 2019, 8:38 a.m. UTC | #10
On 11.07.2019 16:50, Mark Brown wrote:
> On Thu, Jul 11, 2019 at 03:11:56PM +0200, Andrzej Hajda wrote:
>
>> 1. DSI protocol defines actually more than 30 types of transactions[1],
>> but this patchset implements only few of them (dsi generic write/read
>> family). Is it possible to implement multiple types of transactions in
>> regmap?
> You can, there's a couple of different ways depending on how
> exactly things are done.
>
>> 3. DSI devices are no MFDs so regmap abstraction has no big value added
>> (correct me, if there are other significant benefits).
> There's a few extra bits even if you're not using the marshalling
> code to get things onto the bus - the main ones are the register
> cache support (which people often use for simpler suspend/resume
> support) and the debug and trace facilities (things like
> tracepoints and debugfs for dumping the register map).

I do not see cache usable in bridge drivers, I guess default config will
be caching disabled, as it is already in the driver from this patchset.

So beside marshaling, we are left only with debug facilities, not a big
gain :)

Moreover as it was already written DSI is mainly used to transport
COMMANDS to the device, with variable number of arguments - it does not
resembles registry map at all.

On the other side there is some subset of DSI devices which exposes
register memory using MIPI DSI Generic Write/Read packets, for example:
ti-sn65dsi86, tc358764. They fit better to regmap framework. Hard to say
how common is this pattern. Maybe we can try with it? If yes it would be
good to put clear remark that regmap/dsi is for such devices, to avoid
possible confusion.


Regards

Andrzej

Patch
diff mbox series

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index c8bbf5322720..27669afa9d95 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -4,7 +4,7 @@ 
 # subsystems should select the appropriate symbols.
 
 config REGMAP
-	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_I3C)
+	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_I3C || REGMAP_DSI)
 	select IRQ_DOMAIN if REGMAP_IRQ
 	bool
 
@@ -53,3 +53,7 @@  config REGMAP_SCCB
 config REGMAP_I3C
 	tristate
 	depends on I3C
+
+config REGMAP_DSI
+	tristate
+	depends on DRM_MIPI_DSI
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index ff6c7d8ec1cd..c1cc81f3986f 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -17,3 +17,4 @@  obj-$(CONFIG_REGMAP_W1) += regmap-w1.o
 obj-$(CONFIG_REGMAP_SOUNDWIRE) += regmap-sdw.o
 obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o
 obj-$(CONFIG_REGMAP_I3C) += regmap-i3c.o
+obj-$(CONFIG_REGMAP_DSI) += regmap-dsi.o
diff --git a/drivers/base/regmap/regmap-dsi.c b/drivers/base/regmap/regmap-dsi.c
new file mode 100644
index 000000000000..0c2900e2fee0
--- /dev/null
+++ b/drivers/base/regmap/regmap-dsi.c
@@ -0,0 +1,62 @@ 
+// SPDX-License-Identifier: GPL-2.0
+//
+// Register map access API - DSI support
+//
+// Copyright (c) 2019 Jeffrey Hugo
+
+#include <drm/drm_mipi_dsi.h>
+#include <linux/regmap.h>
+#include <linux/module.h>
+
+#include "internal.h"
+
+static int dsi_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct device *dev = context;
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
+	char payload[2];
+	int ret;
+
+	payload[0] = (char)reg;
+	payload[1] = (char)val;
+
+	ret = mipi_dsi_generic_write(dsi, payload, 2);
+	return ret < 0 ? ret : 0;
+}
+
+static int dsi_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct device *dev = context;
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
+	int ret;
+
+	ret = mipi_dsi_generic_read(dsi, &reg, 1, val, 1);
+	return ret < 0 ? ret : 0;
+}
+
+static struct regmap_bus dsi_bus = {
+	.reg_write = dsi_reg_write,
+	.reg_read = dsi_reg_read,
+};
+
+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);
+
+struct regmap *__devm_regmap_init_dsi(struct mipi_dsi_device *dsi,
+				      const struct regmap_config *config,
+				      struct lock_class_key *lock_key,
+				      const char *lock_name)
+{
+	return __devm_regmap_init(&dsi->dev, &dsi_bus, &dsi->dev, config,
+				  lock_key, lock_name);
+}
+EXPORT_SYMBOL_GPL(__devm_regmap_init_dsi);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index dfe493ac692d..858239f7859f 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -32,6 +32,7 @@  struct regmap_range_cfg;
 struct regmap_field;
 struct snd_ac97;
 struct sdw_slave;
+struct mipi_dsi_device;
 
 /* An enum of all the supported cache types */
 enum regcache_type {
@@ -573,6 +574,10 @@  struct regmap *__regmap_init_sdw(struct sdw_slave *sdw,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
+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);
 
 struct regmap *__devm_regmap_init(struct device *dev,
 				  const struct regmap_bus *bus,
@@ -626,6 +631,10 @@  struct regmap *__devm_regmap_init_i3c(struct i3c_device *i3c,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
+struct regmap *__devm_regmap_init_dsi(struct mipi_dsi_device *dsi,
+				 const struct regmap_config *config,
+				 struct lock_class_key *lock_key,
+				 const char *lock_name);
 /*
  * Wrapper for regmap_init macros to include a unique lockdep key and name
  * for each call. No-op if CONFIG_LOCKDEP is not set.
@@ -812,6 +821,19 @@  bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 	__regmap_lockdep_wrapper(__regmap_init_sdw, #config,		\
 				sdw, config)
 
+/**
+ * regmap_init_dsi() - Initialise register map
+ *
+ * @dsi: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+#define regmap_init_dsi(dsi, config)					\
+	__regmap_lockdep_wrapper(__regmap_init_dsi, #config,		\
+				dsi, config)
+
 
 /**
  * devm_regmap_init() - Initialise managed register map
@@ -999,6 +1021,21 @@  bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 	__regmap_lockdep_wrapper(__devm_regmap_init_i3c, #config,	\
 				i3c, config)
 
+/**
+ * devm_regmap_init_dsi() - Initialise managed register map
+ *
+ * @dsi: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap. The regmap will be automatically freed by the
+ * device management code.
+ */
+#define devm_regmap_init_dsi(dsi, config)				\
+	__regmap_lockdep_wrapper(__devm_regmap_init_dsi, #config,       \
+				dsi, config)
+
+
 int regmap_mmio_attach_clk(struct regmap *map, struct clk *clk);
 void regmap_mmio_detach_clk(struct regmap *map);
 void regmap_exit(struct regmap *map);