| Message ID | 20240806170018.638585-1-michael.nemanov@ti.com |
|---|---|
| Headers | show |
| Series | wifi: cc33xx: Add driver for new TI CC33xx wireless device family | expand |
On 06/08/2024 19:00, Michael Nemanov wrote: Thank you for your patch. There is something to discuss/improve. > +properties: > + compatible: > + enum: > + - ti,cc3300 > + - ti,cc3301 > + - ti,cc3350 > + - ti,cc3351 > + > + reg: > + description: > + must be set to 2 Then just const: 2 and drop free form text. > + maxItems: 1 > + > + interrupts: > + description: > + The out-of-band interrupt line. > + Can be IRQ_TYPE_EDGE_RISING or IRQ_TYPE_LEVEL_HIGH. > + If property is omitted, SDIO in-band IRQ will be used. > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + > + // SDIO example: Drop, obvious. > + mmc { > + #address-cells = <1>; > + #size-cells = <0>; > + > + wifi@1{ Missing space. Also, this does not match reg. Test your DTS with W=1 and FIX ALL warnings. Best regards, Krzysztof
On 06/08/2024 19:00, Michael Nemanov wrote: > Add device-tree bindings for the CC33xx family. > > Signed-off-by: Michael Nemanov <michael.nemanov@ti.com> > --- > .../bindings/net/wireless/ti,cc33xx.yaml | 56 +++++++++++++++++++ Also, bindings always come before users, but maybe the maintainers will re-order things since you expect here some squashing. Best regards, Krzysztof
On 06/08/2024 19:00, Michael Nemanov wrote: > These file contain various WLAN-oriented APIs > > Signed-off-by: Michael Nemanov <michael.nemanov@ti.com> > --- > drivers/net/wireless/ti/cc33xx/acx.c | 1011 ++++++++++++++++++++++++++ > drivers/net/wireless/ti/cc33xx/acx.h | 835 +++++++++++++++++++++ > 2 files changed, 1846 insertions(+) > create mode 100644 drivers/net/wireless/ti/cc33xx/acx.c > create mode 100644 drivers/net/wireless/ti/cc33xx/acx.h > > diff --git a/drivers/net/wireless/ti/cc33xx/acx.c b/drivers/net/wireless/ti/cc33xx/acx.c > new file mode 100644 > index 000000000000..3c9b590e69b1 > --- /dev/null > +++ b/drivers/net/wireless/ti/cc33xx/acx.c > @@ -0,0 +1,1011 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2022-2024 Texas Instruments Incorporated - https://www.ti.com/ > + */ > + > +#include "acx.h" > + > +int cc33xx_acx_clear_statistics(struct cc33xx *cc) > +{ > + struct acx_header *acx; > + int ret = 0; > + > + cc33xx_debug(DEBUG_ACX, "acx clear statistics"); So you just re-implemented tracing. No, I asked to drop such silly entry/exit messages because you duplicate existing mechanisms in the kernel. That's a no everywhere. Do not write such code. You can have useful debug statements when tracing or kprobes or whatever you want is not sufficient. Best regards, Krzysztof
On 8/7/2024 10:06 AM, Krzysztof Kozlowski wrote: > On 06/08/2024 19:00, Michael Nemanov wrote: > > Thank you for your patch. There is something to discuss/improve. > >> +properties: >> + compatible: >> + enum: >> + - ti,cc3300 >> + - ti,cc3301 >> + - ti,cc3350 >> + - ti,cc3351 >> + >> + reg: >> + description: >> + must be set to 2 > > Then just const: 2 and drop free form text. > >> + maxItems: 1 >> + >> + interrupts: >> + description: >> + The out-of-band interrupt line. >> + Can be IRQ_TYPE_EDGE_RISING or IRQ_TYPE_LEVEL_HIGH. >> + If property is omitted, SDIO in-band IRQ will be used. >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - reg >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/irq.h> >> + >> + // SDIO example: > > Drop, obvious. > >> + mmc { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + wifi@1{ > > Missing space. > > Also, this does not match reg. Test your DTS with W=1 and FIX ALL warnings. > > Best regards, > Krzysztof > Will fix all above. I'm currently testing my .yaml with: make dt_binding_check DT_CHECKER_FLAGS=-m \ DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml It reports no warnings. Adding W=1 doesn't seem to change anything. Am I missing something? Thanks and regards, Michael.
On 8/7/2024 10:06 AM, Krzysztof Kozlowski wrote: > On 06/08/2024 19:00, Michael Nemanov wrote: >> Add device-tree bindings for the CC33xx family. >> >> Signed-off-by: Michael Nemanov <michael.nemanov@ti.com> >> --- >> .../bindings/net/wireless/ti,cc33xx.yaml | 56 +++++++++++++++++++ > > Also, bindings always come before users, but maybe the maintainers will > re-order things since you expect here some squashing. > > Best regards, > Krzysztof > Will hoist the bindings to be 1st. Thanks and regards, Michael.
On 8/7/2024 10:15 AM, Krzysztof Kozlowski wrote: > On 06/08/2024 19:00, Michael Nemanov wrote: >> These file contain various WLAN-oriented APIs >> >> Signed-off-by: Michael Nemanov <michael.nemanov@ti.com> >> --- >> drivers/net/wireless/ti/cc33xx/acx.c | 1011 ++++++++++++++++++++++++++ >> drivers/net/wireless/ti/cc33xx/acx.h | 835 +++++++++++++++++++++ >> 2 files changed, 1846 insertions(+) >> create mode 100644 drivers/net/wireless/ti/cc33xx/acx.c >> create mode 100644 drivers/net/wireless/ti/cc33xx/acx.h >> >> diff --git a/drivers/net/wireless/ti/cc33xx/acx.c b/drivers/net/wireless/ti/cc33xx/acx.c >> new file mode 100644 >> index 000000000000..3c9b590e69b1 >> --- /dev/null >> +++ b/drivers/net/wireless/ti/cc33xx/acx.c >> @@ -0,0 +1,1011 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (C) 2022-2024 Texas Instruments Incorporated - https://www.ti.com/ >> + */ >> + >> +#include "acx.h" >> + >> +int cc33xx_acx_clear_statistics(struct cc33xx *cc) >> +{ >> + struct acx_header *acx; >> + int ret = 0; >> + >> + cc33xx_debug(DEBUG_ACX, "acx clear statistics"); > > So you just re-implemented tracing. > > No, I asked to drop such silly entry/exit messages because you duplicate > existing mechanisms in the kernel. > > That's a no everywhere. Do not write such code. You can have useful > debug statements when tracing or kprobes or whatever you want is not > sufficient. > > Best regards, > Krzysztof > OK, I misunderstood the previous exchange with you and Kalle Valo. I'll remove all entry / exit cc33xx_debug traces. Non-trivial ones are OK tough, right? Thanks and regards, Michael.
On 07/08/2024 17:51, Nemanov, Michael wrote: > On 8/7/2024 10:06 AM, Krzysztof Kozlowski wrote: >> On 06/08/2024 19:00, Michael Nemanov wrote: >> >> Thank you for your patch. There is something to discuss/improve. >> >>> +properties: >>> + compatible: >>> + enum: >>> + - ti,cc3300 >>> + - ti,cc3301 >>> + - ti,cc3350 >>> + - ti,cc3351 >>> + >>> + reg: >>> + description: >>> + must be set to 2 >> >> Then just const: 2 and drop free form text. >> >>> + maxItems: 1 >>> + >>> + interrupts: >>> + description: >>> + The out-of-band interrupt line. >>> + Can be IRQ_TYPE_EDGE_RISING or IRQ_TYPE_LEVEL_HIGH. >>> + If property is omitted, SDIO in-band IRQ will be used. >>> + maxItems: 1 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/interrupt-controller/irq.h> >>> + >>> + // SDIO example: >> >> Drop, obvious. >> >>> + mmc { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + wifi@1{ >> >> Missing space. >> >> Also, this does not match reg. Test your DTS with W=1 and FIX ALL warnings. >> >> Best regards, >> Krzysztof >> > > Will fix all above. > > I'm currently testing my .yaml with: > make dt_binding_check DT_CHECKER_FLAGS=-m \ > DT_SCHEMA_FILES=Documentation/devicetree/bindings/net/wireless/ti,cc33xx.yaml > > It reports no warnings. Adding W=1 doesn't seem to change anything. Am I > missing something? I said test your DTS, not bindings. Best regards, Krzysztof