[PATCH 0/4] fix some checkpatch style issues in atomisp driver

[PATCH 0/4] fix some checkpatch style issues in atomisp driver

[Riccardo Schirone]

This patch series fixes some coding style issues reported by checkpatch.

It is based on next-20171127.

Riccardo Schirone (4):
  staging: add missing blank line after declarations in atomisp-ov5693
  staging: improve comments usage in atomisp-ov5693
  staging: improves comparisons readability in atomisp-ov5693
  staging: fix indentation in atomisp-ov5693

 .../media/atomisp/i2c/ov5693/atomisp-ov5693.c      | 63 +++++++++++++++-------
 1 file changed, 44 insertions(+), 19 deletions(-)

-- 
2.14.3

[PATCH 1/4] staging: add missing blank line after declarations in atomisp-ov5693

[Riccardo Schirone]

Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
---
 drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index 3e7c3851280f..387c65be10f4 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -82,6 +82,7 @@ static int ad5823_i2c_write(struct i2c_client *client, u8 reg, u8 val)
 {
 	struct i2c_msg msg;
 	u8 buf[2];
+
 	buf[0] = reg;
 	buf[1] = val;
 	msg.addr = AD5823_VCM_ADDR;
@@ -98,6 +99,7 @@ static int ad5823_i2c_read(struct i2c_client *client, u8 reg, u8 *val)
 {
 	struct i2c_msg msg[2];
 	u8 buf[2];
+
 	buf[0] = reg;
 	buf[1] = 0;
 
@@ -228,6 +230,7 @@ static int vcm_detect(struct i2c_client *client)
 	int i, ret;
 	struct i2c_msg msg;
 	u16 data0 = 0, data;
+
 	for (i = 0; i < 4; i++) {
 		msg.addr = VCM_ADDR;
 		msg.flags = I2C_M_RD;
@@ -690,6 +693,7 @@ static long ov5693_s_exposure(struct v4l2_subdev *sd,
 	/* we should not accept the invalid value below */
 	if (analog_gain == 0) {
 		struct i2c_client *client = v4l2_get_subdevdata(sd);
+
 		v4l2_err(client, "%s: invalid value\n", __func__);
 		return -EINVAL;
 	}
@@ -722,6 +726,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
 	int ret;
 	int i;
 	u8 *b = buf;
+
 	dev->otp_size = 0;
 	for (i = 1; i < OV5693_OTP_BANK_MAX; i++) {
 		/*set bank NO and OTP read mode. */
@@ -984,6 +989,7 @@ static int ov5693_t_focus_abs(struct v4l2_subdev *sd, s32 value)
 static int ov5693_t_focus_rel(struct v4l2_subdev *sd, s32 value)
 {
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	return ov5693_t_focus_abs(sd, dev->focus + value);
 }
 
@@ -1033,6 +1039,7 @@ static int ov5693_q_focus_abs(struct v4l2_subdev *sd, s32 *value)
 static int ov5693_t_vcm_slew(struct v4l2_subdev *sd, s32 value)
 {
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	dev->number_of_steps = value;
 	dev->vcm_update = true;
 	return 0;
@@ -1041,6 +1048,7 @@ static int ov5693_t_vcm_slew(struct v4l2_subdev *sd, s32 value)
 static int ov5693_t_vcm_timing(struct v4l2_subdev *sd, s32 value)
 {
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	dev->number_of_steps = value;
 	dev->vcm_update = true;
 	return 0;
@@ -1563,6 +1571,7 @@ static int ov5693_set_fmt(struct v4l2_subdev *sd,
 	struct camera_mipi_info *ov5693_info = NULL;
 	int ret = 0;
 	int idx;
+
 	if (format->pad)
 		return -EINVAL;
 	if (!fmt)
@@ -1599,6 +1608,7 @@ static int ov5693_set_fmt(struct v4l2_subdev *sd,
 	ret = startup(sd);
 	if (ret) {
 		int i = 0;
+
 		dev_err(&client->dev, "ov5693 startup err, retry to power up\n");
 		for (i = 0; i < OV5693_POWER_UP_RETRY_NUM; i++) {
 			dev_err(&client->dev,
@@ -1655,6 +1665,7 @@ static int ov5693_get_fmt(struct v4l2_subdev *sd,
 {
 	struct v4l2_mbus_framefmt *fmt = &format->format;
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	if (format->pad)
 		return -EINVAL;
 
@@ -1818,6 +1829,7 @@ static int ov5693_s_parm(struct v4l2_subdev *sd,
 			struct v4l2_streamparm *param)
 {
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	dev->run_mode = param->parm.capture.capturemode;
 
 	mutex_lock(&dev->input_lock);
@@ -1907,6 +1919,7 @@ static int ov5693_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	dev_dbg(&client->dev, "ov5693_remove...\n");
 
 	dev->platform_data->csi_cfg(sd, 0);
-- 
2.14.3

[PATCH 2/4] staging: improve comments usage in atomisp-ov5693

[Riccardo Schirone]

* Fix "Block comments use a trailing */ on a separate line" issue
* Fix "Block comments use * on subsequent lines" issue

Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
---
 .../media/atomisp/i2c/ov5693/atomisp-ov5693.c      | 38 ++++++++++++++--------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index 387c65be10f4..ecd607b7b005 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -213,7 +213,8 @@ static int vcm_dw_i2c_write(struct i2c_client *client, u16 data)
 	return ret == num_msg ? 0 : -EIO;
 }
 
-/* Theory: per datasheet, the two VCMs both allow for a 2-byte read.
+/*
+ * Theory: per datasheet, the two VCMs both allow for a 2-byte read.
  * The DW9714 doesn't actually specify what this does (it has a
  * two-byte write-only protocol, but specifies the read sequence as
  * legal), but it returns the same data (zeroes) always, after an
@@ -224,7 +225,8 @@ static int vcm_dw_i2c_write(struct i2c_client *client, u16 data)
  * these) in AD5823 are not pairwise repetitions of the same 16 bit
  * word.  So all we have to do is sequentially read two bytes at a
  * time and see if we detect a difference in any of the first four
- * pairs.  */
+ * pairs.
+ */
 static int vcm_detect(struct i2c_client *client)
 {
 	int i, ret;
@@ -238,8 +240,10 @@ static int vcm_detect(struct i2c_client *client)
 		msg.buf = (u8 *)&data;
 		ret = i2c_transfer(client->adapter, &msg, 1);
 
-		/* DW9714 always fails the first read and returns
-		 * zeroes for subsequent ones */
+		/*
+		 * DW9714 always fails the first read and returns
+		 * zeroes for subsequent ones
+		 */
 		if (i == 0 && ret == -EREMOTEIO) {
 			data0 = 0;
 			continue;
@@ -533,9 +537,11 @@ static long __ov5693_set_exposure(struct v4l2_subdev *sd, int coarse_itg,
 
 	hts = ov5693_res[dev->fmt_idx].pixels_per_line;
 	vts = ov5693_res[dev->fmt_idx].lines_per_frame;
-	/*If coarse_itg is larger than 1<<15, can not write to reg directly.
-	  The way is to write coarse_itg/2 to the reg, meanwhile write 2*hts
-	  to the reg. */
+	/*
+	 * If coarse_itg is larger than 1<<15, can not write to reg directly.
+	 * The way is to write coarse_itg/2 to the reg, meanwhile write 2*hts
+	 * to the reg.
+	 */
 	if (coarse_itg > (1 << 15)) {
 		hts = hts * 2;
 		coarse_itg = (int)coarse_itg / 2;
@@ -880,8 +886,10 @@ static long ov5693_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
 	return 0;
 }
 
-/* This returns the exposure time being used. This should only be used
-   for filling in EXIF data, not for actual image processing. */
+/*
+ * This returns the exposure time being used. This should only be used
+ * for filling in EXIF data, not for actual image processing.
+ */
 static int ov5693_q_exposure(struct v4l2_subdev *sd, s32 *value)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -1301,11 +1309,13 @@ static int power_ctrl(struct v4l2_subdev *sd, bool flag)
 	if (!dev || !dev->platform_data)
 		return -ENODEV;
 
-	/* This driver assumes "internal DVDD, PWDNB tied to DOVDD".
+	/*
+	 * This driver assumes "internal DVDD, PWDNB tied to DOVDD".
 	 * In this set up only gpio0 (XSHUTDN) should be available
 	 * but in some products (for example ECS) gpio1 (PWDNB) is
 	 * also available. If gpio1 is available we emulate it being
-	 * tied to DOVDD here. */
+	 * tied to DOVDD here.
+	 */
 	if (flag) {
 		ret = dev->platform_data->v2p8_ctrl(sd, 1);
 		dev->platform_data->gpio1_ctrl(sd, 1);
@@ -1944,10 +1954,12 @@ static int ov5693_probe(struct i2c_client *client)
 	struct acpi_device *adev;
 	unsigned int i;
 
-	/* Firmware workaround: Some modules use a "secondary default"
+	/*
+	 * Firmware workaround: Some modules use a "secondary default"
 	 * address of 0x10 which doesn't appear on schematics, and
 	 * some BIOS versions haven't gotten the memo.  Work around
-	 * via config. */
+	 * via config.
+	 */
 	i2c = gmin_get_var_int(&client->dev, "I2CAddr", -1);
 	if (i2c != -1) {
 		dev_info(&client->dev,
-- 
2.14.3

[PATCH 3/4] staging: improves comparisons readability in atomisp-ov5693

[Riccardo Schirone]

Fix "Comparisons should place the constant on the right side of the
test" issue.

Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
---
 drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index ecd607b7b005..4eeb478ae84b 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -764,7 +764,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
 		//pr_debug("BANK[%2d] %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n", i, *b, *(b+1), *(b+2), *(b+3), *(b+4), *(b+5), *(b+6), *(b+7), *(b+8), *(b+9), *(b+10), *(b+11), *(b+12), *(b+13), *(b+14), *(b+15));
 
 		//Intel OTP map, try to read 320byts first.
-		if (21 == i) {
+		if (i == 21) {
 			if ((*b) == 0) {
 				dev->otp_size = 320;
 				break;
@@ -772,7 +772,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
 				b = buf;
 				continue;
 			}
-		} else if (24 == i) {		//if the first 320bytes data doesn't not exist, try to read the next 32bytes data.
+		} else if (i == 24) {		//if the first 320bytes data doesn't not exist, try to read the next 32bytes data.
 			if ((*b) == 0) {
 				dev->otp_size = 32;
 				break;
@@ -780,7 +780,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
 				b = buf;
 				continue;
 			}
-		} else if (27 == i) {		//if the prvious 32bytes data doesn't exist, try to read the next 32bytes data again.
+		} else if (i == 27) {		//if the prvious 32bytes data doesn't exist, try to read the next 32bytes data again.
 			if ((*b) == 0) {
 				dev->otp_size = 32;
 				break;
@@ -1351,7 +1351,7 @@ static int __power_up(struct v4l2_subdev *sd)
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret;
 
-	if (NULL == dev->platform_data) {
+	if (!dev->platform_data) {
 		dev_err(&client->dev,
 			"no camera_sensor_platform_data");
 		return -ENODEV;
@@ -1399,7 +1399,7 @@ static int power_down(struct v4l2_subdev *sd)
 	int ret = 0;
 
 	dev->focus = OV5693_INVALID_CONFIG;
-	if (NULL == dev->platform_data) {
+	if (!dev->platform_data) {
 		dev_err(&client->dev,
 			"no camera_sensor_platform_data");
 		return -ENODEV;
-- 
2.14.3

[PATCH 4/4] staging: fix indentation in atomisp-ov5693

[Riccardo Schirone]

Fix "suspect code indent for conditional statements" issue

Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
---
 drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index 4eeb478ae84b..6eb6afdc730e 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -776,7 +776,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
 			if ((*b) == 0) {
 				dev->otp_size = 32;
 				break;
-		} else {
+			} else {
 				b = buf;
 				continue;
 			}
-- 
2.14.3

Re: [PATCH 3/4] staging: improves comparisons readability in atomisp-ov5693

[jacopo mondi]

Hi Riccardo,
   thanks for the patch

On Mon, Nov 27, 2017 at 10:44:12PM +0100, Riccardo Schirone wrote:
> Fix "Comparisons should place the constant on the right side of the
> test" issue.
>
> Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
> ---
>  drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> index ecd607b7b005..4eeb478ae84b 100644
> --- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> +++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> @@ -764,7 +764,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
>  		//pr_debug("BANK[%2d] %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n", i, *b, *(b+1), *(b+2), *(b+3), *(b+4), *(b+5), *(b+6), *(b+7), *(b+8), *(b+9), *(b+10), *(b+11), *(b+12), *(b+13), *(b+14), *(b+15));
>
>  		//Intel OTP map, try to read 320byts first.
> -		if (21 == i) {
> +		if (i == 21) {
>  			if ((*b) == 0) {
>  				dev->otp_size = 320;
>  				break;
> @@ -772,7 +772,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
>  				b = buf;
>  				continue;
>  			}
> -		} else if (24 == i) {		//if the first 320bytes data doesn't not exist, try to read the next 32bytes data.
> +		} else if (i == 24) {		//if the first 320bytes data doesn't not exist, try to read the next 32bytes data.
>  			if ((*b) == 0) {
>  				dev->otp_size = 32;
>  				break;
> @@ -780,7 +780,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
>  				b = buf;
>  				continue;
>  			}
> -		} else if (27 == i) {		//if the prvious 32bytes data doesn't exist, try to read the next 32bytes data again.
> +		} else if (i == 27) {		//if the prvious 32bytes data doesn't exist, try to read the next 32bytes data again.

I wonder why checkpatch does not complain about these C++ style
comments clearly exceeding 80 columns...

>  			if ((*b) == 0) {
>  				dev->otp_size = 32;
>  				break;
> @@ -1351,7 +1351,7 @@ static int __power_up(struct v4l2_subdev *sd)
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	int ret;
>
> -	if (NULL == dev->platform_data) {
> +	if (!dev->platform_data) {

Please mention in changelog that you're also substituting a comparison to
NULL with this.

Checkpatch points this out, didn't it?

Thanks
   j

>  		dev_err(&client->dev,
>  			"no camera_sensor_platform_data");
>  		return -ENODEV;
> @@ -1399,7 +1399,7 @@ static int power_down(struct v4l2_subdev *sd)
>  	int ret = 0;
>
>  	dev->focus = OV5693_INVALID_CONFIG;
> -	if (NULL == dev->platform_data) {
> +	if (!dev->platform_data) {
>  		dev_err(&client->dev,
>  			"no camera_sensor_platform_data");
>  		return -ENODEV;
> --
> 2.14.3
>

Re: [PATCH 1/4] staging: add missing blank line after declarations in atomisp-ov5693

[jacopo mondi]

Hi Riccardo,

On Mon, Nov 27, 2017 at 10:44:10PM +0100, Riccardo Schirone wrote:
> Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>

No patch can be accepted without a commit message. I know subject is
self-explanatory here, but please add some lines eg. reporting
checkpatch warnings.

Thanks
   j

> ---
>  drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> index 3e7c3851280f..387c65be10f4 100644
> --- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> +++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
> @@ -82,6 +82,7 @@ static int ad5823_i2c_write(struct i2c_client *client, u8 reg, u8 val)
>  {
>  	struct i2c_msg msg;
>  	u8 buf[2];
> +
>  	buf[0] = reg;
>  	buf[1] = val;
>  	msg.addr = AD5823_VCM_ADDR;
> @@ -98,6 +99,7 @@ static int ad5823_i2c_read(struct i2c_client *client, u8 reg, u8 *val)
>  {
>  	struct i2c_msg msg[2];
>  	u8 buf[2];
> +
>  	buf[0] = reg;
>  	buf[1] = 0;
>
> @@ -228,6 +230,7 @@ static int vcm_detect(struct i2c_client *client)
>  	int i, ret;
>  	struct i2c_msg msg;
>  	u16 data0 = 0, data;
> +
>  	for (i = 0; i < 4; i++) {
>  		msg.addr = VCM_ADDR;
>  		msg.flags = I2C_M_RD;
> @@ -690,6 +693,7 @@ static long ov5693_s_exposure(struct v4l2_subdev *sd,
>  	/* we should not accept the invalid value below */
>  	if (analog_gain == 0) {
>  		struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
>  		v4l2_err(client, "%s: invalid value\n", __func__);
>  		return -EINVAL;
>  	}
> @@ -722,6 +726,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
>  	int ret;
>  	int i;
>  	u8 *b = buf;
> +
>  	dev->otp_size = 0;
>  	for (i = 1; i < OV5693_OTP_BANK_MAX; i++) {
>  		/*set bank NO and OTP read mode. */
> @@ -984,6 +989,7 @@ static int ov5693_t_focus_abs(struct v4l2_subdev *sd, s32 value)
>  static int ov5693_t_focus_rel(struct v4l2_subdev *sd, s32 value)
>  {
>  	struct ov5693_device *dev = to_ov5693_sensor(sd);
> +
>  	return ov5693_t_focus_abs(sd, dev->focus + value);
>  }
>
> @@ -1033,6 +1039,7 @@ static int ov5693_q_focus_abs(struct v4l2_subdev *sd, s32 *value)
>  static int ov5693_t_vcm_slew(struct v4l2_subdev *sd, s32 value)
>  {
>  	struct ov5693_device *dev = to_ov5693_sensor(sd);
> +
>  	dev->number_of_steps = value;
>  	dev->vcm_update = true;
>  	return 0;
> @@ -1041,6 +1048,7 @@ static int ov5693_t_vcm_slew(struct v4l2_subdev *sd, s32 value)
>  static int ov5693_t_vcm_timing(struct v4l2_subdev *sd, s32 value)
>  {
>  	struct ov5693_device *dev = to_ov5693_sensor(sd);
> +
>  	dev->number_of_steps = value;
>  	dev->vcm_update = true;
>  	return 0;
> @@ -1563,6 +1571,7 @@ static int ov5693_set_fmt(struct v4l2_subdev *sd,
>  	struct camera_mipi_info *ov5693_info = NULL;
>  	int ret = 0;
>  	int idx;
> +
>  	if (format->pad)
>  		return -EINVAL;
>  	if (!fmt)
> @@ -1599,6 +1608,7 @@ static int ov5693_set_fmt(struct v4l2_subdev *sd,
>  	ret = startup(sd);
>  	if (ret) {
>  		int i = 0;
> +
>  		dev_err(&client->dev, "ov5693 startup err, retry to power up\n");
>  		for (i = 0; i < OV5693_POWER_UP_RETRY_NUM; i++) {
>  			dev_err(&client->dev,
> @@ -1655,6 +1665,7 @@ static int ov5693_get_fmt(struct v4l2_subdev *sd,
>  {
>  	struct v4l2_mbus_framefmt *fmt = &format->format;
>  	struct ov5693_device *dev = to_ov5693_sensor(sd);
> +
>  	if (format->pad)
>  		return -EINVAL;
>
> @@ -1818,6 +1829,7 @@ static int ov5693_s_parm(struct v4l2_subdev *sd,
>  			struct v4l2_streamparm *param)
>  {
>  	struct ov5693_device *dev = to_ov5693_sensor(sd);
> +
>  	dev->run_mode = param->parm.capture.capturemode;
>
>  	mutex_lock(&dev->input_lock);
> @@ -1907,6 +1919,7 @@ static int ov5693_remove(struct i2c_client *client)
>  {
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  	struct ov5693_device *dev = to_ov5693_sensor(sd);
> +
>  	dev_dbg(&client->dev, "ov5693_remove...\n");
>
>  	dev->platform_data->csi_cfg(sd, 0);
> --
> 2.14.3
>

Re: [PATCH 1/4] staging: add missing blank line after declarations in atomisp-ov5693

[Riccardo S.]

On 11/28, jacopo mondi wrote:
> Hi Riccardo,
> 
> On Mon, Nov 27, 2017 at 10:44:10PM +0100, Riccardo Schirone wrote:
> > Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
> 
> No patch can be accepted without a commit message. I know subject is
> self-explanatory here, but please add some lines eg. reporting
> checkpatch warnings.
> 

Ok, I'll do it in V2.
Thanks,

Riccardo

[PATCHv2 0/4] fix some checkpatch style issues in atomisp driver

[Riccardo Schirone]

This patch series fixes some coding style issues in atomisp driver
reported by checkpatch, like: missing blank lines after declarations,
comments style, comparisons and indentation.

It is based on next-20171128.

Changes since v1:
 - Add commit message to first patch as reported by Jacopo Mondi
   <jacopo@jmondi.org>

Riccardo Schirone (4):
  staging: add missing blank line after declarations in atomisp-ov5693
  staging: improve comments usage in atomisp-ov5693
  staging: improves comparisons readability in atomisp-ov5693
  staging: fix indentation in atomisp-ov5693

 .../media/atomisp/i2c/ov5693/atomisp-ov5693.c      | 63 +++++++++++++++-------
 1 file changed, 44 insertions(+), 19 deletions(-)

-- 
2.14.3

[PATCHv2 1/4] staging: add missing blank line after declarations in atomisp-ov5693

[Riccardo Schirone]

Fix "Missing a blank line after declarations" warning reported by
checkpatch.

Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
---
 drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index 3e7c3851280f..387c65be10f4 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -82,6 +82,7 @@ static int ad5823_i2c_write(struct i2c_client *client, u8 reg, u8 val)
 {
 	struct i2c_msg msg;
 	u8 buf[2];
+
 	buf[0] = reg;
 	buf[1] = val;
 	msg.addr = AD5823_VCM_ADDR;
@@ -98,6 +99,7 @@ static int ad5823_i2c_read(struct i2c_client *client, u8 reg, u8 *val)
 {
 	struct i2c_msg msg[2];
 	u8 buf[2];
+
 	buf[0] = reg;
 	buf[1] = 0;
 
@@ -228,6 +230,7 @@ static int vcm_detect(struct i2c_client *client)
 	int i, ret;
 	struct i2c_msg msg;
 	u16 data0 = 0, data;
+
 	for (i = 0; i < 4; i++) {
 		msg.addr = VCM_ADDR;
 		msg.flags = I2C_M_RD;
@@ -690,6 +693,7 @@ static long ov5693_s_exposure(struct v4l2_subdev *sd,
 	/* we should not accept the invalid value below */
 	if (analog_gain == 0) {
 		struct i2c_client *client = v4l2_get_subdevdata(sd);
+
 		v4l2_err(client, "%s: invalid value\n", __func__);
 		return -EINVAL;
 	}
@@ -722,6 +726,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
 	int ret;
 	int i;
 	u8 *b = buf;
+
 	dev->otp_size = 0;
 	for (i = 1; i < OV5693_OTP_BANK_MAX; i++) {
 		/*set bank NO and OTP read mode. */
@@ -984,6 +989,7 @@ static int ov5693_t_focus_abs(struct v4l2_subdev *sd, s32 value)
 static int ov5693_t_focus_rel(struct v4l2_subdev *sd, s32 value)
 {
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	return ov5693_t_focus_abs(sd, dev->focus + value);
 }
 
@@ -1033,6 +1039,7 @@ static int ov5693_q_focus_abs(struct v4l2_subdev *sd, s32 *value)
 static int ov5693_t_vcm_slew(struct v4l2_subdev *sd, s32 value)
 {
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	dev->number_of_steps = value;
 	dev->vcm_update = true;
 	return 0;
@@ -1041,6 +1048,7 @@ static int ov5693_t_vcm_slew(struct v4l2_subdev *sd, s32 value)
 static int ov5693_t_vcm_timing(struct v4l2_subdev *sd, s32 value)
 {
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	dev->number_of_steps = value;
 	dev->vcm_update = true;
 	return 0;
@@ -1563,6 +1571,7 @@ static int ov5693_set_fmt(struct v4l2_subdev *sd,
 	struct camera_mipi_info *ov5693_info = NULL;
 	int ret = 0;
 	int idx;
+
 	if (format->pad)
 		return -EINVAL;
 	if (!fmt)
@@ -1599,6 +1608,7 @@ static int ov5693_set_fmt(struct v4l2_subdev *sd,
 	ret = startup(sd);
 	if (ret) {
 		int i = 0;
+
 		dev_err(&client->dev, "ov5693 startup err, retry to power up\n");
 		for (i = 0; i < OV5693_POWER_UP_RETRY_NUM; i++) {
 			dev_err(&client->dev,
@@ -1655,6 +1665,7 @@ static int ov5693_get_fmt(struct v4l2_subdev *sd,
 {
 	struct v4l2_mbus_framefmt *fmt = &format->format;
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	if (format->pad)
 		return -EINVAL;
 
@@ -1818,6 +1829,7 @@ static int ov5693_s_parm(struct v4l2_subdev *sd,
 			struct v4l2_streamparm *param)
 {
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	dev->run_mode = param->parm.capture.capturemode;
 
 	mutex_lock(&dev->input_lock);
@@ -1907,6 +1919,7 @@ static int ov5693_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 	struct ov5693_device *dev = to_ov5693_sensor(sd);
+
 	dev_dbg(&client->dev, "ov5693_remove...\n");
 
 	dev->platform_data->csi_cfg(sd, 0);
-- 
2.14.3

[PATCHv2 2/4] staging: improve comments usage in atomisp-ov5693

[Riccardo Schirone]

- Fix "Block comments use a trailing */ on a separate line" checkpatch
  issue
- Fix "Block comments use * on subsequent lines" checkpatch issue

Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
---
 .../media/atomisp/i2c/ov5693/atomisp-ov5693.c      | 38 ++++++++++++++--------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index 387c65be10f4..ecd607b7b005 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -213,7 +213,8 @@ static int vcm_dw_i2c_write(struct i2c_client *client, u16 data)
 	return ret == num_msg ? 0 : -EIO;
 }
 
-/* Theory: per datasheet, the two VCMs both allow for a 2-byte read.
+/*
+ * Theory: per datasheet, the two VCMs both allow for a 2-byte read.
  * The DW9714 doesn't actually specify what this does (it has a
  * two-byte write-only protocol, but specifies the read sequence as
  * legal), but it returns the same data (zeroes) always, after an
@@ -224,7 +225,8 @@ static int vcm_dw_i2c_write(struct i2c_client *client, u16 data)
  * these) in AD5823 are not pairwise repetitions of the same 16 bit
  * word.  So all we have to do is sequentially read two bytes at a
  * time and see if we detect a difference in any of the first four
- * pairs.  */
+ * pairs.
+ */
 static int vcm_detect(struct i2c_client *client)
 {
 	int i, ret;
@@ -238,8 +240,10 @@ static int vcm_detect(struct i2c_client *client)
 		msg.buf = (u8 *)&data;
 		ret = i2c_transfer(client->adapter, &msg, 1);
 
-		/* DW9714 always fails the first read and returns
-		 * zeroes for subsequent ones */
+		/*
+		 * DW9714 always fails the first read and returns
+		 * zeroes for subsequent ones
+		 */
 		if (i == 0 && ret == -EREMOTEIO) {
 			data0 = 0;
 			continue;
@@ -533,9 +537,11 @@ static long __ov5693_set_exposure(struct v4l2_subdev *sd, int coarse_itg,
 
 	hts = ov5693_res[dev->fmt_idx].pixels_per_line;
 	vts = ov5693_res[dev->fmt_idx].lines_per_frame;
-	/*If coarse_itg is larger than 1<<15, can not write to reg directly.
-	  The way is to write coarse_itg/2 to the reg, meanwhile write 2*hts
-	  to the reg. */
+	/*
+	 * If coarse_itg is larger than 1<<15, can not write to reg directly.
+	 * The way is to write coarse_itg/2 to the reg, meanwhile write 2*hts
+	 * to the reg.
+	 */
 	if (coarse_itg > (1 << 15)) {
 		hts = hts * 2;
 		coarse_itg = (int)coarse_itg / 2;
@@ -880,8 +886,10 @@ static long ov5693_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
 	return 0;
 }
 
-/* This returns the exposure time being used. This should only be used
-   for filling in EXIF data, not for actual image processing. */
+/*
+ * This returns the exposure time being used. This should only be used
+ * for filling in EXIF data, not for actual image processing.
+ */
 static int ov5693_q_exposure(struct v4l2_subdev *sd, s32 *value)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -1301,11 +1309,13 @@ static int power_ctrl(struct v4l2_subdev *sd, bool flag)
 	if (!dev || !dev->platform_data)
 		return -ENODEV;
 
-	/* This driver assumes "internal DVDD, PWDNB tied to DOVDD".
+	/*
+	 * This driver assumes "internal DVDD, PWDNB tied to DOVDD".
 	 * In this set up only gpio0 (XSHUTDN) should be available
 	 * but in some products (for example ECS) gpio1 (PWDNB) is
 	 * also available. If gpio1 is available we emulate it being
-	 * tied to DOVDD here. */
+	 * tied to DOVDD here.
+	 */
 	if (flag) {
 		ret = dev->platform_data->v2p8_ctrl(sd, 1);
 		dev->platform_data->gpio1_ctrl(sd, 1);
@@ -1944,10 +1954,12 @@ static int ov5693_probe(struct i2c_client *client)
 	struct acpi_device *adev;
 	unsigned int i;
 
-	/* Firmware workaround: Some modules use a "secondary default"
+	/*
+	 * Firmware workaround: Some modules use a "secondary default"
 	 * address of 0x10 which doesn't appear on schematics, and
 	 * some BIOS versions haven't gotten the memo.  Work around
-	 * via config. */
+	 * via config.
+	 */
 	i2c = gmin_get_var_int(&client->dev, "I2CAddr", -1);
 	if (i2c != -1) {
 		dev_info(&client->dev,
-- 
2.14.3

[PATCHv2 3/4] staging: improves comparisons readability in atomisp-ov5693

[Riccardo Schirone]

Fix "Comparisons should place the constant on the right side of the
test" checkpatch issue.

Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
---
 drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index ecd607b7b005..4eeb478ae84b 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -764,7 +764,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
 		//pr_debug("BANK[%2d] %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n", i, *b, *(b+1), *(b+2), *(b+3), *(b+4), *(b+5), *(b+6), *(b+7), *(b+8), *(b+9), *(b+10), *(b+11), *(b+12), *(b+13), *(b+14), *(b+15));
 
 		//Intel OTP map, try to read 320byts first.
-		if (21 == i) {
+		if (i == 21) {
 			if ((*b) == 0) {
 				dev->otp_size = 320;
 				break;
@@ -772,7 +772,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
 				b = buf;
 				continue;
 			}
-		} else if (24 == i) {		//if the first 320bytes data doesn't not exist, try to read the next 32bytes data.
+		} else if (i == 24) {		//if the first 320bytes data doesn't not exist, try to read the next 32bytes data.
 			if ((*b) == 0) {
 				dev->otp_size = 32;
 				break;
@@ -780,7 +780,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
 				b = buf;
 				continue;
 			}
-		} else if (27 == i) {		//if the prvious 32bytes data doesn't exist, try to read the next 32bytes data again.
+		} else if (i == 27) {		//if the prvious 32bytes data doesn't exist, try to read the next 32bytes data again.
 			if ((*b) == 0) {
 				dev->otp_size = 32;
 				break;
@@ -1351,7 +1351,7 @@ static int __power_up(struct v4l2_subdev *sd)
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret;
 
-	if (NULL == dev->platform_data) {
+	if (!dev->platform_data) {
 		dev_err(&client->dev,
 			"no camera_sensor_platform_data");
 		return -ENODEV;
@@ -1399,7 +1399,7 @@ static int power_down(struct v4l2_subdev *sd)
 	int ret = 0;
 
 	dev->focus = OV5693_INVALID_CONFIG;
-	if (NULL == dev->platform_data) {
+	if (!dev->platform_data) {
 		dev_err(&client->dev,
 			"no camera_sensor_platform_data");
 		return -ENODEV;
-- 
2.14.3

[PATCHv2 4/4] staging: fix indentation in atomisp-ov5693

[Riccardo Schirone]

Fix "suspect code indent for conditional statements" checkpatch issue

Signed-off-by: Riccardo Schirone <sirmy15@gmail.com>
---
 drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index 4eeb478ae84b..6eb6afdc730e 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -776,7 +776,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
 			if ((*b) == 0) {
 				dev->otp_size = 32;
 				break;
-		} else {
+			} else {
 				b = buf;
 				continue;
 			}
-- 
2.14.3

Re: [PATCHv2 0/4] fix some checkpatch style issues in atomisp driver

[Sakari Ailus]

On Tue, Nov 28, 2017 at 09:40:00PM +0100, Riccardo Schirone wrote:
> This patch series fixes some coding style issues in atomisp driver
> reported by checkpatch, like: missing blank lines after declarations,
> comments style, comparisons and indentation.
> 
> It is based on next-20171128.
> 
> Changes since v1:
>  - Add commit message to first patch as reported by Jacopo Mondi
>    <jacopo@jmondi.org>
> 
> Riccardo Schirone (4):
>   staging: add missing blank line after declarations in atomisp-ov5693
>   staging: improve comments usage in atomisp-ov5693
>   staging: improves comparisons readability in atomisp-ov5693
>   staging: fix indentation in atomisp-ov5693

Applied, thanks!

Please use staging: atomisp: prefix in the future.

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

Re: [PATCHv2 0/4] fix some checkpatch style issues in atomisp driver

[Riccardo S.]

On 11/29, Sakari Ailus wrote:
> On Tue, Nov 28, 2017 at 09:40:00PM +0100, Riccardo Schirone wrote:
> > This patch series fixes some coding style issues in atomisp driver
> > reported by checkpatch, like: missing blank lines after declarations,
> > comments style, comparisons and indentation.
> > 
> > It is based on next-20171128.
> > 
> > Changes since v1:
> >  - Add commit message to first patch as reported by Jacopo Mondi
> >    <jacopo@jmondi.org>
> > 
> > Riccardo Schirone (4):
> >   staging: add missing blank line after declarations in atomisp-ov5693
> >   staging: improve comments usage in atomisp-ov5693
> >   staging: improves comparisons readability in atomisp-ov5693
> >   staging: fix indentation in atomisp-ov5693
> 
> Applied, thanks!
> 
> Please use staging: atomisp: prefix in the future.
> 
> -- 
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi

Sure! Thanks for the advice.


Riccardo Schirone

Re: [PATCH 0/4] fix some checkpatch style issues in atomisp driver

[Riccardo S.]

Hi Jacopo,

for some reason your comment about "[PATCH 3/4] staging: improves
comparisons readability in atomisp-ov5693" did not reach my inbox.

Unfortunately I already sent PATCHv2 and it has been accepted.
Anyway...

> > @@ -780,7 +780,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
> >  				b = buf;
> >  				continue;
> >  			}
> > -		} else if (27 == i) {		//if the prvious 32bytes data doesn't exist, try to read the next 32bytes data again.
> > +		} else if (i == 27) {		//if the prvious 32bytes data doesn't exist, try to read the next 32bytes data again.
>
> I wonder why checkpatch does not complain about these C++ style
> comments clearly exceeding 80 columns...
>

It complained, but I didn't put that fix in this series. Should I have
cleaned those lines in the same commit since I was already touching
that part of the code? Or better in a separate patch?

> >  			if ((*b) == 0) {
> >  				dev->otp_size = 32;
> >  				break;
> > @@ -1351,7 +1351,7 @@ static int __power_up(struct v4l2_subdev *sd)
> >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >  	int ret;
> >
> > -	if (NULL == dev->platform_data) {
> > +	if (!dev->platform_data) {

> Please mention in changelog that you're also substituting a comparison to
> NULL with this.
> 
> Checkpatch points this out, didn't it?

It actually warned about the comparison that should place the constant
on the right side of the test. When fixing this, I used the "!foo"
syntax. I got your point though.

Thanks for your review!


Riccardo Schirone

Re: [PATCH 0/4] fix some checkpatch style issues in atomisp driver

[jacopo mondi]

Hello Riccardo,

On Wed, Nov 29, 2017 at 10:31:17AM +0100, Riccardo S. wrote:
> Hi Jacopo,
>
> for some reason your comment about "[PATCH 3/4] staging: improves
> comparisons readability in atomisp-ov5693" did not reach my inbox.
>
> Unfortunately I already sent PATCHv2 and it has been accepted.
> Anyway...

No worries!

>
> > > @@ -780,7 +780,7 @@ static int __ov5693_otp_read(struct v4l2_subdev *sd, u8 *buf)
> > >  				b = buf;
> > >  				continue;
> > >  			}
> > > -		} else if (27 == i) {		//if the prvious 32bytes data doesn't exist, try to read the next 32bytes data again.
> > > +		} else if (i == 27) {		//if the prvious 32bytes data doesn't exist, try to read the next 32bytes data again.
> >
> > I wonder why checkpatch does not complain about these C++ style
> > comments clearly exceeding 80 columns...
> >
>
> It complained, but I didn't put that fix in this series. Should I have
> cleaned those lines in the same commit since I was already touching
> that part of the code? Or better in a separate patch?

As you wish.. This is a cleanup series, and fixing comments is
really a minor issues, so if you like to change them in this single
patch you can do that, imo, and mention it in the commit message:
"Fix C++ style comments exceeding 80 columns while at there."
or similar.

>
> > >  			if ((*b) == 0) {
> > >  				dev->otp_size = 32;
> > >  				break;
> > > @@ -1351,7 +1351,7 @@ static int __power_up(struct v4l2_subdev *sd)
> > >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> > >  	int ret;
> > >
> > > -	if (NULL == dev->platform_data) {
> > > +	if (!dev->platform_data) {
>
> > Please mention in changelog that you're also substituting a comparison to
> > NULL with this.
> >
> > Checkpatch points this out, didn't it?
>
> It actually warned about the comparison that should place the constant
> on the right side of the test. When fixing this, I used the "!foo"
> syntax. I got your point though.

Oh, ok, I thought it gave you back a different warning for
comparisons with NULL!

>
> Thanks for your review!

You're welcome!

Cheers
   j

>
>
> Riccardo Schirone