[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <45037518-41b7-4cc1-a7f4-c4a0c9873950@kernel.org>
Date: Mon, 2 Sep 2024 18:02:24 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: "Sperling, Tobias" <Tobias.Sperling@...ting.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"jdelvare@...e.com" <jdelvare@...e.com>,
"linux@...ck-us.net" <linux@...ck-us.net>, "robh@...nel.org"
<robh@...nel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>, "corbet@....net"
<corbet@....net>
Subject: Re: AW: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8
On 02/09/2024 15:48, Sperling, Tobias wrote:
>> On Fri, Aug 30, 2024 at 11:49:53AM +0000, Sperling, Tobias wrote:
>>> >From b2e04ce5500faf274654be5284be9db4f3abefce Mon Sep 17 00:00:00
>> 2001
>>
>> Some junk ^^^ above. Please investigate how you send patches.
>
> Yeah also saw this line, but of course tried to apply the patch again after sending it
> as mail and that worked fine. But just checked again and seems like this line can be
> dropped.
> And yes, I sent the patches manually, as we likely have some restrictions for smtp,
> but as I was able to apply them again it's fine I guess.
>
>>> From: Tobias Sperling <tobias.sperling@...ting.com>
>>> Date: Fri, 23 Aug 2024 12:08:33 +0200
>>> Subject: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8
>>
>> And all this suggests malformed patch.
>
> Why? If I drop this I'm not able to apply the patch, so I think this should be fine.
OK, it works with b4, but seeing duplicated subject is not expected and
might not work with all tools.
>
>>>
>>> Add documentation for the driver of ADS7128 and ADS7138 12-bit, 8-channel
>>> analog-to-digital converters. These ADCs have a wide operating range and
>>> a wide feature set. Communication is based on an I2C interface.
>>> The driver provides the functionality of manually reading single channels
>>> or sequentially reading all channels automatically.
>>>
>>> Signed-off-by: Tobias Sperling <tobias.sperling@...ting.com>
>>> ---
>>> .../devicetree/bindings/hwmon/ti,ads71x8.yaml | 85 +++++++++++
>>> Documentation/hwmon/ads71x8.rst | 140 ++++++++++++++++++
>>> Documentation/hwmon/index.rst | 1 +
>>
>> Please run scripts/checkpatch.pl and fix reported warnings. Then please
>> run and (probably) fix more warnings.
>> Some warnings can be ignored, especially from --strict run, but the code
>> here looks like it needs a fix. Feel free to get in touch if the warning
>> is not clear.
>
> Had done this already before submitting the patches (at least without --strict),
> but only reports a warning about splitting the patch (which I got wrong here)
> and updating the maintainers.
> I guess you were about suggesting a second script to run. Which one is that?
Please split the patches.
>
>>> +$id:
>> https://deu01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.
>> org%2Fschemas%2Fhwmon%2Fti%2Cads71x8.yaml%23&data=05%7C02%7C%7C
>> ff09fedbe2744394f78508dcc9881ee7%7Cfe3606fad3974238999768dcd7851f64
>> %7C1%7C0%7C638606833686313557%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi
>> MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C
>> %7C%7C&sdata=vZaCpdaNzELpNNnd6wp5P9MNLQTnAmWXYD%2BNKQYCJ78%
>> 3D&reserved=0
>>> +$schema:
>> https://deu01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.
>> org%2Fmeta-
>> schemas%2Fcore.yaml%23&data=05%7C02%7C%7Cff09fedbe2744394f78508dcc
>> 9881ee7%7Cfe3606fad3974238999768dcd7851f64%7C1%7C0%7C63860683368
>> 6326954%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
>> MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=EJflznuTZYGR
>> BRjULwohiHk8gPF9iRusSbmF8CKkl5Q%3D&reserved=0
>>> +
>>> +title: Texas Instruments ADS7128/ADS7138 Analog to Digital Converter (ADC)
>>> +
>>> +maintainers:
>>> + - None
>>
>> Fidn a person... otherwise why would we care?
>
> I know it's not nice, but we are likely not implementing further features, but OK, will
> add myself then.
>
>>> + default: 1
>>> +
>>> + interrupts:
>>> + description: |
>>> + Only considered in mode 1!
>>
>> What is "considered"? Driver considers? This does not matter. Describe
>> the hardware and if this is hardware related, you should have
>> allOf:if:then restricting this.
>
> It's possible to define a mode, either manual sampling or autosampling. In the
> latter mode, also the hardware capabilities change, e.g. the driver is able to
> trigger an interrupt so defining the interrupt only makes sense in that mode.
> Will have a look to allOf:if:then then.
>
>>> + compatible = "ti,ads7138";
>>> + reg = <0x10>;
>>> + avdd-supply = <®_stb_3v3>;
>>> + ti,mode = /bits/ 8 <1>;
>>> + ti,interval = /bits/ 16 <1000>;
>>> + interrupt-parent = <&gpio2>;
>>> + interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
>>> + status = "okay";
>>
>> Drop
>
> I guess, I shall only drop the "status" not the whole section?
Only status
Best regards,
Krzysztof
Powered by blists - more mailing lists