subject:"\[PATCH v9 2\/2\] Add support for OV5647 sensor."

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

2017-02-22 Thread Vladimir Zapolskiy

On 02/22/2017 12:51 PM, Ramiro Oliveira wrote:
> Hi Zakari,
> 
> On 2/21/2017 8:36 PM, Vladimir Zapolskiy wrote:
>> Hi Ramiro,
>>
>> On 02/21/2017 06:42 PM, Ramiro Oliveira wrote:
>>> Hi Vladimir,
>>>
>>> Thank you for your feedback
>>>
>>> On 2/21/2017 3:54 PM, Vladimir Zapolskiy wrote:
 Hi Ramiro,

 please find some review comments below.

 On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
> and RAW 10 output formats, and MIPI CSI-2 interface.
>
> The driver adds support for 640x480 RAW 8.
>
> Signed-off-by: Ramiro Oliveira 
> ---

 [snip]

> +
> +struct ov5647 {
> + struct v4l2_subdev  sd;
> + struct media_padpad;
> + struct mutexlock;
> + struct v4l2_mbus_framefmt   format;
> + unsigned intwidth;
> + unsigned intheight;
> + int power_count;
> + struct clk  *xclk;
> + /* External clock frequency currently supported is 30MHz */
> + u32 xclk_freq;

 See a comment about 25MHz vs 30MHz below.

 Also I assume you can remove 'xclk_freq' from the struct fields,
 it can be replaced by a local variable.

>>>
>>> I'll do that.
>>>
> +};

 [snip]

> +
> +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
> +{
> + int ret;
> + unsigned char data_w[2] = { reg >> 8, reg & 0xff };
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ret = i2c_master_send(client, data_w, 2);
> + if (ret < 0) {
> + dev_dbg(>dev, "%s: i2c read error, reg: %x\n",

 s/i2c read error/i2c write error/

>>>
>>> I'm not sure I understand what you mean.
>>
>> That's a sed expression for string substitution. Here you do 
>> i2c_master_send()
>> but dev_dbg() comment says "i2c read error". It's a simple copy-paste typo 
>> to fix.
>>
> 
> Ohh... now I see. I'll change it.
> 
> + __func__, reg);
> + return ret;
> + }
> +
>>
>> [snip]
>>
> +
> +static int sensor_power(struct v4l2_subdev *sd, int on)
>>
>> On the caller's side (functions ov5647_open() and ov5647_close()) the second
>> argument of the function is of 'bool' type, however .s_power callback from
>> struct v4l2_subdev_core_ops (see include/media/v4l2-subdev.h) defines it as
>> 'int'.
>>
>> It's just a nitpicking, please feel free to ignore the comment above or
>> please consider to change the arguments on callers' side to integers.
>>
>> Also you may consider to add 'ov5647_' prefix to the function name to
>> distinguish it from a potentially added in future sensor_power() function,
>> the original name sounds too generic.
>>
> 
> OK. I'll add the prefix and change the variable type from int to bool.
> 

Just to eliminate any potential misunderstanding, if you consider to reuse
the current sensor_power() function, please change variables from bool to int
on a caller's side, the signature of the function shall not be changed to
match .s_power type.

> +{
> + int ret;
> + struct ov5647 *ov5647 = to_state(sd);
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ret = 0;
> + mutex_lock(>lock);
> +
> + if (on && !ov5647->power_count) {
> + dev_dbg(>dev, "OV5647 power on\n");
> +
> + clk_set_rate(ov5647->xclk, ov5647->xclk_freq);

 Now clk_set_rate() is redundant, please remove it.

 If once it is needed again, please move it to the .probe function, so
 it is called only once in the runtime.

>>>
>>> Ok. I'll remove it for now.
>>>
> +
> + ret = clk_prepare_enable(ov5647->xclk);

 I wonder would it be possible to unload the driver or to unbind the device
 and leave the clock unintentionally enabled? If yes, then this is a bug.

>>>
>>> You're saying that if the driver was unloaded and the clock was left enabled
>>> when the driver was loaded again this line would cause an error?
>>
>> Not exactly, here I saw a potential resource leak, namely a potentially left
>> prepared/enabled clock.
>>
>>>
>>> Should I disable the clock when the driver is removed?
>>>
>>
>> The driver (and framework) shall guarantee that when it is detached from
>> device(s) (e.g. by unloading "ov5647" kernel module or unbinding ov5647 
>> device),
>> all acquired resources are released.
>>
>> But in this particular case most probably I've been overly alert, I believe
>> that V4L2 framework correcly handles device power states, so please ignore my
>> comment.
>>
>> To add something valuable to the review, could you please confirm that
>> ov5647_subdev_internal_ops data is in use by the driver?
>>
>> E.g. shouldn't it be registered by
>>
>>

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

2017-02-22 Thread Ramiro Oliveira

Hi Zakari,

On 2/21/2017 8:36 PM, Vladimir Zapolskiy wrote:
> Hi Ramiro,
> 
> On 02/21/2017 06:42 PM, Ramiro Oliveira wrote:
>> Hi Vladimir,
>>
>> Thank you for your feedback
>>
>> On 2/21/2017 3:54 PM, Vladimir Zapolskiy wrote:
>>> Hi Ramiro,
>>>
>>> please find some review comments below.
>>>
>>> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
 The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
 and RAW 10 output formats, and MIPI CSI-2 interface.

 The driver adds support for 640x480 RAW 8.

 Signed-off-by: Ramiro Oliveira 
 ---
>>>
>>> [snip]
>>>
 +
 +struct ov5647 {
 +  struct v4l2_subdev  sd;
 +  struct media_padpad;
 +  struct mutexlock;
 +  struct v4l2_mbus_framefmt   format;
 +  unsigned intwidth;
 +  unsigned intheight;
 +  int power_count;
 +  struct clk  *xclk;
 +  /* External clock frequency currently supported is 30MHz */
 +  u32 xclk_freq;
>>>
>>> See a comment about 25MHz vs 30MHz below.
>>>
>>> Also I assume you can remove 'xclk_freq' from the struct fields,
>>> it can be replaced by a local variable.
>>>
>>
>> I'll do that.
>>
 +};
>>>
>>> [snip]
>>>
 +
 +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
 +{
 +  int ret;
 +  unsigned char data_w[2] = { reg >> 8, reg & 0xff };
 +  struct i2c_client *client = v4l2_get_subdevdata(sd);
 +
 +  ret = i2c_master_send(client, data_w, 2);
 +  if (ret < 0) {
 +  dev_dbg(>dev, "%s: i2c read error, reg: %x\n",
>>>
>>> s/i2c read error/i2c write error/
>>>
>>
>> I'm not sure I understand what you mean.
> 
> That's a sed expression for string substitution. Here you do i2c_master_send()
> but dev_dbg() comment says "i2c read error". It's a simple copy-paste typo to 
> fix.
> 

Ohh... now I see. I'll change it.

 +  __func__, reg);
 +  return ret;
 +  }
 +
> 
> [snip]
> 
 +
 +static int sensor_power(struct v4l2_subdev *sd, int on)
> 
> On the caller's side (functions ov5647_open() and ov5647_close()) the second
> argument of the function is of 'bool' type, however .s_power callback from
> struct v4l2_subdev_core_ops (see include/media/v4l2-subdev.h) defines it as
> 'int'.
> 
> It's just a nitpicking, please feel free to ignore the comment above or
> please consider to change the arguments on callers' side to integers.
> 
> Also you may consider to add 'ov5647_' prefix to the function name to
> distinguish it from a potentially added in future sensor_power() function,
> the original name sounds too generic.
> 

OK. I'll add the prefix and change the variable type from int to bool.

 +{
 +  int ret;
 +  struct ov5647 *ov5647 = to_state(sd);
 +  struct i2c_client *client = v4l2_get_subdevdata(sd);
 +
 +  ret = 0;
 +  mutex_lock(>lock);
 +
 +  if (on && !ov5647->power_count) {
 +  dev_dbg(>dev, "OV5647 power on\n");
 +
 +  clk_set_rate(ov5647->xclk, ov5647->xclk_freq);
>>>
>>> Now clk_set_rate() is redundant, please remove it.
>>>
>>> If once it is needed again, please move it to the .probe function, so
>>> it is called only once in the runtime.
>>>
>>
>> Ok. I'll remove it for now.
>>
 +
 +  ret = clk_prepare_enable(ov5647->xclk);
>>>
>>> I wonder would it be possible to unload the driver or to unbind the device
>>> and leave the clock unintentionally enabled? If yes, then this is a bug.
>>>
>>
>> You're saying that if the driver was unloaded and the clock was left enabled
>> when the driver was loaded again this line would cause an error?
> 
> Not exactly, here I saw a potential resource leak, namely a potentially left
> prepared/enabled clock.
> 
>>
>> Should I disable the clock when the driver is removed?
>>
> 
> The driver (and framework) shall guarantee that when it is detached from
> device(s) (e.g. by unloading "ov5647" kernel module or unbinding ov5647 
> device),
> all acquired resources are released.
> 
> But in this particular case most probably I've been overly alert, I believe
> that V4L2 framework correcly handles device power states, so please ignore my
> comment.
> 
> To add something valuable to the review, could you please confirm that
> ov5647_subdev_internal_ops data is in use by the driver?
> 
> E.g. shouldn't it be registered by
> 
>   sd->internal_ops = _subdev_internal_ops;
> 
> before calling v4l2_async_register_subdev(sd) ?
> 

You're right, it's not being registered. I think I'll remove them since they
aren't being used.

> --
> With best wishes,
> Vladimir
> 

-- 
Best Regards

Ramiro Oliveira
ramiro.olive...@synopsys.com

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

2017-02-21 Thread Vladimir Zapolskiy

Hi Ramiro,

On 02/21/2017 06:42 PM, Ramiro Oliveira wrote:
> Hi Vladimir,
> 
> Thank you for your feedback
> 
> On 2/21/2017 3:54 PM, Vladimir Zapolskiy wrote:
>> Hi Ramiro,
>>
>> please find some review comments below.
>>
>> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
>>> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
>>> and RAW 10 output formats, and MIPI CSI-2 interface.
>>>
>>> The driver adds support for 640x480 RAW 8.
>>>
>>> Signed-off-by: Ramiro Oliveira 
>>> ---
>>
>> [snip]
>>
>>> +
>>> +struct ov5647 {
>>> +   struct v4l2_subdev  sd;
>>> +   struct media_padpad;
>>> +   struct mutexlock;
>>> +   struct v4l2_mbus_framefmt   format;
>>> +   unsigned intwidth;
>>> +   unsigned intheight;
>>> +   int power_count;
>>> +   struct clk  *xclk;
>>> +   /* External clock frequency currently supported is 30MHz */
>>> +   u32 xclk_freq;
>>
>> See a comment about 25MHz vs 30MHz below.
>>
>> Also I assume you can remove 'xclk_freq' from the struct fields,
>> it can be replaced by a local variable.
>>
> 
> I'll do that.
> 
>>> +};
>>
>> [snip]
>>
>>> +
>>> +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
>>> +{
>>> +   int ret;
>>> +   unsigned char data_w[2] = { reg >> 8, reg & 0xff };
>>> +   struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> +
>>> +   ret = i2c_master_send(client, data_w, 2);
>>> +   if (ret < 0) {
>>> +   dev_dbg(>dev, "%s: i2c read error, reg: %x\n",
>>
>> s/i2c read error/i2c write error/
>>
> 
> I'm not sure I understand what you mean.

That's a sed expression for string substitution. Here you do i2c_master_send()
but dev_dbg() comment says "i2c read error". It's a simple copy-paste typo to 
fix.

>>> +   __func__, reg);
>>> +   return ret;
>>> +   }
>>> +

[snip]

>>> +
>>> +static int sensor_power(struct v4l2_subdev *sd, int on)

On the caller's side (functions ov5647_open() and ov5647_close()) the second
argument of the function is of 'bool' type, however .s_power callback from
struct v4l2_subdev_core_ops (see include/media/v4l2-subdev.h) defines it as
'int'.

It's just a nitpicking, please feel free to ignore the comment above or
please consider to change the arguments on callers' side to integers.

Also you may consider to add 'ov5647_' prefix to the function name to
distinguish it from a potentially added in future sensor_power() function,
the original name sounds too generic.

>>> +{
>>> +   int ret;
>>> +   struct ov5647 *ov5647 = to_state(sd);
>>> +   struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> +
>>> +   ret = 0;
>>> +   mutex_lock(>lock);
>>> +
>>> +   if (on && !ov5647->power_count) {
>>> +   dev_dbg(>dev, "OV5647 power on\n");
>>> +
>>> +   clk_set_rate(ov5647->xclk, ov5647->xclk_freq);
>>
>> Now clk_set_rate() is redundant, please remove it.
>>
>> If once it is needed again, please move it to the .probe function, so
>> it is called only once in the runtime.
>>
> 
> Ok. I'll remove it for now.
> 
>>> +
>>> +   ret = clk_prepare_enable(ov5647->xclk);
>>
>> I wonder would it be possible to unload the driver or to unbind the device
>> and leave the clock unintentionally enabled? If yes, then this is a bug.
>>
> 
> You're saying that if the driver was unloaded and the clock was left enabled
> when the driver was loaded again this line would cause an error?

Not exactly, here I saw a potential resource leak, namely a potentially left
prepared/enabled clock.

> 
> Should I disable the clock when the driver is removed?
> 

The driver (and framework) shall guarantee that when it is detached from
device(s) (e.g. by unloading "ov5647" kernel module or unbinding ov5647 device),
all acquired resources are released.

But in this particular case most probably I've been overly alert, I believe
that V4L2 framework correcly handles device power states, so please ignore my
comment.

To add something valuable to the review, could you please confirm that
ov5647_subdev_internal_ops data is in use by the driver?

E.g. shouldn't it be registered by

  sd->internal_ops = _subdev_internal_ops;

before calling v4l2_async_register_subdev(sd) ?

--
With best wishes,
Vladimir

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

2017-02-21 Thread Ramiro Oliveira

Hi Vladimir,

Thank you for your feedback

On 2/21/2017 3:54 PM, Vladimir Zapolskiy wrote:
> Hi Ramiro,
> 
> please find some review comments below.
> 
> On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
>> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
>> and RAW 10 output formats, and MIPI CSI-2 interface.
>>
>> The driver adds support for 640x480 RAW 8.
>>
>> Signed-off-by: Ramiro Oliveira 
>> ---
> 
> [snip]
> 
>> +
>> +struct ov5647 {
>> +struct v4l2_subdev  sd;
>> +struct media_padpad;
>> +struct mutexlock;
>> +struct v4l2_mbus_framefmt   format;
>> +unsigned intwidth;
>> +unsigned intheight;
>> +int power_count;
>> +struct clk  *xclk;
>> +/* External clock frequency currently supported is 30MHz */
>> +u32 xclk_freq;
> 
> See a comment about 25MHz vs 30MHz below.
> 
> Also I assume you can remove 'xclk_freq' from the struct fields,
> it can be replaced by a local variable.
> 

I'll do that.

>> +};
> 
> [snip]
> 
>> +
>> +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
>> +{
>> +int ret;
>> +unsigned char data_w[2] = { reg >> 8, reg & 0xff };
>> +struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> +ret = i2c_master_send(client, data_w, 2);
>> +if (ret < 0) {
>> +dev_dbg(>dev, "%s: i2c read error, reg: %x\n",
> 
> s/i2c read error/i2c write error/
> 

I'm not sure I understand what you mean.

>> +__func__, reg);
>> +return ret;
>> +}
>> +
>> +ret = i2c_master_recv(client, val, 1);
>> +if (ret < 0)
>> +dev_dbg(>dev, "%s: i2c read error, reg: %x\n",
>> +__func__, reg);
>> +
>> +return ret;
>> +
> 
> Please remove the empty line above.
> 

Ok.

>> +}
>> +
>> +static int ov5647_write_array(struct v4l2_subdev *sd,
>> +struct regval_list *regs, int array_size)
>> +{
>> +int i = 0, ret;
> 
> Assignment of 'i' on declaration is not needed, please remove.
> 

Ok.

>> +
>> +for (i = 0; i < array_size; i++) {
>> +ret = ov5647_write(sd, regs[i].addr, regs[i].data);
>> +if (ret < 0)
>> +return ret;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
>> +{
>> +u8 channel_id;
>> +int ret;
>> +
>> +ret = ov5647_read(sd, 0x4814, _id);
>> +if (ret < 0)
>> +return ret;
>> +
>> +channel_id &= ~(3 << 6);
>> +return ov5647_write(sd, 0x4814, channel_id | (channel << 6));
>> +}
>> +
>> +static int ov5647_stream_on(struct v4l2_subdev *sd)
>> +{
>> +struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> +ov5647_write(sd, 0x4202, 0x00);
> 
> Should you add a check of the returned value?
> 

I'll add it.

>> +
>> +dev_dbg(>dev, "Stream on");
> 
> I would suggest to remove dev_dbg(), because ftrace will report to a user,
> when this function is called.
> 
> Also dev_dbg() in the middle of two I2C transfers in a row looks as being
> placed improperly.
> 

I'll remove it.

>> +
>> +return ov5647_write(sd, 0x300D, 0x00);
>> +}
>> +
>> +static int ov5647_stream_off(struct v4l2_subdev *sd)
>> +{
>> +struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> +ov5647_write(sd, 0x4202, 0x0f);
> 
> Should you add a check of the returned value?
> 

I'll add it.

>> +
>> +dev_dbg(>dev, "Stream off");
> 
> I would suggest to remove dev_dbg(), because ftrace will report to a user,
> when this function is called.
> 
> Also dev_dbg() in the middle of two I2C transfers in a row looks as being
> placed improperly.
> 

I'll remove it.

>> +
>> +return ov5647_write(sd, 0x300D, 0x01);
>> +}
>> +
>> +static int set_sw_standby(struct v4l2_subdev *sd, bool standby)
>> +{
>> +int ret;
>> +u8 rdval;
>> +
>> +ret = ov5647_read(sd, 0x0100, );
>> +if (ret < 0)
>> +return ret;
>> +
>> +if (standby)
>> +rdval &= ~0x01;
>> +else
>> +rdval |= 0x01;
>> +
>> +return ov5647_write(sd, 0x0100, rdval);
>> +}
>> +
>> +static int __sensor_init(struct v4l2_subdev *sd)
>> +{
>> +int ret;
>> +u8 resetval;
>> +u8 rdval;
> 
> It could be possible to put declarations of 'resetval' and 'rdval' on the 
> same line.
> 

Sure.

>> +struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> +dev_dbg(>dev, "sensor init\n");
>> +
>> +ret = ov5647_read(sd, 0x0100, );
>> +if (ret < 0)
>> +return ret;
>> +
>> +ret = ov5647_write_array(sd, ov5647_640x480,
>> +ARRAY_SIZE(ov5647_640x480));
>> +if (ret < 0) {
>> +dev_err(>dev, "write sensor default regs error\n");
>> +return ret;
>> +}
>> +
>> +

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

2017-02-21 Thread Vladimir Zapolskiy

Hi Ramiro,

please find some review comments below.

On 02/17/2017 03:14 PM, Ramiro Oliveira wrote:
> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
> and RAW 10 output formats, and MIPI CSI-2 interface.
> 
> The driver adds support for 640x480 RAW 8.
> 
> Signed-off-by: Ramiro Oliveira 
> ---

[snip]

> +
> +struct ov5647 {
> + struct v4l2_subdev  sd;
> + struct media_padpad;
> + struct mutexlock;
> + struct v4l2_mbus_framefmt   format;
> + unsigned intwidth;
> + unsigned intheight;
> + int power_count;
> + struct clk  *xclk;
> + /* External clock frequency currently supported is 30MHz */
> + u32 xclk_freq;

See a comment about 25MHz vs 30MHz below.

Also I assume you can remove 'xclk_freq' from the struct fields,
it can be replaced by a local variable.

> +};

[snip]

> +
> +static int ov5647_read(struct v4l2_subdev *sd, u16 reg, u8 *val)
> +{
> + int ret;
> + unsigned char data_w[2] = { reg >> 8, reg & 0xff };
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ret = i2c_master_send(client, data_w, 2);
> + if (ret < 0) {
> + dev_dbg(>dev, "%s: i2c read error, reg: %x\n",

s/i2c read error/i2c write error/

> + __func__, reg);
> + return ret;
> + }
> +
> + ret = i2c_master_recv(client, val, 1);
> + if (ret < 0)
> + dev_dbg(>dev, "%s: i2c read error, reg: %x\n",
> + __func__, reg);
> +
> + return ret;
> +

Please remove the empty line above.

> +}
> +
> +static int ov5647_write_array(struct v4l2_subdev *sd,
> + struct regval_list *regs, int array_size)
> +{
> + int i = 0, ret;

Assignment of 'i' on declaration is not needed, please remove.

> +
> + for (i = 0; i < array_size; i++) {
> + ret = ov5647_write(sd, regs[i].addr, regs[i].data);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ov5647_set_virtual_channel(struct v4l2_subdev *sd, int channel)
> +{
> + u8 channel_id;
> + int ret;
> +
> + ret = ov5647_read(sd, 0x4814, _id);
> + if (ret < 0)
> + return ret;
> +
> + channel_id &= ~(3 << 6);
> + return ov5647_write(sd, 0x4814, channel_id | (channel << 6));
> +}
> +
> +static int ov5647_stream_on(struct v4l2_subdev *sd)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ov5647_write(sd, 0x4202, 0x00);

Should you add a check of the returned value?

> +
> + dev_dbg(>dev, "Stream on");

I would suggest to remove dev_dbg(), because ftrace will report to a user,
when this function is called.

Also dev_dbg() in the middle of two I2C transfers in a row looks as being
placed improperly.

> +
> + return ov5647_write(sd, 0x300D, 0x00);
> +}
> +
> +static int ov5647_stream_off(struct v4l2_subdev *sd)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + ov5647_write(sd, 0x4202, 0x0f);

Should you add a check of the returned value?

> +
> + dev_dbg(>dev, "Stream off");

I would suggest to remove dev_dbg(), because ftrace will report to a user,
when this function is called.

Also dev_dbg() in the middle of two I2C transfers in a row looks as being
placed improperly.

> +
> + return ov5647_write(sd, 0x300D, 0x01);
> +}
> +
> +static int set_sw_standby(struct v4l2_subdev *sd, bool standby)
> +{
> + int ret;
> + u8 rdval;
> +
> + ret = ov5647_read(sd, 0x0100, );
> + if (ret < 0)
> + return ret;
> +
> + if (standby)
> + rdval &= ~0x01;
> + else
> + rdval |= 0x01;
> +
> + return ov5647_write(sd, 0x0100, rdval);
> +}
> +
> +static int __sensor_init(struct v4l2_subdev *sd)
> +{
> + int ret;
> + u8 resetval;
> + u8 rdval;

It could be possible to put declarations of 'resetval' and 'rdval' on the same 
line.

> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + dev_dbg(>dev, "sensor init\n");
> +
> + ret = ov5647_read(sd, 0x0100, );
> + if (ret < 0)
> + return ret;
> +
> + ret = ov5647_write_array(sd, ov5647_640x480,
> + ARRAY_SIZE(ov5647_640x480));
> + if (ret < 0) {
> + dev_err(>dev, "write sensor default regs error\n");
> + return ret;
> + }
> +
> + ret = ov5647_set_virtual_channel(sd, 0);
> + if (ret < 0)
> + return ret;
> +
> + ret = ov5647_read(sd, 0x0100, );
> + if (ret < 0)
> + return ret;
> +
> + if (!(resetval & 0x01)) {
> + dev_err(>dev, "Device was in SW standby");
> + ret = ov5647_write(sd, 0x0100, 0x01);
> + if (ret < 0)
> +

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

2017-02-21 Thread Sakari Ailus

Hi Ramiro,

On Tue, Feb 21, 2017 at 02:49:12PM +, Ramiro Oliveira wrote:
> Hi Sakari,
> 
> Thank you for your feedback.

You're welcome.

An additional note as you don't implement any controls --- some CSI-2
receivers may need to know the link frequency to know how to program the
receiver; they'll simply fail to start streaming if you don't.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

2017-02-21 Thread Ramiro Oliveira

Hi Sakari,

Thank you for your feedback.

On 2/21/2017 12:09 PM, Sakari Ailus wrote:
> Hi Ramiro,
> 
> A few minor comments below.
> 
> On Fri, Feb 17, 2017 at 01:14:16PM +, Ramiro Oliveira wrote:
>> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
>> and RAW 10 output formats, and MIPI CSI-2 interface.
>>
>> The driver adds support for 640x480 RAW 8.
>>
>> Signed-off-by: Ramiro Oliveira 
>> ---
>>  MAINTAINERS|   7 +
>>  drivers/media/i2c/Kconfig  |  11 +
>>  drivers/media/i2c/Makefile |   1 +
>>  drivers/media/i2c/ov5647.c | 638 
>> +
>>  4 files changed, 657 insertions(+)
>>  create mode 100644 drivers/media/i2c/ov5647.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8e7e8d7855ee..7bbca271acc8 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9109,6 +9109,13 @@ M:Harald Welte 
>>  S:  Maintained
>>  F:  drivers/char/pcmcia/cm4040_cs.*
>>  
>> +OMNIVISION OV5647 SENSOR DRIVER
>> +M:  Ramiro Oliveira 
>> +L:  linux-media@vger.kernel.org
>> +T:  git git://linuxtv.org/media_tree.git
>> +S:  Maintained
>> +F:  drivers/media/i2c/ov5647.c
>> +
>>  OMNIVISION OV7670 SENSOR DRIVER
>>  M:  Jonathan Corbet 
>>  L:  linux-media@vger.kernel.org
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index cee1dae6e014..8435b99f38bc 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -531,6 +531,17 @@ config VIDEO_OV2659
>>To compile this driver as a module, choose M here: the
>>module will be called ov2659.
>>  
>> +config VIDEO_OV5647
>> +tristate "OmniVision OV5647 sensor support"
>> +depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>> +depends on MEDIA_CAMERA_SUPPORT
>> +---help---
>> +  This is a Video4Linux2 sensor-level driver for the OmniVision
>> +  OV5647 camera.
>> +
>> +  To compile this driver as a module, choose M here: the
>> +  module will be called ov5647.
>> +
>>  config VIDEO_OV7640
>>  tristate "OmniVision OV7640 sensor support"
>>  depends on I2C && VIDEO_V4L2
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index 5bc7bbeb5499..ef2ccf65f94c 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -83,3 +83,4 @@ obj-$(CONFIG_VIDEO_IR_I2C)  += ir-kbd-i2c.o
>>  obj-$(CONFIG_VIDEO_ML86V7667)   += ml86v7667.o
>>  obj-$(CONFIG_VIDEO_OV2659)  += ov2659.o
>>  obj-$(CONFIG_VIDEO_TC358743)+= tc358743.o
>> +obj-$(CONFIG_VIDEO_OV5647)  += ov5647.o
>> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
>> new file mode 100644
>> index ..34e620fabbaf
>> --- /dev/null
>> +++ b/drivers/media/i2c/ov5647.c
>> @@ -0,0 +1,638 @@
>> +/*
>> + * A V4L2 driver for OmniVision OV5647 cameras.
>> + *
>> + * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver
>> + * Copyright (C) 2011 Sylwester Nawrocki 
>> + *
>> + * Based on Omnivision OV7670 Camera Driver
>> + * Copyright (C) 2006-7 Jonathan Corbet 
>> + *
>> + * Copyright (C) 2016, Synopsys, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed .as is. WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
> 
> You don't seem to implement any controls.
> 
> The sensor likely supports exposure time (and maybe also gain) the user
> might want to control. Is there an intent to add that? Otherwise the header
> should be removed.
> 

I plan to add them in the future but for now I think I'll just remove the 
header.

>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define SENSOR_NAME "ov5647"
>> +
>> +#define OV5647_SW_RESET 0x1003
>> +#define OV5647_REG_CHIPID_H 0x300A
>> +#define OV5647_REG_CHIPID_L 0x300B
>> +
>> +#define REG_TERM 0xfffe
>> +#define VAL_TERM 0xfe
>> +#define REG_DLY  0x
>> +
>> +#define OV5647_ROW_START0x01
>> +#define OV5647_ROW_START_MIN0
>> +#define OV5647_ROW_START_MAX2004
>> +#define OV5647_ROW_START_DEF54
>> +
>> +#define OV5647_COLUMN_START 0x02
>> +#define OV5647_COLUMN_START_MIN 0
>> +#define OV5647_COLUMN_START_MAX 2750
>> +#define OV5647_COLUMN_START_DEF 16
>> +
>> +#define OV5647_WINDOW_HEIGHT0x03
>> +#define OV5647_WINDOW_HEIGHT_MIN2
>> +#define

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

2017-02-21 Thread Sakari Ailus

Hi Ramiro,

A few minor comments below.

On Fri, Feb 17, 2017 at 01:14:16PM +, Ramiro Oliveira wrote:
> The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
> and RAW 10 output formats, and MIPI CSI-2 interface.
> 
> The driver adds support for 640x480 RAW 8.
> 
> Signed-off-by: Ramiro Oliveira 
> ---
>  MAINTAINERS|   7 +
>  drivers/media/i2c/Kconfig  |  11 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/ov5647.c | 638 
> +
>  4 files changed, 657 insertions(+)
>  create mode 100644 drivers/media/i2c/ov5647.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8e7e8d7855ee..7bbca271acc8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9109,6 +9109,13 @@ M: Harald Welte 
>  S:   Maintained
>  F:   drivers/char/pcmcia/cm4040_cs.*
>  
> +OMNIVISION OV5647 SENSOR DRIVER
> +M:   Ramiro Oliveira 
> +L:   linux-media@vger.kernel.org
> +T:   git git://linuxtv.org/media_tree.git
> +S:   Maintained
> +F:   drivers/media/i2c/ov5647.c
> +
>  OMNIVISION OV7670 SENSOR DRIVER
>  M:   Jonathan Corbet 
>  L:   linux-media@vger.kernel.org
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index cee1dae6e014..8435b99f38bc 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -531,6 +531,17 @@ config VIDEO_OV2659
> To compile this driver as a module, choose M here: the
> module will be called ov2659.
>  
> +config VIDEO_OV5647
> + tristate "OmniVision OV5647 sensor support"
> + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> + depends on MEDIA_CAMERA_SUPPORT
> + ---help---
> +   This is a Video4Linux2 sensor-level driver for the OmniVision
> +   OV5647 camera.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called ov5647.
> +
>  config VIDEO_OV7640
>   tristate "OmniVision OV7640 sensor support"
>   depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 5bc7bbeb5499..ef2ccf65f94c 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -83,3 +83,4 @@ obj-$(CONFIG_VIDEO_IR_I2C)  += ir-kbd-i2c.o
>  obj-$(CONFIG_VIDEO_ML86V7667)+= ml86v7667.o
>  obj-$(CONFIG_VIDEO_OV2659)   += ov2659.o
>  obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
> +obj-$(CONFIG_VIDEO_OV5647)   += ov5647.o
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> new file mode 100644
> index ..34e620fabbaf
> --- /dev/null
> +++ b/drivers/media/i2c/ov5647.c
> @@ -0,0 +1,638 @@
> +/*
> + * A V4L2 driver for OmniVision OV5647 cameras.
> + *
> + * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver
> + * Copyright (C) 2011 Sylwester Nawrocki 
> + *
> + * Based on Omnivision OV7670 Camera Driver
> + * Copyright (C) 2006-7 Jonathan Corbet 
> + *
> + * Copyright (C) 2016, Synopsys, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed .as is. WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

You don't seem to implement any controls.

The sensor likely supports exposure time (and maybe also gain) the user
might want to control. Is there an intent to add that? Otherwise the header
should be removed.

> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SENSOR_NAME "ov5647"
> +
> +#define OV5647_SW_RESET  0x1003
> +#define OV5647_REG_CHIPID_H  0x300A
> +#define OV5647_REG_CHIPID_L  0x300B
> +
> +#define REG_TERM 0xfffe
> +#define VAL_TERM 0xfe
> +#define REG_DLY  0x
> +
> +#define OV5647_ROW_START 0x01
> +#define OV5647_ROW_START_MIN 0
> +#define OV5647_ROW_START_MAX 2004
> +#define OV5647_ROW_START_DEF 54
> +
> +#define OV5647_COLUMN_START  0x02
> +#define OV5647_COLUMN_START_MIN  0
> +#define OV5647_COLUMN_START_MAX  2750
> +#define OV5647_COLUMN_START_DEF  16
> +
> +#define OV5647_WINDOW_HEIGHT 0x03
> +#define OV5647_WINDOW_HEIGHT_MIN 2
> +#define OV5647_WINDOW_HEIGHT_MAX 2006
> +#define OV5647_WINDOW_HEIGHT_DEF 1944
> +
> +#define OV5647_WINDOW_WIDTH  0x04
> +#define OV5647_WINDOW_WIDTH_MIN  2
> +#define OV5647_WINDOW_WIDTH_MAX  2752
> +#define OV5647_WINDOW_WIDTH_DEF  2592
> +
> +struct regval_list {
> +

[PATCH v9 2/2] Add support for OV5647 sensor.

2017-02-17 Thread Ramiro Oliveira

The OV5647 sensor from Omnivision supports up to 2592x1944 @ 15 fps, RAW 8
and RAW 10 output formats, and MIPI CSI-2 interface.

The driver adds support for 640x480 RAW 8.

Signed-off-by: Ramiro Oliveira 
---
 MAINTAINERS|   7 +
 drivers/media/i2c/Kconfig  |  11 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/ov5647.c | 638 +
 4 files changed, 657 insertions(+)
 create mode 100644 drivers/media/i2c/ov5647.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8e7e8d7855ee..7bbca271acc8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9109,6 +9109,13 @@ M:   Harald Welte 
 S: Maintained
 F: drivers/char/pcmcia/cm4040_cs.*
 
+OMNIVISION OV5647 SENSOR DRIVER
+M: Ramiro Oliveira 
+L: linux-media@vger.kernel.org
+T: git git://linuxtv.org/media_tree.git
+S: Maintained
+F: drivers/media/i2c/ov5647.c
+
 OMNIVISION OV7670 SENSOR DRIVER
 M: Jonathan Corbet 
 L: linux-media@vger.kernel.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index cee1dae6e014..8435b99f38bc 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -531,6 +531,17 @@ config VIDEO_OV2659
  To compile this driver as a module, choose M here: the
  module will be called ov2659.
 
+config VIDEO_OV5647
+   tristate "OmniVision OV5647 sensor support"
+   depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
+   depends on MEDIA_CAMERA_SUPPORT
+   ---help---
+ This is a Video4Linux2 sensor-level driver for the OmniVision
+ OV5647 camera.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ov5647.
+
 config VIDEO_OV7640
tristate "OmniVision OV7640 sensor support"
depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 5bc7bbeb5499..ef2ccf65f94c 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -83,3 +83,4 @@ obj-$(CONFIG_VIDEO_IR_I2C)  += ir-kbd-i2c.o
 obj-$(CONFIG_VIDEO_ML86V7667)  += ml86v7667.o
 obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
 obj-$(CONFIG_VIDEO_TC358743)   += tc358743.o
+obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
new file mode 100644
index ..34e620fabbaf
--- /dev/null
+++ b/drivers/media/i2c/ov5647.c
@@ -0,0 +1,638 @@
+/*
+ * A V4L2 driver for OmniVision OV5647 cameras.
+ *
+ * Based on Samsung S5K6AAFX SXGA 1/6" 1.3M CMOS Image Sensor driver
+ * Copyright (C) 2011 Sylwester Nawrocki 
+ *
+ * Based on Omnivision OV7670 Camera Driver
+ * Copyright (C) 2006-7 Jonathan Corbet 
+ *
+ * Copyright (C) 2016, Synopsys, Inc.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed .as is. WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SENSOR_NAME "ov5647"
+
+#define OV5647_SW_RESET0x1003
+#define OV5647_REG_CHIPID_H0x300A
+#define OV5647_REG_CHIPID_L0x300B
+
+#define REG_TERM 0xfffe
+#define VAL_TERM 0xfe
+#define REG_DLY  0x
+
+#define OV5647_ROW_START   0x01
+#define OV5647_ROW_START_MIN   0
+#define OV5647_ROW_START_MAX   2004
+#define OV5647_ROW_START_DEF   54
+
+#define OV5647_COLUMN_START0x02
+#define OV5647_COLUMN_START_MIN0
+#define OV5647_COLUMN_START_MAX2750
+#define OV5647_COLUMN_START_DEF16
+
+#define OV5647_WINDOW_HEIGHT   0x03
+#define OV5647_WINDOW_HEIGHT_MIN   2
+#define OV5647_WINDOW_HEIGHT_MAX   2006
+#define OV5647_WINDOW_HEIGHT_DEF   1944
+
+#define OV5647_WINDOW_WIDTH0x04
+#define OV5647_WINDOW_WIDTH_MIN2
+#define OV5647_WINDOW_WIDTH_MAX2752
+#define OV5647_WINDOW_WIDTH_DEF2592
+
+struct regval_list {
+   u16 addr;
+   u8 data;
+};
+
+struct ov5647 {
+   struct v4l2_subdev  sd;
+   struct media_padpad;
+   struct mutexlock;
+   struct v4l2_mbus_framefmt   format;
+   unsigned intwidth;
+   unsigned intheight;
+   int power_count;
+   struct clk  *xclk;
+   /* External clock frequency currently supported is 30MHz */
+   u32

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

Re: [PATCH v9 2/2] Add support for OV5647 sensor.

[PATCH v9 2/2] Add support for OV5647 sensor.

9 matches

Site Navigation

Mail list logo

Footer information