On Mon, May 06, 2024 at 10:09:50PM +0300, Andy Shevchenko wrote: > Mon, May 06, 2024 at 05:08:29PM +0200, Johan Hovold kirjoitti: > > From: Satya Priya <quic_c_skakit@xxxxxxxxxxx> > > > > Qualcomm Technologies, Inc. PM8008 is an I2C-controlled PMIC containing > > seven LDO regulators. Add a PM8008 regulator driver to support PMIC > > regulator management via the regulator framework. > > > > Note that this driver, originally submitted by Satya Priya [1], has been > > reworked to match the new devicetree binding which no longer describes > > each regulator as a separate device. > > [1] https://lore.kernel.org/r/1655200111-18357-8-git-send-email-quic_c_skakit@xxxxxxxxxxx > > Make it Link: tag? > > Link: URL [1] Sure. > > [ johan: rework probe to match new binding, amend commit message and > > Kconfig entry] > > Wouldn't be better on one line? Now you're really nit picking. ;) I think I prefer to stay within 72 columns. > + array_size.h > + bits.h Ok. > > +#include <linux/device.h> > > > +#include <linux/kernel.h> > > What is this header for? Probably the ones that are not explicitly included. > + math.h > > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/regulator/driver.h> > > + asm/byteorder.h Ok, thanks. > > +static int pm8008_regulator_get_voltage(struct regulator_dev *rdev) > > +{ > > + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev); > > + __le16 mV; > > + int uV; > > + > > + regmap_bulk_read(pm8008_reg->regmap, > > + LDO_VSET_LB_REG(pm8008_reg->base), (void *)&mV, 2); > > Why casting? I tried not change things in the v15 from Qualcomm that I based this on. I couldn't help cleaning up a few things in probe, which I was touching anyway, but I left it there. I'll drop the unnecessary cast. > > + uV = le16_to_cpu(mV) * 1000; > > + return (uV - pm8008_reg->rdesc.min_uV) / pm8008_reg->rdesc.uV_step; > > +} > > + > > +static inline int pm8008_write_voltage(struct pm8008_regulator *pm8008_reg, > > + int mV) > > +{ > > + __le16 vset_raw; > > + > > + vset_raw = cpu_to_le16(mV); > > Can be joined to a single line. Sure. > > + return regmap_bulk_write(pm8008_reg->regmap, > > + LDO_VSET_LB_REG(pm8008_reg->base), > > + (const void *)&vset_raw, sizeof(vset_raw)); > > Why casting? Same answer as above. Will drop. > > +} > > ... > > > +static int pm8008_regulator_set_voltage(struct regulator_dev *rdev, > > + unsigned int selector) > > +{ > > + struct pm8008_regulator *pm8008_reg = rdev_get_drvdata(rdev); > > + int rc, mV; > > + > > + rc = regulator_list_voltage_linear_range(rdev, selector); > > + if (rc < 0) > > + return rc; > > + > > + /* voltage control register is set with voltage in millivolts */ > > + mV = DIV_ROUND_UP(rc, 1000); > > > + rc = pm8008_write_voltage(pm8008_reg, mV); > > + if (rc < 0) > > + return rc; > > + > > + return 0; > > return pm8008_write_voltage(pm8008_reg, mV); Possibly, but I tend to prefer explicit error paths. > > +} > > > + > > + regmap = dev_get_regmap(dev->parent, "secondary"); > > + if (!regmap) > > + return -EINVAL; > > + > > + for (i = 0; i < ARRAY_SIZE(reg_data); i++) { > > + pm8008_reg = devm_kzalloc(dev, sizeof(*pm8008_reg), GFP_KERNEL); > > + if (!pm8008_reg) > > + return -ENOMEM; > > + > > + pm8008_reg->regmap = regmap; > > + pm8008_reg->base = reg_data[i].base; > > + > > + /* get slew rate */ > > + rc = regmap_bulk_read(pm8008_reg->regmap, > > + LDO_STEPPER_CTL_REG(pm8008_reg->base), &val, 1); > > + if (rc < 0) { > > + dev_err(dev, "failed to read step rate: %d\n", rc); > > + return rc; > > return dev_err_probe(...); Nah, regmap won't trigger a probe deferral. > > +static struct platform_driver pm8008_regulator_driver = { > > + .driver = { > > + .name = "qcom-pm8008-regulator", > > + }, > > + .probe = pm8008_regulator_probe, > > +}; > > > + > > Unneeded blank line. I noticed that one too, but such things are up the author to decide. > > +module_platform_driver(pm8008_regulator_driver); > > ... > > > +MODULE_ALIAS("platform:qcom-pm8008-regulator"); > > Use ID table instead. No, the driver is not using an id-table for matching so the alias is needed for module auto-loading. Johan
