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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
