lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 7 Aug 2014 14:34:39 +0200
From:	Tobias Klauser <tklauser@...tanz.ch>
To:	Gyungoh Yoo <gyungoh@...il.com>
Cc:	rdunlap@...radead.org, robh+dt@...nel.org, pawel.moll@....com,
	mark.rutland@....com, ijc+devicetree@...lion.org.uk,
	galak@...eaurora.org, jg1.han@...sung.com, cooloney@...il.com,
	plagnioj@...osoft.com, tomi.valkeinen@...com,
	grant.likely@...aro.org, sameo@...ux.intel.com,
	lee.jones@...aro.org, lgirdwood@...il.com, broonie@...nel.org,
	jack.yoo@...worksinc.com, florian.vaussard@...l.ch,
	thierry.reding@...il.com, jason@...edaemon.net, andrew@...n.ch,
	silvio.fricke@...il.com, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-fbdev@...r.kernel.org
Subject: Re: [PATCH] Adding a support for Skyworks SKY81452

On 2014-08-07 at 10:05:38 +0200, Gyungoh Yoo <gyungoh@...il.com> wrote:
> Signed-off-by: Gyungoh Yoo <jack.yoo@...worksinc.com>
> ---
>  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@...worksinc.com>");
> +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@...worksinc.com>");
> +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@...worksinc.com>");
> +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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists