diff mbox

[v2,01/10] media: ov772x: allow i2c controllers without I2C_FUNC_PROTOCOL_MANGLING

Message ID 1523847111-12986-2-git-send-email-akinobu.mita@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Akinobu Mita April 16, 2018, 2:51 a.m. UTC 
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.

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>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v2
- Replace the implementation of ov772x_read() instead of adding an
  alternative method

 drivers/media/i2c/ov772x.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Comments

Jacopo Mondi April 18, 2018, 10:05 a.m. UTC | #1 
Hi Akinobu,

On Mon, Apr 16, 2018 at 11:51:42AM +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 changes the reading ov772x register method so that it doesn't
> require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.
>
> 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>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v2
> - Replace the implementation of ov772x_read() instead of adding an
>   alternative method

I now wonder if my initial reply to this patch was wrong, and where
possible we should try to use smbus operations...

From Documentation/i2c/smbus-protocol
"If you write a driver for some I2C device, please try to use the SMBus
commands if at all possible... " and that's because, according to
documentation, most I2c adapters support smbus protocol but may not
support the full i2c command set.

The fact this driver then restricts the supported adapters to the ones
that support protocol mangling makes me think your change is fine,
but as it often happens, I would scale this to more knowledgable
people...

>
>  drivers/media/i2c/ov772x.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index b62860c..7e79da0 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,10 +1265,9 @@ 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;

I think you can remove this flag if we're using plain i2c
transactions. Sorry, I have missed this in v1.

Thanks
   j

> --
> 2.7.4
>
Sakari Ailus April 18, 2018, 10:41 a.m. UTC | #2 
On Wed, Apr 18, 2018 at 12:05:49PM +0200, jacopo mondi wrote:
> Hi Akinobu,
> 
> On Mon, Apr 16, 2018 at 11:51:42AM +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 changes the reading ov772x register method so that it doesn't
> > require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.
> >
> > 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>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> > * v2
> > - Replace the implementation of ov772x_read() instead of adding an
> >   alternative method
> 
> I now wonder if my initial reply to this patch was wrong, and where
> possible we should try to use smbus operations...
> 
> From Documentation/i2c/smbus-protocol
> "If you write a driver for some I2C device, please try to use the SMBus
> commands if at all possible... " and that's because, according to
> documentation, most I2c adapters support smbus protocol but may not
> support the full i2c command set.
> 
> The fact this driver then restricts the supported adapters to the ones
> that support protocol mangling makes me think your change is fine,
> but as it often happens, I would scale this to more knowledgable
> people...

Do you actually need to use this on SMBus adapters? A lot of sensor drivers
just use I²C; if SMBus support is really needed it can be always added back
later on...
Jacopo Mondi April 18, 2018, 12:12 p.m. UTC | #3 
Hi Sakari,

On Wed, Apr 18, 2018 at 01:41:54PM +0300, Sakari Ailus wrote:
> On Wed, Apr 18, 2018 at 12:05:49PM +0200, jacopo mondi wrote:
> > Hi Akinobu,
> >
> > On Mon, Apr 16, 2018 at 11:51:42AM +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 changes the reading ov772x register method so that it doesn't
> > > require I2C_FUNC_PROTOCOL_MANGLING by calling two separated i2c messages.
> > >
> > > 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>
> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > > ---
> > > * v2
> > > - Replace the implementation of ov772x_read() instead of adding an
> > >   alternative method
> >
> > I now wonder if my initial reply to this patch was wrong, and where
> > possible we should try to use smbus operations...
> >
> > From Documentation/i2c/smbus-protocol
> > "If you write a driver for some I2C device, please try to use the SMBus
> > commands if at all possible... " and that's because, according to
> > documentation, most I2c adapters support smbus protocol but may not
> > support the full i2c command set.
> >
> > The fact this driver then restricts the supported adapters to the ones
> > that support protocol mangling makes me think your change is fine,
> > but as it often happens, I would scale this to more knowledgable
> > people...
>
> Do you actually need to use this on SMBus adapters? A lot of sensor drivers
> just use I²C; if SMBus support is really needed it can be always added back
> later on...

That was actually my question, sorry for not being clear.

As the documentation says that SMBus has to be preferred when
possible, I was wondering if ditching it completely in favour of plain
I2c was wrong or not... I assume from your answer it is fine.

>
> --
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi
diff mbox

Patch

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index b62860c..7e79da0 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,10 +1265,9 @@  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;