[-next,v3,2/2] media: ov772x: use SCCB helpers

Message ID 1531150874-4595-3-git-send-email-akinobu.mita@gmail.com
State Superseded
Headers show
Series
  • introduce SCCB helpers
Related show

Commit Message

Akinobu Mita July 9, 2018, 3:41 p.m.
Convert ov772x register access to use SCCB helpers.

Cc: Peter Rosin <peda@axentia.se>
Cc: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Cc: Wolfram Sang <wsa@the-dreams.de>
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>
---
 drivers/media/i2c/ov772x.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

Comments

Wolfram Sang July 9, 2018, 4:14 p.m. | #1
>  static int ov772x_read(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;
> +	return sccb_read_byte(client, addr);
>  }
>  
>  static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
>  {
> -	return i2c_smbus_write_byte_data(client, addr, value);
> +	return sccb_write_byte(client, addr, value);
>  }

Minor nit: I'd rather drop these two functions and use the
sccb-accessors directly.

However, I really like how this looks here: It is totally clear we are
doing SCCB and hide away all the details.
Sebastian Reichel July 9, 2018, 9:23 p.m. | #2
Hi,

On Mon, Jul 09, 2018 at 06:14:43PM +0200, Wolfram Sang wrote:
> >  static int ov772x_read(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;
> > +	return sccb_read_byte(client, addr);
> >  }
> >  
> >  static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
> >  {
> > -	return i2c_smbus_write_byte_data(client, addr, value);
> > +	return sccb_write_byte(client, addr, value);
> >  }

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

> Minor nit: I'd rather drop these two functions and use the
> sccb-accessors directly.
> 
> However, I really like how this looks here: It is totally clear we are
> doing SCCB and hide away all the details.

I think it would be even better to introduce a SSCB regmap layer
and use that.

-- Sebastian
Akinobu Mita July 10, 2018, 4:36 p.m. | #3
2018年7月10日(火) 6:23 Sebastian Reichel <sebastian.reichel@collabora.co.uk>:
>
> Hi,
>
> On Mon, Jul 09, 2018 at 06:14:43PM +0200, Wolfram Sang wrote:
> > >  static int ov772x_read(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;
> > > +   return sccb_read_byte(client, addr);
> > >  }
> > >
> > >  static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
> > >  {
> > > -   return i2c_smbus_write_byte_data(client, addr, value);
> > > +   return sccb_write_byte(client, addr, value);
> > >  }
>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
>
> > Minor nit: I'd rather drop these two functions and use the
> > sccb-accessors directly.
> >
> > However, I really like how this looks here: It is totally clear we are
> > doing SCCB and hide away all the details.
>
> I think it would be even better to introduce a SSCB regmap layer
> and use that.

I'm fine with introducing a SCCB regmap layer.

But do we need to provide both a SCCB regmap and these SCCB helpers?

If we have a regmap_init_sccb(), I feel these three SCCB helper functions
don't need to be exported anymore.
Wolfram Sang July 10, 2018, 8:59 p.m. | #4
> > I think it would be even better to introduce a SSCB regmap layer
> > and use that.
> 
> I'm fine with introducing a SCCB regmap layer.

I am fine with this approach, too.

> But do we need to provide both a SCCB regmap and these SCCB helpers?

I don't know much about the OV sensor drivers. I'd think they would
benefit from regmap, so a-kind-of enforced conversion to regmap doesn't
sound too bad to me.

> If we have a regmap_init_sccb(), I feel these three SCCB helper functions
> don't need to be exported anymore.

Might be true. But other media guys are probably in a better position to
comment on this?

Note that I found SCCB devices with 16 bit regs: ov2659 & ov 2685. I
think we can cover those with SMBus word accesses. regmap is probably
also the cleanest way to hide this detail?

Patch

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 7158c31..8a9a9ca 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -17,7 +17,7 @@ 
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
-#include <linux/i2c.h>
+#include <linux/i2c-sccb.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -551,22 +551,12 @@  static struct ov772x_priv *to_ov772x(struct v4l2_subdev *sd)
 
 static int ov772x_read(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;
+	return sccb_read_byte(client, addr);
 }
 
 static inline int ov772x_write(struct i2c_client *client, u8 addr, u8 value)
 {
-	return i2c_smbus_write_byte_data(client, addr, value);
+	return sccb_write_byte(client, addr, value);
 }
 
 static int ov772x_mask_set(struct i2c_client *client, u8  command, u8  mask,
@@ -1395,9 +1385,9 @@  static int ov772x_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
+	if (!sccb_is_available(adapter)) {
 		dev_err(&adapter->dev,
-			"I2C-Adapter doesn't support SMBUS_BYTE_DATA\n");
+			"I2C-Adapter doesn't support SCCB\n");
 		return -EIO;
 	}