Re: [PATCH] Adding a support for Skyworks SKY81452

From: Gyungoh Yoo
Date: Fri Aug 08 2014 - 02:25:06 EST

Next message: Joonsoo Kim: "Re: [PATCH v2 4/8] mm/isolation: close the two race problems related to pageblock isolation"
Previous message: Gyungoh Yoo: "Re: [PATCH] Adding a support for Skyworks SKY81452"
In reply to: Gyungoh Yoo: "Re: [PATCH] Adding a support for Skyworks SKY81452"
Next in thread: Thierry Reding: "Re: [PATCH] Adding a support for Skyworks SKY81452"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Thank you for your comments.
I will fix them and resubmit.

On Thu, Aug 07, 2014 at 02:34:39PM +0200, Tobias Klauser wrote:
> 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: Joonsoo Kim: "Re: [PATCH v2 4/8] mm/isolation: close the two race problems related to pageblock isolation"
Previous message: Gyungoh Yoo: "Re: [PATCH] Adding a support for Skyworks SKY81452"
In reply to: Gyungoh Yoo: "Re: [PATCH] Adding a support for Skyworks SKY81452"
Next in thread: Thierry Reding: "Re: [PATCH] Adding a support for Skyworks SKY81452"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]