Hi Jean-Philippe,
Thanks for the review.
<snip>
> > +static const struct ov10635_reg ov10635_regs_enable[] = {
> > + { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, {
> 0x3042, 0xf0 },
> > + { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, {
> 0x3042, 0xf0 },
> > + { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, {
> 0x3042, 0xf0 },
> > + { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, {
> 0x3042, 0xf0 },
> > + { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, {
> 0x3042, 0xf0 },
> > + { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, {
> 0x3042, 0xf0 },
> > + { 0x3042, 0xf0 }, { 0x3042, 0xf0 }, { 0x301b, 0xf0 }, {
> 0x301c, 0xf0 },
> > + { 0x301a, 0xf0 },
> > +};
>
> Register 0x3042 is only touched by the enable part, not by the "change
> mode" part
> I think you could move the {0x3042, 0xf0} sequence in the
> standard_regs array, and keep
> only the 0x301b, 0x301c, 0x301a registers.
I tried this, but it doesn't work. You have to write to the 0x3042
register after other setup writes.
> By the way, did you test with a single write ? There is the same
> sequence in ov5642
> init, so I believe it is copy pasted in every omnivision init. Is it
> actually useful ?
I tried this & a single write works so I'll change this. iirc, it was
taken from a reference set of writes from OmniVision.
<snip>
> > +static int ov10635_video_probe(struct i2c_client *client)
> > +{
> > + struct ov10635_priv *priv = to_ov10635(client);
> > + u8 pid, ver;
> > + int ret;
> > +
> > + /* Program all the 'standard' registers */
> > + ret = ov10635_set_regs(client, ov10635_regs_default,
> > + ARRAY_SIZE(ov10635_regs_default));
> > + if (ret)
> > + return ret;
> > +
> > + /* check and show product ID and manufacturer ID */
> > + ret = ov10635_reg_read(client, OV10635_PID, &pid);
> > + if (ret)
> > + return ret;
> > + ret = ov10635_reg_read(client, OV10635_VER, &ver);
> > + if (ret)
> > + return ret;
> > +
> > + if (OV10635_VERSION(pid, ver) != OV10635_VERSION_REG) {
> > + dev_err(&client->dev, "Product ID error %x:%x\n", pid,
ver);
> > + return -ENODEV;
> > + }
>
> Shouldn't the order be reversed here ?
> iow, first chek chip id register, then proceed with the register init ?
Good point! I'll fix this.
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
