Hi Michal, On Tue, 5 May 2020 at 02:26, Michal Simek <michal.si...@xilinx.com> wrote: > > On 15. 04. 20 8:40, Michal Simek wrote: > > On 10. 04. 20 10:49, Heiko Schocher wrote: > >> Hello Michal, > >> > >> Am 10.04.2020 um 08:46 schrieb Michal Simek: > >>> Hi Heiko, > >>> > >>> On 10. 04. 20 7:11, Heiko Schocher wrote: > >>>> Hello Michal, > >>>> > >>>> Am 09.04.2020 um 16:03 schrieb Michal Simek: > >>>>> Hi Heiko and Simon, > >>>>> > >>>>> I have find out one bug in i2c class. I am using zcu104 revC board > >>>>> which > >>>>> has dts in mainline where i2c1 has i2c mux with some channels. > >>>>> In DT clock-frequency = <400000>; is specified and it is read in > >>>>> i2c_post_probe(). But i2c_mux_bus_drv is also UCLASS_I2C which means > >>>>> that post probe is called for it too. And because clock-frequency > >>>>> property is not there default 100k is used. > >>>>> > >>>>> I think that is bug and should be fixed. > >>>>> Heiko: Are you aware about this issue? > >>>> > >>>> No :-( > >>>> > >>>> The question is, is this a bug? > >>> > >>> I have never seen clock-frequency property in i2c mux bus node. Also I > >>> have looked at linux dt binding docs and nothing like that is specified > >>> there. From quick look also pca954x driver is not reading it. > >> > >> Indeed. > >> > >>>> Should a i2c bus behind a mux not be able to set his own speed? > >>> > >>> Not sure if that make sense but Linux will likely ignore it. I am not > >>> saying it doesn't make sense but I haven't seen this feature. > >> > >> Ok. > >> > >>>> But may as a feature (or bugfix?) if "clock-frequency" is not there, > >>>> use the speed of the parent bus...? > >>> > >>> I was thinking about this too. > >>> just c&p quick implementation would look like this. Because it is > >>> i2c->i2c_mux->i2c. But maybe there is a better way how to do it. > >>> > >>> diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c > >>> index 2aa3efe8aaa0..982c467deba3 100644 > >>> --- a/drivers/i2c/i2c-uclass.c > >>> +++ b/drivers/i2c/i2c-uclass.c > >>> @@ -640,9 +640,21 @@ static int i2c_post_probe(struct udevice *dev) > >>> { > >>> #if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA) > >>> struct dm_i2c_bus *i2c = dev_get_uclass_priv(dev); > >>> + int parent_speed = I2C_SPEED_STANDARD_RATE; > >>> + > >>> + if (dev->parent && > >>> + device_get_uclass_id(dev->parent) == UCLASS_I2C_MUX && > >>> + dev->parent->parent && > >>> + device_get_uclass_id(dev->parent->parent) == UCLASS_I2C) { > >>> + struct dm_i2c_bus *i2c_parent; > >>> + > >>> + i2c_parent = dev_get_uclass_priv(dev->parent->parent); > >>> + parent_speed = i2c_parent->speed_hz; > >>> + /* Not sure if make sense to check that parent_speed is > >>> not 0 */ > >> > >> I think this check is not needed. > >> > >>> + } > >>> > >>> i2c->speed_hz = dev_read_u32_default(dev, "clock-frequency", > >>> - I2C_SPEED_STANDARD_RATE); > >>> + parent_speed); > >> > >> Wow, a big if ... may this is clearer (not compile tested)? > >> > >> udevice *parent = dev_get_parent(dev); > >> > >> if (parent && device_get_uclass_id(parent) == UCLASS_I2C_MUX) { > >> udevice *parent2 = dev_get_parent(parent); > >> if (parent2 && device_get_uclass_id(parent2) == UCLASS_I2C) { > >> struct dm_i2c_bus *i2cp = dev_get_uclass_priv(parent2); > >> > >> parent_speed = i2cp->speed_hz; > >> } > >> } > >> > >> but Simon has a deeper DM knowledge, may there is a better approach. > > > > Simon: any comment on this one? > > Simon: Can you please comment this? >
OK will take a look. Regards, Simon