On 11.07.2019 15:56, Rob Clark wrote: > On Thu, Jul 11, 2019 at 6:11 AM Andrzej Hajda <a.hajda@xxxxxxxxxxx> 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. >>
