| Message ID | 1531150874-4595-3-git-send-email-akinobu.mita@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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.
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
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.
> > 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?
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; }
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(-)