Re: [PATCH] Adding a support for Skyworks SKY81452

On 2014-08-07 at 10:05:38 +0200, Gyungoh Yoo [off-list ref] wrote:

Signed-off-by: Gyungoh Yoo <redacted>
---
 Documentation/backlight/sky81452.txt               |  25 ++
 Documentation/devicetree/bindings/mfd/sky81452.txt |  24 ++
 .../bindings/regulator/sky81452-regulator.txt      |  16 +
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 .../video/backlight/sky81452-backlight.txt         |  20 ++
 drivers/mfd/Kconfig                                |  12 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/sky81452.c                             | 115 +++++++
 drivers/regulator/Kconfig                          |  11 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/sky81452-regulator.c             | 127 ++++++++
 drivers/video/backlight/Kconfig                    |  10 +
 drivers/video/backlight/Makefile                   |   1 +
 drivers/video/backlight/sky81452-backlight.c       | 333 +++++++++++++++++++++
 include/linux/mfd/sky81452.h                       |  31 ++
 include/linux/sky81452-backlight.h                 |  46 +++
 16 files changed, 774 insertions(+)
 create mode 100644 Documentation/backlight/sky81452.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/sky81452.txt
 create mode 100644 Documentation/devicetree/bindings/regulator/sky81452-regulator.txt
 create mode 100644 Documentation/devicetree/bindings/video/backlight/sky81452-backlight.txt
 create mode 100644 drivers/mfd/sky81452.c
 create mode 100644 drivers/regulator/sky81452-regulator.c
 create mode 100644 drivers/video/backlight/sky81452-backlight.c
 create mode 100644 include/linux/mfd/sky81452.h
 create mode 100644 include/linux/sky81452-backlight.h

[...]

Mixture of tabs and spaces in the example, please use tabs only. Same
for the example in sky81452-backlight.txt

[...]

[...]

+static int sky81452_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	const struct sky81452_platform_data *pdata;
+	struct device *dev = &client->dev;
+	struct regmap *map;
+
+#ifdef CONFIG_OF
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);

You still should check the return value in this case.

+#else
+	pdata = dev_get_platdata(dev);
+	if (unlikely(!pdata))

There's usually no need to use unlikely() in driver code, especially not
in non-critical functions like probe()

+		return -EINVAL;
+#endif
+
+	map = devm_regmap_init_i2c(client, &sky81452_config);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	i2c_set_clientdata(client, map);
+
+	return sky81452_register_devices(dev, pdata);
+}
+
+static int sky81452_remove(struct i2c_client *client)
+{
+	mfd_remove_devices(&client->dev);
+	return 0;
+}
+
+static const struct i2c_device_id sky81452_ids[] = {
+	{"sky81452", 0},

Space between braces and content please.

+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, sky81452_ids);
+
+#ifdef CONFIG_OF
+static const struct of_device_id sky81452_of_match[] = {
+	{.compatible = "sky,sky81452",},

Space between braces and content.

+	{ }
+};
+MODULE_DEVICE_TABLE(of, sky81452_of_match);
+#endif

The #ifdefery here is not needed since you use of_match_ptr below, whcih
will expand to NULL of CONFIG_OF is not set.

+
+static struct i2c_driver sky81452_driver = {
+	.driver = {
+		.name = "sky81452",
+		.owner = THIS_MODULE,

No need to set this here, module_i2c_driver/i2c_register_driver will
take care of it.

+		.of_match_table = of_match_ptr(sky81452_of_match),
+	},
+	.probe = sky81452_probe,
+	.remove = sky81452_remove,
+	.id_table = sky81452_ids,
+};
+
+module_i2c_driver(sky81452_driver);
+
+MODULE_DESCRIPTION("Skyworks SKY81452 MFD driver");
+MODULE_AUTHOR("Gyungoh Yoo [off-list ref]");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1.0");

[...]

[...]

+#ifdef CONFIG_OF
+static struct regulator_init_data *sky81452_reg_parse_dt(struct device *dev)
+{
+	struct regulator_init_data *init_data;
+	struct device_node *np;
+
+	np = of_get_child_by_name(dev->parent->of_node, "regulator");
+	if (unlikely(!np)) {
+		dev_err(dev, "regulator node not found");
+		return NULL;
+	}
+
+	init_data = of_get_regulator_init_data(dev, np);
+
+	of_node_put(np);
+	return init_data;
+}
+#endif
+
+static int sky81452_reg_probe(struct platform_device *pdev)
+{
+	const struct regulator_init_data *init_data;
+	struct device *dev = &pdev->dev;
+	struct regulator_config config = { };
+	struct regulator_dev *rdev;
+
+#ifdef CONFIG_OF
+	init_data = sky81452_reg_parse_dt(dev);
+#else
+	init_data = dev_get_platdata(dev);
+#endif

Better make the implementation of sky81452_reg_parse_dt dependent on
CONFIG_OF and just return dev_get_platdata(dev) in case of !CONFIG_OF.
You could then also rename sky81452_reg_parse_dt to something like
sky81452_reg_get_init_data.

+	if (unlikely(!init_data))
+		return -EINVAL;
+
+	config.dev = dev;
+	config.init_data = init_data;
+	config.of_node = dev->of_node;
+	config.regmap = dev_get_drvdata(dev->parent);
+
+	rdev = devm_regulator_register(dev, &sky81452_reg, &config);
+	if (IS_ERR(rdev))
+		return PTR_ERR(rdev);
+
+	platform_set_drvdata(pdev, rdev);
+
+	return 0;
+}
+
+static struct platform_driver sky81452_reg_driver = {
+	.driver = {
+		.name = "sky81452-regulator",
+		.owner = THIS_MODULE,

Not needed, this will be set by
module_platform_driver/platform_driver_register

+	},
+	.probe = sky81452_reg_probe,
+};
+
+module_platform_driver(sky81452_reg_driver);
+
+MODULE_DESCRIPTION("Skyworks SKY81452 Regulator driver");
+MODULE_AUTHOR("Gyungoh Yoo [off-list ref]");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1.0");

[...]

[...]

+#ifdef CONFIG_OF
+static struct sky81452_bl_platform_data *sky81452_bl_parse_dt
+		(struct device *dev)
+{
+	struct device_node *np = dev->parent->of_node;
+	struct sky81452_bl_platform_data *pdata;
+	int ret;
+
+	np = of_get_child_by_name(dev->parent->of_node, "backlight");
+	if (unlikely(!np)) {
+		dev_err(dev, "backlight node not found");
+		return ERR_PTR(-EINVAL);
+	}
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (unlikely(!pdata)) {
+		of_node_put(np);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	of_property_read_string(np, "name", &pdata->name);
+	pdata->ignore_pwm = of_property_read_bool(np, "ignore-pwm");
+	pdata->dpwm_mode = of_property_read_bool(np, "dpwm-mode");
+	pdata->phase_shift = of_property_read_bool(np, "phase-shift");
+
+	pdata->gpio_enable = of_get_named_gpio(np, "gpio-enable", 0);
+	if (IS_ERR_VALUE(pdata->gpio_enable))
+		pdata->gpio_enable = -1;
+
+	ret = of_property_read_u32(np, "enable", &pdata->enable);
+	if (IS_ERR_VALUE(ret))
+		pdata->enable = SKY81452_EN >> CTZ(SKY81452_EN);
+
+	ret = of_property_read_u32(np, "short-detection-threshold",
+			&pdata->short_detection_threshold);
+	if (IS_ERR_VALUE(ret))
+		pdata->short_detection_threshold = 7;
+
+	ret = of_property_read_u32(np, "boost-current-limit",
+			&pdata->boost_current_limit);
+	if (IS_ERR_VALUE(ret))
+		pdata->boost_current_limit = 2750;
+
+	of_node_put(np);
+	return pdata;
+}
+#endif
+
+static int sky81452_bl_init_device(struct regmap *map,
+		struct sky81452_bl_platform_data *pdata)
+{
+	unsigned int value;
+
+	value = pdata->ignore_pwm ? SKY81452_IGPW : 0;
+	value |= pdata->dpwm_mode ? SKY81452_PWMMD : 0;
+	value |= pdata->phase_shift ? 0 : SKY81452_PHASE;
+
+	if (pdata->boost_current_limit = 2300)
+		value |= SKY81452_ILIM;
+	else if (pdata->boost_current_limit != 2720)
+		return -EINVAL;
+
+	if (pdata->short_detection_threshold < 4 ||
+			pdata->short_detection_threshold > 7)
+		return -EINVAL;
+	value |= (7 - pdata->short_detection_threshold) << CTZ(SKY81452_VSHRT);
+
+	return regmap_write(map, SKY81452_REG2, value);
+}
+
+static int sky81452_bl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct regmap *map = dev_get_drvdata(dev->parent);
+	struct sky81452_bl_platform_data *pdata;
+	struct backlight_device *bd;
+	struct backlight_properties props;
+	const char *name;
+	int ret;
+
+#ifdef CONFIG_OF
+	pdata = sky81452_bl_parse_dt(dev);
+	if (IS_ERR(pdata))
+		return PTR_ERR(pdata);
+	dev->platform_data = pdata;
+#else
+	pdata = dev_get_platdata(dev);
+	if (unlikely(!pdata))

No need to use unlikely() here.

+		return -EINVAL;
+#endif

Instead of the #ifdefery here, better define a function
sky81452_get_pdata() dependent on CONFIG_OF which will return the right
thing (i.e. what sky81452_bl_parse_dt currently does if CONFIG_OF is
defined and dev_get_platdata or ERR_PTR(-EINVAL) if it is not
defined). Then you could just do:

pdata = skysky81452_get_pdata(dev);
if (IS_ERR(pdata))
	return PTR_ERR(pdata);

+
+	if (pdata->gpio_enable >= 0) {
+		ret = devm_gpio_request_one(dev, pdata->gpio_enable,
+				GPIOF_OUT_INIT_HIGH, "sky81452-en");
+		if (IS_ERR_VALUE(ret))
+			return ret;
+	}
+
+	ret = sky81452_bl_init_device(map, pdata);
+	if (IS_ERR_VALUE(ret))
+		return ret;
+
+	memset(&props, 0, sizeof(props));
+	props.max_brightness = SKY81452_MAX_BRIGHTNESS,
+	name = pdata->name ? pdata->name : SKY81452_DEFAULT_NAME;
+	bd = devm_backlight_device_register(dev, name,
+			dev, map, &sky81452_bl_ops, &props);
+	if (IS_ERR(bd))
+		return PTR_ERR(bd);
+
+	platform_set_drvdata(pdev, bd);
+
+	ret  = sysfs_create_group(&bd->dev.kobj, &sky81452_bl_attr_group);
+	if (IS_ERR_VALUE(ret))
+		goto err;
+
+	return ret;
+err:
+	backlight_device_unregister(bd);
+	return ret;
+}
+
+static int sky81452_bl_remove(struct platform_device *pdev)
+{
+	const struct sky81452_bl_platform_data *pdata > +			dev_get_platdata(&pdev->dev);
+	struct backlight_device *bd = platform_get_drvdata(pdev);
+
+	sysfs_remove_group(&bd->dev.kobj, &sky81452_bl_attr_group);
+
+	bd->props.power = FB_BLANK_UNBLANK;
+	bd->props.brightness = 0;
+	backlight_update_status(bd);
+
+	if (pdata->gpio_enable >= 0)
+		gpio_set_value_cansleep(pdata->gpio_enable, 0);
+
+	return 0;
+}
+
+static struct platform_driver sky81452_bl_driver = {
+	.driver = {
+		.name = "sky81452-bl",
+		.owner = THIS_MODULE,

module_platform_driver/platform_driver_register take care of this.

+	},
+	.probe = sky81452_bl_probe,
+	.remove = sky81452_bl_remove,
+};
+
+module_platform_driver(sky81452_bl_driver);
+
+MODULE_DESCRIPTION("Skyworks SKY81452 backlight driver");
+MODULE_AUTHOR("Gyungoh Yoo [off-list ref]");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1.0");