Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220310004433.5713-1-yusisamerican@gmail.com>
 <3179f015-58d9-6786-c2c4-6a54088c9221@infradead.org> <CAJoG2+9EN9uP0-S+vpfgiRzu_ZXx017vXEZMY7wcuZ+5BZoEYQ@mail.gmail.com>
 <daa82c42-d677-c007-3295-e3f533a28b87@infradead.org>
In-Reply-To: <daa82c42-d677-c007-3295-e3f533a28b87@infradead.org>
From:   Yusuf Khan <yusisamerican@gmail.com>
Date:   Wed, 9 Mar 2022 21:37:32 -0800
Message-ID: <CAJoG2+9pKqhpgCbFFGGK0+KTx9g8bgu37EvqABzmpTvAa8FJkw@mail.gmail.com>
Subject: Re: [PATCH v3] drivers: ddcci: upstream DDCCI driver
To:     Randy Dunlap <rdunlap@infradead.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Jason Wang <jasowang@redhat.com>,
        Michael Kelley <mikelley@microsoft.com>,
        "Michael S. Tsirkin" <mst@redhat.com>,
        gregkh <gregkh@linuxfoundation.org>, javier@javigon.com,
        Arnd Bergmann <arnd@arndb.de>, Will Deacon <will@kernel.org>,
        Jens Axboe <axboe@kernel.dk>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

(2) I agree, it seems fitting.

(4) bootlin a really great source code browser see
this link: https://elixir.bootlin.com/linux/v5.17-rc7/A/ident/modalias,
someone in kindergarten can realistically debug kernel issues and
fix them with it.

On Wed, Mar 9, 2022 at 9:19 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
>
>
> On 3/9/22 21:01, Yusuf Khan wrote:
> > Thanks for the feedback!
> >
> > (2) Where would I place that documentation then,
> > documentation/devicetree/bindings/?
>
> I like the way that you have all the info in one place, but the
> current way Documentation is organized has /sysfs interfaces in
> Documentation/ABI/. Then I would put the rest of the doc info in
> Documentation/driver-api/ddcci.rst.
>
> > (4) A quick look at bootlin shows a symbol called
> > "modalias referenced 176 times.
> >
>
> Well, bootlin is not a kernel driver.
> I see places in generic ACPI or generic (other) firmware code
> or SPI bus working with modalias strings. Maybe it's OK --
> just seems odd to me.
>
> >
> > On Wed, Mar 9, 2022 at 6:49 PM Randy Dunlap <rdunlap@infradead.org> wro=
te:
> >>
> >> Hi--
> >>
> >> On 3/9/22 16:44, Yusuf Khan wrote:
> >>> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> >>> index 740811893c57..c7aa439d23e7 100644
> >>> --- a/drivers/char/Kconfig
> >>> +++ b/drivers/char/Kconfig
> >>> @@ -451,4 +451,13 @@ config RANDOM_TRUST_BOOTLOADER
> >>>       pool. Otherwise, say N here so it will be regarded as device in=
put that
> >>>       only mixes the entropy pool.
> >>>
> >>> +config DDCCI
> >>> +     tristate "DDCCI display protocol support"
> >>> +     help
> >>> +       Display Data Channel Command Interface is a
> >>
> >>                                                  is an
> >>
> >> Also, the line above ends with a space. Please check the entire patch
> >> for lines that end with SPACE and remove the trailing spaces.
> >>
> >>> +       interface that allows the kernel to "talk"
> >>> +       to most displays made after 2005. Check your
> >>> +       display's specification to see if it has
> >>> +       support for this.
> >>> +
> >>>  endmenu
> >>
> >> (2) ddcci appears to be a char driver, not a misc driver,
> >> so its documentation probably should not be in Documentation/misc-devi=
ces/.
> >>
> >> (3) The documentation file ends with:
> >>
> >> +options ddcci dyndbg
> >> +options ddcci-backlight dyndbg
> >> \ No newline at end of file
> >>
> >> so please add a newline at the end of the last line.
> >>
> >> (4) What is the modalias stuff about?
> >> Does the kernel have other drivers that play modalias games
> >> like this?
> >>
> >>
> >> (5) This standalone comment might need some more text.
> >> Doesn't make much sense by itself.
> >>
> >> +       /* Special case: sender to 0x6E is always 0x51 */
> >>
> >> Does that mean something like "reply to 0x6E is always 0x51" ?
> >>
> >>
> >> (6) Be a bit more generous with spaces around operators (kernel style)=
.
> >> And be consistent.  This first line is OK, second line not so much.
> >>
> >> +       xor ^=3D ((p_flag << 7) | len);
> >> +       *(ptr++) =3D (p_flag << 7)|len;
> >>
> >> This line could also be improved with some spaces:
> >>
> >> +       ret =3D payload_len+3+((quirks & DDCCI_QUIRK_SKIP_FIRST_BYTE)?=
1:0);
> >>
> >> (e.g. -- I expect that there are others.)
> >>
> >>
> >> (7) Too much use of likely() and unlikely().
> >>
> >> (8) I don't see anything in the Kconfig entries such as
> >>         depends on I2C
> >>
> >> so do these 2 drivers work without I2C being enabled in a kernel?
> >>
> >> Oh, if I build ddcci without CONFIG_I2C:
> >>
> >> ../drivers/char/ddcci.c: In function =E2=80=98__ddcci_write_bytewise=
=E2=80=99:
> >> ../drivers/char/ddcci.c:104:8: error: implicit declaration of function=
 =E2=80=98i2c_smbus_write_byte=E2=80=99 [-Werror=3Dimplicit-function-declar=
ation]
> >>   ret =3D i2c_smbus_write_byte(client, addr);
> >>         ^~~~~~~~~~~~~~~~~~~~
> >>   CC      kernel/delayacct.o
> >> ../drivers/char/ddcci.c: In function =E2=80=98__ddcci_write_block=E2=
=80=99:
> >> ../drivers/char/ddcci.c:165:9: error: implicit declaration of function=
 =E2=80=98i2c_master_send=E2=80=99; did you mean =E2=80=98i2c_match_id=E2=
=80=99? [-Werror=3Dimplicit-function-declaration]
> >>   return i2c_master_send(client, sendbuf, ptr - sendbuf + 1);
> >>          ^~~~~~~~~~~~~~~
> >>          i2c_match_id
> >> ../drivers/char/ddcci.c: In function =E2=80=98__ddcci_read=E2=80=99:
> >> ../drivers/char/ddcci.c:216:8: error: implicit declaration of function=
 =E2=80=98i2c_master_recv=E2=80=99; did you mean =E2=80=98i2c_match_id=E2=
=80=99? [-Werror=3Dimplicit-function-declaration]
> >>   ret =3D i2c_master_recv(client, buf, len);
> >>         ^~~~~~~~~~~~~~~
> >>         i2c_match_id
> >> ../drivers/char/ddcci.c: In function =E2=80=98ddcci_identify_device=E2=
=80=99:
> >> ../drivers/char/ddcci.c:413:10: error: implicit declaration of functio=
n =E2=80=98i2c_check_functionality=E2=80=99; did you mean =E2=80=98in_lock_=
functions=E2=80=99? [-Werror=3Dimplicit-function-declaration]
> >>        && i2c_check_functionality(client->adapter,
> >>           ^~~~~~~~~~~~~~~~~~~~~~~
> >>           in_lock_functions
> >> ../drivers/char/ddcci.c: In function =E2=80=98ddcci_module_init=E2=80=
=99:
> >> ../drivers/char/ddcci.c:1835:8: error: implicit declaration of functio=
n =E2=80=98i2c_add_driver=E2=80=99; did you mean =E2=80=98ddcci_add_driver=
=E2=80=99? [-Werror=3Dimplicit-function-declaration]
> >>   ret =3D i2c_add_driver(&ddcci_driver);
> >>         ^~~~~~~~~~~~~~
> >>         ddcci_add_driver
> >> ../drivers/char/ddcci.c: In function =E2=80=98ddcci_module_exit=E2=80=
=99:
> >> ../drivers/char/ddcci.c:1868:2: error: implicit declaration of functio=
n =E2=80=98i2c_del_driver=E2=80=99; did you mean =E2=80=98ddcci_del_driver=
=E2=80=99? [-Werror=3Dimplicit-function-declaration]
> >>   i2c_del_driver(&ddcci_driver);
> >>   ^~~~~~~~~~~~~~
> >>   ddcci_del_driver
> >>
> >>
> >>
> >> --
> >> ~Randy
>
> --
> ~Randy