Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ace9cc9d-ccf1-fffa-3504-6504fdfb5a20@kernel.org>
Date:   Thu, 22 Dec 2022 11:32:27 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH 1/2] dt-bindings: net: Add rfkill-gpio binding
Content-Language: en-US
To:     Philipp Zabel <p.zabel@pengutronix.de>,
        Rob Herring <robh@kernel.org>
Cc:     Johannes Berg <johannes@sipsolutions.net>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-wireless@vger.kernel.org, kernel@pengutronix.de
References: <20221221104803.1693874-1-p.zabel@pengutronix.de>
 <20221221144505.GA2848091-robh@kernel.org>
 <432ed015f4ba99d6bddd0a10af72324fea1388da.camel@pengutronix.de>
From:   Krzysztof Kozlowski <krzk@kernel.org>
In-Reply-To: <432ed015f4ba99d6bddd0a10af72324fea1388da.camel@pengutronix.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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