| Message ID | 1525616369-8843-4-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Sun, 6 May 2018 23:19:18 +0900 Akinobu Mita <akinobu.mita@gmail.com> escreveu: > 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 changes the reading ov772x register method so that it doesn't > require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages. I had a Déjà vu when I looked on this patch, as I saw the same issue I pointed to another patch you submitted ;-) So, I ended by not replying to this one. Just to be clear: the same comment I did to the SCCB helpers apply here: https://www.mail-archive.com/linux-media@vger.kernel.org/msg130868.html It is a very bad idea to replace an i2c xfer by a pair of i2c send()/recv(), as, if are there any other device at the bus managed by an independent driver, you may end by mangling i2c transfers and eventually cause device malfunctions. I've seen it a lot on media devices that have devices that need constant polling via I2C (e. g. hardware with remote controllers, for example). So, IMO, the best is to push the patch you proposed that adds a new I2C flag: https://patchwork.linuxtv.org/patch/49396/ > Cc: Jacopo Mondi <jacopo+renesas@jmondi.org> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Hans Verkuil <hans.verkuil@cisco.com> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com> > Cc: Wolfram Sang <wsa@the-dreams.de> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> > --- > * v5 > - Add Reviewed-by: line > > drivers/media/i2c/ov772x.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c > index e255070..b6223bf 100644 > --- a/drivers/media/i2c/ov772x.c > +++ b/drivers/media/i2c/ov772x.c > @@ -542,9 +542,19 @@ 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) > { > - return i2c_smbus_read_byte_data(client, 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) > @@ -1255,13 +1265,11 @@ static int ov772x_probe(struct i2c_client *client, > return -EINVAL; > } > > - if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA | > - I2C_FUNC_PROTOCOL_MANGLING)) { > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { > dev_err(&adapter->dev, > - "I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING\n"); > + "I2C-Adapter doesn't support SMBUS_BYTE_DATA\n"); > return -EIO; > } > - client->flags |= I2C_CLIENT_SCCB; > > priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL); > if (!priv) Thanks, Mauro
> It is a very bad idea to replace an i2c xfer by a pair of i2c > send()/recv(), as, if are there any other device at the bus managed > by an independent driver, you may end by mangling i2c transfers and > eventually cause device malfunctions. For I2C, this is true and a very important detail. Yet, we are talking not I2C but SCCB here and SCCB demands a STOP between messages. So, technically, to avoid what you describe one shouldn't mix I2C and SCCB devices. I am quite aware the reality is very different, but still... My preference would be to stop acting as SCCB was I2C but give it its own set of functions so it becomes clear for everyone what protocol is used for what device. > So, IMO, the best is to push the patch you proposed that adds a > new I2C flag: > > https://patchwork.linuxtv.org/patch/49396/ Sorry, but I don't like it. This makes the I2C core code very unreadable. This is why I think SCCB should be exported to its own realm. Which may live in i2c-core-sccb.c, no need for a seperate subsystem.
Em Tue, 29 May 2018 15:29:29 +0200 Wolfram Sang <wsa@the-dreams.de> escreveu: > > It is a very bad idea to replace an i2c xfer by a pair of i2c > > send()/recv(), as, if are there any other device at the bus managed > > by an independent driver, you may end by mangling i2c transfers and > > eventually cause device malfunctions. > > For I2C, this is true and a very important detail. Yet, we are talking > not I2C but SCCB here and SCCB demands a STOP between messages. So, > technically, to avoid what you describe one shouldn't mix I2C and SCCB > devices. I am quite aware the reality is very different, but still... > > My preference would be to stop acting as SCCB was I2C but give it its > own set of functions so it becomes clear for everyone what protocol is > used for what device. > > > So, IMO, the best is to push the patch you proposed that adds a > > new I2C flag: > > > > https://patchwork.linuxtv.org/patch/49396/ > > Sorry, but I don't like it. This makes the I2C core code very > unreadable. This is why I think SCCB should be exported to its own > realm. Which may live in i2c-core-sccb.c, no need for a seperate > subsystem. We actually have the same issue with pure I2C devices on media. There are several I2C devices that don't accept repeat start. The solution given there was hackish and varies from driver to driver. The most common solution were to patch the I2C xfer callback function of the I2C master code in order to prevent them to do I2C repeated start ops, usually checking for some specific I2C addresses where repeated start is known to not working. So, IMHO, a flag like that would be an improvement not only for SCCB but also for other I2C devices. Probably there would be other ways to do it without making the I2C core code harder to read. Thanks, Mauro
diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c index e255070..b6223bf 100644 --- a/drivers/media/i2c/ov772x.c +++ b/drivers/media/i2c/ov772x.c @@ -542,9 +542,19 @@ 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) { - return i2c_smbus_read_byte_data(client, 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) @@ -1255,13 +1265,11 @@ static int ov772x_probe(struct i2c_client *client, return -EINVAL; } - if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA | - I2C_FUNC_PROTOCOL_MANGLING)) { + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { dev_err(&adapter->dev, - "I2C-Adapter doesn't support SMBUS_BYTE_DATA or PROTOCOL_MANGLING\n"); + "I2C-Adapter doesn't support SMBUS_BYTE_DATA\n"); return -EIO; } - client->flags |= I2C_CLIENT_SCCB; priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL); if (!priv)