linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH -next v3 0/2] introduce SCCB helpers
@ 2018-07-09 15:41 Akinobu Mita
  2018-07-09 15:41 ` [PATCH -next v3 1/2] i2c: add " Akinobu Mita
  2018-07-09 15:41 ` [PATCH -next v3 2/2] media: ov772x: use " Akinobu Mita
  0 siblings, 2 replies; 12+ messages in thread
From: Akinobu Mita @ 2018-07-09 15:41 UTC (permalink / raw)
  To: linux-media, linux-i2c
  Cc: Akinobu Mita, Peter Rosin, Sebastian Reichel, Wolfram Sang,
	Jacopo Mondi, Laurent Pinchart, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

This patchset introduces Serial Camera Control Bus (SCCB) helper functions
and convert ov772x driver to use the helpers.

* v3
- Rewrite the helpers based on the code provided by Wolfram
- Convert ov772x driver to use SCCB helpers

 v2
- Convert all helpers into static inline functions, and remove C source
  and Kconfig option.
- Acquire i2c adapter lock while issuing two requests for sccb_read_byte

Akinobu Mita (2):
  i2c: add SCCB helpers
  media: ov772x: use SCCB helpers

 drivers/media/i2c/ov772x.c | 20 +++---------
 include/linux/i2c-sccb.h   | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 15 deletions(-)
 create mode 100644 include/linux/i2c-sccb.h

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>
-- 
2.7.4

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH -next v3 1/2] i2c: add SCCB helpers
  2018-07-09 15:41 [PATCH -next v3 0/2] introduce SCCB helpers Akinobu Mita
@ 2018-07-09 15:41 ` Akinobu Mita
  2018-07-09 16:11   ` Wolfram Sang
  2018-07-10 10:50   ` Laurent Pinchart
  2018-07-09 15:41 ` [PATCH -next v3 2/2] media: ov772x: use " Akinobu Mita
  1 sibling, 2 replies; 12+ messages in thread
From: Akinobu Mita @ 2018-07-09 15:41 UTC (permalink / raw)
  To: linux-media, linux-i2c
  Cc: Akinobu Mita, Peter Rosin, Sebastian Reichel, Wolfram Sang,
	Jacopo Mondi, Laurent Pinchart, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

This adds Serial Camera Control Bus (SCCB) helpers (sccb_is_available,
sccb_read_byte, and sccb_write_byte) that are intended to be used by some
of Omnivision sensor drivers.

The ov772x driver is going to use these helpers.

It was previously only worked with the i2c controller drivers that support
I2C_FUNC_PROTOCOL_MANGLING, because the ov772x device doesn't support
repeated starts.  After commit 0b964d183cbf ("media: ov772x: allow i2c
controllers without I2C_FUNC_PROTOCOL_MANGLING"), reading ov772x register
is replaced with issuing two separated i2c messages in order to avoid
repeated start.  Using SCCB helpers hides the implementation detail.

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>
---
 include/linux/i2c-sccb.h | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 include/linux/i2c-sccb.h

diff --git a/include/linux/i2c-sccb.h b/include/linux/i2c-sccb.h
new file mode 100644
index 0000000..61dece9
--- /dev/null
+++ b/include/linux/i2c-sccb.h
@@ -0,0 +1,77 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Serial Camera Control Bus (SCCB) helper functions
+ */
+
+#ifndef _LINUX_I2C_SCCB_H
+#define _LINUX_I2C_SCCB_H
+
+#include <linux/i2c.h>
+
+/**
+ * sccb_is_available - Check if the adapter supports SCCB protocol
+ * @adap: I2C adapter
+ *
+ * Return true if the I2C adapter is capable of using SCCB helper functions,
+ * false otherwise.
+ */
+static inline bool sccb_is_available(struct i2c_adapter *adap)
+{
+	u32 needed_funcs = I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA;
+
+#if 0
+	/*
+	 * sccb_xfer not needed yet, since there is no driver support currently.
+	 * Just showing how it should be done if we ever need it.
+	 */
+	if (adap->algo->sccb_xfer)
+		return true;
+#endif
+
+	return (i2c_get_functionality(adap) & needed_funcs) == needed_funcs;
+}
+
+/**
+ * sccb_read_byte - Read data from SCCB slave device
+ * @client: Handle to slave device
+ * @addr: Register to be read from
+ *
+ * This executes the 2-phase write transmission cycle that is followed by a
+ * 2-phase read transmission cycle, returning negative errno else a data byte
+ * received from the device.
+ */
+static inline int sccb_read_byte(struct i2c_client *client, u8 addr)
+{
+	int ret;
+	union i2c_smbus_data data;
+
+	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+				I2C_SMBUS_WRITE, addr, I2C_SMBUS_BYTE, NULL);
+	if (ret < 0)
+		goto out;
+
+	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+				I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data);
+out:
+	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
+
+	return ret < 0 ? ret : data.byte;
+}
+
+/**
+ * sccb_write_byte - Write data to SCCB slave device
+ * @client: Handle to slave device
+ * @addr: Register to write to
+ * @data: Value to be written
+ *
+ * This executes the SCCB 3-phase write transmission cycle, returning negative
+ * errno else zero on success.
+ */
+static inline int sccb_write_byte(struct i2c_client *client, u8 addr, u8 data)
+{
+	return i2c_smbus_write_byte_data(client, addr, data);
+}
+
+#endif /* _LINUX_I2C_SCCB_H */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH -next v3 2/2] media: ov772x: use SCCB helpers
  2018-07-09 15:41 [PATCH -next v3 0/2] introduce SCCB helpers Akinobu Mita
  2018-07-09 15:41 ` [PATCH -next v3 1/2] i2c: add " Akinobu Mita
@ 2018-07-09 15:41 ` Akinobu Mita
  2018-07-09 16:14   ` Wolfram Sang
  1 sibling, 1 reply; 12+ messages in thread
From: Akinobu Mita @ 2018-07-09 15:41 UTC (permalink / raw)
  To: linux-media, linux-i2c
  Cc: Akinobu Mita, Peter Rosin, Sebastian Reichel, Wolfram Sang,
	Jacopo Mondi, Laurent Pinchart, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

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(-)

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;
 	}
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH -next v3 1/2] i2c: add SCCB helpers
  2018-07-09 15:41 ` [PATCH -next v3 1/2] i2c: add " Akinobu Mita
@ 2018-07-09 16:11   ` Wolfram Sang
  2018-07-10 10:50   ` Laurent Pinchart
  1 sibling, 0 replies; 12+ messages in thread
From: Wolfram Sang @ 2018-07-09 16:11 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, linux-i2c, Peter Rosin, Sebastian Reichel,
	Jacopo Mondi, Laurent Pinchart, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

[-- Attachment #1: Type: text/plain, Size: 1220 bytes --]

Hi,

yes! From a first look this nails it for me. Thanks for doing it.

Minor comments follow...

> +#if 0
> +	/*
> +	 * sccb_xfer not needed yet, since there is no driver support currently.
> +	 * Just showing how it should be done if we ever need it.
> +	 */
> +	if (adap->algo->sccb_xfer)
> +		return true;
> +#endif

I think this should be a more generic comment in the header, like

/*
 * If we ever want support for hardware doing SCCB natively, we will
 * introduce a sccb_xfer() callback to struct i2c_algorithm and check
 * for it here.
 */

> +/**
> + * sccb_read_byte - Read data from SCCB slave device
> + * @client: Handle to slave device
> + * @addr: Register to be read from

I think 'addr' is too easy to confuse with client->addr. SMBus uses
'command' but I am also fine with 'reg' because we will deal with
registers.

> + * This executes the 2-phase write transmission cycle that is followed by a
> + * 2-phase read transmission cycle, returning negative errno else a data byte
> + * received from the device.
> + */

Nice docs!

> +/**
> + * sccb_write_byte - Write data to SCCB slave device
> + * @client: Handle to slave device
> + * @addr: Register to write to

'reg'?

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next v3 2/2] media: ov772x: use SCCB helpers
  2018-07-09 15:41 ` [PATCH -next v3 2/2] media: ov772x: use " Akinobu Mita
@ 2018-07-09 16:14   ` Wolfram Sang
  2018-07-09 21:23     ` Sebastian Reichel
  0 siblings, 1 reply; 12+ messages in thread
From: Wolfram Sang @ 2018-07-09 16:14 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, linux-i2c, Peter Rosin, Sebastian Reichel,
	Jacopo Mondi, Laurent Pinchart, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]


>  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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next v3 2/2] media: ov772x: use SCCB helpers
  2018-07-09 16:14   ` Wolfram Sang
@ 2018-07-09 21:23     ` Sebastian Reichel
  2018-07-10 16:36       ` Akinobu Mita
  0 siblings, 1 reply; 12+ messages in thread
From: Sebastian Reichel @ 2018-07-09 21:23 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Akinobu Mita, linux-media, linux-i2c, Peter Rosin, Jacopo Mondi,
	Laurent Pinchart, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next v3 1/2] i2c: add SCCB helpers
  2018-07-09 15:41 ` [PATCH -next v3 1/2] i2c: add " Akinobu Mita
  2018-07-09 16:11   ` Wolfram Sang
@ 2018-07-10 10:50   ` Laurent Pinchart
  2018-07-10 12:07     ` Wolfram Sang
  1 sibling, 1 reply; 12+ messages in thread
From: Laurent Pinchart @ 2018-07-10 10:50 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, linux-i2c, Peter Rosin, Sebastian Reichel,
	Wolfram Sang, Jacopo Mondi, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

Hi Akinobu,

Thank you for the patch.

On Monday, 9 July 2018 18:41:13 EEST Akinobu Mita wrote:
> This adds Serial Camera Control Bus (SCCB) helpers (sccb_is_available,
> sccb_read_byte, and sccb_write_byte) that are intended to be used by some
> of Omnivision sensor drivers.
> 
> The ov772x driver is going to use these helpers.
> 
> It was previously only worked with the i2c controller drivers that support
> I2C_FUNC_PROTOCOL_MANGLING, because the ov772x device doesn't support
> repeated starts.  After commit 0b964d183cbf ("media: ov772x: allow i2c
> controllers without I2C_FUNC_PROTOCOL_MANGLING"), reading ov772x register
> is replaced with issuing two separated i2c messages in order to avoid
> repeated start.  Using SCCB helpers hides the implementation detail.
> 
> 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>
> ---
>  include/linux/i2c-sccb.h | 77 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 include/linux/i2c-sccb.h
> 
> diff --git a/include/linux/i2c-sccb.h b/include/linux/i2c-sccb.h
> new file mode 100644
> index 0000000..61dece9
> --- /dev/null
> +++ b/include/linux/i2c-sccb.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Serial Camera Control Bus (SCCB) helper functions
> + */
> +
> +#ifndef _LINUX_I2C_SCCB_H
> +#define _LINUX_I2C_SCCB_H
> +
> +#include <linux/i2c.h>
> +
> +/**
> + * sccb_is_available - Check if the adapter supports SCCB protocol
> + * @adap: I2C adapter
> + *
> + * Return true if the I2C adapter is capable of using SCCB helper
> functions, + * false otherwise.
> + */
> +static inline bool sccb_is_available(struct i2c_adapter *adap)
> +{
> +	u32 needed_funcs = I2C_FUNC_SMBUS_BYTE | I2C_FUNC_SMBUS_WRITE_BYTE_DATA;
> +
> +#if 0
> +	/*
> +	 * sccb_xfer not needed yet, since there is no driver support currently.
> +	 * Just showing how it should be done if we ever need it.
> +	 */
> +	if (adap->algo->sccb_xfer)
> +		return true;
> +#endif
> +
> +	return (i2c_get_functionality(adap) & needed_funcs) == needed_funcs;
> +}
> +
> +/**
> + * sccb_read_byte - Read data from SCCB slave device
> + * @client: Handle to slave device
> + * @addr: Register to be read from
> + *
> + * This executes the 2-phase write transmission cycle that is followed by a
> + * 2-phase read transmission cycle, returning negative errno else a data
> byte
> + * received from the device.
> + */
> +static inline int sccb_read_byte(struct i2c_client *client, u8 addr)
> +{
> +	int ret;
> +	union i2c_smbus_data data;
> +
> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> +	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> +				I2C_SMBUS_WRITE, addr, I2C_SMBUS_BYTE, NULL);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> +				I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data);
> +out:
> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> +
> +	return ret < 0 ? ret : data.byte;
> +}

I think I mentioned in a previous review of this patch that the function is 
too big to be a static inline. It should instead be moved to a .c file.

> +/**
> + * sccb_write_byte - Write data to SCCB slave device
> + * @client: Handle to slave device
> + * @addr: Register to write to
> + * @data: Value to be written
> + *
> + * This executes the SCCB 3-phase write transmission cycle, returning
> negative
> + * errno else zero on success.
> + */
> +static inline int sccb_write_byte(struct i2c_client *client, u8 addr, u8
> data)
> +{
> +	return i2c_smbus_write_byte_data(client, addr, data);
> +}
> +
> +#endif /* _LINUX_I2C_SCCB_H */

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next v3 1/2] i2c: add SCCB helpers
  2018-07-10 10:50   ` Laurent Pinchart
@ 2018-07-10 12:07     ` Wolfram Sang
  2018-07-10 12:16       ` Laurent Pinchart
  0 siblings, 1 reply; 12+ messages in thread
From: Wolfram Sang @ 2018-07-10 12:07 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Akinobu Mita, linux-media, linux-i2c, Peter Rosin,
	Sebastian Reichel, Jacopo Mondi, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]


> > +static inline int sccb_read_byte(struct i2c_client *client, u8 addr)
> > +{
> > +	int ret;
> > +	union i2c_smbus_data data;
> > +
> > +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +
> > +	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> > +				I2C_SMBUS_WRITE, addr, I2C_SMBUS_BYTE, NULL);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> > +				I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data);
> > +out:
> > +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> > +
> > +	return ret < 0 ? ret : data.byte;
> > +}
> 
> I think I mentioned in a previous review of this patch that the function is 
> too big to be a static inline. It should instead be moved to a .c file.

Can be argued. I assume just removing the 'inline' won't do it for you?
I'd be fine with that, there are not many SCCB useres out there...

But if you insist on drivers/i2c/i2c-sccb.c, then it should be a
seperate module, I'd think?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next v3 1/2] i2c: add SCCB helpers
  2018-07-10 12:07     ` Wolfram Sang
@ 2018-07-10 12:16       ` Laurent Pinchart
  2018-07-10 13:02         ` Wolfram Sang
  0 siblings, 1 reply; 12+ messages in thread
From: Laurent Pinchart @ 2018-07-10 12:16 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Akinobu Mita, linux-media, linux-i2c, Peter Rosin,
	Sebastian Reichel, Jacopo Mondi, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

Hi Wolfram,

On Tuesday, 10 July 2018 15:07:47 EEST Wolfram Sang wrote:
> >> +static inline int sccb_read_byte(struct i2c_client *client, u8 addr)
> >> +{
> >> +	int ret;
> >> +	union i2c_smbus_data data;
> >> +
> >> +	i2c_lock_bus(client->adapter, I2C_LOCK_SEGMENT);
> >> +
> >> +	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> >> +				I2C_SMBUS_WRITE, addr, I2C_SMBUS_BYTE, NULL);
> >> +	if (ret < 0)
> >> +		goto out;
> >> +
> >> +	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> >> +				I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data);
> >> +out:
> >> +	i2c_unlock_bus(client->adapter, I2C_LOCK_SEGMENT);
> >> +
> >> +	return ret < 0 ? ret : data.byte;
> >> +}
> > 
> > I think I mentioned in a previous review of this patch that the function
> > is too big to be a static inline. It should instead be moved to a .c file.
> 
> Can be argued.

Especially if sccb_read_byte() is called in multiple places in a driver, not 
just once in a read helper, as you've advised for patch 2/2 in this series :-)

> I assume just removing the 'inline' won't do it for you?

Just removing the inline keyword will create many instances of the function, 
even when not used. I think it will also cause the compiler to emit warnings 
for unused functions. I don't think that's a good idea.

> I'd be fine with that, there are not many SCCB useres out there...
> 
> But if you insist on drivers/i2c/i2c-sccb.c, then it should be a
> seperate module, I'd think?

Given how small the functions are, I wouldn't request that, as it would 
introduce another Kconfig symbol, but I'm not opposed to such a new module 
either.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next v3 1/2] i2c: add SCCB helpers
  2018-07-10 12:16       ` Laurent Pinchart
@ 2018-07-10 13:02         ` Wolfram Sang
  0 siblings, 0 replies; 12+ messages in thread
From: Wolfram Sang @ 2018-07-10 13:02 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Akinobu Mita, linux-media, linux-i2c, Peter Rosin,
	Sebastian Reichel, Jacopo Mondi, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]


> even when not used. I think it will also cause the compiler to emit warnings 
> for unused functions. I don't think that's a good idea.

Where is my brown paper bag? :/

> > But if you insist on drivers/i2c/i2c-sccb.c, then it should be a
> > seperate module, I'd think?
> 
> Given how small the functions are, I wouldn't request that, as it would 
> introduce another Kconfig symbol, but I'm not opposed to such a new module 
> either.

OK, let's keep it simple for now and introduce
drivers/i2c/i2c-core-sccb.c and link it into the core module. It can
still be factored out later if the need arises.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next v3 2/2] media: ov772x: use SCCB helpers
  2018-07-09 21:23     ` Sebastian Reichel
@ 2018-07-10 16:36       ` Akinobu Mita
  2018-07-10 20:59         ` Wolfram Sang
  0 siblings, 1 reply; 12+ messages in thread
From: Akinobu Mita @ 2018-07-10 16:36 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Wolfram Sang, linux-media, linux-i2c, Peter Rosin, Jacopo Mondi,
	Laurent Pinchart, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH -next v3 2/2] media: ov772x: use SCCB helpers
  2018-07-10 16:36       ` Akinobu Mita
@ 2018-07-10 20:59         ` Wolfram Sang
  0 siblings, 0 replies; 12+ messages in thread
From: Wolfram Sang @ 2018-07-10 20:59 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: Sebastian Reichel, linux-media, linux-i2c, Peter Rosin,
	Jacopo Mondi, Laurent Pinchart, Hans Verkuil, Sakari Ailus,
	Mauro Carvalho Chehab

[-- Attachment #1: Type: text/plain, Size: 823 bytes --]


> > 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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-07-10 21:17 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-09 15:41 [PATCH -next v3 0/2] introduce SCCB helpers Akinobu Mita
2018-07-09 15:41 ` [PATCH -next v3 1/2] i2c: add " Akinobu Mita
2018-07-09 16:11   ` Wolfram Sang
2018-07-10 10:50   ` Laurent Pinchart
2018-07-10 12:07     ` Wolfram Sang
2018-07-10 12:16       ` Laurent Pinchart
2018-07-10 13:02         ` Wolfram Sang
2018-07-09 15:41 ` [PATCH -next v3 2/2] media: ov772x: use " Akinobu Mita
2018-07-09 16:14   ` Wolfram Sang
2018-07-09 21:23     ` Sebastian Reichel
2018-07-10 16:36       ` Akinobu Mita
2018-07-10 20:59         ` Wolfram Sang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).