Re: [PATCH] Adding a support for Skyworks SKY81452

From: Tobias Klauser
Date: Thu Aug 07 2014 - 08:34:54 EST

Next message: Tomas Henzl: "Re: [PATCH v1.3 1/18] arcmsr: Revised interrupt service routine relate function to fix bug"
Previous message: Joonsoo Kim: "Re: [PATCH v2 5/8] mm/isolation: change pageblock isolation logic to fix freepage counting bugs"
In reply to: Lee Jones: "Re: [PATCH] Adding a support for Skyworks SKY81452"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2014-08-07 at 10:05:38 +0200, Gyungoh Yoo <gyungoh@xxxxxxxxx> wrote:
> Signed-off-by: Gyungoh Yoo <jack.yoo@xxxxxxxxxxxxxxx>
> ---
> 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

[...]

> diff --git a/Documentation/devicetree/bindings/mfd/sky81452.txt b/Documentation/devicetree/bindings/mfd/sky81452.txt
> new file mode 100644
> index 0000000..18dd6ac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/sky81452.txt
> @@ -0,0 +1,24 @@
> +SKY81452 bindings
> +
> +Required properties:
> +- compatible : Must be "sky,sky81452"
> +
> +Required child nodes:
> +- backlight : container node for backlight following the binding
> + in video/backlight/sky81452-backlight.txt
> +- regulator : container node for regulators following the binding
> + in regulator/sky81452-regulator.txt
> +
> +Example:
> +
> + sky81452@2C {
> + compatible = "sky,sky81452";
> +
> + backlight {
> + ...
> + };
> +
> + regulator {
> + ...
> + };
> + };

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

[...]

> diff --git a/drivers/mfd/sky81452.c b/drivers/mfd/sky81452.c
> new file mode 100644
> index 0000000..e09552f
> --- /dev/null
> +++ b/drivers/mfd/sky81452.c
> @@ -0,0 +1,115 @@

[...]

> +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 <jack.yoo@xxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("1.0");

[...]

> diff --git a/drivers/regulator/sky81452-regulator.c b/drivers/regulator/sky81452-regulator.c
> new file mode 100644
> index 0000000..15a7c0a
> --- /dev/null
> +++ b/drivers/regulator/sky81452-regulator.c
> @@ -0,0 +1,127 @@

[...]

> +#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 <jack.yoo@xxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("1.0");

[...]

> diff --git a/drivers/video/backlight/sky81452-backlight.c b/drivers/video/backlight/sky81452-backlight.c
> new file mode 100644
> index 0000000..1eb5706
> --- /dev/null
> +++ b/drivers/video/backlight/sky81452-backlight.c
> @@ -0,0 +1,333 @@

[...]

> +#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 <jack.yoo@xxxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("1.0");
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Tomas Henzl: "Re: [PATCH v1.3 1/18] arcmsr: Revised interrupt service routine relate function to fix bug"
Previous message: Joonsoo Kim: "Re: [PATCH v2 5/8] mm/isolation: change pageblock isolation logic to fix freepage counting bugs"
In reply to: Lee Jones: "Re: [PATCH] Adding a support for Skyworks SKY81452"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]