[PATCH] staging: most: dim2: force fcnt=3 on Renesas GEN3

[PATCH] staging: most: dim2: force fcnt=3 on Renesas GEN3

[Nikita Yushchenko]

Per Renesas datasheet, MLBC0 register's fcnt field in the embedded
dim2 module must be never set to value different from 3.

Enforce that, via an optional field in struct dim2_platform_data.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/staging/most/dim2/dim2.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index 093ef9a2b291..d90284d79621 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -108,6 +108,7 @@ struct dim2_hdm {
 struct dim2_platform_data {
 	int (*enable)(struct platform_device *pdev);
 	void (*disable)(struct platform_device *pdev);
+	u8 fcnt;
 };
 
 #define iface_to_hdm(iface) container_of(iface, struct dim2_hdm, most_iface)
@@ -731,7 +732,7 @@ static int dim2_probe(struct platform_device *pdev)
 	struct dim2_hdm *dev;
 	struct resource *res;
 	int ret, i;
-	u8 hal_ret;
+	u8 dev_fcnt, hal_ret;
 	int irq;
 
 	enum { MLB_INT_IDX, AHB0_INT_IDX };
@@ -770,8 +771,10 @@ static int dim2_probe(struct platform_device *pdev)
 
 	dev->disable_platform = pdata ? pdata->disable : NULL;
 
-	dev_info(&pdev->dev, "sync: num of frames per sub-buffer: %u\n", fcnt);
-	hal_ret = dim_startup(dev->io_base, dev->clk_speed, fcnt);
+	dev_fcnt = pdata && pdata->fcnt ? pdata->fcnt : fcnt;
+	dev_info(&pdev->dev, "sync: num of frames per sub-buffer: %u\n",
+		 dev_fcnt);
+	hal_ret = dim_startup(dev->io_base, dev->clk_speed, dev_fcnt);
 	if (hal_ret != DIM_NO_ERROR) {
 		dev_err(&pdev->dev, "dim_startup failed: %d\n", hal_ret);
 		ret = -ENODEV;
@@ -1047,9 +1050,19 @@ static void rcar_m3_disable(struct platform_device *pdev)
 enum dim2_platforms { FSL_MX6, RCAR_H2, RCAR_M3 };
 
 static struct dim2_platform_data plat_data[] = {
-	[FSL_MX6] = { .enable = fsl_mx6_enable, .disable = fsl_mx6_disable },
-	[RCAR_H2] = { .enable = rcar_h2_enable, .disable = rcar_h2_disable },
-	[RCAR_M3] = { .enable = rcar_m3_enable, .disable = rcar_m3_disable },
+	[FSL_MX6] = {
+		.enable = fsl_mx6_enable,
+		.disable = fsl_mx6_disable,
+	},
+	[RCAR_H2] = {
+		.enable = rcar_h2_enable,
+		.disable = rcar_h2_disable,
+	},
+	[RCAR_M3] = {
+		.enable = rcar_m3_enable,
+		.disable = rcar_m3_disable,
+		.fcnt = 3,
+	},
 };
 
 static const struct of_device_id dim2_of_match[] = {
-- 
2.20.1

Re: [PATCH] staging: most: dim2: force fcnt=3 on Renesas GEN3

[Greg Kroah-Hartman]

On Tue, Sep 21, 2021 at 07:51:30PM +0300, Nikita Yushchenko wrote:
> Per Renesas datasheet, MLBC0 register's fcnt field in the embedded
> dim2 module must be never set to value different from 3.
> 
> Enforce that, via an optional field in struct dim2_platform_data.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
>  drivers/staging/most/dim2/dim2.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
> index 093ef9a2b291..d90284d79621 100644
> --- a/drivers/staging/most/dim2/dim2.c
> +++ b/drivers/staging/most/dim2/dim2.c
> @@ -108,6 +108,7 @@ struct dim2_hdm {
>  struct dim2_platform_data {
>  	int (*enable)(struct platform_device *pdev);
>  	void (*disable)(struct platform_device *pdev);
> +	u8 fcnt;
>  };
>  
>  #define iface_to_hdm(iface) container_of(iface, struct dim2_hdm, most_iface)
> @@ -731,7 +732,7 @@ static int dim2_probe(struct platform_device *pdev)
>  	struct dim2_hdm *dev;
>  	struct resource *res;
>  	int ret, i;
> -	u8 hal_ret;
> +	u8 dev_fcnt, hal_ret;
>  	int irq;
>  
>  	enum { MLB_INT_IDX, AHB0_INT_IDX };
> @@ -770,8 +771,10 @@ static int dim2_probe(struct platform_device *pdev)
>  
>  	dev->disable_platform = pdata ? pdata->disable : NULL;
>  
> -	dev_info(&pdev->dev, "sync: num of frames per sub-buffer: %u\n", fcnt);
> -	hal_ret = dim_startup(dev->io_base, dev->clk_speed, fcnt);
> +	dev_fcnt = pdata && pdata->fcnt ? pdata->fcnt : fcnt;

Please use a real if () statement here and do not bury real logic in a
crazy line like this one, as that is all but impossible to maintain over
time.

thanks,

greg k-h

Re: [PATCH] staging: most: dim2: force fcnt=3 on Renesas GEN3

[Nikita Yushchenko]

>> +	dev_fcnt = pdata && pdata->fcnt ? pdata->fcnt : fcnt;
> 
> Please use a real if () statement here and do not bury real logic in a
> crazy line like this one, as that is all but impossible to maintain over
> time.

The same source file already uses the same form of conditional expressions several lines above:

 > ret = pdata && pdata->enable ? pdata->enable(pdev) : 0;

 > dev->disable_platform = pdata ? pdata->disable : NULL;

Shall I use real if statement for my expression while keeping those as-is? This looks ... strange.
Or shall I convert all conditional expressions to if statements?

Nikita

Re: [PATCH] staging: most: dim2: force fcnt=3 on Renesas GEN3

[Greg Kroah-Hartman]

On Mon, Sep 27, 2021 at 06:40:13PM +0300, Nikita Yushchenko wrote:
> > > +	dev_fcnt = pdata && pdata->fcnt ? pdata->fcnt : fcnt;
> > 
> > Please use a real if () statement here and do not bury real logic in a
> > crazy line like this one, as that is all but impossible to maintain over
> > time.
> 
> The same source file already uses the same form of conditional expressions several lines above:
> 
> > ret = pdata && pdata->enable ? pdata->enable(pdev) : 0;

It's not good there either.

There is a reason this code is still in drivers/staging/ and that is one
of them.  Let's not duplicate bad code for no real reason please.

> > dev->disable_platform = pdata ? pdata->disable : NULL;
> 
> Shall I use real if statement for my expression while keeping those as-is? This looks ... strange.
> Or shall I convert all conditional expressions to if statements?

Do not add additional ? : statements in this patch.  I would be glad to
take an add-on patch after this that fixes up the other uses in the
driver, but that should be separate as that is a separate issue here.

thanks,

greg k-h

[PATCH v2] staging: most: dim2: force fcnt=3 on Renesas GEN3

[Nikita Yushchenko]

Per Renesas datasheet, MLBC0 register's fcnt field in the embedded
dim2 module must be never set to value different from 3.

Enforce that, via an optional field in struct dim2_platform_data.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
Changes from v1:
- set dev_fcnt via if statement, not conditional expression

 drivers/staging/most/dim2/dim2.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index 093ef9a2b291..9300040ec84c 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -108,6 +108,7 @@ struct dim2_hdm {
 struct dim2_platform_data {
 	int (*enable)(struct platform_device *pdev);
 	void (*disable)(struct platform_device *pdev);
+	u8 fcnt;
 };
 
 #define iface_to_hdm(iface) container_of(iface, struct dim2_hdm, most_iface)
@@ -731,7 +732,7 @@ static int dim2_probe(struct platform_device *pdev)
 	struct dim2_hdm *dev;
 	struct resource *res;
 	int ret, i;
-	u8 hal_ret;
+	u8 dev_fcnt, hal_ret;
 	int irq;
 
 	enum { MLB_INT_IDX, AHB0_INT_IDX };
@@ -770,8 +771,14 @@ static int dim2_probe(struct platform_device *pdev)
 
 	dev->disable_platform = pdata ? pdata->disable : NULL;
 
-	dev_info(&pdev->dev, "sync: num of frames per sub-buffer: %u\n", fcnt);
-	hal_ret = dim_startup(dev->io_base, dev->clk_speed, fcnt);
+	if (pdata && pdata->fcnt)
+		dev_fcnt = pdata->fcnt;
+	else
+		dev_fcnt = fcnt;
+
+	dev_info(&pdev->dev, "sync: num of frames per sub-buffer: %u\n",
+		 dev_fcnt);
+	hal_ret = dim_startup(dev->io_base, dev->clk_speed, dev_fcnt);
 	if (hal_ret != DIM_NO_ERROR) {
 		dev_err(&pdev->dev, "dim_startup failed: %d\n", hal_ret);
 		ret = -ENODEV;
@@ -1047,9 +1054,19 @@ static void rcar_m3_disable(struct platform_device *pdev)
 enum dim2_platforms { FSL_MX6, RCAR_H2, RCAR_M3 };
 
 static struct dim2_platform_data plat_data[] = {
-	[FSL_MX6] = { .enable = fsl_mx6_enable, .disable = fsl_mx6_disable },
-	[RCAR_H2] = { .enable = rcar_h2_enable, .disable = rcar_h2_disable },
-	[RCAR_M3] = { .enable = rcar_m3_enable, .disable = rcar_m3_disable },
+	[FSL_MX6] = {
+		.enable = fsl_mx6_enable,
+		.disable = fsl_mx6_disable,
+	},
+	[RCAR_H2] = {
+		.enable = rcar_h2_enable,
+		.disable = rcar_h2_disable,
+	},
+	[RCAR_M3] = {
+		.enable = rcar_m3_enable,
+		.disable = rcar_m3_disable,
+		.fcnt = 3,
+	},
 };
 
 static const struct of_device_id dim2_of_match[] = {
-- 
2.30.2

Re: [PATCH v2] staging: most: dim2: force fcnt=3 on Renesas GEN3

[Greg Kroah-Hartman]

On Mon, Sep 27, 2021 at 06:58:05PM +0300, Nikita Yushchenko wrote:
> Per Renesas datasheet, MLBC0 register's fcnt field in the embedded
> dim2 module must be never set to value different from 3.
> 
> Enforce that, via an optional field in struct dim2_platform_data.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
> Changes from v1:
> - set dev_fcnt via if statement, not conditional expression

Much nicer, thanks!

greg k-h

[PATCH] staging: most: dim2: use if statements instead of ?: expressions

[Nikita Yushchenko]

For better maintainability, replace conditional expressions with if
statements in the drivers' probe routine.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/staging/most/dim2/dim2.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c
index 9300040ec84c..e8b03fa90e80 100644
--- a/drivers/staging/most/dim2/dim2.c
+++ b/drivers/staging/most/dim2/dim2.c
@@ -732,7 +732,8 @@ static int dim2_probe(struct platform_device *pdev)
 	struct dim2_hdm *dev;
 	struct resource *res;
 	int ret, i;
-	u8 dev_fcnt, hal_ret;
+	u8 hal_ret;
+	u8 dev_fcnt = fcnt;
 	int irq;
 
 	enum { MLB_INT_IDX, AHB0_INT_IDX };
@@ -765,16 +766,16 @@ static int dim2_probe(struct platform_device *pdev)
 
 	of_id = of_match_node(dim2_of_match, pdev->dev.of_node);
 	pdata = of_id->data;
-	ret = pdata && pdata->enable ? pdata->enable(pdev) : 0;
-	if (ret)
-		return ret;
-
-	dev->disable_platform = pdata ? pdata->disable : NULL;
-
-	if (pdata && pdata->fcnt)
-		dev_fcnt = pdata->fcnt;
-	else
-		dev_fcnt = fcnt;
+	if (pdata) {
+		if (pdata->enable) {
+			ret = pdata->enable(pdev);
+			if (ret)
+				return ret;
+		}
+		dev->disable_platform = pdata->disable;
+		if (pdata->fcnt)
+			dev_fcnt = pdata->fcnt;
+	}
 
 	dev_info(&pdev->dev, "sync: num of frames per sub-buffer: %u\n",
 		 dev_fcnt);
-- 
2.30.2