Hi Akinobu,
On Wed, Apr 11, 2018 at 01:37:03AM +0900, Akinobu Mita wrote:
> 2018-04-09 15:58 GMT+09:00 jacopo mondi <jacopo@xxxxxxxxxx>:
> > Hello Akinobu,
> > thank you for the patch.
> >
> > On which platform have you tested the series (just curious) ?
>
> I use Zynq-7000 development board with Xilinx Video IP driver and
> custom video pipeline design based on the example reference project.
>
That's interesting! I was just wondering if there were other
development boards around that ship with this sensor...
> > On Sun, Apr 08, 2018 at 12:48:05AM +0900, Akinobu Mita wrote:
> >> The ov772x driver only works when the i2c controller have
> >> I2C_FUNC_PROTOCOL_MANGLING. However, many i2c controller drivers don't
> >> support it.
> >>
> >> The reason that the ov772x requires I2C_FUNC_PROTOCOL_MANGLING is that
> >> it doesn't support repeated starts.
> >>
> >> This change adds an alternative method for reading from ov772x register
> >> which uses two separated i2c messages for the i2c controllers without
> >> I2C_FUNC_PROTOCOL_MANGLING.
> >
> > Actually, and please correct me if I'm wrong, what I see here is that
> > an i2c_master_send+i2c_master_recv sequence is equivalent to a mangled
> > smbus transaction:
> >
> > i2c_smbus_read_byte_data | I2C_CLIENT_SCCB:
> > S Addr Wr [A] Comm [A] P S Addr Rd [A] [Data] NA P
> >
> > i2c_master_send() + i2c_master_recv():
> > S Addr Wr [A] Data [A] P
> > S Addr Rd [A] [Data] NA P
> >
> > I wonder if it is not worth to ditch the existing read() function in
> > favour of your new proposed one completely.
> >
> > I have tested it on the Migo-R board where I have an ov772x installed
> > and it works fine.
>
> I'll replace the read() function to the new implementation in the next
> version of this patchset. Although handling in the I2C core is fascinating,
> I feel the area of influence is a bit large.
>
Yep, i2c is huge and complex but fascinating, and the SCCB use case
should probably be handled with a master_send+master_read instead of
relying on the i2c adapter ability to send an additional stop bit
in between an smbus transaction, as Laurent suggested. I wonder why it
has not been implemented this way from day 1. I'm sure there are
reasons :)
I'll wait for v2.
Thanks
j
> > Although I would like to have a confirmation this is fine by people
> > how has seen more i2c adapters in action than me :)
> >
> > Thanks
> > j
> >
> >>
> >> Cc: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> >> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >> Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >> Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
> >> Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
> >> ---
> >> drivers/media/i2c/ov772x.c | 42 +++++++++++++++++++++++++++++++++---------
> >> 1 file changed, 33 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> >> index b62860c..283ae2c 100644
> >> --- a/drivers/media/i2c/ov772x.c
> >> +++ b/drivers/media/i2c/ov772x.c
> >> @@ -424,6 +424,7 @@ struct ov772x_priv {
> >> /* band_filter = COM8[5] ? 256 - BDBASE : 0 */
> >> unsigned short band_filter;
> >> unsigned int fps;
> >> + int (*reg_read)(struct i2c_client *client, u8 addr);
> >> };
> >>
> >> /*
> >> @@ -542,11 +543,34 @@ static struct ov772x_priv *to_ov772x(struct v4l2_subdev *sd)
> >> return container_of(sd, struct ov772x_priv, subdev);
> >> }
> >>
> >> -static inline int ov772x_read(struct i2c_client *client, u8 addr)
> >> +static int ov772x_read(struct i2c_client *client, u8 addr)
> >> +{
> >> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> >> + struct ov772x_priv *priv = to_ov772x(sd);
> >> +
> >> + return priv->reg_read(client, addr);
> >> +}
> >> +
> >> +static int ov772x_reg_read(struct i2c_client *client, u8 addr)
> >> {
> >> return i2c_smbus_read_byte_data(client, addr);
> >> }
> >>
> >> +static int ov772x_reg_read_fallback(struct i2c_client *client, u8 addr)
> >> +{
> >> + int ret;
> >> + u8 val;
> >> +
> >> + ret = i2c_master_send(client, &addr, 1);
> >> + if (ret < 0)
> >> + return ret;
> >> + ret = i2c_master_recv(client, &val, 1);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return val;
> >> +}
> >> +
> >> static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
> >> {
> >> return i2c_smbus_write_byte_data(client, addr, value);
> >> @@ -1255,20 +1279,20 @@ static int ov772x_probe(struct i2c_client *client,
> >> return -EINVAL;
> >> }
> >>
> >> - if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> >> - I2C_FUNC_PROTOCOL_MANGLING)) {
> >> - dev_err(&adapter->dev,
> >> - "I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING\n");
> >> - return -EIO;
> >> - }
> >> - client->flags |= I2C_CLIENT_SCCB;
> >> -
> >> priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL);
> >> if (!priv)
> >> return -ENOMEM;
> >>
> >> priv->info = client->dev.platform_data;
> >>
> >> + if (i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> >> + I2C_FUNC_PROTOCOL_MANGLING))
> >> + priv->reg_read = ov772x_reg_read;
> >> + else
> >> + priv->reg_read = ov772x_reg_read_fallback;
> >> +
> >> + client->flags |= I2C_CLIENT_SCCB;
> >> +
> >> v4l2_i2c_subdev_init(&priv->subdev, client, &ov772x_subdev_ops);
> >> v4l2_ctrl_handler_init(&priv->hdl, 3);
> >> v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
> >> --
> >> 2.7.4
> >>
Attachment:
signature.asc
Description: PGP signature
