On 04/05/2023 13:36, Naresh Solanki wrote: > Hi Krzysztof, > > On 04-05-2023 04:17 pm, Krzysztof Kozlowski wrote: >> On 04/05/2023 11:47, Naresh Solanki wrote: >>> Hi Krzysztof, >>> >>> On 03-05-2023 09:48 pm, Krzysztof Kozlowski wrote: >>>> On 03/05/2023 10:26, Naresh Solanki wrote: >>>>> Hi Krzysztof, >>>>> >>>>> On 24-04-2023 03:23 pm, Naresh Solanki wrote: >>>>>> Hi Krzysztof, >>>>>> >>>>>> On 24-04-2023 03:12 pm, Krzysztof Kozlowski wrote: >>>>>>> On 24/04/2023 11:18, Naresh Solanki wrote: >>>>>>> >>>>>>>>>> Changes in V2: >>>>>>>>>> - Update subject >>>>>>>>>> - Drop blank lines >>>>>>>>>> --- >>>>>>>>>> .../bindings/hwmon/maxim,max6639.yaml | 52 >>>>>>>>>> +++++++++++++++++++ >>>>>>>>>> 1 file changed, 52 insertions(+) >>>>>>>>>> create mode 100644 >>>>>>>>>> Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml >>>>>>>>>> >>>>>>>>>> diff --git >>>>>>>>>> a/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml >>>>>>>>>> b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml >>>>>>>>>> new file mode 100644 >>>>>>>>>> index 000000000000..1aaedfd7cee0 >>>>>>>>>> --- /dev/null >>>>>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/maxim,max6639.yaml >>>>>>>>>> @@ -0,0 +1,52 @@ >>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>>>>>>> +%YAML 1.2 >>>>>>>>>> +--- >>>>>>>>>> +$id: http://devicetree.org/schemas/hwmon/maxim,max6639.yaml# >>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>>>>> + >>>>>>>>>> +title: Maxim max6639 >>>>>>>>> >>>>>>>>> What is this device? fan controller? >>>>>>>> Yes Fan controller. >>>>>>>> Do you want me to update the title here as: >>>>>>>> "Maxim MAC6639 2 channel fan controller & temperature monitor" ? >>>>>>> >>>>>>> Enough would be: >>>>>>> Maxim MAX6639 Fan Controller >>>>>> Ack >>>>>>> >>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>>> + >>>>>>>>>> +maintainers: >>>>>>>>>> + - Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx> >>>>>>>>>> + >>>>>>>>>> +description: | >>>>>>>>>> + The MAX6639 is a 2-channel temperature monitor with dual, >>>>>>>>>> automatic, PWM >>>>>>>>>> + fan-speed controller. It monitors its own temperature and one >>>>>>>>>> external >>>>>>>>>> + diode-connected transistor or the temperatures of two external >>>>>>>>>> diode-connected >>>>>>>>>> + transistors, typically available in CPUs, FPGAs, or GPUs. >>>>>>>>>> + >>>>>>>>>> + Datasheets: >>>>>>>>>> + https://datasheets.maximintegrated.com/en/ds/MAX6639-MAX6639F.pdf >>>>>>>>>> + >>>>>>>>>> +properties: >>>>>>>>>> + compatible: >>>>>>>>>> + enum: >>>>>>>>>> + - maxim,max6639 >>>>>>>>>> + >>>>>>>>>> + reg: >>>>>>>>>> + maxItems: 1 >>>>>>>>>> + >>>>>>>>>> + '#address-cells': >>>>>>>>>> + const: 1 >>>>>>>>>> + >>>>>>>>>> + '#size-cells': >>>>>>>>>> + const: 0 >>>>>>>>> >>>>>>>>> Why do you need these two properties? >>>>>>>> Ack. Will remove them. >>>>>>>>> >>>>>>>>> Anyway, the binding looks incomplete. Where are the supplies? >>>>>>>>> Interrupts? >>>>>>>> This patch just adds basic support to the existing platform driver. >>>>>>>> Intention is to be able to call the driver from DT with basic >>>>>>>> initialization from driver the existing driver. >>>>>>> >>>>>>> Bindings should be rather complete. Here the datasheet is accessible and >>>>>>> few properties quite obvious, so I don't see a reason to skip them. >>>>>> I agree with you. But currently the driver which is already merged >>>>>> doesn't has it implemented. >>>>>> And will be working on separate patch to include this later. >>>>> Please let me know if this approach is acceptable, or if there are any >>>>> other suggestions or concerns that you have. >>>> >>>> You are adding new bindings, so what does the driver has to do with it? >>> The reason for adding these new bindings is to enable the use of the >>> driver on my machine. Without the compatible string, it would not be >>> possible to use the driver. >>> >>> Currently, the driver initializes the device with defaults, which is >>> good enough for my application. Also, as you previously pointed out, it >>> uses the optional 'fan-supply' which will be included in the next patch >>> revision. >>> >>> I hope this clarifies my reasoning. Could you kindly confirm if we can >>> proceed with this approach? >> >> No, we cannot, because we asked you to fix things there. Your entire >> explanation about compatible and driver is not related to the comment >> you received: bindings should be complete. You argue that bindings do >> not have to be complete, because of something with driver. This is not >> related. Bindings are not for driver. > > I understand that complete bindings are important, but as the driver is > already merged and functional, my immediate goal is to enable its use on > my machine. I will work on a separate patch to include the interrupts in > both binding & driver. I don't care about driver and did not comment about it. I don't understand why do you bring it here all'n over. Best regards, Krzysztof
