[U-Boot] [PATCH] i2c: mxc: Hide kconfig based control in DM_I2C mode

[Heiko Schocher]

Hello Trent,

Am 30.04.2019 um 19:21 schrieb Trent Piepho:
> On Tue, 2019-04-30 at 09:20 +0200, Heiko Schocher wrote:
>>> Am 12.04.2019 um 21:19 schrieb Trent Piepho:
>>>> These options only apply when not using DM_I2C.  When using device
>>>> trees, the dt will enable and control the speeds of the I2C
>>>> controller(s) and these configuration options have no effect.
>>>>
>>>> So disable them in DM_I2C mode.  Otherwise they show up as decoys, and
>>>> make it look like one is enabling I2C controllers and setting the speed
>>>> when really it's doing nothing.
> 
>>          arm:  +   wandboard
>> +board/wandboard/wandboard.c: In function 'board_init':
>> +board/wandboard/wandboard.c:466:15: error: 'CONFIG_SYS_MXC_I2C1_SPEED' undeclared (first use in
>> this function); did you mean 'CONFIG_SYS_MMC_ENV_DEV'?
>> +  setup_i2c(1, CONFIG_SYS_MXC_I2C1_SPEED, 0x7f, &mx6dl_i2c2_pad_info);
>> +               ^~~~~~~~~~~~~~~~~~~~~~~~~
>> +               CONFIG_SYS_MMC_ENV_DEV
>> +board/wandboard/wandboard.c:466:15: note: each undeclared identifier is reported only once for each
>> function it appears in
>> +board/wandboard/wandboard.c:469:16: error: 'CONFIG_SYS_MXC_I2C2_SPEED' undeclared (first use in
>> this function); did you mean 'CONFIG_SYS_MXC_I2C1_SPEED'?
>> +   setup_i2c(2, CONFIG_SYS_MXC_I2C2_SPEED, 0x7f, &mx6q_i2c3_pad_info);
>> +                ^~~~~~~~~~~~~~~~~~~~~~~~~
>> +                CONFIG_SYS_MXC_I2C1_SPEED
>> +make[2]: *** [board/wandboard/wandboard.o] Error 1
>> +make[1]: *** [board/wandboard] Error 2
>> +make: *** [sub-make] Error 2
>>
>> Please check.
> 
> This is caused by a conflict with "imx6: wandboard: convert to DM_I2C",
> which was applied after I sent my patch.

Ah!

> I'm not sure if that commit is correct. Copying Anatolij.

added also Stefano as imx6 maintainer.

> It's the only place where non-DM_I2C code uses the kconfig based
> configuration of I2C.  If we look at the code in question:
> 
> setup_i2c(1, CONFIG_SYS_MXC_I2C1_SPEED, 0x7f, &mx6dl_i2c2_pad_info);
> 
> Seems a bit odd bus 1 is being configured with i2c*2* pad info?  But
> let's take a look at setup_i2c().
> 
> int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
>                struct i2c_pads_info *p)
> {
>          ....
>          /* Enable i2c clock */
>          ret = enable_i2c_clk(1, i2c_index);
>          if (ret)
>                  goto err_clk;
> 
>          /* Make sure bus is idle */
>          ret = force_idle_bus(p);
>          if (ret)
>                  goto err_idle;
> 
> #ifndef CONFIG_DM_I2C
>          bus_i2c_init(i2c_index, speed, slave_addr, force_idle_bus, p);
> #endif
>          return 0;
>           
> err_idle:
> err_clk:
>          gpio_free(p->scl.gp);
> err_req:
>          gpio_free(p->sda.gp);
> 
>          return ret;
> }
> 
> Nothing in the elided section uses "speed".  It's only used in the
> bus_i2c_init() call, and that call is not made when using DM_I2C!
> 
> So while the wandboard code references CONFIG_SYS_MXC_I2C1_SPEED when
> using DM_i2C, it's never used by anything.
> 
> So I believe I'm still correct, even after the wandboard change that
> CONFIG_SYS_MXC_I2C1_SPEED et al are decoys that aren't used.

Yes, I think you are correct.

@Anatolij: Is this code needed anymore, as board is moved to DM ?

If I look into board/wandboard/wandboard.c

  259 static int detect_i2c(struct display_info_t const *dev)
  260 {
  261 #ifdef CONFIG_DM_I2C
  262         struct udevice *bus, *udev;
  263         int rc;
  264
  265         rc = uclass_get_device_by_seq(UCLASS_I2C, dev->bus, &bus);
  266         if (rc)
  267                 return rc;
  268         rc = dm_i2c_probe(bus, dev->addr, 0, &udev);
  269         if (rc)
  270                 return 0;
  271         return 1;
  272 #else
  273         return (0 == i2c_set_bus_num(dev->bus)) &&
  274                         (0 == i2c_probe(dev->addr));
  275 #endif
  276 }

dm_i2c_probe should initialize the i2c bus completly, also we need
not longer the:

#ifdef CONFIG_DM_I2C

and remove the else part ... or ? Can you test this change?

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