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 <[email protected]> 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
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 <[email protected]> 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
--
~Randy
(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 <[email protected]> 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 <[email protected]> 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
>
> --
> ~Randy