[PATCH v3 04/14] i2c: add nexell driver

[Heiko Schocher]

Hello Stefan,

Am 29.06.2020 um 19:46 schrieb Stefan Bosch:
> Changes in relation to FriendlyARM's U-Boot nanopi2-v2016.01:
> - i2c/nx_i2c.c: Some adaptions mainly because of changes in
>    "struct udevice".
> - several Bugfixes in nx_i2c.c.
> - the driver has been for s5p6818 only. Code extended appropriately
>    in order s5p4418 is also working.
> - "probe_chip" added.
> - pinctrl-driver/dt is used instead of configuring the i2c I/O-pins
>    in the i2c-driver.
> - '#ifdef CONFIG...' changed to 'if (IS_ENABLED(CONFIG...))' where
>    possible (and similar).
> - livetree API (dev_read_...) is used instead of fdt one (fdt...).
> 
> Signed-off-by: Stefan Bosch <stefan_b at posteo.net>

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

Nitpick only ...

[...]
> diff --git a/drivers/i2c/nx_i2c.c b/drivers/i2c/nx_i2c.c
> new file mode 100644
> index 0000000..cefc774
> --- /dev/null
> +++ b/drivers/i2c/nx_i2c.c
> @@ -0,0 +1,624 @@
[...]
> +static uint i2c_set_clk(struct nx_i2c_bus *bus, uint enb)
> +{
> +	struct clk *clk;
> +	char name[50];
> +
> +	sprintf(name, "%s.%d", DEV_NAME_I2C, bus->bus_num);
> +	clk = clk_get((const char *)name);
> +	if (!clk) {
> +		debug("%s(): clk_get(%s) error!\n",
> +		      __func__, (const char *)name);
> +		return -EINVAL;
> +	}
> +
> +	if (enb) {
> +		clk_disable(clk);
> +		clk_enable(clk);
> +	} else {
> +		clk_disable(clk);
> +	}

You can do here:

	clk_disable(clk);
	if (enb)
		clk_enable(clk);

> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_ARCH_S5P6818
> +/* Set SDA line delay, not available at S5P4418 */
> +static int nx_i2c_set_sda_delay(struct nx_i2c_bus *bus)
> +{
> +	struct nx_i2c_regs *i2c = bus->regs;
> +	uint pclk = 0;
> +	uint t_pclk = 0;
> +	uint delay = 0;
> +
> +	/* get input clock of the I2C-controller */
> +	pclk = i2c_get_clkrate(bus);
> +
> +	if (bus->sda_delay) {
> +		/* t_pclk = period time of one pclk [ns] */
> +		t_pclk = DIV_ROUND_UP(1000, pclk / 1000000);
> +		/* delay = number of pclks required for sda_delay [ns] */
> +		delay = DIV_ROUND_UP(bus->sda_delay, t_pclk);
> +		/* delay = register value (step of 5 clocks) */
> +		delay = DIV_ROUND_UP(delay, 5);
> +		/* max. possible register value = 3 */
> +		if (delay > 3) {

May you use defines here?

> +			delay = 3;
> +			debug("%s(): sda-delay des.: %dns, sat. to max.: %dns (granularity: %dns)\n",
> +			      __func__, bus->sda_delay, t_pclk * delay * 5,
> +			      t_pclk * 5);
> +		} else {
> +			debug("%s(): sda-delay des.: %dns, act.: %dns (granularity: %dns)\n",
> +			      __func__, bus->sda_delay, t_pclk * delay * 5,
> +			      t_pclk * 5);
> +		}
> +
> +		delay |= I2CLC_FILTER;
> +	} else {
> +		delay = 0;
> +		debug("%s(): sda-delay = 0\n", __func__);
> +	}
> +
> +	delay &= 0x7;
> +	writel(delay, &i2c->iiclc);
> +
> +	return 0;
> +}
> +#endif
> +
> +static int nx_i2c_set_bus_speed(struct udevice *dev, uint speed)
> +{
> +	struct nx_i2c_bus *bus = dev_get_priv(dev);
> +	struct nx_i2c_regs *i2c = bus->regs;
> +	unsigned long pclk, pres = 16, div;
> +
> +	if (i2c_set_clk(bus, 1))
> +		return -EINVAL;
> +
> +	/* get input clock of the I2C-controller */
> +	pclk = i2c_get_clkrate(bus);
> +
> +	/* calculate prescaler and divisor values */
> +	if ((pclk / pres / (16 + 1)) > speed)
> +		/* set prescaler to 256 */
> +		pres = 256;
> +
> +	div = 0;
> +	/* actual divider = div + 1 */
> +	while ((pclk / pres / (div + 1)) > speed)
> +		div++;
> +
> +	if (div > 0xF) {
> +		debug("%s(): pres==%ld, div==0x%lx is saturated to 0xF !)\n",
> +		      __func__, pres, div);
> +		div = 0xF;
> +	} else {
> +		debug("%s(): pres==%ld, div==0x%lx)\n", __func__, pres, div);
> +	}
> +
> +	/* set prescaler, divisor according to pclk, also set ACKGEN, IRQ */
> +	writel((div & 0x0F) | ((pres == 256) ? 0x40 : 0), &i2c->iiccon);

Ok, I am really nitpicking now ... can you use defines here instead
this magic values?

[...]

Thanks!

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