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

[Guenter Roeck]

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@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 :

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