[PATCH 0/2] ti-sn65dsi86 DSI configuration support

[PATCH 0/2] ti-sn65dsi86 DSI configuration support

[Jeffrey Hugo]

The ti-sn65dsi86 bridge on the Lenovo Miix 630 does not appear to be
wired to an I2C bus - there is no indication from the FW which bus to
use and scanning all of the busses does not result in an unknown device
being discovered.

As a result, we must utilize the ability of the ti-sn65dsi86 to be
configured "inband" via the DSI connection.  Since the driver already
utilizes regmap (over I2C), adding the ability of the driver to
configure via DSI is rather minimal effort once we have established a
regmap over DSI handle.

However, regmap currently doesn't support the MIPI DSI bus, so add that
option first.

Jeffrey Hugo (2):
  regmap: Add DSI bus support
  drm/bridge: ti-sn65dsi86: Add support to be a DSI device

 drivers/base/regmap/Kconfig           |   6 +-
 drivers/base/regmap/Makefile          |   1 +
 drivers/base/regmap/regmap-dsi.c      |  62 ++++++++++
 drivers/gpu/drm/bridge/Kconfig        |   1 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 160 +++++++++++++++++++-------
 include/linux/regmap.h                |  37 ++++++
 6 files changed, 222 insertions(+), 45 deletions(-)
 create mode 100644 drivers/base/regmap/regmap-dsi.c

-- 
2.17.1

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

[Jeffrey Hugo]

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

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);
-- 
2.17.1

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jeffrey Hugo]

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?

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2487 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Andrzej Hajda]

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.

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

[Rob Clark]

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

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

[Andrzej Hajda]

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

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

[Jeffrey Hugo]

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.

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

[Rob Clark]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Andrzej Hajda]

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 2/2] drm/bridge: ti-sn65dsi86: Add support to be a DSI device

[Jeffrey Hugo]

The ti-sn65dsi86 can be configured in two ways - via i2c or "inband" via
DSI.  The DSI option can be useful on platforms where the i2c link does
not seem to be wired up.

To support configuration via DSI, register as a DSI device, use the
provided DSI device instead of creating our own, and utilize the regmap-dsi
support to abstract away the init differences between i2c and DSI.

Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
---
 drivers/gpu/drm/bridge/Kconfig        |   1 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 160 +++++++++++++++++++-------
 2 files changed, 117 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index ee777469293a..b74c8ef47cb6 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -143,6 +143,7 @@ config DRM_TI_SN65DSI86
 	depends on OF
 	select DRM_KMS_HELPER
 	select REGMAP_I2C
+	select REGMAP_DSI
 	select DRM_PANEL
 	select DRM_MIPI_DSI
 	help
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index b77a52d05061..45fb91afd01b 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -258,18 +258,22 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge)
 	 * will satisfy most of the existing host drivers. Once the host driver
 	 * is fixed we can move the below code to bridge probe safely.
 	 */
-	host = of_find_mipi_dsi_host_by_node(pdata->host_node);
-	if (!host) {
-		DRM_ERROR("failed to find dsi host\n");
-		ret = -ENODEV;
-		goto err_dsi_host;
-	}
-
-	dsi = mipi_dsi_device_register_full(host, &info);
-	if (IS_ERR(dsi)) {
-		DRM_ERROR("failed to create dsi device\n");
-		ret = PTR_ERR(dsi);
-		goto err_dsi_host;
+	if (!pdata->dsi) {
+		host = of_find_mipi_dsi_host_by_node(pdata->host_node);
+		if (!host) {
+			DRM_ERROR("failed to find dsi host\n");
+			ret = -ENODEV;
+			goto err_dsi_host;
+		}
+
+		dsi = mipi_dsi_device_register_full(host, &info);
+		if (IS_ERR(dsi)) {
+			DRM_ERROR("failed to create dsi device\n");
+			ret = PTR_ERR(dsi);
+			goto err_dsi_host;
+		}
+	} else {
+		dsi = pdata->dsi;
 	}
 
 	/* TODO: setting to 4 lanes always for now */
@@ -290,7 +294,9 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge)
 		DRM_ERROR("failed to attach dsi to host\n");
 		goto err_dsi_attach;
 	}
-	pdata->dsi = dsi;
+
+	if (!pdata->dsi)
+		pdata->dsi = dsi;
 
 	/* attach panel to bridge */
 	drm_panel_attach(pdata->panel, &pdata->connector);
@@ -298,7 +304,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge)
 	return 0;
 
 err_dsi_attach:
-	mipi_dsi_device_unregister(dsi);
+	if (!pdata->dsi)
+		mipi_dsi_device_unregister(dsi);
 err_dsi_host:
 	drm_connector_cleanup(&pdata->connector);
 	return ret;
@@ -656,30 +663,23 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
 	return 0;
 }
 
-static int ti_sn_bridge_probe(struct i2c_client *client,
-			      const struct i2c_device_id *id)
+static int ti_sn_bridge_common_probe(struct device *dev, struct regmap *regmap,
+				     struct mipi_dsi_device *dsi)
 {
 	struct ti_sn_bridge *pdata;
 	int ret;
 
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
-		DRM_ERROR("device doesn't support I2C\n");
-		return -ENODEV;
-	}
-
-	pdata = devm_kzalloc(&client->dev, sizeof(struct ti_sn_bridge),
+	pdata = devm_kzalloc(dev, sizeof(struct ti_sn_bridge),
 			     GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
 
-	pdata->regmap = devm_regmap_init_i2c(client,
-					     &ti_sn_bridge_regmap_config);
-	if (IS_ERR(pdata->regmap)) {
-		DRM_ERROR("regmap i2c init failed\n");
-		return PTR_ERR(pdata->regmap);
-	}
+	pdata->regmap = regmap;
 
-	pdata->dev = &client->dev;
+	pdata->dev = dev;
+
+	if (dsi)
+		pdata->dsi = dsi;
 
 	ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0,
 					  &pdata->panel, NULL);
@@ -688,7 +688,7 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	dev_set_drvdata(&client->dev, pdata);
+	dev_set_drvdata(dev, pdata);
 
 	pdata->enable_gpio = devm_gpiod_get(pdata->dev, "enable",
 					    GPIOD_OUT_LOW);
@@ -719,25 +719,21 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 
 	pm_runtime_enable(pdata->dev);
 
-	i2c_set_clientdata(client, pdata);
-
 	pdata->aux.name = "ti-sn65dsi86-aux";
-	pdata->aux.dev = pdata->dev;
+	pdata->aux.dev = dev;
 	pdata->aux.transfer = ti_sn_aux_transfer;
 	drm_dp_aux_register(&pdata->aux);
 
 	pdata->bridge.funcs = &ti_sn_bridge_funcs;
-	pdata->bridge.of_node = client->dev.of_node;
+	pdata->bridge.of_node = dev->of_node;
 
 	drm_bridge_add(&pdata->bridge);
 
 	return 0;
 }
 
-static int ti_sn_bridge_remove(struct i2c_client *client)
+static int ti_sn_bridge_common_remove(struct ti_sn_bridge *pdata)
 {
-	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
-
 	if (!pdata)
 		return -EINVAL;
 
@@ -755,11 +751,53 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
 	return 0;
 }
 
-static struct i2c_device_id ti_sn_bridge_id[] = {
+static int ti_sn_bridge_i2c_remove(struct i2c_client *client)
+{
+	return ti_sn_bridge_common_remove(i2c_get_clientdata(client));
+}
+
+static int ti_sn_bridge_i2c_probe(struct i2c_client *client,
+			      const struct i2c_device_id *id)
+{
+	struct regmap *regmap;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		DRM_ERROR("device doesn't support I2C\n");
+		return -ENODEV;
+	}
+
+	regmap = devm_regmap_init_i2c(client, &ti_sn_bridge_regmap_config);
+	if (IS_ERR(regmap)) {
+		DRM_ERROR("regmap i2c init failed\n");
+		return PTR_ERR(regmap);
+	}
+
+	return ti_sn_bridge_common_probe(&client->dev, regmap, NULL);
+}
+
+static int ti_sn_bridge_dsi_probe(struct mipi_dsi_device *dsi)
+{
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_dsi(dsi, &ti_sn_bridge_regmap_config);
+	if (IS_ERR(regmap)) {
+		DRM_ERROR("regmap dsi init failed\n");
+		return PTR_ERR(regmap);
+	}
+
+	return ti_sn_bridge_common_probe(&dsi->dev, regmap, dsi);
+}
+
+static int ti_sn_bridge_dsi_remove(struct mipi_dsi_device *dsi)
+{
+	return ti_sn_bridge_common_remove(mipi_dsi_get_drvdata(dsi));
+}
+
+static struct i2c_device_id ti_sn_bridge_i2c_id[] = {
 	{ "ti,sn65dsi86", 0},
 	{},
 };
-MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
+MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_i2c_id);
 
 static const struct of_device_id ti_sn_bridge_match_table[] = {
 	{.compatible = "ti,sn65dsi86"},
@@ -767,17 +805,51 @@ static const struct of_device_id ti_sn_bridge_match_table[] = {
 };
 MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
 
-static struct i2c_driver ti_sn_bridge_driver = {
+static struct i2c_driver ti_sn_bridge_i2c_driver = {
 	.driver = {
 		.name = "ti_sn65dsi86",
 		.of_match_table = ti_sn_bridge_match_table,
 		.pm = &ti_sn_bridge_pm_ops,
 	},
-	.probe = ti_sn_bridge_probe,
-	.remove = ti_sn_bridge_remove,
-	.id_table = ti_sn_bridge_id,
+	.probe = ti_sn_bridge_i2c_probe,
+	.remove = ti_sn_bridge_i2c_remove,
+	.id_table = ti_sn_bridge_i2c_id,
 };
-module_i2c_driver(ti_sn_bridge_driver);
+
+static struct mipi_dsi_driver ti_sn_bridge_dsi_driver = {
+	.driver = {
+		.name = "ti_sn65dsi86",
+		.of_match_table = ti_sn_bridge_match_table,
+		.pm = &ti_sn_bridge_pm_ops,
+	},
+	.probe = ti_sn_bridge_dsi_probe,
+	.remove = ti_sn_bridge_dsi_remove,
+};
+
+static int __init tisn65dsi86_init(void)
+{
+	int ret;
+
+	ret = i2c_add_driver(&ti_sn_bridge_i2c_driver);
+
+	if (ret)
+		return ret;
+
+	ret = mipi_dsi_driver_register(&ti_sn_bridge_dsi_driver);
+
+	if (ret)
+		i2c_del_driver(&ti_sn_bridge_i2c_driver);
+
+	return ret;
+}
+module_init(tisn65dsi86_init);
+
+static void __exit tisn65dsi86_exit(void)
+{
+	i2c_del_driver(&ti_sn_bridge_i2c_driver);
+	mipi_dsi_driver_unregister(&ti_sn_bridge_dsi_driver);
+}
+module_exit(tisn65dsi86_exit);
 
 MODULE_AUTHOR("Sandeep Panda <spanda@codeaurora.org>");
 MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
-- 
2.17.1