* [PATCH v2 0/2] as3645a flash driver @ 2011-11-14 0:19 Laurent Pinchart 2011-11-14 0:19 ` [PATCH v2 1/2] v4l: Add over-current and indicator flash fault bits Laurent Pinchart 2011-11-14 0:19 ` [PATCH v2 2/2] as3645a: Add driver for LED flash controller Laurent Pinchart 0 siblings, 2 replies; 11+ messages in thread From: Laurent Pinchart @ 2011-11-14 0:19 UTC (permalink / raw) To: linux-media; +Cc: sakari.ailus, andriy.shevchenko Hi everybody, Here's the second version of the as3645a flash controller driver. Compared to v1, this version addresses Hans' comments, and removes the overheat protection code. The overheat protection implementation was specific to Nokia's use cases, and thus not ready for mainline. A more generic overheat protection implementation (possibly as a library usable by several flash controllers) can be integrated later. Laurent Pinchart (2): v4l: Add over-current and indicator flash fault bits as3645a: Add driver for LED flash controller Documentation/DocBook/media/v4l/controls.xml | 10 + drivers/media/video/Kconfig | 7 + drivers/media/video/Makefile | 1 + drivers/media/video/as3645a.c | 951 ++++++++++++++++++++++++++ include/linux/videodev2.h | 2 + include/media/as3645a.h | 69 ++ 6 files changed, 1040 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/as3645a.c create mode 100644 include/media/as3645a.h -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] v4l: Add over-current and indicator flash fault bits 2011-11-14 0:19 [PATCH v2 0/2] as3645a flash driver Laurent Pinchart @ 2011-11-14 0:19 ` Laurent Pinchart 2011-11-14 0:19 ` [PATCH v2 2/2] as3645a: Add driver for LED flash controller Laurent Pinchart 1 sibling, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2011-11-14 0:19 UTC (permalink / raw) To: linux-media; +Cc: sakari.ailus, andriy.shevchenko Flash controllers can report over-current and indicator fault conditions. Define flash fault control bits for them. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Documentation/DocBook/media/v4l/controls.xml | 10 ++++++++++ include/linux/videodev2.h | 2 ++ 2 files changed, 12 insertions(+), 0 deletions(-) diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml index 3bc5ee8..a978b88 100644 --- a/Documentation/DocBook/media/v4l/controls.xml +++ b/Documentation/DocBook/media/v4l/controls.xml @@ -3329,6 +3329,16 @@ interface and may change in the future.</para> <entry>The short circuit protection of the flash controller has been triggered.</entry> </row> + <row> + <entry><constant>V4L2_FLASH_FAULT_OVER_CURRENT</constant></entry> + <entry>Current in the LED power supply has exceeded the limit + specific to the flash controller.</entry> + </row> + <row> + <entry><constant>V4L2_FLASH_FAULT_INDICATOR</constant></entry> + <entry>The flash controller has detected a short or open + circuit condition on the indicator LED.</entry> + </row> </tbody> </entrytbl> </row> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 4b752d5..3d62631 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -1682,6 +1682,8 @@ enum v4l2_flash_strobe_source { #define V4L2_FLASH_FAULT_TIMEOUT (1 << 1) #define V4L2_FLASH_FAULT_OVER_TEMPERATURE (1 << 2) #define V4L2_FLASH_FAULT_SHORT_CIRCUIT (1 << 3) +#define V4L2_FLASH_FAULT_OVER_CURRENT (1 << 4) +#define V4L2_FLASH_FAULT_INDICATOR (1 << 5) #define V4L2_CID_FLASH_CHARGE (V4L2_CID_FLASH_CLASS_BASE + 11) #define V4L2_CID_FLASH_READY (V4L2_CID_FLASH_CLASS_BASE + 12) -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] as3645a: Add driver for LED flash controller 2011-11-14 0:19 [PATCH v2 0/2] as3645a flash driver Laurent Pinchart 2011-11-14 0:19 ` [PATCH v2 1/2] v4l: Add over-current and indicator flash fault bits Laurent Pinchart @ 2011-11-14 0:19 ` Laurent Pinchart 2011-11-14 9:34 ` Sakari Ailus 1 sibling, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2011-11-14 0:19 UTC (permalink / raw) To: linux-media; +Cc: sakari.ailus, andriy.shevchenko This patch adds the driver for the as3645a LED flash controller. This controller supports a high power led in flash and torch modes and an indicator light, sometimes also called privacy light. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Nayden Kanchev <nkanchev@mm-sol.com> Signed-off-by: Tuukka Toivonen <tuukkat76@gmail.com> Signed-off-by: Antti Koskipaa <antti.koskipaa@gmail.com> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com> Signed-off-by: Vimarsh Zutshi <vimarsh.zutshi@gmail.com> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com> Signed-off-by: David Cohen <dacohen@gmail.com> --- drivers/media/video/Kconfig | 7 + drivers/media/video/Makefile | 1 + drivers/media/video/as3645a.c | 951 +++++++++++++++++++++++++++++++++++++++++ include/media/as3645a.h | 69 +++ 4 files changed, 1028 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/as3645a.c create mode 100644 include/media/as3645a.h diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 4e8a0c4..ba27f89 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -533,6 +533,13 @@ config VIDEO_ADP1653 This is a driver for the ADP1653 flash controller. It is used for example in Nokia N900. +config VIDEO_AS3645A + tristate "AS3645A flash driver support" + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER + ---help--- + This is a driver for the AS3645A flash chip. It has build in control + for Flash, Torch and Indicator LEDs. + comment "Video improvement chips" config VIDEO_UPD64031A diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile index ddeaa6c..86aabd6 100644 --- a/drivers/media/video/Makefile +++ b/drivers/media/video/Makefile @@ -74,6 +74,7 @@ obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o obj-$(CONFIG_VIDEO_M5MOLS) += m5mols/ obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o obj-$(CONFIG_VIDEO_ADP1653) += adp1653.o +obj-$(CONFIG_VIDEO_AS3645A) += as3645a.o obj-$(CONFIG_SOC_CAMERA_IMX074) += imx074.o obj-$(CONFIG_SOC_CAMERA_MT9M001) += mt9m001.o diff --git a/drivers/media/video/as3645a.c b/drivers/media/video/as3645a.c new file mode 100644 index 0000000..4138eb7 --- /dev/null +++ b/drivers/media/video/as3645a.c @@ -0,0 +1,951 @@ +/* + * drivers/media/video/as3645a.c + * + * Copyright (C) 2008-2011 Nokia Corporation + * + * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.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. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA + * + * NOTES: + * - Inductor peak current limit setting fixed to 1.75A + * - VREF offset fixed to 0V + * + * TODO: + * - Check hardware FSTROBE control when sensor driver add support for this + * + */ + +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/mutex.h> + +#include <media/as3645a.h> +#include <media/v4l2-ctrls.h> +#include <media/v4l2-device.h> + +#define AS_TIMER_MS_TO_CODE(t) (((t) - 100) / 50) +#define AS_TIMER_CODE_TO_MS(c) (50 * (c) + 100) + +/* Register definitions */ + +/* Read-only Design info register: Reset state: xxxx 0001 */ +#define AS_DESIGN_INFO_REG 0x00 +#define AS_DESIGN_INFO_FACTORY(x) (((x) >> 4)) +#define AS_DESIGN_INFO_MODEL(x) ((x) & 0x0f) + +/* Read-only Version control register: Reset state: 0000 0000 + * for first engineering samples + */ +#define AS_VERSION_CONTROL_REG 0x01 +#define AS_VERSION_CONTROL_RFU(x) (((x) >> 4)) +#define AS_VERSION_CONTROL_VERSION(x) ((x) & 0x0f) + +/* Read / Write (Indicator and timer register): Reset state: 0000 1111 */ +#define AS_INDICATOR_AND_TIMER_REG 0x02 +#define AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT 0 +#define AS_INDICATOR_AND_TIMER_VREF_SHIFT 4 +#define AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT 6 + +/* Read / Write (Current set register): Reset state: 0110 1001 */ +#define AS_CURRENT_SET_REG 0x03 +#define AS_CURRENT_ASSIST_LIGHT_SHIFT 0 +#define AS_CURRENT_LED_DET_ON (1 << 3) +#define AS_CURRENT_FLASH_CURRENT_SHIFT 4 + +/* Read / Write (Control register): Reset state: 1011 0100 */ +#define AS_CONTROL_REG 0x04 +#define AS_CONTROL_MODE_SETTING_SHIFT 0 +#define AS_CONTROL_STROBE_ON (1 << 2) +#define AS_CONTROL_OUT_ON (1 << 3) +#define AS_CONTROL_EXT_TORCH_ON (1 << 4) +#define AS_CONTROL_STROBE_TYPE_EDGE (0 << 5) +#define AS_CONTROL_STROBE_TYPE_LEVEL (1 << 5) +#define AS_CONTROL_COIL_PEAK_SHIFT 6 + +/* Read only (D3 is read / write) (Fault and info): Reset state: 0000 x000 */ +#define AS_FAULT_INFO_REG 0x05 +#define AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT (1 << 1) +#define AS_FAULT_INFO_INDICATOR_LED (1 << 2) +#define AS_FAULT_INFO_LED_AMOUNT (1 << 3) +#define AS_FAULT_INFO_TIMEOUT (1 << 4) +#define AS_FAULT_INFO_OVER_TEMPERATURE (1 << 5) +#define AS_FAULT_INFO_SHORT_CIRCUIT (1 << 6) +#define AS_FAULT_INFO_OVER_VOLTAGE (1 << 7) + +/* Boost register */ +#define AS_BOOST_REG 0x0d +#define AS_BOOST_CURRENT_DISABLE (0 << 0) +#define AS_BOOST_CURRENT_ENABLE (1 << 0) + +/* Password register is used to unlock boost register writing */ +#define AS_PASSWORD_REG 0x0f +#define AS_PASSWORD_UNLOCK_VALUE 0x55 + +/* AS_CONTROL_EXT_TORCH_ON - on, 0 - off */ +#define TORCH_IN_STANDBY 0 + +#define AS3645A_FLASH_TIMEOUT_MIN 100000 /* us */ +#define AS3645A_FLASH_TIMEOUT_MAX 850000 +#define AS3645A_FLASH_TIMEOUT_STEP 50000 + +#define AS3645A_FLASH_INTENSITY_MIN 200 /* mA */ +#define AS3645A_FLASH_INTENSITY_MAX_1LED 500 +#define AS3645A_FLASH_INTENSITY_MAX_2LEDS 400 +#define AS3645A_FLASH_INTENSITY_STEP 20 + +#define AS3645A_TORCH_INTENSITY_MIN 20 /* mA */ +#define AS3645A_TORCH_INTENSITY_MAX 160 +#define AS3645A_TORCH_INTENSITY_STEP 20 + +#define AS3645A_INDICATOR_INTENSITY_MIN 0 /* uA */ +#define AS3645A_INDICATOR_INTENSITY_MAX 10000 +#define AS3645A_INDICATOR_INTENSITY_STEP 2500 + +enum as_mode { + AS_MODE_EXT_TORCH = 0 << AS_CONTROL_MODE_SETTING_SHIFT, + AS_MODE_INDICATOR = 1 << AS_CONTROL_MODE_SETTING_SHIFT, + AS_MODE_ASSIST = 2 << AS_CONTROL_MODE_SETTING_SHIFT, + AS_MODE_FLASH = 3 << AS_CONTROL_MODE_SETTING_SHIFT, +}; + +/* + * struct as3645a + * + * @subdev: V4L2 subdev + * @platform_data: Flash platform data + * @power_lock: Protects power_count + * @power_count: Power reference count + * @vref: VREF offset (0=0V, 1=+0.3V, 2=-0.3V, 3=+0.6V) + * @peak: Inductor peak current limit (0=1.25A, 1=1.5A, 2=1.75A, + * 3=2.0A) + * @led_mode: V4L2 flash LED mode + * @timeout: Flash timeout in microseconds + * @flash_current: Flash current (0=200mA ... 15=500mA). Maximum + * values are 400mA for two LEDs and 500mA for one LED. + * @assist_current: Torch/Assist light current (0=20mA, 1=40mA ... 7=160mA) + * @indicator_current: Indicator LED current (0=0mA, 1=2.5mA ... 4=10mA) + * @strobe_source: Flash strobe source (software or external) + */ +struct as3645a { + struct v4l2_subdev subdev; + struct as3645a_platform_data *platform_data; + + struct mutex power_lock; + int power_count; + + /* Static parameters */ + u8 vref; + u8 peak; + + /* Controls */ + struct v4l2_ctrl_handler ctrls; + + enum v4l2_flash_led_mode led_mode; + unsigned int timeout; + u8 flash_current; + u8 assist_current; + u8 indicator_current; + enum v4l2_flash_strobe_source strobe_source; +}; + +#define to_as3645a(sd) container_of(sd, struct as3645a, subdev) + +/* Return negative errno else zero on success */ +static int as3645a_write(struct as3645a *flash, u8 addr, u8 val) +{ + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); + int rval; + + rval = i2c_smbus_write_byte_data(client, addr, val); + + dev_dbg(&client->dev, "Write Addr:%02X Val:%02X %s\n", addr, val, + rval < 0 ? "fail" : "ok"); + + return rval; +} + +/* Return negative errno else a data byte received from the device. */ +static int as3645a_read(struct as3645a *flash, u8 addr) +{ + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); + int rval; + + rval = i2c_smbus_read_byte_data(client, addr); + + dev_dbg(&client->dev, "Read Addr:%02X Val:%02X %s\n", addr, rval, + rval < 0 ? "fail" : "ok"); + + return rval; +} + +/* ----------------------------------------------------------------------------- + * Hardware configuration and trigger + */ + +/* + * as3645a_set_config - Set flash configuration registers + * @flash: The flash + * + * Configure the hardware with flash, assist and indicator currents, as well as + * flash timeout. + * + * Return 0 on success, or a negative error code if an I2C communication error + * occurred. + */ +static int as3645a_set_config(struct as3645a *flash) +{ + int ret; + u8 val; + + val = (flash->flash_current << AS_CURRENT_FLASH_CURRENT_SHIFT) + | (flash->assist_current << AS_CURRENT_ASSIST_LIGHT_SHIFT) + | AS_CURRENT_LED_DET_ON; + + ret = as3645a_write(flash, AS_CURRENT_SET_REG, val); + if (ret < 0) + return ret; + + if (flash->strobe_source == V4L2_FLASH_STROBE_SOURCE_EXTERNAL) { + /* Use timeout to protect the flash in case the external + * strobe gets stuck. Minimum value 100 ms, maximum 850 ms. + */ + u32 timeout = DIV_ROUND_UP(flash->timeout, 1000); + timeout = max_t(u32, DIV_ROUND_UP(timeout, 50) * 50, 100); + val = AS_TIMER_MS_TO_CODE(timeout) + << AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT; + } else { + val = AS_TIMER_MS_TO_CODE(flash->timeout / 1000) + << AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT; + } + + val |= (flash->vref << AS_INDICATOR_AND_TIMER_VREF_SHIFT) + | ((flash->indicator_current ? flash->indicator_current - 1 : 0) + << AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT); + + return as3645a_write(flash, AS_INDICATOR_AND_TIMER_REG, val); +} + +/* + * as3645a_set_control - Set flash control register + * @flash: The flash + * @mode: Desired output mode + * @on: Desired output state + * + * Configure the hardware with output mode and state. + * + * Return 0 on success, or a negative error code if an I2C communication error + * occurred. + */ +static int +as3645a_set_control(struct as3645a *flash, enum as_mode mode, bool on) +{ + u8 reg; + + /* Configure output parameters and operation mode. */ + reg = (flash->peak << AS_CONTROL_COIL_PEAK_SHIFT) + | TORCH_IN_STANDBY + | (on ? AS_CONTROL_OUT_ON : 0) + | mode; + + if (flash->led_mode == V4L2_FLASH_LED_MODE_FLASH && + flash->strobe_source == V4L2_FLASH_STROBE_SOURCE_EXTERNAL) { + if (flash->platform_data->setup_ext_strobe) + flash->platform_data->setup_ext_strobe(1); + reg |= AS_CONTROL_STROBE_TYPE_LEVEL + | AS_CONTROL_STROBE_ON; + } + + return as3645a_write(flash, AS_CONTROL_REG, reg); +} + +/* + * as3645a_set_output - Configure output and operation mode + * @flash: Flash controller + * @strobe: Strobe the flash (only valid in flash mode) + * + * Turn the LEDs output on/off and set the operation mode based on the current + * parameters. + * + * The AS3645A can't control the indicator LED independently of the flash/torch + * LED. If the flash controller is in V4L2_FLASH_LED_MODE_NONE mode, set the + * chip to indicator mode. Otherwise set it to assist light (torch) or flash + * mode. + * + * In indicator and assist modes, turn the output on/off based on the indicator + * and torch currents. In software strobe flash mode, turn the output on/off + * based on the strobe parameter. + */ +static int as3645a_set_output(struct as3645a *flash, bool strobe) +{ + enum as_mode mode; + bool on; + int ret; + + switch (flash->led_mode) { + case V4L2_FLASH_LED_MODE_NONE: + on = flash->indicator_current != 0; + mode = AS_MODE_INDICATOR; + break; + case V4L2_FLASH_LED_MODE_TORCH: + on = true; + mode = AS_MODE_ASSIST; + break; + case V4L2_FLASH_LED_MODE_FLASH: + on = strobe; + mode = AS_MODE_FLASH; + break; + default: + return 0; + } + + /* Configure output parameters and operation mode. */ + ret = as3645a_set_control(flash, mode, on); + if (ret < 0) + return ret; + + return 0; +} + +/* ----------------------------------------------------------------------------- + * V4L2 controls + */ + +static int as3645a_read_fault(struct as3645a *flash) +{ + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); + int rval; + + /* NOTE: reading register clear fault status */ + rval = as3645a_read(flash, AS_FAULT_INFO_REG); + if (rval < 0) + return rval; + + if (rval & AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT) + dev_err(&client->dev, "Inductor Peak limit fault\n"); + + if (rval & AS_FAULT_INFO_INDICATOR_LED) + dev_err(&client->dev, "Indicator LED fault: " + "Short circuit or open loop\n"); + + dev_dbg(&client->dev, "%u connected LEDs\n", + rval & AS_FAULT_INFO_LED_AMOUNT ? 2 : 1); + + if (rval & AS_FAULT_INFO_TIMEOUT) + dev_err(&client->dev, "Timeout fault\n"); + + if (rval & AS_FAULT_INFO_OVER_TEMPERATURE) + dev_err(&client->dev, "Over temperature fault\n"); + + if (rval & AS_FAULT_INFO_SHORT_CIRCUIT) + dev_err(&client->dev, "Short circuit fault\n"); + + if (rval & AS_FAULT_INFO_OVER_VOLTAGE) + dev_err(&client->dev, "Over voltage fault: " + "Indicates missing capacitor or open connection\n"); + + if (rval & ~AS_FAULT_INFO_INDICATOR_LED) + dev_dbg(&client->dev, "No faults, nice\n"); + + return rval; +} + +static int as3645a_get_ctrl(struct v4l2_ctrl *ctrl) +{ + struct as3645a *flash = + container_of(ctrl->handler, struct as3645a, ctrls); + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); + int fault; + + switch (ctrl->id) { + case V4L2_CID_FLASH_FAULT: + fault = as3645a_read_fault(flash); + if (fault < 0) + return fault; + + if (fault & AS_FAULT_INFO_SHORT_CIRCUIT) + ctrl->cur.val |= V4L2_FLASH_FAULT_SHORT_CIRCUIT; + if (fault & AS_FAULT_INFO_OVER_TEMPERATURE) + ctrl->cur.val |= V4L2_FLASH_FAULT_OVER_TEMPERATURE; + if (fault & AS_FAULT_INFO_TIMEOUT) + ctrl->cur.val |= V4L2_FLASH_FAULT_TIMEOUT; + if (fault & AS_FAULT_INFO_OVER_VOLTAGE) + ctrl->cur.val |= V4L2_FLASH_FAULT_OVER_VOLTAGE; + if (fault & AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT) + ctrl->cur.val |= V4L2_FLASH_FAULT_OVER_CURRENT; + if (fault & AS_FAULT_INFO_INDICATOR_LED) + ctrl->cur.val |= V4L2_FLASH_FAULT_INDICATOR; + break; + + default: + return -EINVAL; + } + + dev_dbg(&client->dev, "G_CTRL %08x:%d\n", ctrl->id, ctrl->cur.val); + + return 0; +} + +static int as3645a_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct as3645a *flash = + container_of(ctrl->handler, struct as3645a, ctrls); + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); + int ret; + + dev_dbg(&client->dev, "S_CTRL %08x:%d\n", ctrl->id, ctrl->val); + + /* If a control that doesn't apply to the current mode is modified, + * we store the value and return immediately. The setting will be + * applied when the LED mode is changed. Otherwise we apply the setting + * immediately. + */ + + switch (ctrl->id) { + case V4L2_CID_FLASH_LED_MODE: + if (flash->indicator_current) + return -EBUSY; + + ret = as3645a_set_config(flash); + if (ret < 0) + return ret; + + flash->led_mode = ctrl->val; + return as3645a_set_output(flash, false); + + case V4L2_CID_FLASH_STROBE_SOURCE: + flash->strobe_source = ctrl->val; + + /* Applies to flash mode only. */ + if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) + break; + + return as3645a_set_output(flash, false); + + case V4L2_CID_FLASH_STROBE: + if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) + return -EBUSY; + + return as3645a_set_output(flash, true); + + case V4L2_CID_FLASH_STROBE_STOP: + if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) + return -EBUSY; + + return as3645a_set_output(flash, false); + + case V4L2_CID_FLASH_TIMEOUT: + /* This is ugly. The step value is different depending on strobe + * mode, so only round the value when using I2C strobing. + */ + if (flash->strobe_source == V4L2_FLASH_STROBE_SOURCE_EXTERNAL && + flash->platform_data->set_strobe_width != NULL) { + flash->platform_data->set_strobe_width(ctrl->val); + } else { + if (ctrl->val < AS3645A_FLASH_TIMEOUT_MIN) + ctrl->val = AS3645A_FLASH_TIMEOUT_MIN; + ctrl->val = ctrl->val / AS3645A_FLASH_TIMEOUT_STEP + * AS3645A_FLASH_TIMEOUT_STEP; + } + flash->timeout = ctrl->val; + + /* Applies to flash mode only. */ + if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) + break; + + return as3645a_set_config(flash); + + case V4L2_CID_FLASH_INTENSITY: + flash->flash_current = (ctrl->val - AS3645A_FLASH_INTENSITY_MIN) + / AS3645A_FLASH_INTENSITY_STEP; + + /* Applies to flash mode only. */ + if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) + break; + + return as3645a_set_config(flash); + + case V4L2_CID_FLASH_TORCH_INTENSITY: + flash->assist_current = + (ctrl->val - AS3645A_TORCH_INTENSITY_MIN) + / AS3645A_TORCH_INTENSITY_STEP; + + /* Applies to torch mode only. */ + if (flash->led_mode != V4L2_FLASH_LED_MODE_TORCH) + break; + + return as3645a_set_config(flash); + + case V4L2_CID_FLASH_INDICATOR_INTENSITY: + if (flash->led_mode != V4L2_FLASH_LED_MODE_NONE) + return -EBUSY; + + flash->indicator_current = + (ctrl->val - AS3645A_INDICATOR_INTENSITY_MIN) + / AS3645A_INDICATOR_INTENSITY_STEP; + + ret = as3645a_set_config(flash); + if (ret < 0) + return ret; + + if ((ctrl->val == 0) == (ctrl->cur.val == 0)) + break; + + return as3645a_set_output(flash, false); + + default: + return -EINVAL; + } + + return 0; +} + +static const struct v4l2_ctrl_ops as3645a_ctrl_ops = { + .g_volatile_ctrl = as3645a_get_ctrl, + .s_ctrl = as3645a_set_ctrl, +}; + +/* ----------------------------------------------------------------------------- + * V4L2 subdev core operations + */ + +/* Put device into know state. */ +static int as3645a_setup(struct as3645a *flash) +{ + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); + int ret; + + /* clear errors */ + ret = as3645a_read(flash, AS_FAULT_INFO_REG); + if (ret < 0) + return ret; + + dev_dbg(&client->dev, "Fault info: %02x\n", ret); + + ret = as3645a_set_config(flash); + if (ret < 0) + return ret; + + ret = as3645a_set_output(flash, false); + if (ret < 0) + return ret; + + /* read status */ + ret = as3645a_read_fault(flash); + if (ret < 0) + return ret; + + dev_dbg(&client->dev, "AS_INDICATOR_AND_TIMER_REG: %02x\n", + as3645a_read(flash, AS_INDICATOR_AND_TIMER_REG)); + dev_dbg(&client->dev, "AS_CURRENT_SET_REG: %02x\n", + as3645a_read(flash, AS_CURRENT_SET_REG)); + dev_dbg(&client->dev, "AS_CONTROL_REG: %02x\n", + as3645a_read(flash, AS_CONTROL_REG)); + + return ret & ~AS_FAULT_INFO_LED_AMOUNT ? -EIO : 0; +} + +static int __as3645a_set_power(struct as3645a *flash, int on) +{ + int ret; + + if (!on) + as3645a_set_control(flash, AS_MODE_EXT_TORCH, false); + + if (flash->platform_data && flash->platform_data->set_power) { + ret = flash->platform_data->set_power(&flash->subdev, on); + if (ret < 0) + return ret; + } + + return on ? as3645a_setup(flash) : 0; +} + +static int as3645a_set_power(struct v4l2_subdev *sd, int on) +{ + struct as3645a *flash = to_as3645a(sd); + int ret = 0; + + mutex_lock(&flash->power_lock); + + if (flash->power_count == !on) { + ret = __as3645a_set_power(flash, !!on); + if (ret < 0) + goto done; + } + + flash->power_count += on ? 1 : -1; + WARN_ON(flash->power_count < 0); + +done: + mutex_unlock(&flash->power_lock); + return ret; +} + +static int as3645a_registered(struct v4l2_subdev *sd) +{ + struct as3645a *flash = to_as3645a(sd); + struct i2c_client *client = v4l2_get_subdevdata(sd); + int rval, man, model, rfu, version; + const char *factory; + + /* Power up the flash driver and read manufacturer ID, model ID, RFU + * and version. + */ + as3645a_set_power(&flash->subdev, 1); + + rval = as3645a_read(flash, AS_DESIGN_INFO_REG); + if (rval < 0) + goto power_off; + + man = AS_DESIGN_INFO_FACTORY(rval); + model = AS_DESIGN_INFO_MODEL(rval); + + rval = as3645a_read(flash, AS_VERSION_CONTROL_REG); + if (rval < 0) + goto power_off; + + rfu = AS_VERSION_CONTROL_RFU(rval); + version = AS_VERSION_CONTROL_VERSION(rval); + + /* Verify the chip model and version. */ + if (model != 0x0001 || rfu != 0x0000) { + dev_err(&client->dev, "AS3645A not detected " + "(model %d rfu %d)\n", model, rfu); + rval = -ENODEV; + goto power_off; + } + + switch (man) { + case 1: + factory = "AMS, Austria Micro Systems"; + break; + case 2: + factory = "ADI, Analog Devices Inc."; + break; + case 3: + factory = "NSC, National Semiconductor"; + break; + case 4: + factory = "NXP"; + break; + case 5: + factory = "TI, Texas Instrument"; + break; + default: + factory = "Unknown"; + } + + dev_dbg(&client->dev, "Factory: %s(%d) Version: %d\n", factory, man, + version); + + rval = as3645a_write(flash, AS_PASSWORD_REG, AS_PASSWORD_UNLOCK_VALUE); + if (rval < 0) + goto power_off; + + rval = as3645a_write(flash, AS_BOOST_REG, AS_BOOST_CURRENT_DISABLE); + if (rval < 0) + goto power_off; + + /* Setup default values. This makes sure that the chip is in a known + * state, in case the power rail can't be controlled. + */ + rval = as3645a_setup(flash); + +power_off: + as3645a_set_power(&flash->subdev, 0); + + return rval; +} + +static int as3645a_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) +{ + return as3645a_set_power(sd, 1); +} + +static int as3645a_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) +{ + return as3645a_set_power(sd, 0); +} + +static const struct v4l2_subdev_core_ops as3645a_core_ops = { + .s_power = as3645a_set_power, +}; + +static const struct v4l2_subdev_ops as3645a_ops = { + .core = &as3645a_core_ops, +}; + +static const struct v4l2_subdev_internal_ops as3645a_internal_ops = { + .registered = as3645a_registered, + .open = as3645a_open, + .close = as3645a_close, +}; + +/* ----------------------------------------------------------------------------- + * I2C driver + */ +#ifdef CONFIG_PM + +static int as3645a_suspend(struct i2c_client *client, pm_message_t mesg) +{ + struct v4l2_subdev *subdev = i2c_get_clientdata(client); + struct as3645a *flash = to_as3645a(subdev); + int rval; + + if (flash->power_count == 0) + return 0; + + rval = __as3645a_set_power(flash, 0); + + dev_dbg(&client->dev, "Suspend %s\n", rval < 0 ? "failed" : "ok"); + + return rval; +} + +static int as3645a_resume(struct i2c_client *client) +{ + struct v4l2_subdev *subdev = i2c_get_clientdata(client); + struct as3645a *flash = to_as3645a(subdev); + int rval; + + if (flash->power_count == 0) + return 0; + + rval = __as3645a_set_power(flash, 1); + + dev_dbg(&client->dev, "Resume %s\n", rval < 0 ? "fail" : "ok"); + + return rval; +} + +#else + +#define as3645a_suspend NULL +#define as3645a_resume NULL + +#endif /* CONFIG_PM */ + +/* + * as3645a_init_controls - Create controls + * @flash: The flash + * + * The number of LEDs reported in platform data is used to compute default + * limits. Parameters passed through platform data can override those limits. + */ +static int as3645a_init_controls(struct as3645a *flash) +{ + struct as3645a_flash_torch_parms *flash_params = NULL; + bool use_ext_strobe = false; + unsigned int leds = 2; + struct v4l2_ctrl *ctrl; + int minimum; + int maximum; + + if (flash->platform_data) { + if (flash->platform_data->num_leds) + leds = flash->platform_data->num_leds; + + flash_params = flash->platform_data->flash_torch_limits; + use_ext_strobe = flash->platform_data->use_ext_flash_strobe; + } + + v4l2_ctrl_handler_init(&flash->ctrls, 9); + + /* V4L2_CID_FLASH_LED_MODE */ + v4l2_ctrl_new_std_menu(&flash->ctrls, &as3645a_ctrl_ops, + V4L2_CID_FLASH_LED_MODE, 2, ~7, + V4L2_FLASH_LED_MODE_NONE); + + /* V4L2_CID_FLASH_STROBE_SOURCE */ + v4l2_ctrl_new_std_menu(&flash->ctrls, &as3645a_ctrl_ops, + V4L2_CID_FLASH_STROBE_SOURCE, + use_ext_strobe ? 1 : 0, use_ext_strobe ? ~3 : ~1, + V4L2_FLASH_STROBE_SOURCE_SOFTWARE); + + flash->strobe_source = V4L2_FLASH_STROBE_SOURCE_SOFTWARE; + + /* V4L2_CID_FLASH_STROBE */ + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, + V4L2_CID_FLASH_STROBE, 0, 0, 0, 0); + + /* V4L2_CID_FLASH_STROBE_STOP */ + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, + V4L2_CID_FLASH_STROBE_STOP, 0, 0, 0, 0); + + /* V4L2_CID_FLASH_TIMEOUT */ + if (flash_params) { + minimum = flash_params->timeout_min; + maximum = flash_params->timeout_max; + } else { + minimum = 1; + maximum = AS3645A_FLASH_TIMEOUT_MAX; + } + + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, + V4L2_CID_FLASH_TIMEOUT, minimum, maximum, + 1, maximum); + + flash->timeout = maximum; + + /* V4L2_CID_FLASH_INTENSITY */ + if (flash_params) { + minimum = flash_params->flash_min_current; + maximum = flash_params->flash_max_current; + } else { + minimum = AS3645A_FLASH_INTENSITY_MIN; + maximum = leds == 1 ? AS3645A_FLASH_INTENSITY_MAX_1LED + : AS3645A_FLASH_INTENSITY_MAX_2LEDS; + } + + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, + V4L2_CID_FLASH_INTENSITY, minimum, maximum, + AS3645A_FLASH_INTENSITY_STEP, maximum); + + flash->flash_current = (maximum - AS3645A_FLASH_INTENSITY_MIN) + / AS3645A_FLASH_INTENSITY_STEP; + + /* V4L2_CID_FLASH_TORCH_INTENSITY */ + if (flash_params) { + minimum = flash_params->torch_min_current; + maximum = flash_params->torch_max_current; + } else { + minimum = AS3645A_TORCH_INTENSITY_MIN; + maximum = AS3645A_TORCH_INTENSITY_MAX; + } + + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, + V4L2_CID_FLASH_TORCH_INTENSITY, minimum, maximum, + AS3645A_TORCH_INTENSITY_STEP, minimum); + + flash->assist_current = (minimum - AS3645A_TORCH_INTENSITY_MIN) + / AS3645A_TORCH_INTENSITY_STEP; + + /* V4L2_CID_FLASH_INDICATOR_INTENSITY */ + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, + V4L2_CID_FLASH_INDICATOR_INTENSITY, + AS3645A_INDICATOR_INTENSITY_MIN, + AS3645A_INDICATOR_INTENSITY_MAX, + AS3645A_INDICATOR_INTENSITY_STEP, + AS3645A_INDICATOR_INTENSITY_MIN); + + flash->indicator_current = 0; + + /* V4L2_CID_FLASH_FAULT */ + ctrl = v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, + V4L2_CID_FLASH_FAULT, 0, + V4L2_FLASH_FAULT_OVER_VOLTAGE | + V4L2_FLASH_FAULT_TIMEOUT | + V4L2_FLASH_FAULT_OVER_TEMPERATURE | + V4L2_FLASH_FAULT_SHORT_CIRCUIT, 0, 0); + if (ctrl != NULL) + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; + + flash->subdev.ctrl_handler = &flash->ctrls; + + return flash->ctrls.error; +} + +static int as3645a_probe(struct i2c_client *client, + const struct i2c_device_id *devid) +{ + struct as3645a *flash; + int ret; + + flash = kzalloc(sizeof(*flash), GFP_KERNEL); + if (flash == NULL) + return -ENOMEM; + + flash->platform_data = client->dev.platform_data; + + v4l2_i2c_subdev_init(&flash->subdev, client, &as3645a_ops); + flash->subdev.internal_ops = &as3645a_internal_ops; + flash->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; + + ret = media_entity_init(&flash->subdev.entity, 0, NULL, 0); + if (ret < 0) { + kfree(flash); + return ret; + } + + mutex_init(&flash->power_lock); + + /* FIXME: These are hard coded for now */ + flash->vref = 0; /* 0V */ + flash->peak = 2; /* 1.75A */ + + flash->led_mode = V4L2_FLASH_LED_MODE_NONE; + + ret = as3645a_init_controls(flash); + if (ret < 0) { + kfree(flash); + return ret; + } + + return 0; +} + +static int __exit as3645a_remove(struct i2c_client *client) +{ + struct v4l2_subdev *subdev = i2c_get_clientdata(client); + struct as3645a *flash = to_as3645a(subdev); + + v4l2_device_unregister_subdev(subdev); + v4l2_ctrl_handler_free(&flash->ctrls); + + kfree(flash); + + return 0; +} + +static const struct i2c_device_id as3645a_id_table[] = { + { AS3645A_NAME, 0 }, + { }, +}; +MODULE_DEVICE_TABLE(i2c, as3645a_id_table); + +static struct i2c_driver as3645a_i2c_driver = { + .driver = { + .name = AS3645A_NAME, + }, + .probe = as3645a_probe, + .remove = __exit_p(as3645a_remove), + .suspend = as3645a_suspend, + .resume = as3645a_resume, + .id_table = as3645a_id_table, +}; + +static int __init as3645a_init(void) +{ + int rval; + + rval = i2c_add_driver(&as3645a_i2c_driver); + if (rval) + printk(KERN_ERR "Failed registering driver" AS3645A_NAME"\n"); + + return rval; +} + +static void __exit as3645a_exit(void) +{ + i2c_del_driver(&as3645a_i2c_driver); +} + +module_init(as3645a_init); +module_exit(as3645a_exit); + +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>"); +MODULE_DESCRIPTION("AS3645A Flash driver"); +MODULE_LICENSE("GPL"); diff --git a/include/media/as3645a.h b/include/media/as3645a.h new file mode 100644 index 0000000..1556a97 --- /dev/null +++ b/include/media/as3645a.h @@ -0,0 +1,69 @@ +/* + * include/media/as3645a.h + * + * Copyright (C) 2008-2011 Nokia Corporation + * + * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.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. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA + * 02110-1301 USA + * + */ + +#ifndef __AS3645A_H__ +#define __AS3645A_H__ + +#include <media/v4l2-subdev.h> + +#define AS3645A_NAME "as3645a" +#define AS3645A_I2C_ADDR (0x60 >> 1) /* W:0x60, R:0x61 */ + +/* + * as3645a_flash_torch_parms - Flash and torch currents and timeout limits + * @flash_min_current: Min flash current (mA) + * @flash_max_current: Max flash current (mA) + * @torch_min_current: Min torch current (mA) + * @torch_max_current: Max torch current (mA) + * @timeout_min: Min flash timeout (us) + * @timeout_max: Max flash timeout (us) + */ +struct as3645a_flash_torch_parms { + unsigned int flash_min_current; + unsigned int flash_max_current; + unsigned int torch_min_current; + unsigned int torch_max_current; + unsigned int timeout_min; + unsigned int timeout_max; +}; + +struct as3645a_platform_data { + int (*set_power)(struct v4l2_subdev *subdev, int on); + /* used to notify the entity which trigger external strobe signal */ + void (*setup_ext_strobe)(int enable); + /* Sends the strobe width to the sensor strobe configuration */ + void (*set_strobe_width)(u32 width_in_us); + /* positive value if Torch pin is used */ + int ext_torch; + /* positive value if Flash Strobe pin is used for triggering + * the Flash light (no matter where is connected to, host processor or + * image sensor) + */ + int use_ext_flash_strobe; + /* Number of attached LEDs, 1 or 2 */ + int num_leds; + /* LED limitations with this flash chip */ + struct as3645a_flash_torch_parms *flash_torch_limits; +}; + +#endif /* __AS3645A_H__ */ -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] as3645a: Add driver for LED flash controller 2011-11-14 0:19 ` [PATCH v2 2/2] as3645a: Add driver for LED flash controller Laurent Pinchart @ 2011-11-14 9:34 ` Sakari Ailus 2011-11-15 12:01 ` Andy Shevchenko 2011-11-15 13:12 ` Laurent Pinchart 0 siblings, 2 replies; 11+ messages in thread From: Sakari Ailus @ 2011-11-14 9:34 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media, andriy.shevchenko Hi Laurent, Thanks for the patch!! I have a few comments below. Laurent Pinchart wrote: > This patch adds the driver for the as3645a LED flash controller. This > controller supports a high power led in flash and torch modes and an > indicator light, sometimes also called privacy light. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Nayden Kanchev <nkanchev@mm-sol.com> > Signed-off-by: Tuukka Toivonen <tuukkat76@gmail.com> > Signed-off-by: Antti Koskipaa <antti.koskipaa@gmail.com> > Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com> > Signed-off-by: Vimarsh Zutshi <vimarsh.zutshi@gmail.com> > Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com> > Signed-off-by: Mika Westerberg <ext-mika.1.westerberg@nokia.com> > Signed-off-by: David Cohen <dacohen@gmail.com> > --- > drivers/media/video/Kconfig | 7 + > drivers/media/video/Makefile | 1 + > drivers/media/video/as3645a.c | 951 +++++++++++++++++++++++++++++++++++++++++ > include/media/as3645a.h | 69 +++ > 4 files changed, 1028 insertions(+), 0 deletions(-) > create mode 100644 drivers/media/video/as3645a.c > create mode 100644 include/media/as3645a.h > > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig > index 4e8a0c4..ba27f89 100644 > --- a/drivers/media/video/Kconfig > +++ b/drivers/media/video/Kconfig > @@ -533,6 +533,13 @@ config VIDEO_ADP1653 > This is a driver for the ADP1653 flash controller. It is used for > example in Nokia N900. > > +config VIDEO_AS3645A > + tristate "AS3645A flash driver support" > + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER > + ---help--- > + This is a driver for the AS3645A flash chip. It has build in control > + for Flash, Torch and Indicator LEDs. > + You could mention the LM3555 (?) is equal to this; the same driver supports it. > comment "Video improvement chips" > > config VIDEO_UPD64031A > diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile > index ddeaa6c..86aabd6 100644 > --- a/drivers/media/video/Makefile > +++ b/drivers/media/video/Makefile > @@ -74,6 +74,7 @@ obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o > obj-$(CONFIG_VIDEO_M5MOLS) += m5mols/ > obj-$(CONFIG_VIDEO_S5K6AA) += s5k6aa.o > obj-$(CONFIG_VIDEO_ADP1653) += adp1653.o > +obj-$(CONFIG_VIDEO_AS3645A) += as3645a.o > > obj-$(CONFIG_SOC_CAMERA_IMX074) += imx074.o > obj-$(CONFIG_SOC_CAMERA_MT9M001) += mt9m001.o > diff --git a/drivers/media/video/as3645a.c b/drivers/media/video/as3645a.c > new file mode 100644 > index 0000000..4138eb7 > --- /dev/null > +++ b/drivers/media/video/as3645a.c > @@ -0,0 +1,951 @@ > +/* > + * drivers/media/video/as3645a.c Same here. This is a "Driver for the as3652a and lm3555 led flash controllers." For example. > + * Copyright (C) 2008-2011 Nokia Corporation > + * > + * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.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. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA > + * 02110-1301 USA > + * > + * NOTES: > + * - Inductor peak current limit setting fixed to 1.75A > + * - VREF offset fixed to 0V > + * > + * TODO: > + * - Check hardware FSTROBE control when sensor driver add support for this > + * > + */ > + > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/mutex.h> > + > +#include <media/as3645a.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-device.h> > + > +#define AS_TIMER_MS_TO_CODE(t) (((t) - 100) / 50) > +#define AS_TIMER_CODE_TO_MS(c) (50 * (c) + 100) > + > +/* Register definitions */ > + > +/* Read-only Design info register: Reset state: xxxx 0001 */ > +#define AS_DESIGN_INFO_REG 0x00 > +#define AS_DESIGN_INFO_FACTORY(x) (((x) >> 4)) > +#define AS_DESIGN_INFO_MODEL(x) ((x) & 0x0f) > + > +/* Read-only Version control register: Reset state: 0000 0000 > + * for first engineering samples > + */ > +#define AS_VERSION_CONTROL_REG 0x01 > +#define AS_VERSION_CONTROL_RFU(x) (((x) >> 4)) > +#define AS_VERSION_CONTROL_VERSION(x) ((x) & 0x0f) > + > +/* Read / Write (Indicator and timer register): Reset state: 0000 1111 */ > +#define AS_INDICATOR_AND_TIMER_REG 0x02 > +#define AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT 0 > +#define AS_INDICATOR_AND_TIMER_VREF_SHIFT 4 > +#define AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT 6 > + > +/* Read / Write (Current set register): Reset state: 0110 1001 */ > +#define AS_CURRENT_SET_REG 0x03 > +#define AS_CURRENT_ASSIST_LIGHT_SHIFT 0 > +#define AS_CURRENT_LED_DET_ON (1 << 3) > +#define AS_CURRENT_FLASH_CURRENT_SHIFT 4 > + > +/* Read / Write (Control register): Reset state: 1011 0100 */ > +#define AS_CONTROL_REG 0x04 > +#define AS_CONTROL_MODE_SETTING_SHIFT 0 > +#define AS_CONTROL_STROBE_ON (1 << 2) > +#define AS_CONTROL_OUT_ON (1 << 3) > +#define AS_CONTROL_EXT_TORCH_ON (1 << 4) > +#define AS_CONTROL_STROBE_TYPE_EDGE (0 << 5) > +#define AS_CONTROL_STROBE_TYPE_LEVEL (1 << 5) > +#define AS_CONTROL_COIL_PEAK_SHIFT 6 > + > +/* Read only (D3 is read / write) (Fault and info): Reset state: 0000 x000 */ > +#define AS_FAULT_INFO_REG 0x05 > +#define AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT (1 << 1) > +#define AS_FAULT_INFO_INDICATOR_LED (1 << 2) > +#define AS_FAULT_INFO_LED_AMOUNT (1 << 3) > +#define AS_FAULT_INFO_TIMEOUT (1 << 4) > +#define AS_FAULT_INFO_OVER_TEMPERATURE (1 << 5) > +#define AS_FAULT_INFO_SHORT_CIRCUIT (1 << 6) > +#define AS_FAULT_INFO_OVER_VOLTAGE (1 << 7) > + > +/* Boost register */ > +#define AS_BOOST_REG 0x0d > +#define AS_BOOST_CURRENT_DISABLE (0 << 0) > +#define AS_BOOST_CURRENT_ENABLE (1 << 0) > + > +/* Password register is used to unlock boost register writing */ > +#define AS_PASSWORD_REG 0x0f > +#define AS_PASSWORD_UNLOCK_VALUE 0x55 > + > +/* AS_CONTROL_EXT_TORCH_ON - on, 0 - off */ > +#define TORCH_IN_STANDBY 0 > + > +#define AS3645A_FLASH_TIMEOUT_MIN 100000 /* us */ > +#define AS3645A_FLASH_TIMEOUT_MAX 850000 > +#define AS3645A_FLASH_TIMEOUT_STEP 50000 > + > +#define AS3645A_FLASH_INTENSITY_MIN 200 /* mA */ > +#define AS3645A_FLASH_INTENSITY_MAX_1LED 500 > +#define AS3645A_FLASH_INTENSITY_MAX_2LEDS 400 > +#define AS3645A_FLASH_INTENSITY_STEP 20 > + > +#define AS3645A_TORCH_INTENSITY_MIN 20 /* mA */ > +#define AS3645A_TORCH_INTENSITY_MAX 160 > +#define AS3645A_TORCH_INTENSITY_STEP 20 > + > +#define AS3645A_INDICATOR_INTENSITY_MIN 0 /* uA */ > +#define AS3645A_INDICATOR_INTENSITY_MAX 10000 > +#define AS3645A_INDICATOR_INTENSITY_STEP 2500 > + > +enum as_mode { > + AS_MODE_EXT_TORCH = 0 << AS_CONTROL_MODE_SETTING_SHIFT, > + AS_MODE_INDICATOR = 1 << AS_CONTROL_MODE_SETTING_SHIFT, > + AS_MODE_ASSIST = 2 << AS_CONTROL_MODE_SETTING_SHIFT, > + AS_MODE_FLASH = 3 << AS_CONTROL_MODE_SETTING_SHIFT, > +}; > + > +/* > + * struct as3645a > + * > + * @subdev: V4L2 subdev > + * @platform_data: Flash platform data > + * @power_lock: Protects power_count > + * @power_count: Power reference count > + * @vref: VREF offset (0=0V, 1=+0.3V, 2=-0.3V, 3=+0.6V) > + * @peak: Inductor peak current limit (0=1.25A, 1=1.5A, 2=1.75A, > + * 3=2.0A) > + * @led_mode: V4L2 flash LED mode > + * @timeout: Flash timeout in microseconds > + * @flash_current: Flash current (0=200mA ... 15=500mA). Maximum > + * values are 400mA for two LEDs and 500mA for one LED. > + * @assist_current: Torch/Assist light current (0=20mA, 1=40mA ... 7=160mA) > + * @indicator_current: Indicator LED current (0=0mA, 1=2.5mA ... 4=10mA) > + * @strobe_source: Flash strobe source (software or external) > + */ > +struct as3645a { > + struct v4l2_subdev subdev; > + struct as3645a_platform_data *platform_data; > + > + struct mutex power_lock; > + int power_count; > + > + /* Static parameters */ > + u8 vref; > + u8 peak; > + > + /* Controls */ > + struct v4l2_ctrl_handler ctrls; > + > + enum v4l2_flash_led_mode led_mode; > + unsigned int timeout; > + u8 flash_current; > + u8 assist_current; > + u8 indicator_current; > + enum v4l2_flash_strobe_source strobe_source; Do you think we should store this information in the controls instead, or not? > +}; > + > +#define to_as3645a(sd) container_of(sd, struct as3645a, subdev) > + > +/* Return negative errno else zero on success */ > +static int as3645a_write(struct as3645a *flash, u8 addr, u8 val) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); > + int rval; > + > + rval = i2c_smbus_write_byte_data(client, addr, val); > + > + dev_dbg(&client->dev, "Write Addr:%02X Val:%02X %s\n", addr, val, > + rval < 0 ? "fail" : "ok"); > + > + return rval; > +} > + > +/* Return negative errno else a data byte received from the device. */ > +static int as3645a_read(struct as3645a *flash, u8 addr) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); > + int rval; > + > + rval = i2c_smbus_read_byte_data(client, addr); > + > + dev_dbg(&client->dev, "Read Addr:%02X Val:%02X %s\n", addr, rval, > + rval < 0 ? "fail" : "ok"); > + > + return rval; > +} > + > +/* ----------------------------------------------------------------------------- > + * Hardware configuration and trigger > + */ > + > +/* > + * as3645a_set_config - Set flash configuration registers > + * @flash: The flash > + * > + * Configure the hardware with flash, assist and indicator currents, as well as > + * flash timeout. > + * > + * Return 0 on success, or a negative error code if an I2C communication error > + * occurred. > + */ > +static int as3645a_set_config(struct as3645a *flash) > +{ > + int ret; > + u8 val; > + > + val = (flash->flash_current << AS_CURRENT_FLASH_CURRENT_SHIFT) > + | (flash->assist_current << AS_CURRENT_ASSIST_LIGHT_SHIFT) > + | AS_CURRENT_LED_DET_ON; > + > + ret = as3645a_write(flash, AS_CURRENT_SET_REG, val); > + if (ret < 0) > + return ret; > + > + if (flash->strobe_source == V4L2_FLASH_STROBE_SOURCE_EXTERNAL) { > + /* Use timeout to protect the flash in case the external > + * strobe gets stuck. Minimum value 100 ms, maximum 850 ms. > + */ > + u32 timeout = DIV_ROUND_UP(flash->timeout, 1000); > + timeout = max_t(u32, DIV_ROUND_UP(timeout, 50) * 50, 100); > + val = AS_TIMER_MS_TO_CODE(timeout) > + << AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT; > + } else { > + val = AS_TIMER_MS_TO_CODE(flash->timeout / 1000) > + << AS_INDICATOR_AND_TIMER_TIMEOUT_SHIFT; > + } > + > + val |= (flash->vref << AS_INDICATOR_AND_TIMER_VREF_SHIFT) > + | ((flash->indicator_current ? flash->indicator_current - 1 : 0) > + << AS_INDICATOR_AND_TIMER_INDICATOR_SHIFT); > + > + return as3645a_write(flash, AS_INDICATOR_AND_TIMER_REG, val); > +} > + > +/* > + * as3645a_set_control - Set flash control register > + * @flash: The flash > + * @mode: Desired output mode > + * @on: Desired output state > + * > + * Configure the hardware with output mode and state. > + * > + * Return 0 on success, or a negative error code if an I2C communication error > + * occurred. > + */ > +static int > +as3645a_set_control(struct as3645a *flash, enum as_mode mode, bool on) > +{ > + u8 reg; > + > + /* Configure output parameters and operation mode. */ > + reg = (flash->peak << AS_CONTROL_COIL_PEAK_SHIFT) > + | TORCH_IN_STANDBY > + | (on ? AS_CONTROL_OUT_ON : 0) > + | mode; > + > + if (flash->led_mode == V4L2_FLASH_LED_MODE_FLASH && > + flash->strobe_source == V4L2_FLASH_STROBE_SOURCE_EXTERNAL) { > + if (flash->platform_data->setup_ext_strobe) > + flash->platform_data->setup_ext_strobe(1); > + reg |= AS_CONTROL_STROBE_TYPE_LEVEL > + | AS_CONTROL_STROBE_ON; > + } > + > + return as3645a_write(flash, AS_CONTROL_REG, reg); > +} > + > +/* > + * as3645a_set_output - Configure output and operation mode > + * @flash: Flash controller > + * @strobe: Strobe the flash (only valid in flash mode) > + * > + * Turn the LEDs output on/off and set the operation mode based on the current > + * parameters. > + * > + * The AS3645A can't control the indicator LED independently of the flash/torch > + * LED. If the flash controller is in V4L2_FLASH_LED_MODE_NONE mode, set the > + * chip to indicator mode. Otherwise set it to assist light (torch) or flash > + * mode. > + * > + * In indicator and assist modes, turn the output on/off based on the indicator > + * and torch currents. In software strobe flash mode, turn the output on/off > + * based on the strobe parameter. > + */ > +static int as3645a_set_output(struct as3645a *flash, bool strobe) > +{ > + enum as_mode mode; > + bool on; > + int ret; > + > + switch (flash->led_mode) { > + case V4L2_FLASH_LED_MODE_NONE: > + on = flash->indicator_current != 0; > + mode = AS_MODE_INDICATOR; > + break; > + case V4L2_FLASH_LED_MODE_TORCH: > + on = true; > + mode = AS_MODE_ASSIST; > + break; > + case V4L2_FLASH_LED_MODE_FLASH: > + on = strobe; > + mode = AS_MODE_FLASH; > + break; > + default: > + return 0; Isn't this expected not to happen ever? > + } > + > + /* Configure output parameters and operation mode. */ > + ret = as3645a_set_control(flash, mode, on); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +/* ----------------------------------------------------------------------------- > + * V4L2 controls > + */ > + > +static int as3645a_read_fault(struct as3645a *flash) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); > + int rval; > + > + /* NOTE: reading register clear fault status */ > + rval = as3645a_read(flash, AS_FAULT_INFO_REG); > + if (rval < 0) > + return rval; > + > + if (rval & AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT) > + dev_err(&client->dev, "Inductor Peak limit fault\n"); > + > + if (rval & AS_FAULT_INFO_INDICATOR_LED) > + dev_err(&client->dev, "Indicator LED fault: " > + "Short circuit or open loop\n"); > + > + dev_dbg(&client->dev, "%u connected LEDs\n", > + rval & AS_FAULT_INFO_LED_AMOUNT ? 2 : 1); > + > + if (rval & AS_FAULT_INFO_TIMEOUT) > + dev_err(&client->dev, "Timeout fault\n"); > + > + if (rval & AS_FAULT_INFO_OVER_TEMPERATURE) > + dev_err(&client->dev, "Over temperature fault\n"); > + > + if (rval & AS_FAULT_INFO_SHORT_CIRCUIT) > + dev_err(&client->dev, "Short circuit fault\n"); > + > + if (rval & AS_FAULT_INFO_OVER_VOLTAGE) > + dev_err(&client->dev, "Over voltage fault: " > + "Indicates missing capacitor or open connection\n"); Shouldn't at least some of the above use dev_dbg() instead, if not all? > + if (rval & ~AS_FAULT_INFO_INDICATOR_LED) > + dev_dbg(&client->dev, "No faults, nice\n"); > + > + return rval; > +} > + > +static int as3645a_get_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct as3645a *flash = > + container_of(ctrl->handler, struct as3645a, ctrls); > + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); > + int fault; > + > + switch (ctrl->id) { > + case V4L2_CID_FLASH_FAULT: > + fault = as3645a_read_fault(flash); > + if (fault < 0) > + return fault; ctrl->cur.val = 0 here? > + > + if (fault & AS_FAULT_INFO_SHORT_CIRCUIT) > + ctrl->cur.val |= V4L2_FLASH_FAULT_SHORT_CIRCUIT; > + if (fault & AS_FAULT_INFO_OVER_TEMPERATURE) > + ctrl->cur.val |= V4L2_FLASH_FAULT_OVER_TEMPERATURE; > + if (fault & AS_FAULT_INFO_TIMEOUT) > + ctrl->cur.val |= V4L2_FLASH_FAULT_TIMEOUT; > + if (fault & AS_FAULT_INFO_OVER_VOLTAGE) > + ctrl->cur.val |= V4L2_FLASH_FAULT_OVER_VOLTAGE; > + if (fault & AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT) > + ctrl->cur.val |= V4L2_FLASH_FAULT_OVER_CURRENT; > + if (fault & AS_FAULT_INFO_INDICATOR_LED) > + ctrl->cur.val |= V4L2_FLASH_FAULT_INDICATOR; > + break; > + > + default: > + return -EINVAL; > + } > + > + dev_dbg(&client->dev, "G_CTRL %08x:%d\n", ctrl->id, ctrl->cur.val); > + > + return 0; > +} > + > +static int as3645a_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct as3645a *flash = > + container_of(ctrl->handler, struct as3645a, ctrls); > + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); > + int ret; > + > + dev_dbg(&client->dev, "S_CTRL %08x:%d\n", ctrl->id, ctrl->val); > + > + /* If a control that doesn't apply to the current mode is modified, > + * we store the value and return immediately. The setting will be > + * applied when the LED mode is changed. Otherwise we apply the setting > + * immediately. > + */ > + > + switch (ctrl->id) { > + case V4L2_CID_FLASH_LED_MODE: > + if (flash->indicator_current) > + return -EBUSY; > + > + ret = as3645a_set_config(flash); > + if (ret < 0) > + return ret; > + > + flash->led_mode = ctrl->val; > + return as3645a_set_output(flash, false); > + > + case V4L2_CID_FLASH_STROBE_SOURCE: > + flash->strobe_source = ctrl->val; > + > + /* Applies to flash mode only. */ > + if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) > + break; > + > + return as3645a_set_output(flash, false); > + > + case V4L2_CID_FLASH_STROBE: > + if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) > + return -EBUSY; > + > + return as3645a_set_output(flash, true); > + > + case V4L2_CID_FLASH_STROBE_STOP: > + if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) > + return -EBUSY; > + > + return as3645a_set_output(flash, false); > + > + case V4L2_CID_FLASH_TIMEOUT: > + /* This is ugly. The step value is different depending on strobe > + * mode, so only round the value when using I2C strobing. > + */ > + if (flash->strobe_source == V4L2_FLASH_STROBE_SOURCE_EXTERNAL && > + flash->platform_data->set_strobe_width != NULL) { > + flash->platform_data->set_strobe_width(ctrl->val); > + } else { > + if (ctrl->val < AS3645A_FLASH_TIMEOUT_MIN) > + ctrl->val = AS3645A_FLASH_TIMEOUT_MIN; > + ctrl->val = ctrl->val / AS3645A_FLASH_TIMEOUT_STEP > + * AS3645A_FLASH_TIMEOUT_STEP; > + } > + flash->timeout = ctrl->val; > + > + /* Applies to flash mode only. */ > + if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) > + break; > + > + return as3645a_set_config(flash); > + > + case V4L2_CID_FLASH_INTENSITY: > + flash->flash_current = (ctrl->val - AS3645A_FLASH_INTENSITY_MIN) > + / AS3645A_FLASH_INTENSITY_STEP; > + > + /* Applies to flash mode only. */ > + if (flash->led_mode != V4L2_FLASH_LED_MODE_FLASH) > + break; > + > + return as3645a_set_config(flash); > + > + case V4L2_CID_FLASH_TORCH_INTENSITY: > + flash->assist_current = > + (ctrl->val - AS3645A_TORCH_INTENSITY_MIN) > + / AS3645A_TORCH_INTENSITY_STEP; > + > + /* Applies to torch mode only. */ > + if (flash->led_mode != V4L2_FLASH_LED_MODE_TORCH) > + break; > + > + return as3645a_set_config(flash); > + > + case V4L2_CID_FLASH_INDICATOR_INTENSITY: > + if (flash->led_mode != V4L2_FLASH_LED_MODE_NONE) > + return -EBUSY; > + > + flash->indicator_current = > + (ctrl->val - AS3645A_INDICATOR_INTENSITY_MIN) > + / AS3645A_INDICATOR_INTENSITY_STEP; > + > + ret = as3645a_set_config(flash); > + if (ret < 0) > + return ret; > + > + if ((ctrl->val == 0) == (ctrl->cur.val == 0)) > + break; > + > + return as3645a_set_output(flash, false); > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static const struct v4l2_ctrl_ops as3645a_ctrl_ops = { > + .g_volatile_ctrl = as3645a_get_ctrl, > + .s_ctrl = as3645a_set_ctrl, > +}; > + > +/* ----------------------------------------------------------------------------- > + * V4L2 subdev core operations > + */ > + > +/* Put device into know state. */ > +static int as3645a_setup(struct as3645a *flash) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); > + int ret; > + > + /* clear errors */ > + ret = as3645a_read(flash, AS_FAULT_INFO_REG); > + if (ret < 0) > + return ret; > + > + dev_dbg(&client->dev, "Fault info: %02x\n", ret); > + > + ret = as3645a_set_config(flash); > + if (ret < 0) > + return ret; > + > + ret = as3645a_set_output(flash, false); > + if (ret < 0) > + return ret; > + > + /* read status */ > + ret = as3645a_read_fault(flash); > + if (ret < 0) > + return ret; > + > + dev_dbg(&client->dev, "AS_INDICATOR_AND_TIMER_REG: %02x\n", > + as3645a_read(flash, AS_INDICATOR_AND_TIMER_REG)); > + dev_dbg(&client->dev, "AS_CURRENT_SET_REG: %02x\n", > + as3645a_read(flash, AS_CURRENT_SET_REG)); > + dev_dbg(&client->dev, "AS_CONTROL_REG: %02x\n", > + as3645a_read(flash, AS_CONTROL_REG)); > + > + return ret & ~AS_FAULT_INFO_LED_AMOUNT ? -EIO : 0; > +} > + > +static int __as3645a_set_power(struct as3645a *flash, int on) > +{ > + int ret; > + > + if (!on) > + as3645a_set_control(flash, AS_MODE_EXT_TORCH, false); > + > + if (flash->platform_data && flash->platform_data->set_power) { > + ret = flash->platform_data->set_power(&flash->subdev, on); > + if (ret < 0) > + return ret; > + } > + > + return on ? as3645a_setup(flash) : 0; > +} > + > +static int as3645a_set_power(struct v4l2_subdev *sd, int on) > +{ > + struct as3645a *flash = to_as3645a(sd); > + int ret = 0; > + > + mutex_lock(&flash->power_lock); > + > + if (flash->power_count == !on) { > + ret = __as3645a_set_power(flash, !!on); > + if (ret < 0) > + goto done; > + } > + > + flash->power_count += on ? 1 : -1; > + WARN_ON(flash->power_count < 0); > + > +done: > + mutex_unlock(&flash->power_lock); > + return ret; > +} > + > +static int as3645a_registered(struct v4l2_subdev *sd) > +{ > + struct as3645a *flash = to_as3645a(sd); > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + int rval, man, model, rfu, version; > + const char *factory; > + > + /* Power up the flash driver and read manufacturer ID, model ID, RFU > + * and version. > + */ > + as3645a_set_power(&flash->subdev, 1); > + > + rval = as3645a_read(flash, AS_DESIGN_INFO_REG); > + if (rval < 0) > + goto power_off; > + > + man = AS_DESIGN_INFO_FACTORY(rval); > + model = AS_DESIGN_INFO_MODEL(rval); > + > + rval = as3645a_read(flash, AS_VERSION_CONTROL_REG); > + if (rval < 0) > + goto power_off; > + > + rfu = AS_VERSION_CONTROL_RFU(rval); > + version = AS_VERSION_CONTROL_VERSION(rval); > + > + /* Verify the chip model and version. */ > + if (model != 0x0001 || rfu != 0x0000) { > + dev_err(&client->dev, "AS3645A not detected " > + "(model %d rfu %d)\n", model, rfu); > + rval = -ENODEV; Is this so grave issue we should discontinue? I'd perhaps print a warning if even that. > + goto power_off; > + } > + > + switch (man) { > + case 1: > + factory = "AMS, Austria Micro Systems"; > + break; > + case 2: > + factory = "ADI, Analog Devices Inc."; > + break; > + case 3: > + factory = "NSC, National Semiconductor"; > + break; > + case 4: > + factory = "NXP"; > + break; > + case 5: > + factory = "TI, Texas Instrument"; > + break; > + default: > + factory = "Unknown"; > + } > + > + dev_dbg(&client->dev, "Factory: %s(%d) Version: %d\n", factory, man, > + version); Is that really a factor or is it the chip vendor --- which might contract another factory to actually manufacture the chips? > + rval = as3645a_write(flash, AS_PASSWORD_REG, AS_PASSWORD_UNLOCK_VALUE); > + if (rval < 0) > + goto power_off; > + > + rval = as3645a_write(flash, AS_BOOST_REG, AS_BOOST_CURRENT_DISABLE); > + if (rval < 0) > + goto power_off; > + > + /* Setup default values. This makes sure that the chip is in a known > + * state, in case the power rail can't be controlled. > + */ > + rval = as3645a_setup(flash); > + > +power_off: > + as3645a_set_power(&flash->subdev, 0); > + > + return rval; > +} > + > +static int as3645a_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +{ > + return as3645a_set_power(sd, 1); > +} > + > +static int as3645a_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +{ > + return as3645a_set_power(sd, 0); > +} > + > +static const struct v4l2_subdev_core_ops as3645a_core_ops = { > + .s_power = as3645a_set_power, > +}; > + > +static const struct v4l2_subdev_ops as3645a_ops = { > + .core = &as3645a_core_ops, > +}; > + > +static const struct v4l2_subdev_internal_ops as3645a_internal_ops = { > + .registered = as3645a_registered, > + .open = as3645a_open, > + .close = as3645a_close, > +}; > + > +/* ----------------------------------------------------------------------------- > + * I2C driver > + */ > +#ifdef CONFIG_PM > + > +static int as3645a_suspend(struct i2c_client *client, pm_message_t mesg) > +{ > + struct v4l2_subdev *subdev = i2c_get_clientdata(client); > + struct as3645a *flash = to_as3645a(subdev); > + int rval; > + > + if (flash->power_count == 0) > + return 0; > + > + rval = __as3645a_set_power(flash, 0); > + > + dev_dbg(&client->dev, "Suspend %s\n", rval < 0 ? "failed" : "ok"); > + > + return rval; > +} > + > +static int as3645a_resume(struct i2c_client *client) > +{ > + struct v4l2_subdev *subdev = i2c_get_clientdata(client); > + struct as3645a *flash = to_as3645a(subdev); > + int rval; > + > + if (flash->power_count == 0) > + return 0; > + > + rval = __as3645a_set_power(flash, 1); > + > + dev_dbg(&client->dev, "Resume %s\n", rval < 0 ? "fail" : "ok"); > + > + return rval; > +} > + > +#else > + > +#define as3645a_suspend NULL > +#define as3645a_resume NULL > + > +#endif /* CONFIG_PM */ > + > +/* > + * as3645a_init_controls - Create controls > + * @flash: The flash > + * > + * The number of LEDs reported in platform data is used to compute default > + * limits. Parameters passed through platform data can override those limits. > + */ > +static int as3645a_init_controls(struct as3645a *flash) > +{ > + struct as3645a_flash_torch_parms *flash_params = NULL; > + bool use_ext_strobe = false; > + unsigned int leds = 2; > + struct v4l2_ctrl *ctrl; > + int minimum; > + int maximum; > + > + if (flash->platform_data) { > + if (flash->platform_data->num_leds) > + leds = flash->platform_data->num_leds; > + > + flash_params = flash->platform_data->flash_torch_limits; > + use_ext_strobe = flash->platform_data->use_ext_flash_strobe; > + } > + > + v4l2_ctrl_handler_init(&flash->ctrls, 9); > + > + /* V4L2_CID_FLASH_LED_MODE */ > + v4l2_ctrl_new_std_menu(&flash->ctrls, &as3645a_ctrl_ops, > + V4L2_CID_FLASH_LED_MODE, 2, ~7, > + V4L2_FLASH_LED_MODE_NONE); > + > + /* V4L2_CID_FLASH_STROBE_SOURCE */ > + v4l2_ctrl_new_std_menu(&flash->ctrls, &as3645a_ctrl_ops, > + V4L2_CID_FLASH_STROBE_SOURCE, > + use_ext_strobe ? 1 : 0, use_ext_strobe ? ~3 : ~1, > + V4L2_FLASH_STROBE_SOURCE_SOFTWARE); > + > + flash->strobe_source = V4L2_FLASH_STROBE_SOURCE_SOFTWARE; > + > + /* V4L2_CID_FLASH_STROBE */ > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > + V4L2_CID_FLASH_STROBE, 0, 0, 0, 0); > + > + /* V4L2_CID_FLASH_STROBE_STOP */ > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > + V4L2_CID_FLASH_STROBE_STOP, 0, 0, 0, 0); > + > + /* V4L2_CID_FLASH_TIMEOUT */ > + if (flash_params) { Shouldn't we require valid values for the flash parameters always? Chances are very high the default ones for the flash controller won't apply for the LED connected to it. > + minimum = flash_params->timeout_min; > + maximum = flash_params->timeout_max; > + } else { > + minimum = 1; > + maximum = AS3645A_FLASH_TIMEOUT_MAX; > + } > + > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > + V4L2_CID_FLASH_TIMEOUT, minimum, maximum, > + 1, maximum); > + > + flash->timeout = maximum; > + > + /* V4L2_CID_FLASH_INTENSITY */ > + if (flash_params) { > + minimum = flash_params->flash_min_current; > + maximum = flash_params->flash_max_current; > + } else { > + minimum = AS3645A_FLASH_INTENSITY_MIN; > + maximum = leds == 1 ? AS3645A_FLASH_INTENSITY_MAX_1LED > + : AS3645A_FLASH_INTENSITY_MAX_2LEDS; > + } > + > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > + V4L2_CID_FLASH_INTENSITY, minimum, maximum, > + AS3645A_FLASH_INTENSITY_STEP, maximum); > + > + flash->flash_current = (maximum - AS3645A_FLASH_INTENSITY_MIN) > + / AS3645A_FLASH_INTENSITY_STEP; > + > + /* V4L2_CID_FLASH_TORCH_INTENSITY */ > + if (flash_params) { > + minimum = flash_params->torch_min_current; > + maximum = flash_params->torch_max_current; > + } else { > + minimum = AS3645A_TORCH_INTENSITY_MIN; > + maximum = AS3645A_TORCH_INTENSITY_MAX; > + } > + > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > + V4L2_CID_FLASH_TORCH_INTENSITY, minimum, maximum, > + AS3645A_TORCH_INTENSITY_STEP, minimum); > + > + flash->assist_current = (minimum - AS3645A_TORCH_INTENSITY_MIN) > + / AS3645A_TORCH_INTENSITY_STEP; > + > + /* V4L2_CID_FLASH_INDICATOR_INTENSITY */ > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > + V4L2_CID_FLASH_INDICATOR_INTENSITY, > + AS3645A_INDICATOR_INTENSITY_MIN, > + AS3645A_INDICATOR_INTENSITY_MAX, > + AS3645A_INDICATOR_INTENSITY_STEP, > + AS3645A_INDICATOR_INTENSITY_MIN); > + > + flash->indicator_current = 0; > + > + /* V4L2_CID_FLASH_FAULT */ > + ctrl = v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > + V4L2_CID_FLASH_FAULT, 0, > + V4L2_FLASH_FAULT_OVER_VOLTAGE | > + V4L2_FLASH_FAULT_TIMEOUT | > + V4L2_FLASH_FAULT_OVER_TEMPERATURE | > + V4L2_FLASH_FAULT_SHORT_CIRCUIT, 0, 0); > + if (ctrl != NULL) > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > + > + flash->subdev.ctrl_handler = &flash->ctrls; > + > + return flash->ctrls.error; > +} > + > +static int as3645a_probe(struct i2c_client *client, > + const struct i2c_device_id *devid) > +{ > + struct as3645a *flash; > + int ret; > + > + flash = kzalloc(sizeof(*flash), GFP_KERNEL); > + if (flash == NULL) > + return -ENOMEM; > + > + flash->platform_data = client->dev.platform_data; > + > + v4l2_i2c_subdev_init(&flash->subdev, client, &as3645a_ops); > + flash->subdev.internal_ops = &as3645a_internal_ops; > + flash->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + > + ret = media_entity_init(&flash->subdev.entity, 0, NULL, 0); > + if (ret < 0) { > + kfree(flash); > + return ret; > + } + + flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; > + > + mutex_init(&flash->power_lock); > + > + /* FIXME: These are hard coded for now */ > + flash->vref = 0; /* 0V */ > + flash->peak = 2; /* 1.75A */ How about putting these to the platform data? > + flash->led_mode = V4L2_FLASH_LED_MODE_NONE; > + > + ret = as3645a_init_controls(flash); > + if (ret < 0) { > + kfree(flash); > + return ret; > + } > + > + return 0; > +} > + > +static int __exit as3645a_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *subdev = i2c_get_clientdata(client); > + struct as3645a *flash = to_as3645a(subdev); > + > + v4l2_device_unregister_subdev(subdev); > + v4l2_ctrl_handler_free(&flash->ctrls); > + > + kfree(flash); > + > + return 0; > +} > + > +static const struct i2c_device_id as3645a_id_table[] = { > + { AS3645A_NAME, 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(i2c, as3645a_id_table); > + > +static struct i2c_driver as3645a_i2c_driver = { > + .driver = { > + .name = AS3645A_NAME, > + }, > + .probe = as3645a_probe, > + .remove = __exit_p(as3645a_remove), > + .suspend = as3645a_suspend, > + .resume = as3645a_resume, > + .id_table = as3645a_id_table, > +}; > + > +static int __init as3645a_init(void) > +{ > + int rval; > + > + rval = i2c_add_driver(&as3645a_i2c_driver); > + if (rval) > + printk(KERN_ERR "Failed registering driver" AS3645A_NAME"\n"); > + > + return rval; > +} > + > +static void __exit as3645a_exit(void) > +{ > + i2c_del_driver(&as3645a_i2c_driver); > +} > + > +module_init(as3645a_init); > +module_exit(as3645a_exit); > + > +MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>"); > +MODULE_DESCRIPTION("AS3645A Flash driver"); > +MODULE_LICENSE("GPL"); > diff --git a/include/media/as3645a.h b/include/media/as3645a.h > new file mode 100644 > index 0000000..1556a97 > --- /dev/null > +++ b/include/media/as3645a.h > @@ -0,0 +1,69 @@ > +/* > + * include/media/as3645a.h > + * > + * Copyright (C) 2008-2011 Nokia Corporation > + * > + * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.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. > + * > + * This program is distributed in the hope that it will be useful, but > + * WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA > + * 02110-1301 USA > + * > + */ > + > +#ifndef __AS3645A_H__ > +#define __AS3645A_H__ > + > +#include <media/v4l2-subdev.h> > + > +#define AS3645A_NAME "as3645a" > +#define AS3645A_I2C_ADDR (0x60 >> 1) /* W:0x60, R:0x61 */ > + > +/* > + * as3645a_flash_torch_parms - Flash and torch currents and timeout limits > + * @flash_min_current: Min flash current (mA) > + * @flash_max_current: Max flash current (mA) > + * @torch_min_current: Min torch current (mA) > + * @torch_max_current: Max torch current (mA) > + * @timeout_min: Min flash timeout (us) > + * @timeout_max: Max flash timeout (us) > + */ > +struct as3645a_flash_torch_parms { > + unsigned int flash_min_current; > + unsigned int flash_max_current; > + unsigned int torch_min_current; > + unsigned int torch_max_current; > + unsigned int timeout_min; > + unsigned int timeout_max; > +}; > + > +struct as3645a_platform_data { > + int (*set_power)(struct v4l2_subdev *subdev, int on); > + /* used to notify the entity which trigger external strobe signal */ > + void (*setup_ext_strobe)(int enable); > + /* Sends the strobe width to the sensor strobe configuration */ > + void (*set_strobe_width)(u32 width_in_us); I don't think we should have the above two callbacks at all. This should be controlled from the user space instead. > + /* positive value if Torch pin is used */ > + int ext_torch; ext_torch is unused. > + /* positive value if Flash Strobe pin is used for triggering > + * the Flash light (no matter where is connected to, host processor or > + * image sensor) > + */ > + int use_ext_flash_strobe; > + /* Number of attached LEDs, 1 or 2 */ > + int num_leds; > + /* LED limitations with this flash chip */ > + struct as3645a_flash_torch_parms *flash_torch_limits; This struct could be part of the as3654a_platform_data struct directly. > +}; > + > +#endif /* __AS3645A_H__ */ Kind regards, -- Sakari Ailus sakari.ailus@maxwell.research.nokia.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] as3645a: Add driver for LED flash controller 2011-11-14 9:34 ` Sakari Ailus @ 2011-11-15 12:01 ` Andy Shevchenko 2011-11-15 13:12 ` Laurent Pinchart 1 sibling, 0 replies; 11+ messages in thread From: Andy Shevchenko @ 2011-11-15 12:01 UTC (permalink / raw) To: Sakari Ailus On Mon, 2011-11-14 at 11:34 +0200, Sakari Ailus wrote: > Hi Laurent, > > Thanks for the patch!! I have a few comments below. Most of your comments is about to be addressed by my patches. Please, wait a bit, And I send my series as I promised. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] as3645a: Add driver for LED flash controller 2011-11-14 9:34 ` Sakari Ailus 2011-11-15 12:01 ` Andy Shevchenko @ 2011-11-15 13:12 ` Laurent Pinchart 2011-11-15 15:28 ` Sakari Ailus 2011-11-15 16:55 ` Andy Shevchenko 1 sibling, 2 replies; 11+ messages in thread From: Laurent Pinchart @ 2011-11-15 13:12 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media, andriy.shevchenko Hi Sakari, On Monday 14 November 2011 10:34:57 Sakari Ailus wrote: > Hi Laurent, > > Thanks for the patch!! I have a few comments below. Thanks for the review. > Laurent Pinchart wrote: > > This patch adds the driver for the as3645a LED flash controller. This > > controller supports a high power led in flash and torch modes and an > > indicator light, sometimes also called privacy light. [snip] > > @@ -533,6 +533,13 @@ config VIDEO_ADP1653 > > > > This is a driver for the ADP1653 flash controller. It is used for > > example in Nokia N900. > > > > +config VIDEO_AS3645A > > + tristate "AS3645A flash driver support" > > + depends on I2C && VIDEO_V4L2 && MEDIA_CONTROLLER > > + ---help--- > > + This is a driver for the AS3645A flash chip. It has build in control > > + for Flash, Torch and Indicator LEDs. > > + > > You could mention the LM3555 (?) is equal to this; the same driver > supports it. OK. > > comment "Video improvement chips" > > > > config VIDEO_UPD64031A > > [snip] > > diff --git a/drivers/media/video/as3645a.c > > b/drivers/media/video/as3645a.c new file mode 100644 > > index 0000000..4138eb7 > > --- /dev/null > > +++ b/drivers/media/video/as3645a.c > > @@ -0,0 +1,951 @@ > > +/* > > + * drivers/media/video/as3645a.c > > Same here. This is a "Driver for the as3652a and lm3555 led flash > controllers." For example. OK. > > + * Copyright (C) 2008-2011 Nokia Corporation > > + * > > + * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.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. > > + * > > + * This program is distributed in the hope that it will be useful, but > > + * WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > + * General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA > > + * 02110-1301 USA > > + * > > + * NOTES: > > + * - Inductor peak current limit setting fixed to 1.75A > > + * - VREF offset fixed to 0V > > + * > > + * TODO: > > + * - Check hardware FSTROBE control when sensor driver add support for > > this + * > > + */ [snip] > > +struct as3645a { > > + struct v4l2_subdev subdev; > > + struct as3645a_platform_data *platform_data; > > + > > + struct mutex power_lock; > > + int power_count; > > + > > + /* Static parameters */ > > + u8 vref; > > + u8 peak; > > + > > + /* Controls */ > > + struct v4l2_ctrl_handler ctrls; > > + > > + enum v4l2_flash_led_mode led_mode; > > + unsigned int timeout; > > + u8 flash_current; > > + u8 assist_current; > > + u8 indicator_current; > > + enum v4l2_flash_strobe_source strobe_source; > > Do you think we should store this information in the controls instead, > or not? I've been thinking about that as well. The reason why the control values were copied to the as3645a structure is that they were accessed in timer context, where taking the control lock wasn't possible. I could switch to accessing the information in controls directly now, but that would require storing pointers to the controls in the as3645a structure, which might not be that better :-) And the code will need to change back to storing values when overheat protection will be implemented anyway. If you still think it's better, I can change it. > > +}; [snip] > > +static int as3645a_set_output(struct as3645a *flash, bool strobe) > > +{ > > + enum as_mode mode; > > + bool on; > > + int ret; > > + > > + switch (flash->led_mode) { > > + case V4L2_FLASH_LED_MODE_NONE: > > + on = flash->indicator_current != 0; > > + mode = AS_MODE_INDICATOR; > > + break; > > + case V4L2_FLASH_LED_MODE_TORCH: > > + on = true; > > + mode = AS_MODE_ASSIST; > > + break; > > + case V4L2_FLASH_LED_MODE_FLASH: > > + on = strobe; > > + mode = AS_MODE_FLASH; > > + break; > > + default: > > + return 0; > > Isn't this expected not to happen ever? Right, I'll fix that. > > + } > > + > > + /* Configure output parameters and operation mode. */ > > + ret = as3645a_set_control(flash, mode, on); > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > +} [snip] > > +static int as3645a_read_fault(struct as3645a *flash) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); > > + int rval; > > + > > + /* NOTE: reading register clear fault status */ > > + rval = as3645a_read(flash, AS_FAULT_INFO_REG); > > + if (rval < 0) > > + return rval; > > + > > + if (rval & AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT) > > + dev_err(&client->dev, "Inductor Peak limit fault\n"); > > + > > + if (rval & AS_FAULT_INFO_INDICATOR_LED) > > + dev_err(&client->dev, "Indicator LED fault: " > > + "Short circuit or open loop\n"); > > + > > + dev_dbg(&client->dev, "%u connected LEDs\n", > > + rval & AS_FAULT_INFO_LED_AMOUNT ? 2 : 1); > > + > > + if (rval & AS_FAULT_INFO_TIMEOUT) > > + dev_err(&client->dev, "Timeout fault\n"); > > + > > + if (rval & AS_FAULT_INFO_OVER_TEMPERATURE) > > + dev_err(&client->dev, "Over temperature fault\n"); > > + > > + if (rval & AS_FAULT_INFO_SHORT_CIRCUIT) > > + dev_err(&client->dev, "Short circuit fault\n"); > > + > > + if (rval & AS_FAULT_INFO_OVER_VOLTAGE) > > + dev_err(&client->dev, "Over voltage fault: " > > + "Indicates missing capacitor or open connection\n"); > > Shouldn't at least some of the above use dev_dbg() instead, if not all? All of them I think. I'll fix that. > > + if (rval & ~AS_FAULT_INFO_INDICATOR_LED) > > + dev_dbg(&client->dev, "No faults, nice\n"); > > + > > + return rval; > > +} > > + > > +static int as3645a_get_ctrl(struct v4l2_ctrl *ctrl) > > +{ > > + struct as3645a *flash = > > + container_of(ctrl->handler, struct as3645a, ctrls); > > + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); > > + int fault; > > + > > + switch (ctrl->id) { > > + case V4L2_CID_FLASH_FAULT: > > + fault = as3645a_read_fault(flash); > > + if (fault < 0) > > + return fault; > > ctrl->cur.val = 0 here? fault being negative means that reading the fault register failed. In that case I don't think we should update ctrl->cur.val. > > + > > + if (fault & AS_FAULT_INFO_SHORT_CIRCUIT) > > + ctrl->cur.val |= V4L2_FLASH_FAULT_SHORT_CIRCUIT; > > + if (fault & AS_FAULT_INFO_OVER_TEMPERATURE) > > + ctrl->cur.val |= V4L2_FLASH_FAULT_OVER_TEMPERATURE; > > + if (fault & AS_FAULT_INFO_TIMEOUT) > > + ctrl->cur.val |= V4L2_FLASH_FAULT_TIMEOUT; > > + if (fault & AS_FAULT_INFO_OVER_VOLTAGE) > > + ctrl->cur.val |= V4L2_FLASH_FAULT_OVER_VOLTAGE; > > + if (fault & AS_FAULT_INFO_INDUCTOR_PEAK_LIMIT) > > + ctrl->cur.val |= V4L2_FLASH_FAULT_OVER_CURRENT; > > + if (fault & AS_FAULT_INFO_INDICATOR_LED) > > + ctrl->cur.val |= V4L2_FLASH_FAULT_INDICATOR; > > + break; > > + > > + default: > > + return -EINVAL; > > + } > > + > > + dev_dbg(&client->dev, "G_CTRL %08x:%d\n", ctrl->id, ctrl->cur.val); > > + > > + return 0; > > +} [snip] > > +static int as3645a_registered(struct v4l2_subdev *sd) > > +{ > > + struct as3645a *flash = to_as3645a(sd); > > + struct i2c_client *client = v4l2_get_subdevdata(sd); > > + int rval, man, model, rfu, version; > > + const char *factory; > > + > > + /* Power up the flash driver and read manufacturer ID, model ID, RFU > > + * and version. > > + */ > > + as3645a_set_power(&flash->subdev, 1); > > + > > + rval = as3645a_read(flash, AS_DESIGN_INFO_REG); > > + if (rval < 0) > > + goto power_off; > > + > > + man = AS_DESIGN_INFO_FACTORY(rval); > > + model = AS_DESIGN_INFO_MODEL(rval); > > + > > + rval = as3645a_read(flash, AS_VERSION_CONTROL_REG); > > + if (rval < 0) > > + goto power_off; > > + > > + rfu = AS_VERSION_CONTROL_RFU(rval); > > + version = AS_VERSION_CONTROL_VERSION(rval); > > + > > + /* Verify the chip model and version. */ > > + if (model != 0x0001 || rfu != 0x0000) { > > + dev_err(&client->dev, "AS3645A not detected " > > + "(model %d rfu %d)\n", model, rfu); > > + rval = -ENODEV; > > Is this so grave issue we should discontinue? I'd perhaps print a > warning if even that. This could mean that the chip isn't an AS3645A/LM3555 at all. Many I2C drivers perform the same check, they read the ID register and fail if it doesn't contain the expected value. > > + goto power_off; > > + } > > + > > + switch (man) { > > + case 1: > > + factory = "AMS, Austria Micro Systems"; > > + break; > > + case 2: > > + factory = "ADI, Analog Devices Inc."; > > + break; > > + case 3: > > + factory = "NSC, National Semiconductor"; > > + break; > > + case 4: > > + factory = "NXP"; > > + break; > > + case 5: > > + factory = "TI, Texas Instrument"; > > + break; > > + default: > > + factory = "Unknown"; > > + } > > + > > + dev_dbg(&client->dev, "Factory: %s(%d) Version: %d\n", factory, man, > > + version); > > Is that really a factor or is it the chip vendor --- which might > contract another factory to actually manufacture the chips? I don't know :-) > > + rval = as3645a_write(flash, AS_PASSWORD_REG, AS_PASSWORD_UNLOCK_VALUE); > > + if (rval < 0) > > + goto power_off; > > + > > + rval = as3645a_write(flash, AS_BOOST_REG, AS_BOOST_CURRENT_DISABLE); > > + if (rval < 0) > > + goto power_off; > > + > > + /* Setup default values. This makes sure that the chip is in a known > > + * state, in case the power rail can't be controlled. > > + */ > > + rval = as3645a_setup(flash); > > + > > +power_off: > > + as3645a_set_power(&flash->subdev, 0); > > + > > + return rval; > > +} [snip] > > +static int as3645a_init_controls(struct as3645a *flash) > > +{ > > + struct as3645a_flash_torch_parms *flash_params = NULL; > > + bool use_ext_strobe = false; > > + unsigned int leds = 2; > > + struct v4l2_ctrl *ctrl; > > + int minimum; > > + int maximum; > > + > > + if (flash->platform_data) { > > + if (flash->platform_data->num_leds) > > + leds = flash->platform_data->num_leds; > > + > > + flash_params = flash->platform_data->flash_torch_limits; > > + use_ext_strobe = flash->platform_data->use_ext_flash_strobe; > > + } > > + > > + v4l2_ctrl_handler_init(&flash->ctrls, 9); > > + > > + /* V4L2_CID_FLASH_LED_MODE */ > > + v4l2_ctrl_new_std_menu(&flash->ctrls, &as3645a_ctrl_ops, > > + V4L2_CID_FLASH_LED_MODE, 2, ~7, > > + V4L2_FLASH_LED_MODE_NONE); > > + > > + /* V4L2_CID_FLASH_STROBE_SOURCE */ > > + v4l2_ctrl_new_std_menu(&flash->ctrls, &as3645a_ctrl_ops, > > + V4L2_CID_FLASH_STROBE_SOURCE, > > + use_ext_strobe ? 1 : 0, use_ext_strobe ? ~3 : ~1, > > + V4L2_FLASH_STROBE_SOURCE_SOFTWARE); > > + > > + flash->strobe_source = V4L2_FLASH_STROBE_SOURCE_SOFTWARE; > > + > > + /* V4L2_CID_FLASH_STROBE */ > > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > > + V4L2_CID_FLASH_STROBE, 0, 0, 0, 0); > > + > > + /* V4L2_CID_FLASH_STROBE_STOP */ > > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > > + V4L2_CID_FLASH_STROBE_STOP, 0, 0, 0, 0); > > + > > + /* V4L2_CID_FLASH_TIMEOUT */ > > + if (flash_params) { > > Shouldn't we require valid values for the flash parameters always? > Chances are very high the default ones for the flash controller won't > apply for the LED connected to it. Good question. If no flash parameters are provided the driver uses the chip's maximum ratings. I'm fine with both. > > + minimum = flash_params->timeout_min; > > + maximum = flash_params->timeout_max; > > + } else { > > + minimum = 1; > > + maximum = AS3645A_FLASH_TIMEOUT_MAX; > > + } > > + > > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > > + V4L2_CID_FLASH_TIMEOUT, minimum, maximum, > > + 1, maximum); > > + > > + flash->timeout = maximum; > > + > > + /* V4L2_CID_FLASH_INTENSITY */ > > + if (flash_params) { > > + minimum = flash_params->flash_min_current; > > + maximum = flash_params->flash_max_current; > > + } else { > > + minimum = AS3645A_FLASH_INTENSITY_MIN; > > + maximum = leds == 1 ? AS3645A_FLASH_INTENSITY_MAX_1LED > > + : AS3645A_FLASH_INTENSITY_MAX_2LEDS; > > + } > > + > > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > > + V4L2_CID_FLASH_INTENSITY, minimum, maximum, > > + AS3645A_FLASH_INTENSITY_STEP, maximum); > > + > > + flash->flash_current = (maximum - AS3645A_FLASH_INTENSITY_MIN) > > + / AS3645A_FLASH_INTENSITY_STEP; > > + > > + /* V4L2_CID_FLASH_TORCH_INTENSITY */ > > + if (flash_params) { > > + minimum = flash_params->torch_min_current; > > + maximum = flash_params->torch_max_current; > > + } else { > > + minimum = AS3645A_TORCH_INTENSITY_MIN; > > + maximum = AS3645A_TORCH_INTENSITY_MAX; > > + } > > + > > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > > + V4L2_CID_FLASH_TORCH_INTENSITY, minimum, maximum, > > + AS3645A_TORCH_INTENSITY_STEP, minimum); > > + > > + flash->assist_current = (minimum - AS3645A_TORCH_INTENSITY_MIN) > > + / AS3645A_TORCH_INTENSITY_STEP; > > + > > + /* V4L2_CID_FLASH_INDICATOR_INTENSITY */ > > + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > > + V4L2_CID_FLASH_INDICATOR_INTENSITY, > > + AS3645A_INDICATOR_INTENSITY_MIN, > > + AS3645A_INDICATOR_INTENSITY_MAX, > > + AS3645A_INDICATOR_INTENSITY_STEP, > > + AS3645A_INDICATOR_INTENSITY_MIN); > > + > > + flash->indicator_current = 0; > > + > > + /* V4L2_CID_FLASH_FAULT */ > > + ctrl = v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > > + V4L2_CID_FLASH_FAULT, 0, > > + V4L2_FLASH_FAULT_OVER_VOLTAGE | > > + V4L2_FLASH_FAULT_TIMEOUT | > > + V4L2_FLASH_FAULT_OVER_TEMPERATURE | > > + V4L2_FLASH_FAULT_SHORT_CIRCUIT, 0, 0); > > + if (ctrl != NULL) > > + ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE; > > + > > + flash->subdev.ctrl_handler = &flash->ctrls; > > + > > + return flash->ctrls.error; > > +} > > + > > +static int as3645a_probe(struct i2c_client *client, > > + const struct i2c_device_id *devid) > > +{ > > + struct as3645a *flash; > > + int ret; > > + > > + flash = kzalloc(sizeof(*flash), GFP_KERNEL); > > + if (flash == NULL) > > + return -ENOMEM; > > + > > + flash->platform_data = client->dev.platform_data; > > + > > + v4l2_i2c_subdev_init(&flash->subdev, client, &as3645a_ops); > > + flash->subdev.internal_ops = &as3645a_internal_ops; > > + flash->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > + > > + ret = media_entity_init(&flash->subdev.entity, 0, NULL, 0); > > + if (ret < 0) { > > + kfree(flash); > > + return ret; > > + } > > + > + flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH; OK. > > + > > + mutex_init(&flash->power_lock); > > + > > + /* FIXME: These are hard coded for now */ > > + flash->vref = 0; /* 0V */ > > + flash->peak = 2; /* 1.75A */ > > How about putting these to the platform data? Sounds good to me. > > + flash->led_mode = V4L2_FLASH_LED_MODE_NONE; > > + > > + ret = as3645a_init_controls(flash); > > + if (ret < 0) { > > + kfree(flash); > > + return ret; > > + } > > + > > + return 0; > > +} [snip] > > +struct as3645a_platform_data { > > + int (*set_power)(struct v4l2_subdev *subdev, int on); > > + /* used to notify the entity which trigger external strobe signal */ > > + void (*setup_ext_strobe)(int enable); > > + /* Sends the strobe width to the sensor strobe configuration */ > > + void (*set_strobe_width)(u32 width_in_us); > > I don't think we should have the above two callbacks at all. This should > be controlled from the user space instead. Should I remove external strobe mode completely then ? > > + /* positive value if Torch pin is used */ > > + int ext_torch; > > ext_torch is unused. OK. > > + /* positive value if Flash Strobe pin is used for triggering > > + * the Flash light (no matter where is connected to, host processor or > > + * image sensor) > > + */ > > + int use_ext_flash_strobe; > > + /* Number of attached LEDs, 1 or 2 */ > > + int num_leds; > > + /* LED limitations with this flash chip */ > > + struct as3645a_flash_torch_parms *flash_torch_limits; > > This struct could be part of the as3654a_platform_data struct directly. OK. > > +}; > > + > > +#endif /* __AS3645A_H__ */ -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] as3645a: Add driver for LED flash controller 2011-11-15 13:12 ` Laurent Pinchart @ 2011-11-15 15:28 ` Sakari Ailus 2011-11-15 15:53 ` Laurent Pinchart 2011-11-15 16:55 ` Andy Shevchenko 1 sibling, 1 reply; 11+ messages in thread From: Sakari Ailus @ 2011-11-15 15:28 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media, andriy.shevchenko Laurent Pinchart wrote: > Hi Sakari, Hi Laurent, > On Monday 14 November 2011 10:34:57 Sakari Ailus wrote: >> Hi Laurent, >> >> Thanks for the patch!! I have a few comments below. > > Thanks for the review. Thanks for the reply to my review. :-) ... >>> +struct as3645a { >>> + struct v4l2_subdev subdev; >>> + struct as3645a_platform_data *platform_data; >>> + >>> + struct mutex power_lock; >>> + int power_count; >>> + >>> + /* Static parameters */ >>> + u8 vref; >>> + u8 peak; >>> + >>> + /* Controls */ >>> + struct v4l2_ctrl_handler ctrls; >>> + >>> + enum v4l2_flash_led_mode led_mode; >>> + unsigned int timeout; >>> + u8 flash_current; >>> + u8 assist_current; >>> + u8 indicator_current; >>> + enum v4l2_flash_strobe_source strobe_source; >> >> Do you think we should store this information in the controls instead, >> or not? > > I've been thinking about that as well. The reason why the control values were > copied to the as3645a structure is that they were accessed in timer context, > where taking the control lock wasn't possible. > > I could switch to accessing the information in controls directly now, but that > would require storing pointers to the controls in the as3645a structure, which > might not be that better :-) And the code will need to change back to storing > values when overheat protection will be implemented anyway. If you still think > it's better, I can change it. I think it should stay as-is, at least for now. ... >>> + if (rval & ~AS_FAULT_INFO_INDICATOR_LED) >>> + dev_dbg(&client->dev, "No faults, nice\n"); >>> + >>> + return rval; >>> +} >>> + >>> +static int as3645a_get_ctrl(struct v4l2_ctrl *ctrl) >>> +{ >>> + struct as3645a *flash = >>> + container_of(ctrl->handler, struct as3645a, ctrls); >>> + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); >>> + int fault; >>> + >>> + switch (ctrl->id) { >>> + case V4L2_CID_FLASH_FAULT: >>> + fault = as3645a_read_fault(flash); >>> + if (fault < 0) >>> + return fault; >> >> ctrl->cur.val = 0 here? > > fault being negative means that reading the fault register failed. In that > case I don't think we should update ctrl->cur.val. I thought that ctrl->cur.val should be assigned to zero before settings bits in it. Does that sound better? :-) ... >>> +static int as3645a_registered(struct v4l2_subdev *sd) >>> +{ >>> + struct as3645a *flash = to_as3645a(sd); >>> + struct i2c_client *client = v4l2_get_subdevdata(sd); >>> + int rval, man, model, rfu, version; >>> + const char *factory; >>> + >>> + /* Power up the flash driver and read manufacturer ID, model ID, RFU >>> + * and version. >>> + */ >>> + as3645a_set_power(&flash->subdev, 1); >>> + >>> + rval = as3645a_read(flash, AS_DESIGN_INFO_REG); >>> + if (rval < 0) >>> + goto power_off; >>> + >>> + man = AS_DESIGN_INFO_FACTORY(rval); >>> + model = AS_DESIGN_INFO_MODEL(rval); >>> + >>> + rval = as3645a_read(flash, AS_VERSION_CONTROL_REG); >>> + if (rval < 0) >>> + goto power_off; >>> + >>> + rfu = AS_VERSION_CONTROL_RFU(rval); >>> + version = AS_VERSION_CONTROL_VERSION(rval); >>> + >>> + /* Verify the chip model and version. */ >>> + if (model != 0x0001 || rfu != 0x0000) { >>> + dev_err(&client->dev, "AS3645A not detected " >>> + "(model %d rfu %d)\n", model, rfu); >>> + rval = -ENODEV; >> >> Is this so grave issue we should discontinue? I'd perhaps print a >> warning if even that. > > This could mean that the chip isn't an AS3645A/LM3555 at all. Many I2C drivers > perform the same check, they read the ID register and fail if it doesn't > contain the expected value. Shouldn't we instead check the design info register has a valid id in it only? The spec I have doesn't say anything about the version of the chip. I.e. I'd leave the rfu check out. Also, the registers are 8 bit wide, with each four fields having four bits in all of them. >>> + goto power_off; >>> + } >>> + >>> + switch (man) { >>> + case 1: >>> + factory = "AMS, Austria Micro Systems"; >>> + break; >>> + case 2: >>> + factory = "ADI, Analog Devices Inc."; >>> + break; >>> + case 3: >>> + factory = "NSC, National Semiconductor"; >>> + break; >>> + case 4: >>> + factory = "NXP"; >>> + break; >>> + case 5: >>> + factory = "TI, Texas Instrument"; >>> + break; >>> + default: >>> + factory = "Unknown"; >>> + } >>> + >>> + dev_dbg(&client->dev, "Factory: %s(%d) Version: %d\n", factory, man, >>> + version); >> >> Is that really a factor or is it the chip vendor --- which might >> contract another factory to actually manufacture the chips? > > I don't know :-) > >>> + rval = as3645a_write(flash, AS_PASSWORD_REG, AS_PASSWORD_UNLOCK_VALUE); >>> + if (rval < 0) >>> + goto power_off; >>> + >>> + rval = as3645a_write(flash, AS_BOOST_REG, AS_BOOST_CURRENT_DISABLE); >>> + if (rval < 0) >>> + goto power_off; >>> + >>> + /* Setup default values. This makes sure that the chip is in a known >>> + * state, in case the power rail can't be controlled. >>> + */ >>> + rval = as3645a_setup(flash); >>> + >>> +power_off: >>> + as3645a_set_power(&flash->subdev, 0); >>> + >>> + return rval; >>> +} > > [snip] > >>> +static int as3645a_init_controls(struct as3645a *flash) >>> +{ >>> + struct as3645a_flash_torch_parms *flash_params = NULL; >>> + bool use_ext_strobe = false; >>> + unsigned int leds = 2; >>> + struct v4l2_ctrl *ctrl; >>> + int minimum; >>> + int maximum; >>> + >>> + if (flash->platform_data) { >>> + if (flash->platform_data->num_leds) >>> + leds = flash->platform_data->num_leds; >>> + >>> + flash_params = flash->platform_data->flash_torch_limits; >>> + use_ext_strobe = flash->platform_data->use_ext_flash_strobe; >>> + } >>> + >>> + v4l2_ctrl_handler_init(&flash->ctrls, 9); >>> + >>> + /* V4L2_CID_FLASH_LED_MODE */ >>> + v4l2_ctrl_new_std_menu(&flash->ctrls, &as3645a_ctrl_ops, >>> + V4L2_CID_FLASH_LED_MODE, 2, ~7, >>> + V4L2_FLASH_LED_MODE_NONE); >>> + >>> + /* V4L2_CID_FLASH_STROBE_SOURCE */ >>> + v4l2_ctrl_new_std_menu(&flash->ctrls, &as3645a_ctrl_ops, >>> + V4L2_CID_FLASH_STROBE_SOURCE, >>> + use_ext_strobe ? 1 : 0, use_ext_strobe ? ~3 : ~1, >>> + V4L2_FLASH_STROBE_SOURCE_SOFTWARE); >>> + >>> + flash->strobe_source = V4L2_FLASH_STROBE_SOURCE_SOFTWARE; >>> + >>> + /* V4L2_CID_FLASH_STROBE */ >>> + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, >>> + V4L2_CID_FLASH_STROBE, 0, 0, 0, 0); >>> + >>> + /* V4L2_CID_FLASH_STROBE_STOP */ >>> + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, >>> + V4L2_CID_FLASH_STROBE_STOP, 0, 0, 0, 0); >>> + >>> + /* V4L2_CID_FLASH_TIMEOUT */ >>> + if (flash_params) { >> >> Shouldn't we require valid values for the flash parameters always? >> Chances are very high the default ones for the flash controller won't >> apply for the LED connected to it. > > Good question. If no flash parameters are provided the driver uses the chip's > maximum ratings. I'm fine with both. I guess that the most common case of nonexistent flash current and timeout limits could be just telling they're missing for a reason or another, which can be harmful for the hardware. I'd require them. ... >>> +struct as3645a_platform_data { >>> + int (*set_power)(struct v4l2_subdev *subdev, int on); >>> + /* used to notify the entity which trigger external strobe signal */ >>> + void (*setup_ext_strobe)(int enable); >>> + /* Sends the strobe width to the sensor strobe configuration */ >>> + void (*set_strobe_width)(u32 width_in_us); >> >> I don't think we should have the above two callbacks at all. This should >> be controlled from the user space instead. > > Should I remove external strobe mode completely then ? Not necessarily. I think the control should come from the sensor (or whichever component is controlling the external strobe) instead. Another question is then how this should interact with the flash temperature protection code. Cheers, -- Sakari Ailus sakari.ailus@maxwell.research.nokia.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] as3645a: Add driver for LED flash controller 2011-11-15 15:28 ` Sakari Ailus @ 2011-11-15 15:53 ` Laurent Pinchart 2011-11-15 15:59 ` Sakari Ailus 0 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2011-11-15 15:53 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-media, andriy.shevchenko Hi Sakari, On Tuesday 15 November 2011 16:28:06 Sakari Ailus wrote: > Laurent Pinchart wrote: > > On Monday 14 November 2011 10:34:57 Sakari Ailus wrote: [snip] > >>> + if (rval & ~AS_FAULT_INFO_INDICATOR_LED) > >>> + dev_dbg(&client->dev, "No faults, nice\n"); > >>> + > >>> + return rval; > >>> +} > >>> + > >>> +static int as3645a_get_ctrl(struct v4l2_ctrl *ctrl) > >>> +{ > >>> + struct as3645a *flash = > >>> + container_of(ctrl->handler, struct as3645a, ctrls); > >>> + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); > >>> + int fault; > >>> + > >>> + switch (ctrl->id) { > >>> + case V4L2_CID_FLASH_FAULT: > >>> + fault = as3645a_read_fault(flash); > >>> + if (fault < 0) > >>> + return fault; > >> > >> ctrl->cur.val = 0 here? > > > > fault being negative means that reading the fault register failed. In > > that case I don't think we should update ctrl->cur.val. > > I thought that ctrl->cur.val should be assigned to zero before settings > bits in it. Does that sound better? :-) Oops, my bad, I misunderstood your comment. I'll fix that. > >>> +static int as3645a_registered(struct v4l2_subdev *sd) > >>> +{ > >>> + struct as3645a *flash = to_as3645a(sd); > >>> + struct i2c_client *client = v4l2_get_subdevdata(sd); > >>> + int rval, man, model, rfu, version; > >>> + const char *factory; > >>> + > >>> + /* Power up the flash driver and read manufacturer ID, model ID, RFU > >>> + * and version. > >>> + */ > >>> + as3645a_set_power(&flash->subdev, 1); > >>> + > >>> + rval = as3645a_read(flash, AS_DESIGN_INFO_REG); > >>> + if (rval < 0) > >>> + goto power_off; > >>> + > >>> + man = AS_DESIGN_INFO_FACTORY(rval); > >>> + model = AS_DESIGN_INFO_MODEL(rval); > >>> + > >>> + rval = as3645a_read(flash, AS_VERSION_CONTROL_REG); > >>> + if (rval < 0) > >>> + goto power_off; > >>> + > >>> + rfu = AS_VERSION_CONTROL_RFU(rval); > >>> + version = AS_VERSION_CONTROL_VERSION(rval); > >>> + > >>> + /* Verify the chip model and version. */ > >>> + if (model != 0x0001 || rfu != 0x0000) { > >>> + dev_err(&client->dev, "AS3645A not detected " > >>> + "(model %d rfu %d)\n", model, rfu); > >>> + rval = -ENODEV; > >> > >> Is this so grave issue we should discontinue? I'd perhaps print a > >> warning if even that. > > > > This could mean that the chip isn't an AS3645A/LM3555 at all. Many I2C > > drivers perform the same check, they read the ID register and fail if it > > doesn't contain the expected value. > > Shouldn't we instead check the design info register has a valid id in it > only? The spec I have doesn't say anything about the version of the > chip. I agree that checking the model shoudl be enough. However, if a new chip comes out with an RFU value != 0, it will probably mean that the driver will need to be adapted anyway. Isn't it better to catch that instead of ignoring it ? > I.e. I'd leave the rfu check out. Also, the registers are 8 bit > wide, with each four fields having four bits in all of them. [snip] > >>> +static int as3645a_init_controls(struct as3645a *flash) > >>> +{ > >>> + struct as3645a_flash_torch_parms *flash_params = NULL; > >>> + bool use_ext_strobe = false; > >>> + unsigned int leds = 2; > >>> + struct v4l2_ctrl *ctrl; > >>> + int minimum; > >>> + int maximum; > >>> + > >>> + if (flash->platform_data) { > >>> + if (flash->platform_data->num_leds) > >>> + leds = flash->platform_data->num_leds; > >>> + > >>> + flash_params = flash->platform_data->flash_torch_limits; > >>> + use_ext_strobe = flash->platform_data->use_ext_flash_strobe; > >>> + } > >>> + > >>> + v4l2_ctrl_handler_init(&flash->ctrls, 9); > >>> + > >>> + /* V4L2_CID_FLASH_LED_MODE */ > >>> + v4l2_ctrl_new_std_menu(&flash->ctrls, &as3645a_ctrl_ops, > >>> + V4L2_CID_FLASH_LED_MODE, 2, ~7, > >>> + V4L2_FLASH_LED_MODE_NONE); > >>> + > >>> + /* V4L2_CID_FLASH_STROBE_SOURCE */ > >>> + v4l2_ctrl_new_std_menu(&flash->ctrls, &as3645a_ctrl_ops, > >>> + V4L2_CID_FLASH_STROBE_SOURCE, > >>> + use_ext_strobe ? 1 : 0, use_ext_strobe ? ~3 : ~1, > >>> + V4L2_FLASH_STROBE_SOURCE_SOFTWARE); > >>> + > >>> + flash->strobe_source = V4L2_FLASH_STROBE_SOURCE_SOFTWARE; > >>> + > >>> + /* V4L2_CID_FLASH_STROBE */ > >>> + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > >>> + V4L2_CID_FLASH_STROBE, 0, 0, 0, 0); > >>> + > >>> + /* V4L2_CID_FLASH_STROBE_STOP */ > >>> + v4l2_ctrl_new_std(&flash->ctrls, &as3645a_ctrl_ops, > >>> + V4L2_CID_FLASH_STROBE_STOP, 0, 0, 0, 0); > >>> + > >>> + /* V4L2_CID_FLASH_TIMEOUT */ > >>> + if (flash_params) { > >> > >> Shouldn't we require valid values for the flash parameters always? > >> Chances are very high the default ones for the flash controller won't > >> apply for the LED connected to it. > > > > Good question. If no flash parameters are provided the driver uses the > > chip's maximum ratings. I'm fine with both. > > I guess that the most common case of nonexistent flash current and > timeout limits could be just telling they're missing for a reason or > another, which can be harmful for the hardware. I'd require them. OK. > >>> +struct as3645a_platform_data { > >>> + int (*set_power)(struct v4l2_subdev *subdev, int on); > >>> + /* used to notify the entity which trigger external strobe signal */ > >>> + void (*setup_ext_strobe)(int enable); > >>> + /* Sends the strobe width to the sensor strobe configuration */ > >>> + void (*set_strobe_width)(u32 width_in_us); > >> > >> I don't think we should have the above two callbacks at all. This should > >> be controlled from the user space instead. > > > > Should I remove external strobe mode completely then ? > > Not necessarily. I think the control should come from the sensor (or > whichever component is controlling the external strobe) instead. Another > question is then how this should interact with the flash temperature > protection code. OK, I'll keep external strobe mode. v4 is on its way. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] as3645a: Add driver for LED flash controller 2011-11-15 15:53 ` Laurent Pinchart @ 2011-11-15 15:59 ` Sakari Ailus 0 siblings, 0 replies; 11+ messages in thread From: Sakari Ailus @ 2011-11-15 15:59 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-media, andriy.shevchenko Laurent Pinchart wrote: > Hi Sakari, Heippa, > On Tuesday 15 November 2011 16:28:06 Sakari Ailus wrote: >> Laurent Pinchart wrote: >>> On Monday 14 November 2011 10:34:57 Sakari Ailus wrote: > > [snip] > >>>>> + if (rval & ~AS_FAULT_INFO_INDICATOR_LED) >>>>> + dev_dbg(&client->dev, "No faults, nice\n"); >>>>> + >>>>> + return rval; >>>>> +} >>>>> + >>>>> +static int as3645a_get_ctrl(struct v4l2_ctrl *ctrl) >>>>> +{ >>>>> + struct as3645a *flash = >>>>> + container_of(ctrl->handler, struct as3645a, ctrls); >>>>> + struct i2c_client *client = v4l2_get_subdevdata(&flash->subdev); >>>>> + int fault; >>>>> + >>>>> + switch (ctrl->id) { >>>>> + case V4L2_CID_FLASH_FAULT: >>>>> + fault = as3645a_read_fault(flash); >>>>> + if (fault < 0) >>>>> + return fault; >>>> >>>> ctrl->cur.val = 0 here? >>> >>> fault being negative means that reading the fault register failed. In >>> that case I don't think we should update ctrl->cur.val. >> >> I thought that ctrl->cur.val should be assigned to zero before settings >> bits in it. Does that sound better? :-) > > Oops, my bad, I misunderstood your comment. I'll fix that. Thanks. >>>>> +static int as3645a_registered(struct v4l2_subdev *sd) >>>>> +{ >>>>> + struct as3645a *flash = to_as3645a(sd); >>>>> + struct i2c_client *client = v4l2_get_subdevdata(sd); >>>>> + int rval, man, model, rfu, version; >>>>> + const char *factory; >>>>> + >>>>> + /* Power up the flash driver and read manufacturer ID, model ID, RFU >>>>> + * and version. >>>>> + */ >>>>> + as3645a_set_power(&flash->subdev, 1); >>>>> + >>>>> + rval = as3645a_read(flash, AS_DESIGN_INFO_REG); >>>>> + if (rval < 0) >>>>> + goto power_off; >>>>> + >>>>> + man = AS_DESIGN_INFO_FACTORY(rval); >>>>> + model = AS_DESIGN_INFO_MODEL(rval); >>>>> + >>>>> + rval = as3645a_read(flash, AS_VERSION_CONTROL_REG); >>>>> + if (rval < 0) >>>>> + goto power_off; >>>>> + >>>>> + rfu = AS_VERSION_CONTROL_RFU(rval); >>>>> + version = AS_VERSION_CONTROL_VERSION(rval); >>>>> + >>>>> + /* Verify the chip model and version. */ >>>>> + if (model != 0x0001 || rfu != 0x0000) { >>>>> + dev_err(&client->dev, "AS3645A not detected " >>>>> + "(model %d rfu %d)\n", model, rfu); >>>>> + rval = -ENODEV; >>>> >>>> Is this so grave issue we should discontinue? I'd perhaps print a >>>> warning if even that. >>> >>> This could mean that the chip isn't an AS3645A/LM3555 at all. Many I2C >>> drivers perform the same check, they read the ID register and fail if it >>> doesn't contain the expected value. >> >> Shouldn't we instead check the design info register has a valid id in it >> only? The spec I have doesn't say anything about the version of the >> chip. > > I agree that checking the model shoudl be enough. However, if a new chip comes > out with an RFU value != 0, it will probably mean that the driver will need to > be adapted anyway. Isn't it better to catch that instead of ignoring it ? Ok. I'm fine with that. -- Sakari Ailus sakari.ailus@maxwell.research.nokia.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] as3645a: Add driver for LED flash controller 2011-11-15 13:12 ` Laurent Pinchart 2011-11-15 15:28 ` Sakari Ailus @ 2011-11-15 16:55 ` Andy Shevchenko 2011-11-16 0:37 ` Laurent Pinchart 1 sibling, 1 reply; 11+ messages in thread From: Andy Shevchenko @ 2011-11-15 16:55 UTC (permalink / raw) To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media On Tue, 2011-11-15 at 14:12 +0100, Laurent Pinchart wrote: > > > +struct as3645a { > > > + struct v4l2_subdev subdev; > > > + struct as3645a_platform_data *platform_data; > > > + > > > + struct mutex power_lock; > > > + int power_count; > > > + > > > + /* Static parameters */ > > > + u8 vref; > > > + u8 peak; > > > + > > > + /* Controls */ > > > + struct v4l2_ctrl_handler ctrls; > > > + > > > + enum v4l2_flash_led_mode led_mode; > > > + unsigned int timeout; > > > + u8 flash_current; > > > + u8 assist_current; > > > + u8 indicator_current; > > > + enum v4l2_flash_strobe_source strobe_source; > > > > Do you think we should store this information in the controls instead, > > or not? > > I've been thinking about that as well. The reason why the control values were > copied to the as3645a structure is that they were accessed in timer context, > where taking the control lock wasn't possible. > > I could switch to accessing the information in controls directly now, but that > would require storing pointers to the controls in the as3645a structure, which > might not be that better :-) And the code will need to change back to storing > values when overheat protection will be implemented anyway. If you still think > it's better, I can change it. We don't need to solve the issue which is absent. We have in-kernel adp1653 driver w/o overheat protection. It requires to be updated anyway. I prefer to update drivers in common way when we will have overheat protection framework in place. > > > + switch (man) { > > > + case 1: > > > + factory = "AMS, Austria Micro Systems"; > > > + break; > > > + case 2: > > > + factory = "ADI, Analog Devices Inc."; > > > + break; > > > + case 3: > > > + factory = "NSC, National Semiconductor"; > > > + break; > > > + case 4: > > > + factory = "NXP"; > > > + break; > > > + case 5: > > > + factory = "TI, Texas Instrument"; > > > + break; > > > + default: > > > + factory = "Unknown"; > > > + } > > > + > > > + dev_dbg(&client->dev, "Factory: %s(%d) Version: %d\n", factory, man, > > > + version); > > > > Is that really a factor or is it the chip vendor --- which might > > contract another factory to actually manufacture the chips? > > I don't know :-) I guess the vendor is proper word here. For example, lm3555 (NSC) is slightly different from as3645a. And why dev_dbg? I think dev_info here might be suitable. -- Andy Shevchenko <andriy.shevchenko@linux.intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] as3645a: Add driver for LED flash controller 2011-11-15 16:55 ` Andy Shevchenko @ 2011-11-16 0:37 ` Laurent Pinchart 0 siblings, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2011-11-16 0:37 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Sakari Ailus, linux-media Hi Andy, On Tuesday 15 November 2011 17:55:55 Andy Shevchenko wrote: > On Tue, 2011-11-15 at 14:12 +0100, Laurent Pinchart wrote: > > > > +struct as3645a { > > > > + struct v4l2_subdev subdev; > > > > + struct as3645a_platform_data *platform_data; > > > > + > > > > + struct mutex power_lock; > > > > + int power_count; > > > > + > > > > + /* Static parameters */ > > > > + u8 vref; > > > > + u8 peak; > > > > + > > > > + /* Controls */ > > > > + struct v4l2_ctrl_handler ctrls; > > > > + > > > > + enum v4l2_flash_led_mode led_mode; > > > > + unsigned int timeout; > > > > + u8 flash_current; > > > > + u8 assist_current; > > > > + u8 indicator_current; > > > > + enum v4l2_flash_strobe_source strobe_source; > > > > > > Do you think we should store this information in the controls instead, > > > or not? > > > > I've been thinking about that as well. The reason why the control values > > were copied to the as3645a structure is that they were accessed in timer > > context, where taking the control lock wasn't possible. > > > > I could switch to accessing the information in controls directly now, but > > that would require storing pointers to the controls in the as3645a > > structure, which might not be that better :-) And the code will need to > > change back to storing values when overheat protection will be > > implemented anyway. If you still think it's better, I can change it. > > We don't need to solve the issue which is absent. We have in-kernel > adp1653 driver w/o overheat protection. It requires to be updated > anyway. I prefer to update drivers in common way when we will have > overheat protection framework in place. I'm fine with both solutions. This part of the code works correctly and is not particularly dirty in my opinion. > > > > + switch (man) { > > > > + case 1: > > > > + factory = "AMS, Austria Micro Systems"; > > > > + break; > > > > + case 2: > > > > + factory = "ADI, Analog Devices Inc."; > > > > + break; > > > > + case 3: > > > > + factory = "NSC, National Semiconductor"; > > > > + break; > > > > + case 4: > > > > + factory = "NXP"; > > > > + break; > > > > + case 5: > > > > + factory = "TI, Texas Instrument"; > > > > + break; > > > > + default: > > > > + factory = "Unknown"; > > > > + } > > > > + > > > > + dev_dbg(&client->dev, "Factory: %s(%d) Version: %d\n", factory, > > > > man, + version); > > > > > > Is that really a factor or is it the chip vendor --- which might > > > contract another factory to actually manufacture the chips? > > > > I don't know :-) > > I guess the vendor is proper word here. For example, lm3555 (NSC) is > slightly different from as3645a. OK, I'll fix that. > And why dev_dbg? I think dev_info here might be suitable. I agree, I'll fix that as well. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-11-16 0:37 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-11-14 0:19 [PATCH v2 0/2] as3645a flash driver Laurent Pinchart 2011-11-14 0:19 ` [PATCH v2 1/2] v4l: Add over-current and indicator flash fault bits Laurent Pinchart 2011-11-14 0:19 ` [PATCH v2 2/2] as3645a: Add driver for LED flash controller Laurent Pinchart 2011-11-14 9:34 ` Sakari Ailus 2011-11-15 12:01 ` Andy Shevchenko 2011-11-15 13:12 ` Laurent Pinchart 2011-11-15 15:28 ` Sakari Ailus 2011-11-15 15:53 ` Laurent Pinchart 2011-11-15 15:59 ` Sakari Ailus 2011-11-15 16:55 ` Andy Shevchenko 2011-11-16 0:37 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox