On Tue, 5 Jan 2010, Kuninori Morimoto wrote:
>
> Dear Guennadi
>
> Thank you for checking patch
>
> > Can you explain a bit why this patch is needed? Apart from a slight
> > stylistic improvement and a saving of 4 bytes of platform data per camera
> > instance? Is it going to be needed for some future changes?
>
> This patch is not so important/necessary at once.
> -> for saving of 4 bytes.
>
> > if (!is_power_of_2(priv->info->flags & (OV772X_FLAG_8BIT | OV772X_FLAG_10BIT)))
> > return 0;
> >
> > make sense here? Or even just modify your tests above to
> (snip)
> > Adding a "default:" case just above the "case OV772X_FLAG_10BIT:" line
> > would seem like a good idea to me too.
>
> I understand.
>
> > > +#define OV772X_FLAG_8BIT (1 << 2) /* 8bit buswidth */
> > > +#define OV772X_FLAG_10BIT (1 << 3) /* 10bit buswidth */
>
> May I suggest here ?
> What do you think if it have only 10BIT flag,
> and check/operation like this ?
>
> if (priv->info->flags & OV772X_FLAG_10BIT) {
> flags |= SOCAM_DATAWIDTH_10;
> else
> flags |= SOCAM_DATAWIDTH_8;
>
> This case, below check became not needed,
> Does this operation make sense for you ?
Do you really want to make 8 bits default? Even though this is, probably,
how most implementations will connect the sensor, it is actually a 10-bit
device.
>
> > > /*
> > > - * ov772x only use 8 or 10 bit bus width
> > > - */
> > > - if (SOCAM_DATAWIDTH_10 != priv->info->buswidth &&
> > > - SOCAM_DATAWIDTH_8 != priv->info->buswidth) {
> > > - dev_err(&client->dev, "bus width error\n");
> > > - return -ENODEV;
> > > - }
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
