Hi Sylwester On 2 August 2012 22:18, Sylwester Nawrocki <sylvester.nawro...@gmail.com>wrote:
> 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+yrof...@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); > > + else > > + ret = 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, struct > s5k4ecgx, > > + 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 >
_______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev