Re: [PATCH v3] ov10635: Add OmniVision ov10635 SoC camera driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Guennadi,

> > > > +/* read a register */
> > > > +static int ov10635_reg_read(struct i2c_client *client, u16 reg, 
u8 
> > *val)
> > > > +{
> > > > +   int ret;
> > > > +   u8 data[2] = { reg >> 8, reg & 0xff };
> > > > +   struct i2c_msg msg = {
> > > > +      .addr   = client->addr,
> > > > +      .flags   = 0,
> > > > +      .len   = 2,
> > > > +      .buf   = data,
> > > > +   };
> > > > +
> > > > +   ret = i2c_transfer(client->adapter, &msg, 1);
> > > > +   if (ret < 0)
> > > > +      goto err;
> > > > +
> > > > +   msg.flags = I2C_M_RD;
> > > > +   msg.len   = 1;
> > > > +   msg.buf   = data,
> > > > +   ret = i2c_transfer(client->adapter, &msg, 1);
> > > > +   if (ret < 0)
> > > > +      goto err;
> > > > +
> > > > +   *val = data[0];
> > > 
> > > I think, you can do this in one I2C transfer with 2 messages. Look 
e.g. 
> > > imx074.c. Although, now looking at it, I'm not sure why it has .len 
= 2 
> > in 
> > > the second message...
> > Ok, I'll change this to one i2c transfer. As you sauy, no idea why the 
imx 
> > code is reading 2 bytes though...
> 
> But I don't have any way to test it anymore, anyway: the only user - 
> ap4evb - is gone now. So, that driver doesn't matter much anymore. We 
can 
> just fix that blindly without testing, or we can leave it as is and mark 

> the driver broken, or we can remove it completely.
ok

> [snip]
> 
> > > > +         continue;
> > > > +
> > > > +      /* Mult = reg 0x3003, bits 5:0 */
> > > 
> > > You could also define macros for 0x3003, 0x3004 and others, where 
you 
> > know 
> > > the role of those registers, even if not their official names.
> > Do you mean macros for the bit fields?
> 
> No, primarily I mean macros for register addresses.
ok

> [snip]
> 
> > > > +            /* 2 clock cycles for every YUV422 pixel */
> > > > +            if (pclk < (((hts * *vtsmin)/fps_denominator)
> > > > +               * fps_numerator * 2))
> > > 
> > > Actually just
> > > 
> > > +            if (pclk < hts * *vtsmin / fps_denominator
> > > +               * fps_numerator * 2)
> > > 
> > > would do just fine
> > It would, but I think we should use parenthesis here to ensure the 
divide 
> > by the denominator happens before multiplying by the numerator. This 
is to 
> > ensure the value doesn't overflow.
> 
> I think the C standard already guarantees that. You only need 
parenthesis 
> to enforce a non-standard calculation order.
ok, but I think I'll add a comment about being careful to avoid overflow.

> [snip]
> 
> > > > +static int ov10635_g_crop(struct v4l2_subdev *sd, struct 
v4l2_crop *a)
> > > > +{
> > > > +   struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > > +   struct ov10635_priv *priv = to_ov10635(client);
> > > > +
> > > > +   if (priv) {
> > > > +      a->c.width = priv->width;
> > > > +      a->c.height = priv->height;
> > > 
> > > Wait, what is priv->width and priv->height? Are they sensor output 
sizes 
> > > or crop sizes?
> > Sensor output sizes. Ah, I guess this is one of the few 
cameras/drivers 
> > that can support setup the sensor for any size (except restrictions 
for 
> > 4:2:2 format). So maybe I should not implement these functions? 
Looking at 
> > the CEU SoC camera host driver, it would then use the defrect cropcap. 
I 
> > am not sure what that will be though.
> 
> Cropping and scaling are two different functions. Cropping selects an 
area 
> on the sensor matrix to use for data sampling. Scaling configures to 
which 
> output rectangle to scale that area. So, since your camera can do both 
and 
> your driver supports both, you shouldn't return the same sizes in 
.g_fmt() 
> and .g_crop() unless, of course, a 1:1 scale has been set. And currently 

> you do exactly that - return priv->width and priv->height in both.
Ok, now I see. My comment about the sensor output size changing is wrong. 
The sensor doesn't do any scaling, so we are cropping it.

Thanks
Phil
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux