[PATCH 07/14] regulator: SY8106A regulator driver

On Fri, Jun 24, 2016 at 3:20 AM,  [off-list ref] wrote:

Maybe you should also depend on OF since the driver is going to crippled
without any constraints set, or (OF || COMPILE_TEST) if you want some
compile test coverage.

Follow the existing ordering in the Makefile.

Do you need this one?

+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>

And this one?

+#include <linux/regulator/of_regulator.h>
+#include <linux/regmap.h>

Sort alphabetically please.

+
+#define SY8106A_REG_VOUT1_SEL          0x01
+#define SY8106A_REG_VOUT_COM           0x02
+#define SY8106A_REG_VOUT1_SEL_MASK     0x7f
+#define SY8106A_DISABLE_REG            0x01

BIT(0) would be clearer.

+
+struct sy8106a {
+       struct regulator_dev *rdev;
+       struct regmap *regmap;
+};
+
+static const struct regmap_config sy8106a_regmap_config = {
+       .reg_bits = 8,
+       .val_bits = 8,
+};
+
+static int sy8106a_set_voltage_sel(struct regulator_dev *rdev, unsigned sel)
+{
+       return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
+                                 0xff, sel | 0x80);

Can you use .apply_bit / .apply_reg with regulator_set_voltage_sel_regmap?

+}
+
+static const struct regulator_ops sy8106a_ops = {
+       .is_enabled = regulator_is_enabled_regmap,
+       .set_voltage_sel = sy8106a_set_voltage_sel,
+       .set_voltage_time_sel = regulator_set_voltage_time_sel,
+       .get_voltage_sel = regulator_get_voltage_sel_regmap,
+       .list_voltage = regulator_list_voltage_linear,
+};
+
+/* Default limits measured in millivolts and milliamps */
+#define SY8106A_MIN_MV         680
+#define SY8106A_MAX_MV         1950
+#define SY8106A_STEP_MV                10
+
+static const struct regulator_desc sy8106a_reg = {
+       .name = "SY8106A",
+       .id = 0,
+       .ops = &sy8106a_ops,
+       .type = REGULATOR_VOLTAGE,
+       .n_voltages = ((SY8106A_MAX_MV - SY8106A_MIN_MV) / SY8106A_STEP_MV) + 1,
+       .min_uV = (SY8106A_MIN_MV * 1000),
+       .uV_step = (SY8106A_STEP_MV * 1000),
+       .vsel_reg = SY8106A_REG_VOUT1_SEL,
+       .vsel_mask = SY8106A_REG_VOUT1_SEL_MASK,
+       .enable_reg = SY8106A_REG_VOUT_COM,
+       .enable_mask = SY8106A_DISABLE_REG,
+       .disable_val = SY8106A_DISABLE_REG,
+       .enable_is_inverted = 1,
+       .owner = THIS_MODULE,
+};
+
+/*
+ * I2C driver interface functions
+ */
+static int sy8106a_i2c_probe(struct i2c_client *i2c,
+                           const struct i2c_device_id *id)
+{
+       struct sy8106a *chip;
+       struct device *dev = &i2c->dev;
+       struct regulator_dev *rdev = NULL;
+       struct regulator_config config = { };
+       unsigned int selector;
+       int error;
+
+       chip = devm_kzalloc(&i2c->dev, sizeof(struct sy8106a), GFP_KERNEL);
+       if (!chip)
+               return -ENOMEM;
+
+       chip->regmap = devm_regmap_init_i2c(i2c, &sy8106a_regmap_config);
+       if (IS_ERR(chip->regmap)) {
+               error = PTR_ERR(chip->regmap);
+               dev_err(&i2c->dev, "Failed to allocate register map: %d\n",
+                       error);
+               return error;
+       }
+
+       config.dev = &i2c->dev;
+       config.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg);

Check for possible failures?

+       config.driver_data = chip;
+       config.regmap = chip->regmap;
+       config.of_node = dev->of_node;
+
+       /* Probe regulator */
+       error = regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, &selector);
+       dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));
+       if (error) {
+               dev_err(&i2c->dev, "Failed to read voltage at probe time: %d\n", error);
+               return error;
+       }
+
+       rdev = devm_regulator_register(&i2c->dev, &sy8106a_reg, &config);
+       if (IS_ERR(rdev)) {
+               dev_err(&i2c->dev, "Failed to register SY8106A regulator\n");
+               return PTR_ERR(rdev);
+       }
+
+       chip->rdev = rdev;
+
+       i2c_set_clientdata(i2c, chip);
+
+       return 0;
+}
+
+static const struct i2c_device_id sy8106a_i2c_id[] = {
+       {"sy8106a", 0},
+       {},
+};
+

Extra line.

+MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);

IMHO probing explicitly through DT (of_device_id) would be better.


Regards
ChenYu

+
+static struct i2c_driver sy8106a_regulator_driver = {
+       .driver = {
+               .name = "sy8106a",
+       },
+       .probe = sy8106a_i2c_probe,
+       .id_table = sy8106a_i2c_id,
+};
+
+module_i2c_driver(sy8106a_regulator_driver);
+
+MODULE_AUTHOR("Ond?ej Jirman [off-list ref]");
+MODULE_DESCRIPTION("Regulator device driver for Silergy SY8106A");
+MODULE_LICENSE("GPL v2");
--
2.9.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel