Re: [PATCH v3] drivers: ddcci: upstream DDCCI driver

[Yusuf Khan]

Thanks for the feedback!

(2) Where would I place that documentation then,
documentation/devicetree/bindings/?

(4) A quick look at bootlin shows a symbol called
"modalias referenced 176 times.


On Wed, Mar 9, 2022 at 6:49 PM Randy Dunlap <rdunlap@xxxxxxxxxxxxx> wrote:
>
> 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 input 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-devices/.
>
> (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 ^= ((p_flag << 7) | len);
> +       *(ptr++) = (p_flag << 7)|len;
>
> This line could also be improved with some spaces:
>
> +       ret = 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 ‘__ddcci_write_bytewise’:
> ../drivers/char/ddcci.c:104:8: error: implicit declaration of function ‘i2c_smbus_write_byte’ [-Werror=implicit-function-declaration]
>   ret = i2c_smbus_write_byte(client, addr);
>         ^~~~~~~~~~~~~~~~~~~~
>   CC      kernel/delayacct.o
> ../drivers/char/ddcci.c: In function ‘__ddcci_write_block’:
> ../drivers/char/ddcci.c:165:9: error: implicit declaration of function ‘i2c_master_send’; did you mean ‘i2c_match_id’? [-Werror=implicit-function-declaration]
>   return i2c_master_send(client, sendbuf, ptr - sendbuf + 1);
>          ^~~~~~~~~~~~~~~
>          i2c_match_id
> ../drivers/char/ddcci.c: In function ‘__ddcci_read’:
> ../drivers/char/ddcci.c:216:8: error: implicit declaration of function ‘i2c_master_recv’; did you mean ‘i2c_match_id’? [-Werror=implicit-function-declaration]
>   ret = i2c_master_recv(client, buf, len);
>         ^~~~~~~~~~~~~~~
>         i2c_match_id
> ../drivers/char/ddcci.c: In function ‘ddcci_identify_device’:
> ../drivers/char/ddcci.c:413:10: error: implicit declaration of function ‘i2c_check_functionality’; did you mean ‘in_lock_functions’? [-Werror=implicit-function-declaration]
>        && i2c_check_functionality(client->adapter,
>           ^~~~~~~~~~~~~~~~~~~~~~~
>           in_lock_functions
> ../drivers/char/ddcci.c: In function ‘ddcci_module_init’:
> ../drivers/char/ddcci.c:1835:8: error: implicit declaration of function ‘i2c_add_driver’; did you mean ‘ddcci_add_driver’? [-Werror=implicit-function-declaration]
>   ret = i2c_add_driver(&ddcci_driver);
>         ^~~~~~~~~~~~~~
>         ddcci_add_driver
> ../drivers/char/ddcci.c: In function ‘ddcci_module_exit’:
> ../drivers/char/ddcci.c:1868:2: error: implicit declaration of function ‘i2c_del_driver’; did you mean ‘ddcci_del_driver’? [-Werror=implicit-function-declaration]
>   i2c_del_driver(&ddcci_driver);
>   ^~~~~~~~~~~~~~
>   ddcci_del_driver
>
>
>
> --
> ~Randy