Message-id: <000001cf895d$322e4250$968ac6f0$%han@samsung.com>
Date:	Mon, 16 Jun 2014 21:19:18 +0900
From:	Jingoo Han <jg1.han@...sung.com>
To:	'Daniel Jeong' <gshark.jeong@...il.com>
Cc:	'Daniel Jeong' <daniel.jeong@...com>, linux-kernel@...r.kernel.org,
	linux-fbdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-doc@...r.kernel.org, 'Bryan Wu' <cooloney@...il.com>,
	'Lee Jones' <lee.jones@...aro.org>,
	'Jean-Christophe Plagniol-Villard' <plagnioj@...osoft.com>,
	'Tomi Valkeinen' <tomi.valkeinen@...com>,
	'Grant Likely' <grant.likely@...aro.org>,
	'Rob Herring' <robh+dt@...nel.org>,
	'Randy Dunlap' <rdunlap@...radead.org>,
	'Jingoo Han' <jg1.han@...sung.com>
Subject: Re: [RFC 1/2] backlight: add new tps611xx backlight driver

On Monday, June 16, 2014 11:17 AM, Daniel Jeong wrote:
> 
> This driver a general version for tps611xx backlgiht chips of TI.
> It supports tps61158, tps61161, tps61163 and tps61165 backlight driver
> based on EasyScale protocol.

"EasyScale" protocol is a TI-specific protocol. How about adding more
detailed description as below? I referred to the datasheet of tps61158.

based on EasyScale protocol (1-Wire Control Interface). The protocol
consists of a device specific address byte and a data byte. The device
specific address byte is fixed to 58 hex. The data byte consists of
five bits for information, two address bits ("00"), and the RFA bit.

> 
> Signed-off-by: Daniel Jeong <gshark.jeong@...il.com>
> ---
>  drivers/video/backlight/Kconfig           |    7 +
>  drivers/video/backlight/Makefile          |    1 +
>  drivers/video/backlight/tps611xx_bl.c     |  486 +++++++++++++++++++++++++++++
>  include/linux/platform_data/tps611xx_bl.h |   30 ++
>  4 files changed, 524 insertions(+)
>  create mode 100644 drivers/video/backlight/tps611xx_bl.c
>  create mode 100644 include/linux/platform_data/tps611xx_bl.h
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 5a3eb2e..c779a85 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -418,6 +418,13 @@ config BACKLIGHT_TPS65217
>  	  If you have a Texas Instruments TPS65217 say Y to enable the
>  	  backlight driver.
> 
> +config BACKLIGHT_TPS611xx
> +	tristate "TPS611xx Backlight"
> +	depends on BACKLIGHT_CLASS_DEVICE && GPIOLIB
> +	help
> +	  This supports TI TPS61158, TPS61161, TPS61163 and TPS61165
> +	  backlight driver based on EasyScale Protocol.
> +
>  config BACKLIGHT_AS3711
>  	tristate "AS3711 Backlight"
>  	depends on BACKLIGHT_CLASS_DEVICE && MFD_AS3711
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index bb82002..44f1641 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -52,4 +52,5 @@ obj-$(CONFIG_BACKLIGHT_PWM)		+= pwm_bl.o
>  obj-$(CONFIG_BACKLIGHT_SAHARA)		+= kb3886_bl.o
>  obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
>  obj-$(CONFIG_BACKLIGHT_TPS65217)	+= tps65217_bl.o
> +obj-$(CONFIG_BACKLIGHT_TPS611xx)	+= tps611xx_bl.o
>  obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
> diff --git a/drivers/video/backlight/tps611xx_bl.c b/drivers/video/backlight/tps611xx_bl.c
> new file mode 100644
> index 0000000..4b76cdd
> --- /dev/null
> +++ b/drivers/video/backlight/tps611xx_bl.c
> @@ -0,0 +1,486 @@
> +/*
> + * Simple driver for Texas Instruments TPS611XX Backlight driver chip
> + *        using EasyScale Interface. It supports TPS61158, TPS61161,
> + *        TPS61163 and TPS61165.
> + *
> + * Copyright (C) 2014 Texas Instruments
> + * Author: Daniel Jeong  <gshark.jeong@...il.com>
> + *	       Ldd Mlp <ldd-mlp@...t.ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/fb.h>
> +#include <linux/gpio.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_data/tps611xx_bl.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define CMD_FORWARD 0
> +#define CMD_BACKWARD 1
> +
> +enum tps611xx_id {
> +	TPS61158_ID = 0,
> +	TPS61161_ID,
> +	TPS61163_ID,
> +	TPS61165_ID,
> +};
> +
> +/*
> + * easyscale time spec
> + * @es_delay : es delay time(ns)
> + * @es_det   : es detection time(ns)
> + * @start    : start time of data stream(ns)
> + * @eos      : end time of data stream(ns)
> + * @reset    : ic shutdown time(ms)
> + * @logic_1_low : low time high bit(ns)
> + * @logic_0_low : low time low bit(ns)
> + * @ackn        : duation of ack condistion(ns)
> + * @ack_poll    : ack polling duration(ns)
> + */
> +struct tps611xx_time {
> +	unsigned int es_delay;
> +	unsigned int es_det;
> +	unsigned int start;
> +	unsigned int eos;
> +	unsigned int reset;
> +	unsigned int logic_1_low;
> +	unsigned int logic_0_low;
> +	unsigned int ackn;
> +	unsigned int ack_poll;
> +};
> +
> +/*
> + * @seq : sequence of data transfer
> + * @size: size of data
> + * @brt_max : max brightness
> + * @brt_bmask : bit mask of dimming bits
> + * @rfa_bmask : bit mask of request of ack

s/request of ack/request for ack ?

> + */
> +struct tps611xx_command {
> +	int seq;
> +	int size;
> +	int brt_max;
> +	int brt_bmask;
> +	int rfa_bmask;
> +};
> +
> +/*
> + * @id : product id
> + * @name : product name
> + * @addr : device address
> + * @cmd  : es command info
> + * @time : es time info
> + */
> +struct tps611xx_esdata {
> +	enum tps611xx_id id;
> +	char *name;
> +	int addr;
> +	struct tps611xx_command cmd;
> +	struct tps611xx_time time;
> +};
> +
> +struct tps611xx_bl_data {
> +	struct device *dev;
> +	struct backlight_device *bled;
> +	struct tps611xx_platform_data *pdata;
> +
> +	/*
> +	 * @rfa_en : acknowlege request enable

s/acknowledge/acknowledge

How about the following?

+	 * @rfa_en : request for acknowledge enable

> +	 * @en_gpio: enable pin gpio no.
> +	 * @esdata : easyscale data
> +	 */
> +	int rfa_en;
> +	unsigned int en_gpio;
> +	const struct tps611xx_esdata *esdata;
> +};
> +
> +static struct tps611xx_esdata tps611xx_info[] = {
> +	[TPS61158_ID] = {
> +			 .id = TPS61158_ID,
> +			 .name = "tps61158",
> +			 .addr = 0x5800,
> +			 .cmd = {
> +				 .seq = CMD_FORWARD,
> +				 .size = 16,
> +				 .brt_max = 31,
> +				 .brt_bmask = 0x1f,
> +				 .rfa_bmask = 0x80},

Would you keep the coding style? The following looks better.

+				 .rfa_bmask = 0x80
+			},


> +			.time = {
> +				  .es_delay = 100000,
> +				  .es_det = 450000,
> +				  .start = 3500,
> +				  .eos = 3500,
> +				  .reset = 4,
> +				  .logic_1_low = 5000,
> +				  .logic_0_low = 15000,
> +				  .ackn = 900000,
> +				  .ack_poll = 2000},
> +			 },
> +
> +	[TPS61161_ID] = {
> +			 .id = TPS61161_ID,
> +			 .name = "tps61161",
> +			 .addr = 0x7200,
> +			 .cmd = {
> +				 .seq = CMD_FORWARD,
> +				 .size = 16,
> +				 .brt_max = 31,
> +				 .brt_bmask = 0x1f,
> +				 .rfa_bmask = 0x80},
> +			 .time = {
> +				  .es_delay = 120000,
> +				  .es_det = 280000,
> +				  .start = 2000,
> +				  .eos = 2000,
> +				  .reset = 3,
> +				  .logic_1_low = 3000,
> +				  .logic_0_low = 7000,
> +				  .ackn = 512000,
> +				  .ack_poll = 2000},
> +			 },
> +
> +	[TPS61163_ID] = {
> +			 .id = TPS61163_ID,
> +			 .name = "tps61163",
> +			 .addr = 0x8F0000,
> +			 .cmd = {
> +				 .seq = CMD_BACKWARD,
> +				 .size = 24,
> +				 .brt_max = 511,
> +				 .brt_bmask = 0x1ff,
> +				 .rfa_bmask = 0x400},
> +			 .time = {
> +				  .es_delay = 100000,
> +				  .es_det = 260000,
> +				  .start = 2000,
> +				  .eos = 2000,
> +				  .reset = 3,
> +				  .logic_1_low = 3000,
> +				  .logic_0_low = 7000,
> +				  .ackn = 512000,
> +				  .ack_poll = 2000},
> +			 },
> +
> +	[TPS61165_ID] = {
> +			 .id = TPS61165_ID,
> +			 .name = "tps61165",
> +			 .addr = 0x7200,
> +			 .cmd = {
> +				 .seq = CMD_FORWARD,
> +				 .size = 16,
> +				 .brt_max = 31,
> +				 .brt_bmask = 0x1f,
> +				 .rfa_bmask = 0x80},
> +			 .time = {
> +				  .es_delay = 120000,
> +				  .es_det = 280000,
> +				  .start = 4000,
> +				  .eos = 4000,
> +				  .reset = 3,
> +				  .logic_1_low = 3000,
> +				  .logic_0_low = 7000,
> +				  .ackn = 512000,
> +				  .ack_poll = 2000},
> +			 },
> +};
> +
> +static int tps611xx_bl_update_status(struct backlight_device *bl)
> +{
> +	struct tps611xx_bl_data *pchip = bl_get_data(bl);
> +	const struct tps611xx_esdata *esdata = pchip->esdata;
> +	int data_in, t_low, t_logic, max_bmask;
> +	unsigned long flags;
> +
> +	data_in = esdata->addr | (bl->props.brightness & esdata->cmd.brt_bmask);
> +	if (pchip->rfa_en)
> +		data_in |= esdata->cmd.rfa_bmask;
> +
> +	max_bmask = 0x1 << esdata->cmd.size;
> +	t_logic = esdata->time.logic_1_low + esdata->time.logic_0_low;
> +
> +	local_irq_save(flags);
> +	/* t_start : 2us high before data byte */
> +	gpio_direction_output(pchip->en_gpio, 1);
> +	ndelay(esdata->time.start);
> +
> +	/* forward command transfer */
> +	if (esdata->cmd.seq == CMD_FORWARD) {
> +		int addr_bmask = max_bmask >> 8;
> +
> +		for (max_bmask >>= 1; max_bmask > 0x0; max_bmask >>= 1) {
> +			if (data_in & max_bmask)
> +				t_low = esdata->time.logic_1_low;
> +			else
> +				t_low = esdata->time.logic_0_low;
> +
> +			gpio_direction_output(pchip->en_gpio, 0);
> +			ndelay(t_low);
> +			gpio_direction_output(pchip->en_gpio, 1);
> +			ndelay(t_logic - t_low);
> +
> +			if (max_bmask == addr_bmask) {
> +				gpio_direction_output(pchip->en_gpio, 0);
> +				/* t_eos : low after address byte */
> +				ndelay(esdata->time.eos);
> +				gpio_direction_output(pchip->en_gpio, 1);
> +				/* t_start : high before data byte */
> +				ndelay(esdata->time.start);
> +			}
> +		}
> +	} else {
> +		/* backward command tansfer */
> +		int bmask;
> +
> +		for (bmask = 0x01; bmask < max_bmask; bmask <<= 1) {
> +			if (data_in & bmask)
> +				t_low = esdata->time.logic_1_low;
> +			else
> +				t_low = esdata->time.logic_0_low;
> +
> +			gpio_direction_output(pchip->en_gpio, 0);
> +			ndelay(t_low);
> +			gpio_direction_output(pchip->en_gpio, 1);
> +			ndelay(t_logic - t_low);
> +		}
> +	}
> +
> +	/*
> +	 * t_eos : low after address byte
> +	 * t_ackVal is also t_eos
> +	 */
> +	gpio_direction_output(pchip->en_gpio, 0);
> +	ndelay(esdata->time.eos);
> +
> +	/* RFA management  */
> +	if (pchip->rfa_en) {
> +		int max_ack_time = esdata->time.ackn;
> +		/* set input */
> +		gpio_direction_input(pchip->en_gpio);
> +		/* read acknowledge from chip */
> +		while (max_ack_time > 0) {
> +			if (gpio_get_value(pchip->en_gpio) == 0)
> +				break;
> +			max_ack_time -= esdata->time.ack_poll;
> +		}
> +		if (max_ack_time <= 0)
> +			dev_err(pchip->dev,
> +				"easyscale : no ack from %s\n", esdata->name);
> +		else
> +			ndelay(max_ack_time);
> +	}
> +	gpio_direction_output(pchip->en_gpio, 1);
> +	local_irq_restore(flags);
> +
> +	return bl->props.brightness;
> +}
> +
> +static int tps611xx_bl_get_brightness(struct backlight_device *bl)
> +{
> +	return bl->props.brightness;
> +}
> +
> +static const struct backlight_ops tps611xx_bl_ops = {
> +	.update_status = tps611xx_bl_update_status,
> +	.get_brightness = tps611xx_bl_get_brightness,
> +};
> +
> +static ssize_t tps611xx_enable_store(struct device *dev,
> +				     struct device_attribute *devAttr,
> +				     const char *buf, size_t size)
> +{
> +	struct tps611xx_bl_data *pchip = dev_get_drvdata(dev);
> +	const struct tps611xx_esdata *esdata = pchip->esdata;
> +	unsigned long flags;
> +	unsigned int input;
> +	int ret;
> +
> +	ret = kstrtouint(buf, 10, &input);
> +	if (ret)
> +		return -EINVAL;
> +
> +	local_irq_save(flags);
> +	if (input == 0) {
> +		/* chip disable */
> +		gpio_direction_output(pchip->en_gpio, 0);
> +		/* low more than reset ms to reset */
> +		mdelay(esdata->time.reset);
> +	} else {
> +		/* easyscale detection window */
> +		gpio_direction_output(pchip->en_gpio, 1);
> +		ndelay(esdata->time.es_delay);
> +		gpio_direction_output(pchip->en_gpio, 0);
> +		ndelay(esdata->time.es_det);
> +		gpio_direction_output(pchip->en_gpio, 1);
> +	}
> +	local_irq_restore(flags);
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR(enable, S_IWUSR, NULL, tps611xx_enable_store);
> +
> +#ifdef CONFIG_OF
> +static struct of_device_id tps611xx_backlight_of_match[] = {

Please add 'const' as below.

+static const struct of_device_id tps611xx_backlight_of_match[] = {

> +	{.compatible = "ti,tps61158_bl", .data = &tps611xx_info[TPS61158_ID]},
> +	{.compatible = "ti,tps61161_bl", .data = &tps611xx_info[TPS61161_ID]},
> +	{.compatible = "ti,tps61163_bl", .data = &tps611xx_info[TPS61163_ID]},
> +	{.compatible = "ti,tps61165_bl", .data = &tps611xx_info[TPS61165_ID]},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(of, tps611xx_backlight_of_match);
> +
> +static int tps611xx_backlight_parse_dt(struct tps611xx_bl_data *pchip)
> +{
> +	struct device *dev = pchip->dev;
> +	struct device_node *node = dev->of_node;
> +	const struct of_device_id *of_id =
> +	    of_match_device(tps611xx_backlight_of_match, dev);
> +	u32 value;
> +	int ret;
> +
> +	if (!node)
> +		return -ENODEV;
> +
> +	if (!of_id || !of_id->data) {
> +		dev_err(dev, "Failed to find tps611xx chip id\n");
> +		return -EFAULT;
> +	}
> +	pchip->esdata = of_id->data;
> +
> +	ret = of_property_read_u32(node, "en_gpio_num", &value);
> +	if (ret < 0)
> +		return ret;
> +	pchip->en_gpio = value;
> +	ret = of_property_read_u32(node, "rfa_en", &value);
> +	if (ret < 0)
> +		return ret;
> +	pchip->rfa_en = value;
> +
> +	return 0;
> +}
> +#else
> +static int tps611xx_backlight_parse_dt(struct tps611xx_bl_data *pchip)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
> +static int tps611xx_backlight_probe(struct platform_device *pdev)
> +{
> +	struct tps611xx_bl_data *pchip;
> +	struct backlight_properties props;
> +	const struct tps611xx_esdata *esdata;
> +	struct tps611xx_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	unsigned long flags;
> +	int ret;
> +
> +	pchip = devm_kzalloc(&pdev->dev,
> +			     sizeof(struct tps611xx_bl_data), GFP_KERNEL);
> +	if (pchip == NULL)
> +		return -ENOMEM;
> +	pchip->dev = &pdev->dev;
> +
> +	if (pdata == NULL) {
> +		ret = tps611xx_backlight_parse_dt(pchip);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		pchip->rfa_en = pdata->rfa_en;
> +		pchip->en_gpio = pdata->en_gpio_num;
> +		pchip->esdata = (const struct tps611xx_esdata *)
> +		    platform_get_device_id(pdev)->driver_data;
> +	}
> +	esdata = pchip->esdata;
> +
> +	memset(&props, 0, sizeof(struct backlight_properties));
> +	props.brightness = esdata->cmd.brt_max;
> +	props.max_brightness = esdata->cmd.brt_max;
> +	props.type = BACKLIGHT_RAW;
> +	pchip->bled =
> +	    devm_backlight_device_register(pchip->dev, TPS611XX_NAME,
> +					   pchip->dev, pchip,
> +					   &tps611xx_bl_ops, &props);
> +	if (IS_ERR(pchip->bled))
> +		return PTR_ERR(pchip->bled);
> +
> +	/* for enable/disable */
> +	ret = device_create_file(&(pchip->bled->dev), &dev_attr_enable);
> +	if (ret < 0) {
> +		dev_err(pchip->dev, "failed : add sysfs entries\n");
> +		return ret;
> +	}
> +	platform_set_drvdata(pdev, pchip);
> +
> +	/* EasyScale init */
> +	ret = gpio_request_one(pchip->en_gpio, GPIOF_OUT_INIT_HIGH, "tps611xx");
> +	if (ret) {
> +		device_remove_file(&(pchip->bled->dev), &dev_attr_enable);
> +		dev_err(pchip->dev, "failed : get gpio %d\n", pchip->en_gpio);
> +		return ret;
> +	}
> +
> +	/*
> +	 * ES Detection Window
> +	 *   - ES detect delay
> +	 *   - ES detect time
> +	 */
> +	local_irq_save(flags);
> +	gpio_direction_output(pchip->en_gpio, 1);
> +	ndelay(esdata->time.es_delay);
> +	gpio_direction_output(pchip->en_gpio, 0);
> +	ndelay(esdata->time.es_det);
> +	gpio_direction_output(pchip->en_gpio, 1);
> +	local_irq_restore(flags);
> +	dev_info(pchip->dev,
> +		 "%s EasyScale is initialized\n", pchip->esdata->name);
> +	return 0;
> +}
> +
> +static int tps611xx_backlight_remove(struct platform_device *pdev)
> +{
> +	struct tps611xx_bl_data *pchip = platform_get_drvdata(pdev);
> +	const struct tps611xx_esdata *esdata = pchip->esdata;
> +
> +	device_remove_file(&(pchip->bled->dev), &dev_attr_enable);
> +	gpio_direction_output(pchip->en_gpio, 0);
> +	mdelay(esdata->time.reset);
> +	return 0;
> +}
> +
> +static const struct platform_device_id tps611xx_id_table[] = {
> +	{TPS61158_NAME, (unsigned long)&tps611xx_info[TPS61158_ID]},
> +	{TPS61161_NAME, (unsigned long)&tps611xx_info[TPS61161_ID]},
> +	{TPS61163_NAME, (unsigned long)&tps611xx_info[TPS61163_ID]},
> +	{TPS61165_NAME, (unsigned long)&tps611xx_info[TPS61165_ID]},
> +	{}
> +};
> +
> +static struct platform_driver tps611xx_backlight_driver = {
> +	.driver = {
> +		   .name = TPS611XX_NAME,
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = of_match_ptr(tps611xx_backlight_of_match),
> +		   },
> +	.probe = tps611xx_backlight_probe,
> +	.remove = tps611xx_backlight_remove,
> +	.id_table = tps611xx_id_table,
> +};
> +
> +module_platform_driver(tps611xx_backlight_driver);
> +
> +MODULE_DESCRIPTION("EasyScale based tps611xx Backlight Driver");
> +MODULE_LICENSE("GPL");

How about adding 'GPL v2' instead of 'GPL'?

+ MODULE_LICENSE("GPL v2");

> +MODULE_ALIAS("platform:tps611xx_bl");
> diff --git a/include/linux/platform_data/tps611xx_bl.h b/include/linux/platform_data/tps611xx_bl.h
> new file mode 100644
> index 0000000..b7c4504
> --- /dev/null
> +++ b/include/linux/platform_data/tps611xx_bl.h
> @@ -0,0 +1,30 @@
> +/*
> + * Simple driver for Texas Instruments TPS61163a Backlight driver chip
> + * Copyright (C) 2014 Texas Instruments
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#ifndef __TPS611XX_H
> +#define __TPS611XX_H
> +
> +#define TPS611XX_NAME "tps611xx_bl"
> +#define TPS61158_NAME "tps61158_bl"
> +#define TPS61161_NAME "tps61161_bl"
> +#define TPS61163_NAME "tps61163_bl"
> +#define TPS61165_NAME "tps61165_bl"
> +
> +/* struct tps61163a platform data

Please use the proper coding style as below.

+/*
+ * struct tps61163a platform data

> + * @rfa_en : request for acknowledge
> + * @en_gpio_num : gpio number for en_pin
> + */
> +struct tps611xx_platform_data {
> +
> +	int rfa_en;
> +	unsigned int en_gpio_num;
> +};
> +
> +#endif /* __TPS61163A_H */

s/__TPS61163A_H/__TPS611XX_H


Best regards,
Jingoo Han

> --
> 1.7.9.5

--
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