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

[Krzysztof Kozlowski]

On 09/05/2025 09:41, Paweł Dembicki wrote:
> pt., 9 maj 2025 o 09:03 Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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@xxxxxxxxx>
>>>
>>> ---
>>> 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 :
> 
> 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.


Best regards,
Krzysztof