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

Reply via email to