[PATCH v7 3/8] i2c: i2c-cortina: added CAxxxx I2C support

[Heiko Schocher]

Hello Alex,

Am 14.05.2020 um 22:11 schrieb Alex Nemirovsky:
> From: Arthur Li <arthur.li at cortina-access.com>
> 
> Add I2C controller support for Cortina Access CAxxxx SoCs
> 
> Signed-off-by: Arthur Li <arthur.li at cortina-access.com>
> Signed-off-by: Alex Nemirovsky <alex.nemirovsky at cortina-access.com>
> CC: Heiko Schocher <hs at denx.de>
> 
> CA_I2C: DT binding for I2C controller
> 
> DT binding document for Cortina I2C driver
> 
> ---
> 
> Changes in v7:
> - Added additional description info in I2C KConfig
> 
> Changes in v6:
> - Add I2C DT binding document
> 
> Changes in v5: None
> Changes in v4:
> - Utilize standard I2C macros from <i2c.h>
> - Return ETIMEDOUT in funcs that can timeout
> - Return i2c_xfer_init() result to caller of i2c_read() if it
> 	fails within i2c_read() execution
> - Fix misc. style guide conformance issues
> - Use printf() to report i2c_xfer() runtime errors
> 	instead of debug()
> 
> Changes in v3: None
> Changes in v2: None
> 
>   MAINTAINERS                                  |   4 +
>   doc/device-tree-bindings/i2c/i2c-cortina.txt |  18 ++
>   drivers/i2c/Kconfig                          |   8 +
>   drivers/i2c/Makefile                         |   1 +
>   drivers/i2c/i2c-cortina.c                    | 346 +++++++++++++++++++++++++++
>   drivers/i2c/i2c-cortina.h                    |  84 +++++++
>   6 files changed, 461 insertions(+)
>   create mode 100644 doc/device-tree-bindings/i2c/i2c-cortina.txt
>   create mode 100644 drivers/i2c/i2c-cortina.c
>   create mode 100644 drivers/i2c/i2c-cortina.h

Reviewed-by: Heiko Schocher <hs at denx.de>

Nitpick only ...

[...]
> diff --git a/drivers/i2c/i2c-cortina.c b/drivers/i2c/i2c-cortina.c
> new file mode 100644
> index 0000000..08b812a
> --- /dev/null
> +++ b/drivers/i2c/i2c-cortina.c
> @@ -0,0 +1,346 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2020
> + * Arthur Li, Cortina Access, arthur.li at cortina-access.com.
> + */
> +
> +#include <common.h>
> +#include <i2c.h>
> +#include <asm/io.h>
> +#include <dm.h>
> +#include <mapmem.h>
> +#include "i2c-cortina.h"
> +
> +static void set_speed(struct i2c_regs *regs, int i2c_spd)
> +{
> +	union ca_biw_cfg i2c_cfg;
> +
> +	i2c_cfg.wrd = readl(&regs->i2c_cfg);
> +	i2c_cfg.bf.core_en = 0;
> +	writel(i2c_cfg.wrd, &regs->i2c_cfg);
> +
> +	switch (i2c_spd) {
> +	case IC_SPEED_MODE_FAST_PLUS:
> +		i2c_cfg.bf.prer = CORTINA_PER_IO_FREQ /
> +				  (5 * I2C_SPEED_FAST_PLUS_RATE) - 1;
> +		break;
> +
> +	case IC_SPEED_MODE_STANDARD:
> +		i2c_cfg.bf.prer = CORTINA_PER_IO_FREQ /
> +				  (5 * I2C_SPEED_STANDARD_RATE) - 1;
> +		break;
> +
> +	case IC_SPEED_MODE_FAST:
> +	default:
> +		i2c_cfg.bf.prer = CORTINA_PER_IO_FREQ /
> +				  (5 * I2C_SPEED_FAST_RATE) - 1;
> +		break;
> +	}
> +
> +	i2c_cfg.bf.core_en = 1;
> +	writel(i2c_cfg.wrd, &regs->i2c_cfg);
> +}
> +
> +static int ca_i2c_set_bus_speed(struct udevice *bus, unsigned int speed)
> +{
> +	struct ca_i2c *priv = dev_get_priv(bus);
> +	int i2c_spd;
> +
> +	if (speed >= I2C_SPEED_FAST_PLUS_RATE) {
> +		i2c_spd = IC_SPEED_MODE_FAST_PLUS;
> +		priv->speed = I2C_SPEED_FAST_PLUS_RATE;
> +	} else if (speed >= I2C_SPEED_FAST_RATE) {
> +		i2c_spd = IC_SPEED_MODE_FAST;
> +		priv->speed = I2C_SPEED_FAST_RATE;
> +	} else {
> +		i2c_spd = IC_SPEED_MODE_STANDARD;
> +		priv->speed = I2C_SPEED_STANDARD_RATE;
> +	}

you could set only one vairable in the if paths (and drop {)
and set the other variable later ...
[...]

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de