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

[Randy Dunlap]

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