Received-SPF: pass (google.com: domain of linux-kernel+bounces-105923-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <ab8b45c5-2ef0-4a87-87bf-f797954b4574@amperemail.onmicrosoft.com>
Date: Mon, 18 Mar 2024 16:51:46 +0700
User-Agent: Mozilla Thunderbird
From: Chanh Nguyen <chanh@amperemail.onmicrosoft.com>
Subject: Re: [PATCH 1/3] dt-bindings: hwmon: Add maxim max31790 driver
 bindings
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
 Chanh Nguyen <chanh@os.amperecomputing.com>, Jean Delvare
 <jdelvare@suse.com>, Guenter Roeck <linux@roeck-us.net>,
 Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, Justin Ledford
 <justinledford@google.com>, devicetree@vger.kernel.org,
 linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
 OpenBMC Maillist <openbmc@lists.ozlabs.org>,
 Open Source Submission <patches@amperecomputing.com>
Cc: Phong Vo <phong@os.amperecomputing.com>,
 Thang Nguyen <thang@os.amperecomputing.com>,
 Quan Nguyen <quan@os.amperecomputing.com>
References: <20240311111347.23067-1-chanh@os.amperecomputing.com>
 <20240311111347.23067-2-chanh@os.amperecomputing.com>
 <6fb70adb-aa85-4b9c-b093-afa4ec7ed056@linaro.org>
Content-Language: en-US
In-Reply-To: <6fb70adb-aa85-4b9c-b093-afa4ec7ed056@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?WjBCVGJrbStIbzJXRUUydlNjRElSQ2NSVy85cGtBKzE1RWVlNXRyd3orbTMx?=
 =?utf-8?B?dTZiYWd4S29HTzVleFJsdm5uazRVSFR2Sm9FK2sxQjZaMi9Gb2o4OUcwdGph?=
 =?utf-8?B?Y3QvNDFZOXk3TWpBSjBiUktNVStOL2FKckxTVHFmMDlHcmRYQ2krRHhVNHR5?=
 =?utf-8?B?Nkh1NWx3bXQ3UmR1K2JmR3lvRStEKzZDaXhVTmxjVkZFSWVMRklLSndFd2xt?=
 =?utf-8?B?QWxPdUlkVHZsZlY5L3J3UXZRVHVYbGJnZ1JHS1NWQlhTL1pMUFl0S2xzVVpa?=
 =?utf-8?B?a2xPMWlWVHM2Q0F2ZG9iMHR0MUpiSVJ1OUhQMS9WR1dTbFB3MHlZTXhuSnFs?=
 =?utf-8?B?bG9WaEg5cUF2cTFjZnFzc0ZVcGxsVmZHaXRIK1AySzF0ZndjM3M2cTFDMUhw?=
 =?utf-8?B?ckJnRjlVQTUramF2T2s3U1Rud2IxNDVNamFTVGFYcUJMcDg3N24rSGRjZXAr?=
 =?utf-8?B?MXdZODhVeDdPb004TmNweFJ4NzFRZk5uS09UaktGQXNnWWFNUVlvcUFkU3NC?=
 =?utf-8?B?cUJxSXhQdGpyalgxS1RISG5VdUpUdlorYnFLSlFvalJQeVhUclNydnM2Wksv?=
 =?utf-8?B?UEhqUGZqSG12bEp5UW55MHUrNmErK3Z5MkEvcktoNVVOdjlnYWxLbzk3cGYw?=
 =?utf-8?B?eis0MldoTVJTR0VBV05VZS8zclNQMzFjRUlOeEVCNnlHcThrK3krcTR3VFlW?=
 =?utf-8?B?MVA3YzQzcnkwM0JDNDRLTVF5UUttb090WFNMV2gvZy9EVkE3NnAzcDQ4VEVs?=
 =?utf-8?B?SWptV0UwcEpVdmkxS3VpWU9nQm15aFhXblliZ1pmT08walFQNzZpSCswWXZ6?=
 =?utf-8?B?d2NLbTg4WU1QQUFLVmRjQlpmOTVSbWpiSFZDN3JQdkZDMDkyR0M3SzlBc2VF?=
 =?utf-8?B?c2NTZzdNVVQ5b2FwQmQvYi9xV2lhTlRrSVhqVEpJaGx3cVdRUkNiMWhiYmpP?=
 =?utf-8?B?VUUySzVBUWhPSGhMK0xvMjBxUUJDR2tzcDZzdHcxM0U3Rjl1eFcyOUhPZlN3?=
 =?utf-8?B?MmFKQ2JvVHl3bjZaNzBWSnAzMUpUTmd0cXc0VmZkMGFJR0QvaG8xMEJscE9H?=
 =?utf-8?B?NHhWdVBkdHpHR0FjeTNydlY2MUlWYjBsbjhxVi94ZXprNUd2RTdyMzFDVDlj?=
 =?utf-8?B?S0p0Qy82SVJXYVFiWHVFTGZ4dERqWE50eDhyV2tyeFFKbWU5VEVETUFzRjAw?=
 =?utf-8?B?Q2hDOFFSSWJWcHJYYmZWRXZzVUcrYlMwV0pudGswakJiODJpRGM1WFdyT0hV?=
 =?utf-8?B?SFl1RW5xemJIY2Q4QWVyN3k0K0UzUm1WVENQQS9GcFhlOTdwdFQ0d3cxdG5v?=
 =?utf-8?B?OWdaVUlkTTlPdlJiOS9EWE5tUXNBQkkvL2pGSG5UdmgzZUhWOVg3cDZaZ0gr?=
 =?utf-8?B?RlZiRlVhZTVsWS9yeUNlQm5zc0N4Ui9aQnVyVHU1OTRRdk91czhqN0k5dktl?=
 =?utf-8?B?NmNNRUZuRFdzUWpuUXBjQ2JwOEFmTTBleXhRZTY4RUVQTkJRRXI0SktUTCty?=
 =?utf-8?B?eVZpdWU3UW1DMWNVUFNkQjVtSGRwZnk4ZEJCSVdmK0FzSTYzaEFKZ2tjNytQ?=
 =?utf-8?B?SW5obmJESnp6eGlCdjNDVkI5QnNHUllMRmM5U3A5WTV4U0VYbXMzck9ab1pK?=
 =?utf-8?B?TXIzL0FlSEpnbFRoZEZwOUYvL1RUN3VDTld5Um03eDBDTVJsSmhDKy8zcmJY?=
 =?utf-8?B?b3hMbG1VYm04Nk1tN1V5RUpOQUp3VVdqcWZNS2o1R3BjTHRsREgva2JjK0Y0?=
 =?utf-8?B?TGFXMmUrdEdKVm5DNnpORW52aGRZL1V2bzYzS2g4Vm5WU0l3OENHSkk2WVdr?=
 =?utf-8?B?NEpsRCtrYlVGM0grWUZ5YWUrbTh4TVlIYlhnT3RiNEdCUU5nZDlJenB4K2Vz?=
 =?utf-8?B?SmgzenJPYXJ1S1V5Sm02MkFhZW9OVHVYOGYza3luaUVNTkNjSko4NW1lWk5m?=
 =?utf-8?B?OEtvWGRPTkdpeHgvcURQdE5jQTcraEtnQW0xV01ORUdNL2wrbUc3dW92cmU0?=
 =?utf-8?B?Snh0M0tBVnhCTGZROEZzWjBoaGI2MEcyN3FkM3VpQkFWMFR3d3lHemJYazQ2?=
 =?utf-8?B?RlJoQkZ5WW9yUjkxNFlEMFBvOCtFenFDanpxbk9TZDA5QXgybzBpZEdTT1o0?=
 =?utf-8?B?b0dUaUJ6TVdnZm9pSkR3Z1hOMXVxTmZQU1hZd282ektqSi9pMm8wU2xWVVVK?=
 =?utf-8?Q?7x2mGcXh3FUHVGx7s7zSy9I=3D?=
X-OriginatorOrg: amperemail.onmicrosoft.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 688e077c-0db0-46e5-b900-08dc47310d6f
X-MS-Exchange-CrossTenant-AuthSource: DM6PR01MB5947.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Mar 2024 09:51:58.5510
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: lJQasXtzwVfSFSuleFuKCOHbfVMCmaucZeVZ6bOSyxWMKRYLsU72alxkogHVbpexeriyyUnP6AE6lmMjTk+oeTf9UuYj7HK+zETLGEKK+kH6mdXXhY12A2F3Bu/MI7v/
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR01MB6473


On 11/03/2024 23:55, Krzysztof Kozlowski wrote:
> On 11/03/2024 12:13, Chanh Nguyen wrote:
>> Add a device tree bindings for max31790 device.
> 
> Subject: drop "driver", bindings are about hardware.
> 

Yes, I'll drop "driver" at v2 updating.

> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
> 


I tested the binding, I didn't see any warning/error log. Please review 
my logs as below

=> make dt_binding_check DT_SCHEMA_FILES=/hwmon/max31790.yaml
make[1]: Entering directory '/DISK4T/work/community/linux/out'
   DTEX    Documentation/devicetree/bindings/hwmon/max31790.example.dts
   DTC_CHK Documentation/devicetree/bindings/hwmon/max31790.example.dtb
make[1]: Leaving directory '/DISK4T/work/community/linux/out'

>>
>> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com>
>> ---
>>   .../devicetree/bindings/hwmon/max31790.yaml   | 44 +++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/max31790.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml b/Documentation/devicetree/bindings/hwmon/max31790.yaml
>> new file mode 100644
>> index 000000000000..5a93e6bdebda
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml
> 
> Filename like compatible.

Yes, I'll update that in v2

> 
>> @@ -0,0 +1,44 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hwmon/max31790.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: The Maxim MAX31790 Fan Controller
>> +
>> +maintainers:
>> +  - Jean Delvare <jdelvare@suse.com>
>> +  - Guenter Roeck <linux@roeck-us.net>
> 
> You should have here someone responsible for hardware, not subsystem
> maintainers.
> 

Hi Krzysztof,
I checked the history of the drivers/hwmon/max31790.c and see Guenter 
Roeck <linux@roeck-us.net> as an important maintainer. I saw many 
commits from him. So, I add him to maintainer list.

>> +
>> +description: >
>> +  The MAX31790 controls the speeds of up to six fans using six
>> +  independent PWM outputs. The desired fan speeds (or PWM duty cycles)
>> +  are written through the I2C interface.
>> +
>> +  Datasheets:
>> +    https://datasheets.maximintegrated.com/en/ds/MAX31790.pdf
>> +
>> +properties:
>> +  compatible:
>> +    const: maxim,max31790
>> +
>> +  reg:
>> +    maxItems: 1
> 
> That's weirdly empty.
> 

Hi Krzysztof,
I have not yet understood your comment here. Please help give more 
details for my missing! Thank Krzysztof!
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
> 
> You miss allOf: with $ref to fan controller schema.
> 

Thank Krzysztof,
I'll add the allOf at v2.

>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      max31790@20 {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 

I suggest some node names, such as "i2c-fan" or "fan-controller" . Can 
you please share your ideas with me!

> 
> Best regards,
> Krzysztof
>