Re: [PATCH v2 1/5] hwmon: pmbus: mpq8785: Prepare driver for multiple device support

[Guenter Roeck <linux@roeck-us.net>]

On 5/9/25 02:22, Krzysztof Kozlowski wrote:
> On 09/05/2025 09:41, Paweł Dembicki wrote:
>> pt., 9 maj 2025 o 09:03 Krzysztof Kozlowski <krzk@kernel.org> napisał(a):
>>>
>>> On 09/05/2025 08:51, Pawel Dembicki wrote:
>>>> Refactor the driver to support multiple Monolithic Power Systems devices.
>>>> Introduce chip ID handling based on device tree matching.
>>>>
>>>> No functional changes intended.
>>>>
>>>> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
>>>>
>>>> ---
>>>> v2:
>>>>   - no changes done
>>>> ---
>>>>   drivers/hwmon/pmbus/mpq8785.c | 38 +++++++++++++++++++++++++++--------
>>>>   1 file changed, 30 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/hwmon/pmbus/mpq8785.c b/drivers/hwmon/pmbus/mpq8785.c
>>>> index 331c274ca892..00ec21b081cb 100644
>>>> --- a/drivers/hwmon/pmbus/mpq8785.c
>>>> +++ b/drivers/hwmon/pmbus/mpq8785.c
>>>> @@ -8,6 +8,8 @@
>>>>   #include <linux/of_device.h>
>>>>   #include "pmbus.h"
>>>>
>>>> +enum chips { mpq8785 };
>>>
>>> Use Linux coding style, so:
>>> 1. missing wrapping after/before each {}
>>> 2. missing descriptive name for the type (mpq8785_chips)
>>> 3. CAPITALICS see Linux coding style - there is a chapter exactly about
>>> this.
>>>
>>>
>>
>> Sorry, I was thinking that it is a local pmbus tradition.
>> Many drivers have the same enum without capitalics :
>>

hwmon, really, not just pmbus.

>> grep -r "enum chips {" .
>> ./isl68137.c:enum chips {
>> ./bel-pfe.c:enum chips {pfe1100, pfe3000};
>> ./mp2975.c:enum chips {
>> ./ucd9200.c:enum chips { ucd9200, ucd9220, ucd9222, ucd9224, ucd9240,
>> ucd9244, ucd9246,
>> ./zl6100.c:enum chips { zl2004, zl2005, zl2006, zl2008, zl2105,
>> zl2106, zl6100, zl6105,
>> ./ucd9000.c:enum chips { ucd9000, ucd90120, ucd90124, ucd90160,
>> ucd90320, ucd9090,
>> ./max16601.c:enum chips { max16508, max16600, max16601, max16602 };
>> ./q54sj108a2.c:enum chips {
>> ./bpa-rs600.c:enum chips { bpa_rs600, bpd_rs600 };
>> ./adm1275.c:enum chips { adm1075, adm1272, adm1273, adm1275, adm1276,
>> adm1278, adm1281, adm1293, adm1294 };
>> ./max20730.c:enum chips {
>> ./mp2856.c:enum chips { mp2856, mp2857 };
>> ./tps53679.c:enum chips {
>> ./ltc2978.c:enum chips {
>> ./max34440.c:enum chips {
>> ./pim4328.c:enum chips { pim4006, pim4328, pim4820 };
>> ./fsp-3y.c:enum chips {
>> ./lm25066.c:enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
> 
> If that's standard for this subsystem, then it's fine, although to me it
> feels odd to see all over the code lower case constant.
> 

hwmon traditionally (as in: from the very beginning) uses lowercase enums
for chip types. It would be even more odd to change that now.

Thanks,
Guenter