Re: AW: [PATCH 1/2] dt-bindings: hwmon: Introduce ADS71x8

[Krzysztof Kozlowski <krzk@kernel.org>]

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@softing.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@softing.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 = <&reg_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