On Thu, Dec 10, 2020 at 11:16:29PM +0100, Adrien Grassein wrote: > + { .compatible = "nxp,pf8200",}, > + { } > +}; > +MODULE_DEVICE_TABLE(of, pf8x_dt_ids); > + > +const struct id_name id_list[] = { > + {PF8100, "PF8100"}, > + {PF8121A, "PF8121A"}, > + {PF8200, "PF8200"}, > + {0, "???"}, > +}; It's strange to see the list terminated with this ??? entry - these things usually have a NULL for both the ID and the string. The string on the terminator is never used so may as well stick with the usual pattern. > +static int encode_phase(struct pf8x_chip *pf, int phase) > +{ > + int ph; > + > + if (phase < 0) > + return -1; > + > + ph = phase / 45; > + if ((ph * 45) != phase) { > + dev_err(pf->dev, "ignoring, illegal phase %d\n", phase); > + return -1; > + } > + > + return (ph >= 1) ? ph - 1 : 7; As I said on the previousl review please write normal condidional statements to improve the legibility of the code. > +static inline struct regulator_init_data *match_init_data(int index) > +{ > + return pf8x00_matches[index].init_data; > +} > + > +static inline struct device_node *match_of_node(int index) > +{ > + return pf8x00_matches[index].of_node; > +} If you need these your driver has a problem, as previously advised please use the standard DT parsing support. > + memcpy(pf->regulator_descs, pf8x00_regulators, > + sizeof(pf->regulator_descs)); Why do you need to take a copy of this? The descriptors should not be being modified. > + num_regulators = ARRAY_SIZE(pf->regulator_descs); > + for (i = 0; i < num_regulators; i++) { > + struct regulator_init_data *init_data; > + struct regulator_desc *desc; > + > + desc = &pf->regulator_descs[i].desc; > + init_data = match_init_data(i); > + > + config.dev = &client->dev; > + config.init_data = init_data; > + config.driver_data = pf; > + config.of_node = match_of_node(i); > + config.ena_gpiod = NULL; You've not done anything to parse the init data (which is good) so the init data handling is at best redundant. > + if ((i >= REG_SW1) && (i <= REG_SW7)) { > + if (mask) { > + ret = regmap_update_bits(pf->regmap, reg, mask, > + val); > + } > + > + if (fast_slew > 1) { > + ret = regmap_read(pf->regmap, reg, &fast_slew); > + fast_slew &= 0x20; > + if (ret < 0) > + fast_slew = 0; > + pf->regulator_descs[i].fast_slew = fast_slew >> 5; > + } May as well just write configurations out when you parse them, no need to defer to here and it makes things simpler. > +module_i2c_driver(pf8x_driver); > + > +static void __exit pf8x_exit(void) > +{ > + i2c_del_driver(&pf8x_driver); > +} > +module_exit(pf8x_exit); Does this build cleanly? module_i2c_driver() does both the add and delete.