netdev - Re: [PATCH 1/2] dt-bindings: net: Add rfkill-gpio binding

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 22 Dec 2022 11:32:27 +0100
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Philipp Zabel <p.zabel@...gutronix.de>,
        Rob Herring <robh@...nel.org>
Cc:     Johannes Berg <johannes@...solutions.net>, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-wireless@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH 1/2] dt-bindings: net: Add rfkill-gpio binding

On 21/12/2022 16:36, Philipp Zabel wrote:
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +    description: rfkill switch name, defaults to node name
>>> +
>>> +  type:
>>
>> Too generic. Property names should ideally have 1 type globally. I think 
>> 'type' is already in use. 'radio-type' instead?
> 
> These values correspond to the 'enum rfkill_type' in Linux UAPI, but I
> think in this context 'radio-type' would be better than 'rfkill-type'.

Do not map Linux driver to DT, but rather describe the actual hardware.

> 
>>> +    description: rfkill radio type
>>> +    enum:
>>> +      - wlan
>>> +      - bluetooth
>>> +      - ultrawideband
>>> +      - wimax
>>> +      - wwan
>>> +      - gps
>>> +      - fm
>>> +      - nfc
>>> +
>>> +  shutdown-gpios:
>>> +    maxItems: 1
>>> +
>>> +  reset-gpios:
>>> +    maxItems: 1
>>
>> I'm lost as to why there are 2 GPIOs.
> 
> I don't know either.  My assumption is that this is for devices that
> are radio silenced by just asserting their reset pin (for example GPS
> chips). The driver handles them the same.
> 
> I could remove reset-gpios and make shutdown-gpios required.
> 
>>> +
>>> +required:
>>> +  - compatible
>>> +  - type
>>> +
>>> +oneOf:
>>> +  - required:
>>> +      - shutdown-gpios
>>> +  - required:
>>> +      - reset-gpios
>>
>> But only 1 can be present? So just define 1 GPIO name.
> 
> The intent was that only one of them would be required.
> 
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/gpio/gpio.h>
>>> +
>>> +    rfkill-pcie-wlan {
>>
>> Node names should be generic.
> 
> What could be a generic name for this - is "rfkill" acceptable even
> though it is a Linux subsystem name? Or would "rf-kill-switch" be
> better?

rfkill

> 
> How should they be called if there are multiple of them?

The same as in all other cases (leds, gpios, regulators), so rfkill-1,
rfkill-2...


Best regards,
Krzysztof