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.pinch...@ideasonboard.com>
> Signed-off-by: Nayden Kanchev <nkanc...@mm-sol.com>
> Signed-off-by: Tuukka Toivonen <tuukka...@gmail.com>
> Signed-off-by: Antti Koskipaa <antti.koski...@gmail.com>
> Signed-off-by: Stanimir Varbanov <svarba...@mm-sol.com>
> Signed-off-by: Vimarsh Zutshi <vimarsh.zut...@gmail.com>
> Signed-off-by: Sakari Ailus <sakari.ai...@iki.fi>
> Signed-off-by: Ivan T. Ivanov <iiva...@mm-sol.com>
> Signed-off-by: Mika Westerberg <ext-mika.1.westerb...@nokia.com>
> Signed-off-by: David Cohen <daco...@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.pinch...@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.pinch...@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.pinch...@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.ai...@maxwell.research.nokia.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to