Re: [PATCH v3 3/3] drm/bridge: parade-ps8640: Add support for AUX channel

[Stephen Boyd <swboyd@chromium.org>]

Quoting Doug Anderson (2021-09-15 14:27:40)
> Hi,
>
> On Tue, Sep 14, 2021 at 5:57 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Philip Chen (2021-09-14 16:28:45)
> > > diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > index 8d3e7a147170..dc349d729f5a 100644
> > > --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> > > +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> > > @@ -117,6 +144,129 @@ static inline struct ps8640 *bridge_to_ps8640(struct drm_bridge *e)
> > [...]
> > > +       case DP_AUX_I2C_WRITE:
> > > +       case DP_AUX_I2C_READ:
> > > +               break;
> > > +       default:
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       ret = regmap_write(map, PAGE0_AUXCH_CFG3, AUXCH_CFG3_RESET);
> > > +       if (ret) {
> > > +               dev_err(dev, "failed to write PAGE0_AUXCH_CFG3: %d\n", ret);
> >
> > Can we use DRM_DEV_ERROR()?
>
> I've never gotten clear guidance here. For instance, in some other
> review I suggested using the DRM wrapper and got told "no" [1]. ;-)
> The driver landed without the DRM_ERROR versions. I don't really care
> lots so it's fine with me to use use DRM_DEV_ERROR, I just wish I
> understood the rules...
>
> [1] https://lore.kernel.org/all/49db7ef3-fa53-a274-7c69-c2d840b13058@denx.de/

I think the rule is that the DRM specific printk stuff should be used so
that they can be stuck into the drm logs. On chromeOS we also have a
record of the drm logs that we can use to debug things, split away from
the general kernel printk logs. So using DRM prints when there's a DRM
device around is a good thing to do.

>
>
> > > +               return ret;
> > > +       }
> > > +
> > > +       /* Assume it's good */
> > > +       msg->reply = 0;
> > > +
> > > +       addr_len[0] = msg->address & 0xff;
> > > +       addr_len[1] = (msg->address >> 8) & 0xff;
> > > +       addr_len[2] = ((msg->request << 4) & SWAUX_CMD_MASK) |
> > > +               ((msg->address >> 16) & SWAUX_ADDR_19_16_MASK);
> >
> > It really feels like this out to be possible with some sort of
> > cpu_to_le32() API. We're shoving msg->address into 3 bytes and then
> > adding in the request and some length. So we could do something like:
> >
> >         u32 addr_len;
> >
> >         addr_len = FIELD_PREP(SWAUX_ADDR_MASK, msg->address);
> >         addr_len |= FIELD_PREP(SWAUX_CMD_MASK, msg->request);
> >         if (len)
> >                 addr_len |= FIELD_PREP(LEN_MASK, len - 1);
> >         else
> >                 addr_len |= FIELD_PREP(LEN_MASK, SWAUX_NO_PAYLOAD );
> >
> >         cpu_to_le32s(&addr_len);
> >
> >         regmap_bulk_write(map, PAGE0_SWAUX_ADDR_7_0, &addr_len, sizeof(addr_len));
>
> You're arguing that your version of the code is more efficient? Easier
> to understand? Something else? To me, Philip's initial version is
> crystal clear and easy to map to the bridge datasheet but I need to
> think more to confirm that your version is right. Thinking is hard and
> I like to avoid it when possible.
>
> In any case, it's definitely bikeshedding and I'll yield if everyone
> likes the other version better. ;-)

Yeah it's bikeshedding. I don't really care about this either but I find
it easier to read when the assignment isn't wrapped across multiple
lines. If the buffer approach is preferable then maybe use the address
macros to clarify which register is being set?

	unsigned int base = PAGE0_SWAUX_ADDR_7_0;

	addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address;
	addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8;
	addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = msg->address >> 16;
	addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= msg->request << 4;
	...

>
>
> > > +               return ret;
> > > +       }
> > > +
> > > +       switch (data & SWAUX_STATUS_MASK) {
> > > +       /* Ignore the DEFER cases as they are already handled in hardware */
> > > +       case SWAUX_STATUS_NACK:
> > > +       case SWAUX_STATUS_I2C_NACK:
> > > +               /*
> > > +                * The programming guide is not clear about whether a I2C NACK
> > > +                * would trigger SWAUX_STATUS_NACK or SWAUX_STATUS_I2C_NACK. So
> > > +                * we handle both cases together.
> > > +                */
> > > +               if (is_native_aux)
> > > +                       msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
> > > +               else
> > > +                       msg->reply |= DP_AUX_I2C_REPLY_NACK;
> > > +
> > > +               len = data & SWAUX_M_MASK;
> > > +               return len;
> >
> > Why no 'return data & SWAUX_M_MASK;' and skip the assignment?
>
> Actually, I think it's the "return" that's a bug, isn't it? If we're
> doing a "read" and we're returning a positive number of bytes then we
> need to actually _read_ them. Reading happens below, doesn't it?
>

Oh I missed that. We're still supposed to return data to upper
layers on a NACKed read?