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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>> + 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
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
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 <[email protected]>
---
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
For better maintainability, replace conditional expressions with if
statements in the drivers' probe routine.
Signed-off-by: Nikita Yushchenko <[email protected]>
---
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
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 <[email protected]>
> ---
> Changes from v1:
> - set dev_fcnt via if statement, not conditional expression
Much nicer, thanks!
greg k-h