[PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver

[Doug Anderson]

Hi,

On Thu, Feb 4, 2021 at 10:41 AM Marek Vasut <marex at denx.de> wrote:
>
> >> +static const struct regmap_range sn65dsi83_volatile_ranges[] = {
> >> +       regmap_reg_range(REG_RC_LVDS_PLL, REG_RC_LVDS_PLL),
> >
> > Why is REG_RC_LVDS_PLL volatile?
>
> See register 0xa bit 7, PLL_EN_STAT .

Wow, I looked at it a few times and still didn't see it.  OK, fair enough.


> >> +       regmap_reg_range(REG_IRQ_STAT, REG_IRQ_STAT),
> >
> > Do you need to list REG_RC_RESET as volatile?  Specifically you need
> > to make sure it's not cached...
>
> Isn't volatile table exactly for this purpose -- to make sure the reg is
> not cached ?

Sorry, I was unclear I guess.  I'm suggesting that you add
REG_RC_RESET to the list of volatile ones since I don't see it there.


> >> +static const struct regmap_config sn65dsi83_regmap_config = {
> >> +       .reg_bits = 8,
> >> +       .val_bits = 8,
> >> +       .rd_table = &sn65dsi83_readable_table,
> >> +       .wr_table = &sn65dsi83_writeable_table,
> >> +       .volatile_table = &sn65dsi83_volatile_table,
> >> +       .cache_type = REGCACHE_RBTREE,
> >> +       .max_register = REG_IRQ_STAT,
> >> +};
> >
> > I'm curious how much the "readable" and "writable" sections get you.
> > In theory only the "volatile" should really matter, right?
>
> They are useful when dumping the regs from debugfs regmap registers .

OK, fair enough.  When I thought about doing this on sn65dsi86, it
came to be that a better way might be something like:

#define ACC_RO BIT(0)
#define ACC_RW BIT(1)
#define ACC_W1C BIT(2)
#define ACC_WO BIT(3)

u8 reg_acceess[] = {
  [0x00] = ACC_RO,
  [0x01] = ACC_RO,
  ...
  [0x0a] = ACC_RO | ACC_RW,
  [0x0b] = ACC_RW,
  [0x0d] = ACC_RW
  ...
};

The above maps really nicely to the public datasheet and is easy to
validate.  Then you can just look up in that array in a constant time
lookup.  In other words, "readable" if either RO or RW is set.
"writable" if any of RW, W1C, or WO is set.  Everything that's not RW
is volatile (technically you could differentiate between RO things
that are hardcoded and ones that aren't, but you probably don't need
to).

Anyway, feel free to ignore...  What you have is fine too.


> >> +static void sn65dsi83_pre_enable(struct drm_bridge *bridge)
> >> +{
> >> +       struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> >> +
> >> +       /*
> >> +        * Reset the chip, pull EN line low for t_reset=10ms,
> >> +        * then high for t_en=1ms.
> >> +        */
> >> +       gpiod_set_value(ctx->enable_gpio, 0);
> >
> > Why not use the "cansleep" version to give some flexibility?
>
> Does that make a difference in non-interrupt context ?

As I understand it:

* If a client calls gpiod_set_value() then the underlying GPIO can
only be one that doesn't sleep.

* If a client calls gpiod_set_value_cansleep() then the underlying
GPIO can be either one that does or doesn't sleep.

* A client is only allowed to call gpiod_set_value_cansleep() if it's
not in interrupt context.

You are definitely not in an interrupt context (right?), so calling
the "cansleep" version has no downsides but allows board designers to
hook up an enable that can sleep.


> >> +       usleep_range(10000, 11000);
> >
> > It seems like it would be worth it to read the enable_gpio first?  If
> > it was already 0 maybe you can skip the 10 ms delay?  I would imagine
> > that most of the time the bridge would already be disabled to start?
>
> How do you guarantee the GPIO was LO for 10 mS here? You can sample that
> it is LO, but you won't know how long it was LO before this code was
> executed.

Ah, true.  I guess the best we could do would be keep track of the
GPIO ourselves so that if we were the one to last turn it off we could
avoid the delay.


> >> +       regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> >
> > Probably you don't need this?  It's the default and in pre-enable you
> > just reset the chip.  Maybe it was needed since you don't flush the
> > cache in pre-enable?
>
> Have a look at the Example Script in the DSI83 datasheet, this PLL part
> is needed.

I think that script is written without the assumption that you have
just reset the chip using the enable GPIO.  If you have just reset
with the enable GPIO it shouldn't be needed.