From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Bosch <stefan_b@posteo.net>
Date: Mon, 6 Jul 2020 09:04:47 +0200
Subject: [PATCH v3 04/14] i2c: add nexell driver
In-Reply-To: <627bc0e4-ee39-f622-b0f0-a76c87c1af70@denx.de>
References: <1593452806-8609-1-git-send-email-stefan_b@posteo.net>
 <1593452806-8609-5-git-send-email-stefan_b@posteo.net>
 <627bc0e4-ee39-f622-b0f0-a76c87c1af70@denx.de>
Message-ID: <65fae6fc-b60f-eafe-2742-3d047cfde367@posteo.net>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hello Heiko,

thank you for your proposals. I'll make the appropriate changes.

Regards
Stefan

Am 03.07.20 um 08:03 schrieb 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@posteo.net>
> 
> Reviewed-by: Heiko Schocher <hs@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