Hi Sylwester
On 2 August 2012 22:18, Sylwester Nawrocki sylvester.nawrocki@gmail.comwrote:
On 08/02/2012 03:42 PM, Sangwook Lee wrote:
This driver implements preview mode of the S5K4ECGX sensor. capture (snapshot) operation, face detection are missing now.
Following controls are supported: contrast/saturation/brightness/sharpness
Signed-off-by: Sangwook Lee<
sangwook.lee-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
...
+static const char * const s5k4ecgx_supply_names[] = {
/** vdd_2.8v is for Analog power supply 2.8V(vdda)* and Digital IO(vddio, vddd_core)*/"vdd_2.8v",Might be better to avoid voltage value in regulator supply names. Can you just make it on of: vdda, vddio, vddcore ? On some systems all 3 power pads might be used and all 3 voltage supply names might be needed. I guess it can be changed if there is a need for it. Also we could specify all 3 entries as above and add such regulator supply names at a corresponding regulator.
Ok, I will change this.
/* vdd_1.8v is for regulator input */"vdd_1.8v",I would suggest just using "vddreg".
+static int s5k4ecgx_write(struct i2c_client *client, u32 addr, u16 val) +{
int ret = 0;Unneeded initialization.
ditto
u16 high = addr>> 16, low = addr& 0xffff;ret = s5k4ecgx_i2c_write(client, REG_CMDWR_ADDRH, high);ret |= s5k4ecgx_i2c_write(client, REG_CMDWR_ADDRL, low);ret |= s5k4ecgx_i2c_write(client, REG_CMDBUF0_ADDR, val);if (ret)return -ENODEV;return 0;+}
+static int s5k4ecgx_read(struct i2c_client *client, u32 addr, u16 *val) +{
int ret = 0;Ditto.
u16 high = addr>> 16, low = addr& 0xffff;ret = s5k4ecgx_i2c_write(client, REG_CMDRD_ADDRH, high);ret |= s5k4ecgx_i2c_write(client, REG_CMDRD_ADDRL, low);ret |= s5k4ecgx_i2c_read(client, REG_CMDBUF0_ADDR, val);if (ret) {dev_err(&client->dev, "Failed to execute read command\n");return -ENODEV;}return 0;+}
+static int s5k4ecgx_set_ahb_address(struct v4l2_subdev *sd) +{
int ret;struct i2c_client *client = v4l2_get_subdevdata(sd);/* Set APB peripherals start address */ret = s5k4ecgx_i2c_write(client, AHB_MSB_ADDR_PTR, GEN_REG_OFFSH);if (ret)return ret;/** FIMXE: This is copied from s5k6aa, because of no information* in s5k4ecgx's datasheet.* sw_reset is activated to put device into idle status*/ret = s5k4ecgx_i2c_write(client, 0x0010, 0x0001);if (ret)return ret;/* FIXME: no information avaialbe about this register */avaialbe -> available
Oops!, I will change this.
ret = s5k4ecgx_i2c_write(client, 0x1030, 0x0000);if (ret)return ret;/* Halt ARM CPU */ret = s5k4ecgx_i2c_write(client, 0x0014, 0x0001);return ret;return s5k4ecgx_i2c_write(...); ?+}
+static int s5k4ecgx_write_array(struct v4l2_subdev *sd,
const struct regval_list *reg)+{
struct i2c_client *client = v4l2_get_subdevdata(sd);u16 addr_incr = 0;int ret = 0;Unneeded initialization.
ditto
while (reg->addr != TOK_TERM) {if (addr_incr != 2)ret = s5k4ecgx_write(client, reg->addr, reg->val);elseret = s5k4ecgx_i2c_write(client, REG_CMDBUF0_ADDR,reg->val);if (ret)break;/* Assume that msg->addr is always less than 0xfffc */addr_incr = (reg + 1)->addr - reg->addr;reg++;}return ret;+}
...
+static int s5k4ecgx_init_sensor(struct v4l2_subdev *sd) +{
int ret = 0;Ditto.
ditto
ret = s5k4ecgx_set_ahb_address(sd);/* The delay is from manufacturer's settings */msleep(100);ret |= s5k4ecgx_write_array(sd, s5k4ecgx_apb_regs);ret |= s5k4ecgx_write_array(sd, s5k4ecgx_img_regs);if (ret)v4l2_err(sd, "Failed to write initial settings\n");return 0;+}
...
+static int s5k4ecgx_s_ctrl(struct v4l2_ctrl *ctrl) +{
struct v4l2_subdev *sd =&container_of(ctrl->handler, structs5k4ecgx,
handler)->sd;struct i2c_client *client = v4l2_get_subdevdata(sd);struct s5k4ecgx *priv = to_s5k4ecgx(sd);int err = 0;Unneded initilization.
Ok, I will change this.
v4l2_dbg(1, debug, sd, "ctrl: 0x%x, value: %d\n", ctrl->id,ctrl->val);
mutex_lock(&priv->lock);switch (ctrl->id) {case V4L2_CID_CONTRAST:err = s5k4ecgx_write(client, REG_USER_CONTRAST, ctrl->val);break;case V4L2_CID_SATURATION:err = s5k4ecgx_write(client, REG_USER_SATURATION,ctrl->val);
break;case V4L2_CID_SHARPNESS:ctrl->val *= SHARPNESS_DIV;err |= s5k4ecgx_write(client, REG_USER_SHARP1, ctrl->val);err |= s5k4ecgx_write(client, REG_USER_SHARP2, ctrl->val);err |= s5k4ecgx_write(client, REG_USER_SHARP3, ctrl->val);err |= s5k4ecgx_write(client, REG_USER_SHARP4, ctrl->val);err |= s5k4ecgx_write(client, REG_USER_SHARP5, ctrl->val);break;case V4L2_CID_BRIGHTNESS:err = s5k4ecgx_write(client, REG_USER_BRIGHTNESS,ctrl->val);
break;}mutex_unlock(&priv->lock);if (err< 0)v4l2_err(sd, "Failed to write s_ctrl err %d\n", err);return err;+}
...
+static const struct v4l2_subdev_core_ops s5k4ecgx_core_ops = {
.s_power = s5k4ecgx_s_power,.log_status = s5k4ecgx_log_status,nit: inconsistent indentation.
ditto
+};
+static int __s5k4ecgx_s_stream(struct v4l2_subdev *sd, int on) +{
struct s5k4ecgx *priv = to_s5k4ecgx(sd);int err = 0;if (on)err = s5k4ecgx_write_array(sd,prev_regs[priv->curr_win->idx]);
return err;if (on) return s5k4ecgx_write_array(sd,prev_regs[priv->curr_win->idx]);
return 0;+}
...
+/*
- Fetching platform data is being done with s_config subdev call.
This comment is false, care to remove it ?
Ok, I will fix this.
- In probe routine, we just register subdev device
- */
+static int s5k4ecgx_probe(struct i2c_client *client,
const struct i2c_device_id *id)+{
int ret, i;struct v4l2_subdev *sd;struct s5k4ecgx *priv;struct s5k4ecgx_platform_data *pdata = client->dev.platform_data;if (pdata == NULL) {dev_err(&client->dev, "platform data is missing!\n");return -EINVAL;}priv = kzalloc(sizeof(struct s5k4ecgx), GFP_KERNEL);devm_kzalloc ?
Ok, I wil apply this.
if (!priv)return -ENOMEM;mutex_init(&priv->lock);priv->msleep = pdata->msleep;priv->streaming = 0;sd =&priv->sd;/* Registering subdev */v4l2_i2c_subdev_init(sd, client,&s5k4ecgx_ops);strlcpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name));sd->internal_ops =&s5k4ecgx_subdev_internal_ops;/* Support v4l2 sub-device userspace API */sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;priv->pad.flags = MEDIA_PAD_FL_SOURCE;sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;ret = media_entity_init(&sd->entity, 1,&priv->pad, 0);if (ret)goto out_err1;ret = s5k4ecgx_config_gpios(priv, pdata);if (ret) {dev_err(&client->dev, "Failed to set gpios\n");goto out_err2;}for (i = 0; i< S5K4ECGX_NUM_SUPPLIES; i++)priv->supplies[i].supply = s5k4ecgx_supply_names[i];ret = regulator_bulk_get(&client->dev, S5K4ECGX_NUM_SUPPLIES,priv->supplies);How about using devm_regulator_bulk_get() ?
Ok, let me look into this.
if (ret) {dev_err(&client->dev, "Failed to get regulators\n");goto out_err3;}ret = s5k4ecgx_init_v4l2_ctrls(priv);if (ret)goto out_err4;return 0;+out_err4:
regulator_bulk_free(S5K4ECGX_NUM_SUPPLIES, priv->supplies);+out_err3:
s5k4ecgx_free_gpios(priv);+out_err2:
media_entity_cleanup(&priv->sd.entity);+out_err1:
kfree(priv);return ret;+}
...
+/**
- struct ss5k4ecgx_platform_data- s5k4ecgx driver platform data
- @gpio_reset: GPIO driving RESET pin
- @gpio_stby : GPIO driving STBY pin
- @msleep : delay (ms) needed after enabling power
Can't it be some default value hardcoded at the driver ?
Ok, let me look into this again.
- */
+struct s5k4ecgx_platform_data {
struct s5k4ecgx_gpio gpio_reset;struct s5k4ecgx_gpio gpio_stby;int msleep;+};
+#endif /* S5K4ECGX_H */
Thanks a lot!
Regards Sangwook
--
Regards, Sylwester