[PATCH v2 0/2] MAX96714/7 style fixup

[PATCH v2 0/2] MAX96714/7 style fixup

[Julien Massot]

There was still some open comments from Sakari that haven't been
addressed.

Signed-off-by: Julien Massot <julien.massot@collabora.com>
---
Changes in v2:
- Fix uppercase in register address for max96714 driver as suggested by Tommaso
- Link to v1: https://lore.kernel.org/r/20240628-gmsl2-drivers-style-fixup-v1-0-a4bb49f4c7a1@collabora.com

---
Julien Massot (2):
      media: i2c: max96717: coding style fixes
      media: i2c: max96714: coding style fixes

 drivers/media/i2c/max96714.c | 18 +++++++++---------
 drivers/media/i2c/max96717.c | 31 ++++++++++++-------------------
 2 files changed, 21 insertions(+), 28 deletions(-)
---
base-commit: aac87ca083749ad835878df10915d8569c9fb34a
change-id: 20240628-gmsl2-drivers-style-fixup-8b7456273b39
prerequisite-message-id: 20240627151806.3999400-2-tomm.merciai@gmail.com
prerequisite-patch-id: 962642e5bbea7583a1b4f6eb0a962e60ef9ffba3

Best regards,
-- 
Julien Massot <julien.massot@collabora.com>

[PATCH v2 1/2] media: i2c: max96717: coding style fixes

[Julien Massot]

Coding style fixes suggested by Sakari during the
driver review.

Signed-off-by: Julien Massot <julien.massot@collabora.com>
---
 drivers/media/i2c/max96717.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
index 859a439b64d9..4e85b8eb1e77 100644
--- a/drivers/media/i2c/max96717.c
+++ b/drivers/media/i2c/max96717.c
@@ -25,6 +25,7 @@
 #define MAX96717_PORTS      2
 #define MAX96717_PAD_SINK   0
 #define MAX96717_PAD_SOURCE 1
+#define MAX96717_CSI_NLANES 4
 
 #define MAX96717_DEFAULT_CLKOUT_RATE	24000000UL
 
@@ -495,7 +496,6 @@ static int max96717_enable_streams(struct v4l2_subdev *sd,
 				   u64 streams_mask)
 {
 	struct max96717_priv *priv = sd_to_max96717(sd);
-	struct device *dev = &priv->client->dev;
 	u64 sink_streams;
 	int ret;
 
@@ -516,11 +516,8 @@ static int max96717_enable_streams(struct v4l2_subdev *sd,
 		ret = v4l2_subdev_enable_streams(priv->source_sd,
 						 priv->source_sd_pad,
 						 sink_streams);
-		if (ret) {
-			dev_err(dev, "Fail to start streams:%llu on remote subdev\n",
-				sink_streams);
+		if (ret)
 			goto stop_csi;
-		}
 	}
 
 	priv->enabled_source_streams |= streams_mask;
@@ -530,6 +527,7 @@ static int max96717_enable_streams(struct v4l2_subdev *sd,
 stop_csi:
 	if (!priv->enabled_source_streams)
 		max96717_start_csi(priv, false);
+
 	return ret;
 }
 
@@ -769,11 +767,8 @@ max96717_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 static unsigned int max96717_clk_find_best_index(struct max96717_priv *priv,
 						 unsigned long rate)
 {
-	unsigned int i, idx;
-	unsigned long diff_new, diff_old;
-
-	diff_old = U32_MAX;
-	idx = 0;
+	unsigned int i, idx = 0;
+	unsigned long diff_new, diff_old = U32_MAX;
 
 	for (i = 0; i < ARRAY_SIZE(max96717_predef_freqs); i++) {
 		diff_new = abs(rate - max96717_predef_freqs[i].freq);
@@ -860,8 +855,7 @@ static int max96717_register_clkout(struct max96717_priv *priv)
 	struct clk_init_data init = { .ops = &max96717_clk_ops };
 	int ret;
 
-	init.name = kasprintf(GFP_KERNEL, "max96717.%s.clk_out",
-			      dev_name(dev));
+	init.name = kasprintf(GFP_KERNEL, "max96717.%s.clk_out", dev_name(dev));
 	if (!init.name)
 		return -ENOMEM;
 
@@ -944,8 +938,9 @@ static int max96717_init_csi_lanes(struct max96717_priv *priv)
 	 * Unused lanes need to be mapped as well to not have
 	 * the same lanes mapped twice.
 	 */
-	for (; lane < 4; lane++) {
-		unsigned int idx = find_first_zero_bit(&lanes_used, 4);
+	for (; lane < MAX96717_CSI_NLANES; lane++) {
+		unsigned int idx = find_first_zero_bit(&lanes_used,
+						       MAX96717_CSI_NLANES);
 
 		val |= idx << (lane * 2);
 		lanes_used |= BIT(idx);
@@ -999,9 +994,7 @@ static int max96717_hw_init(struct max96717_priv *priv)
 static int max96717_parse_dt(struct max96717_priv *priv)
 {
 	struct device *dev = &priv->client->dev;
-	struct v4l2_fwnode_endpoint vep = {
-		.bus_type = V4L2_MBUS_CSI2_DPHY
-	};
+	struct v4l2_fwnode_endpoint vep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
 	struct fwnode_handle *ep_fwnode;
 	unsigned char num_data_lanes;
 	int ret;
@@ -1019,11 +1012,11 @@ static int max96717_parse_dt(struct max96717_priv *priv)
 		return dev_err_probe(dev, ret, "Failed to parse sink endpoint");
 
 	num_data_lanes = vep.bus.mipi_csi2.num_data_lanes;
-	if (num_data_lanes < 1 || num_data_lanes > 4)
+	if (num_data_lanes < 1 || num_data_lanes > MAX96717_CSI_NLANES)
 		return dev_err_probe(dev, -EINVAL,
 				     "Invalid data lanes must be 1 to 4\n");
 
-	memcpy(&priv->mipi_csi2, &vep.bus.mipi_csi2, sizeof(priv->mipi_csi2));
+	priv->mipi_csi2 = vep.bus.mipi_csi2;
 
 	return 0;
 }

-- 
2.45.2

[PATCH v2 2/2] media: i2c: max96714: coding style fixes

[Julien Massot]

Coding style fixes suggested by Sakari during the
driver review.

Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
Tested-by: Tommaso Merciai <tomm.merciai@gmail.com>
Signed-off-by: Julien Massot <julien.massot@collabora.com>
---
 drivers/media/i2c/max96714.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/max96714.c b/drivers/media/i2c/max96714.c
index c97de66631e0..159753b13777 100644
--- a/drivers/media/i2c/max96714.c
+++ b/drivers/media/i2c/max96714.c
@@ -25,6 +25,7 @@
 #define MAX96714_NPORTS     2
 #define MAX96714_PAD_SINK   0
 #define MAX96714_PAD_SOURCE 1
+#define MAX96714_CSI_NLANES 4
 
 /* DEV */
 #define MAX96714_REG13                 CCI_REG8(0x0d)
@@ -52,9 +53,9 @@
 #define MAX96714_PATGEN_V2D            CCI_REG24(0x254)
 #define MAX96714_PATGEN_DE_HIGH        CCI_REG16(0x257)
 #define MAX96714_PATGEN_DE_LOW         CCI_REG16(0x259)
-#define MAX96714_PATGEN_DE_CNT         CCI_REG16(0x25B)
+#define MAX96714_PATGEN_DE_CNT         CCI_REG16(0x25b)
 #define MAX96714_PATGEN_GRAD_INC       CCI_REG8(0x25d)
-#define MAX96714_PATGEN_CHKB_COLOR_A   CCI_REG24(0x25E)
+#define MAX96714_PATGEN_CHKB_COLOR_A   CCI_REG24(0x25e)
 #define MAX96714_PATGEN_CHKB_COLOR_B   CCI_REG24(0x261)
 #define MAX96714_PATGEN_CHKB_RPT_CNT_A CCI_REG8(0x264)
 #define MAX96714_PATGEN_CHKB_RPT_CNT_B CCI_REG8(0x265)
@@ -724,8 +725,9 @@ static int max96714_init_tx_port(struct max96714_priv *priv)
 	 * Unused lanes need to be mapped as well to not have
 	 * the same lanes mapped twice.
 	 */
-	for (; lane < 4; lane++) {
-		unsigned int idx = find_first_zero_bit(&lanes_used, 4);
+	for (; lane < MAX96714_CSI_NLANES; lane++) {
+		unsigned int idx = find_first_zero_bit(&lanes_used,
+						       MAX96714_CSI_NLANES);
 
 		val |= idx << (lane * 2);
 		lanes_used |= BIT(idx);
@@ -757,9 +759,7 @@ static int max96714_rxport_disable_poc(struct max96714_priv *priv)
 static int max96714_parse_dt_txport(struct max96714_priv *priv)
 {
 	struct device *dev = &priv->client->dev;
-	struct v4l2_fwnode_endpoint vep = {
-		.bus_type = V4L2_MBUS_CSI2_DPHY
-	};
+	struct v4l2_fwnode_endpoint vep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
 	struct fwnode_handle *ep_fwnode;
 	u32 num_data_lanes;
 	int ret;
@@ -791,14 +791,14 @@ static int max96714_parse_dt_txport(struct max96714_priv *priv)
 	}
 
 	num_data_lanes = vep.bus.mipi_csi2.num_data_lanes;
-	if (num_data_lanes < 1 || num_data_lanes > 4) {
+	if (num_data_lanes < 1 || num_data_lanes > MAX96714_CSI_NLANES) {
 		dev_err(dev,
 			"tx: invalid number of data lanes must be 1 to 4\n");
 		ret = -EINVAL;
 		goto err_free_vep;
 	}
 
-	memcpy(&priv->mipi_csi2, &vep.bus.mipi_csi2, sizeof(priv->mipi_csi2));
+	priv->mipi_csi2 = vep.bus.mipi_csi2;
 
 err_free_vep:
 	v4l2_fwnode_endpoint_free(&vep);

-- 
2.45.2

Re: [PATCH v2 1/2] media: i2c: max96717: coding style fixes

[Tommaso Merciai]

Hi Julien,
Sorry for delay on reviewing/testing this.

On Mon, Jul 01, 2024 at 11:31:42AM +0200, Julien Massot wrote:
> Coding style fixes suggested by Sakari during the
> driver review.
> 
> Signed-off-by: Julien Massot <julien.massot@collabora.com>
> ---
>  drivers/media/i2c/max96717.c | 31 ++++++++++++-------------------
>  1 file changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c
> index 859a439b64d9..4e85b8eb1e77 100644
> --- a/drivers/media/i2c/max96717.c
> +++ b/drivers/media/i2c/max96717.c
> @@ -25,6 +25,7 @@
>  #define MAX96717_PORTS      2
>  #define MAX96717_PAD_SINK   0
>  #define MAX96717_PAD_SOURCE 1
> +#define MAX96717_CSI_NLANES 4
>  
>  #define MAX96717_DEFAULT_CLKOUT_RATE	24000000UL
>  
> @@ -495,7 +496,6 @@ static int max96717_enable_streams(struct v4l2_subdev *sd,
>  				   u64 streams_mask)
>  {
>  	struct max96717_priv *priv = sd_to_max96717(sd);
> -	struct device *dev = &priv->client->dev;
>  	u64 sink_streams;
>  	int ret;
>  
> @@ -516,11 +516,8 @@ static int max96717_enable_streams(struct v4l2_subdev *sd,
>  		ret = v4l2_subdev_enable_streams(priv->source_sd,
>  						 priv->source_sd_pad,
>  						 sink_streams);
> -		if (ret) {
> -			dev_err(dev, "Fail to start streams:%llu on remote subdev\n",
> -				sink_streams);
> +		if (ret)
>  			goto stop_csi;
> -		}
>  	}
>  
>  	priv->enabled_source_streams |= streams_mask;
> @@ -530,6 +527,7 @@ static int max96717_enable_streams(struct v4l2_subdev *sd,
>  stop_csi:
>  	if (!priv->enabled_source_streams)
>  		max96717_start_csi(priv, false);
> +
>  	return ret;
>  }
>  
> @@ -769,11 +767,8 @@ max96717_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
>  static unsigned int max96717_clk_find_best_index(struct max96717_priv *priv,
>  						 unsigned long rate)
>  {
> -	unsigned int i, idx;
> -	unsigned long diff_new, diff_old;
> -
> -	diff_old = U32_MAX;
> -	idx = 0;
> +	unsigned int i, idx = 0;
> +	unsigned long diff_new, diff_old = U32_MAX;
>  
>  	for (i = 0; i < ARRAY_SIZE(max96717_predef_freqs); i++) {
>  		diff_new = abs(rate - max96717_predef_freqs[i].freq);
> @@ -860,8 +855,7 @@ static int max96717_register_clkout(struct max96717_priv *priv)
>  	struct clk_init_data init = { .ops = &max96717_clk_ops };
>  	int ret;
>  
> -	init.name = kasprintf(GFP_KERNEL, "max96717.%s.clk_out",
> -			      dev_name(dev));
> +	init.name = kasprintf(GFP_KERNEL, "max96717.%s.clk_out", dev_name(dev));
>  	if (!init.name)
>  		return -ENOMEM;
>  
> @@ -944,8 +938,9 @@ static int max96717_init_csi_lanes(struct max96717_priv *priv)
>  	 * Unused lanes need to be mapped as well to not have
>  	 * the same lanes mapped twice.
>  	 */
> -	for (; lane < 4; lane++) {
> -		unsigned int idx = find_first_zero_bit(&lanes_used, 4);
> +	for (; lane < MAX96717_CSI_NLANES; lane++) {
> +		unsigned int idx = find_first_zero_bit(&lanes_used,
> +						       MAX96717_CSI_NLANES);
>  
>  		val |= idx << (lane * 2);
>  		lanes_used |= BIT(idx);
> @@ -999,9 +994,7 @@ static int max96717_hw_init(struct max96717_priv *priv)
>  static int max96717_parse_dt(struct max96717_priv *priv)
>  {
>  	struct device *dev = &priv->client->dev;
> -	struct v4l2_fwnode_endpoint vep = {
> -		.bus_type = V4L2_MBUS_CSI2_DPHY
> -	};
> +	struct v4l2_fwnode_endpoint vep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
>  	struct fwnode_handle *ep_fwnode;
>  	unsigned char num_data_lanes;
>  	int ret;
> @@ -1019,11 +1012,11 @@ static int max96717_parse_dt(struct max96717_priv *priv)
>  		return dev_err_probe(dev, ret, "Failed to parse sink endpoint");
>  
>  	num_data_lanes = vep.bus.mipi_csi2.num_data_lanes;
> -	if (num_data_lanes < 1 || num_data_lanes > 4)
> +	if (num_data_lanes < 1 || num_data_lanes > MAX96717_CSI_NLANES)
>  		return dev_err_probe(dev, -EINVAL,
>  				     "Invalid data lanes must be 1 to 4\n");
>  
> -	memcpy(&priv->mipi_csi2, &vep.bus.mipi_csi2, sizeof(priv->mipi_csi2));
> +	priv->mipi_csi2 = vep.bus.mipi_csi2;
>  
>  	return 0;
>  }
> 
> -- 
> 2.45.2
> 

Patch looks good to me. I tested this on my gmsl2 alvium and all is working fine.

Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
Tested-by: Tommaso Merciai <tomm.merciai@gmail.com>

Thanks & Regards,
Tommaso

Re: [PATCH v2 0/2] MAX96714/7 style fixup

[Sakari Ailus]

Hi Julien,

On Mon, Jul 01, 2024 at 11:31:41AM +0200, Julien Massot wrote:
> There was still some open comments from Sakari that haven't been
> addressed.
> 
> Signed-off-by: Julien Massot <julien.massot@collabora.com>

This no longer applies to the current media stage tree. Could you rebase
it, please?

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 0/2] MAX96714/7 style fixup

[Sakari Ailus]

On Tue, Jul 30, 2024 at 08:35:31AM +0000, Sakari Ailus wrote:
> Hi Julien,
> 
> On Mon, Jul 01, 2024 at 11:31:41AM +0200, Julien Massot wrote:
> > There was still some open comments from Sakari that haven't been
> > addressed.
> > 
> > Signed-off-by: Julien Massot <julien.massot@collabora.com>
> 
> This no longer applies to the current media stage tree. Could you rebase
> it, please?

Oh, it depended on the test pattern patch. Even better. Thanks!

-- 
Sakari Ailus