From: Andrzej Hajda <a.hajda@samsung.com> To: Mark Brown <broonie@kernel.org>, Jeffrey Hugo <jeffrey.l.hugo@gmail.com> Cc: Laurent.pinchart@ideasonboard.com, airlied@linux.ie, daniel@ffwll.ch, robdclark@gmail.com, bjorn.andersson@linaro.org, dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] regmap: Add DSI bus support Date: Thu, 11 Jul 2019 15:11:56 +0200 [thread overview] Message-ID: <64ca3a74-374f-d4f3-bee6-a607cc5c0fc5@samsung.com> (raw) In-Reply-To: <20190706010604.GG20625@sirena.org.uk> 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.
WARNING: multiple messages have this Message-ID (diff)
From: Andrzej Hajda <a.hajda@samsung.com> To: Mark Brown <broonie@kernel.org>, Jeffrey Hugo <jeffrey.l.hugo@gmail.com> Cc: airlied@linux.ie, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, bjorn.andersson@linaro.org, Laurent.pinchart@ideasonboard.com Subject: Re: [PATCH 1/2] regmap: Add DSI bus support Date: Thu, 11 Jul 2019 15:11:56 +0200 [thread overview] Message-ID: <64ca3a74-374f-d4f3-bee6-a607cc5c0fc5@samsung.com> (raw) In-Reply-To: <20190706010604.GG20625@sirena.org.uk> 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. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-07-11 13:11 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-03 21:43 [PATCH 0/2] ti-sn65dsi86 DSI configuration support Jeffrey Hugo 2019-07-03 21:43 ` Jeffrey Hugo 2019-07-03 21:45 ` [PATCH 1/2] regmap: Add DSI bus support Jeffrey Hugo 2019-07-06 1:06 ` Mark Brown 2019-07-06 1:06 ` Mark Brown 2019-07-10 18:08 ` Jeffrey Hugo 2019-07-11 14:41 ` Mark Brown 2019-07-11 13:11 ` Andrzej Hajda [this message] 2019-07-11 13:11 ` Andrzej Hajda 2019-07-11 13:56 ` Rob Clark 2019-07-12 13:01 ` Andrzej Hajda 2019-07-12 14:22 ` Jeffrey Hugo 2019-07-13 0:49 ` Rob Clark 2019-07-13 0:49 ` Rob Clark 2019-07-11 14:50 ` Mark Brown 2019-07-11 14:50 ` Mark Brown 2019-07-15 8:38 ` Andrzej Hajda 2019-07-03 21:45 ` [PATCH 2/2] drm/bridge: ti-sn65dsi86: Add support to be a DSI device Jeffrey Hugo
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=64ca3a74-374f-d4f3-bee6-a607cc5c0fc5@samsung.com \ --to=a.hajda@samsung.com \ --cc=Laurent.pinchart@ideasonboard.com \ --cc=airlied@linux.ie \ --cc=bjorn.andersson@linaro.org \ --cc=broonie@kernel.org \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=jeffrey.l.hugo@gmail.com \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=robdclark@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.