On Mon, Jan 28, 2019 at 12:45:03PM +0100, Jorge Ramirez-Ortiz wrote: > @@ -653,6 +708,10 @@ spmi_regulator_find_range(struct spmi_regulator *vreg) > range = vreg->set_points->range; > end = range + vreg->set_points->count; > > + /* we know we only have one range for this type */ > + if (vreg->logical_type == SPMI_REGULATOR_LOGICAL_TYPE_HFS430) > + return range; > + > spmi_vreg_read(vreg, SPMI_COMMON_REG_VOLTAGE_RANGE, &range_sel, 1); > > for (; range < end; range++) Rather than have special casing for the logical type in here it seems better to just provide a specific op for this logical type, you could always make _find_range() call into that one if you really want code reuse here. You already have separate ops for this regulator type anyway. > +static unsigned int spmi_regulator_hfs430_get_mode(struct regulator_dev *rdev) > +{ > + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > + u8 reg; > + int ret; > + > + ret = spmi_vreg_read(vreg, SPMI_HFS430_REG_MODE, ®, 1); > + if (ret) { > + dev_err(&rdev->dev, "failed to get mode"); > + return ret; > + } > + > + if (reg == SPMI_HFS430_MODE_PWM) > + return REGULATOR_MODE_NORMAL; > + > + return REGULATOR_MODE_IDLE; > +} I'd have expected a switch statement here, ideally flagging a warning or error if we get a surprising value in there. > +static int spmi_regulator_hfs430_set_mode(struct regulator_dev *rdev, > + unsigned int mode) > +{ > + struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > + u8 reg = mode == REGULATOR_MODE_NORMAL ? SPMI_HFS430_MODE_PWM : > + SPMI_HFS430_MODE_AUTO; Please write a normal if statement (or switch statement) to help legibility.
Attachment:
signature.asc
Description: PGP signature