linux-kernel - Re: [PATCH v2] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 21 Jun 2017 10:41:28 +0200
From:   Maxime Ripard <maxime.ripard@...e-electrons.com>
To:     Jonathan Liu <net147@...il.com>
Cc:     David Airlie <airlied@...ux.ie>, Chen-Yu Tsai <wens@...e.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-sunxi <linux-sunxi@...glegroups.com>
Subject: Re: [PATCH v2] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC
 bus

On Tue, Jun 13, 2017 at 09:53:31PM +1000, Jonathan Liu wrote:
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c
> >> @@ -0,0 +1,163 @@
> >> +/*
> >> + * Copyright (C) 2017 Jonathan Liu
> >> + *
> >> + * Jonathan Liu <net147@...il.com>
> >
> > Is it?
> 
> I could add you to the copyright since you did the old one. But the
> implementation is different.
> I intend to rework this I2C driver to use the FIFO more to avoid
> splitting larger transfers > 16 bytes and do the transfer in a single
> command. Would you like to be added to the copyright?

You took some code that you didn't create, and added some more
stuff. The copyright on the initial code remains, and it has nothing
to do with whether the author wants it or not, (s)he should be
mentionned, along with you of course.

> >> +            ? SUN4I_HDMI_DDC_CMD_IMPLICIT_READ
> >> +            : SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE,
> >> +            hdmi->base + SUN4I_HDMI_DDC_CMD_REG);
> >> +
> >> +     reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
> >> +     writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD,
> >> +            hdmi->base + SUN4I_HDMI_DDC_CTRL_REG);
> >> +
> >> +     if (!(msg->flags & I2C_M_RD)) {
> >> +             for (i = 0; i < count; i++) {
> >> +                     writeb(*msg->buf++, hdmi->base
> >> +                            + SUN4I_HDMI_DDC_FIFO_DATA_REG);
> >
> > writesb?
> I intend to rework the FIFO handling so will need to continue using writeb.

Then you'll change it when you'll rework it. Before then, you can use
writesb.

> >
> >> +                     --msg->len;
> >> +             }
> >> +     }
> >> +
> >> +     if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG,
> >> +                            reg,
> >> +                            !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD),
> >> +                            100, 100000))
> >> +             return -EIO;
> >> +
> >> +     reg = readl(hdmi->base + SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG);
> >> +     reg &= ~SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST;
> I want to check all the other bits that are set if there are errors.

Same thing here: you'll change it when that happens.

> >
> > You don't need to use that mask.
> >
> >> +     if (reg != SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE)
> >> +             return -EIO;
> >
> > !(reg & SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE) would be enough.
> I want to check all the other bits that are set if there are errors.

Same thing here.

> >
> >> +
> >> +     if (msg->flags & I2C_M_RD) {
> >> +             for (i = 0; i < count; i++) {
> >> +                     *msg->buf++ = readb(hdmi->base
> >> +                                         + SUN4I_HDMI_DDC_FIFO_DATA_REG);
> >
> > readsb ?
> I am reworking the FIFO handling so I will need to continue to use readb.

Same thing here.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)