| Message ID | 20150319100944.GA914@katana (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> > I am still not too eager working on it but if you insist, I can see > > what I can do as long as Stephen sticks with testing it on Tegra. ;) > > Please decide if you want to work on it. Remember, I am not short of > patches to deal with. Maybe sounds more harsh than it was meant. I just want to know if I need to schedule time for this issue.
On 03/19/2015 04:09 AM, Wolfram Sang wrote: > >>>> Possible. But this change just makes i2c-mux-pinctrl honor status >>>> property at all. There is no functional change except it now allows >>>> you to disable any of the sub-busses. >>> >>> Actually, this is the feature I like. However, I wonder if we shouldn't >>> have that in the core, say in of_i2c_register_devices()? >> >> Hmm, looking at of_i2c_register_devices(): >> >> for_each_available_child_of_node(adap->dev.of_node, node) >> of_i2c_register_device(adap, node); >> >> already honors status properties by using for_each_available_foo. >> Therefore, i2c-core will also skip i2c device nodes disabled by >> status property. > > Yes, but only child nodes, not the complete bus. Here is an RFC of what > I mean: > > From: Wolfram Sang <wsa@the-dreams.de> > Subject: [RFC] i2c: of: always check if busses are enabled > > Allow all busses to have a "status" property which allows busses to not > be probed when set to "disabled". Needed for DTS overlays with i2c mux > scenarios. > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > @@ -1305,8 +1305,8 @@ static void of_i2c_register_devices(struct i2c_adapter *adap) > { > struct device_node *node; > > - /* Only register child devices if the adapter has a node pointer set */ > - if (!adap->dev.of_node) > + /* Only register childs if adapter has a node pointer with enabled status */ > + if (!adap->dev.of_node || !of_device_is_available(adap->dev.of_node)) > return; That feels a bit odd to me. For a regular non-mux I2C controller, that extra case would never trigger if the controller node was disabled, since the device core would never probe the controller device itself. So, we'd end up with inconsistent paths through the I2C core for regular controllers and muxes. Perhaps better would be to have a mux-specific function to iterate over a mux's child nodes and instantiate buses for those. That function would check whether each bus node was disabled or not. That'd isolate the special case into the place where it was relevant.
> >- /* Only register child devices if the adapter has a node pointer set */ > >- if (!adap->dev.of_node) > >+ /* Only register childs if adapter has a node pointer with enabled status */ > >+ if (!adap->dev.of_node || !of_device_is_available(adap->dev.of_node)) > > return; > > That feels a bit odd to me. For a regular non-mux I2C controller, that extra > case would never trigger if the controller node was disabled, since the > device core would never probe the controller device itself. So, we'd end up > with inconsistent paths through the I2C core for regular controllers and > muxes. I first thought the no-op for the non-mux case wouldn't hurt, but I agree about the consistent code path. I mentioned in my previous mail that i2c-mux might be a better place for this... > Perhaps better would be to have a mux-specific function to iterate over a > mux's child nodes and instantiate buses for those. That function would check > whether each bus node was disabled or not. That'd isolate the special case > into the place where it was relevant. ... so I wonder what you think about putting the of_device_is_available() check into i2c_add_mux_adapter() once the reg-property and chan_id have been matched?
On 03/19/2015 10:02 AM, Wolfram Sang wrote: >>> - /* Only register child devices if the adapter has a node pointer set */ >>> - if (!adap->dev.of_node) >>> + /* Only register childs if adapter has a node pointer with enabled status */ >>> + if (!adap->dev.of_node || !of_device_is_available(adap->dev.of_node)) >>> return; >> >> That feels a bit odd to me. For a regular non-mux I2C controller, that extra >> case would never trigger if the controller node was disabled, since the >> device core would never probe the controller device itself. So, we'd end up >> with inconsistent paths through the I2C core for regular controllers and >> muxes. > > I first thought the no-op for the non-mux case wouldn't hurt, but I > agree about the consistent code path. I mentioned in my previous mail > that i2c-mux might be a better place for this... > >> Perhaps better would be to have a mux-specific function to iterate over a >> mux's child nodes and instantiate buses for those. That function would check >> whether each bus node was disabled or not. That'd isolate the special case >> into the place where it was relevant. > > ... so I wonder what you think about putting the > of_device_is_available() check into i2c_add_mux_adapter() once the > reg-property and chan_id have been matched? I think that looks like a good place, yes.
On 19.03.2015 17:02, Wolfram Sang wrote: >> Perhaps better would be to have a mux-specific function to iterate over a >> mux's child nodes and instantiate buses for those. That function would check >> whether each bus node was disabled or not. That'd isolate the special case >> into the place where it was relevant. > > ... so I wonder what you think about putting the > of_device_is_available() check into i2c_add_mux_adapter() once the > reg-property and chan_id have been matched? > Ok, I see what you mean. I had a look at the place in question and wonder what to return from i2c_add_mux_adapter() in the disabled case so that i2c-mux-pinctrl is still happy with the returned value. I guess what you want to have is that i2c_add_adapter() is not called for the disabled case, right? Is the i2c_adapter struct prepared in i2c_mux_add_adapter() still valid if i2c_add_adapter() is not called? Sorry, I am not too deep into i2c subsystem, I just reworked i2c-mux- pinctrl to make it work on Dove. If you are fine with giving me some guidance how you prefer to have it done, I can try to free some spare time. Unfortunately there is already little of it, so please don't expect a quick tested patch. Sebastian
> Ok, I see what you mean. I had a look at the place in question and > wonder what to return from i2c_add_mux_adapter() in the disabled > case so that i2c-mux-pinctrl is still happy with the returned value. Ouch, you are right. The crux of interfaces returning NULL instead of an ERR_PTR :( I'll have a look, I maybe started to fix this somewhen. > I guess what you want to have is that i2c_add_adapter() is not called > for the disabled case, right? I think that makes sense. > Is the i2c_adapter struct prepared in i2c_mux_add_adapter() still valid > if i2c_add_adapter() is not called? I will have a closer look to the issue this weekend. > Sorry, I am not too deep into i2c subsystem, I just reworked i2c-mux- > pinctrl to make it work on Dove. If you are fine with giving me some > guidance how you prefer to have it done, I can try to free some spare > time. Cool, thanks. Learning by doing is a good way to get such knowledge :) > Unfortunately there is already little of it, so please don't > expect a quick tested patch. I understand. Thanks, Wolfram
> > I guess what you want to have is that i2c_add_adapter() is not called > > for the disabled case, right? > > I think that makes sense. But maybe we should just start simple and keep calling i2c_add_adapter() for the disabled case. We will just skip probing devices on the bus. Would that help the issue you were originally trying to solve?
On 21.03.2015 22:00, Wolfram Sang wrote: >>> I guess what you want to have is that i2c_add_adapter() is not called >>> for the disabled case, right? >> >> I think that makes sense. > > But maybe we should just start simple and keep calling i2c_add_adapter() > for the disabled case. We will just skip probing devices on the bus. > Would that help the issue you were originally trying to solve? It is not about probing devices on the mux sub-busses, I'd expect no devices on the optional sub-busses in DT on boards where those pins are not used as i2c. The idea was to hide those busses completely in particular from userspace on boards where they'll never be available. If modifying i2c-mux-pinctrl to respect the sub-bus status property is such a big issue, I'd rather leave the driver as is and expose all sub-busses to userspace. Sebastian
> If modifying i2c-mux-pinctrl to respect the sub-bus status property is > such a big issue, I'd rather leave the driver as is and expose all > sub-busses to userspace. Well, dunno what 'big issue' is in your book :) What definately needs to be done is: * handle "status" at mux-core level, not mux-driver * that probably needs us to convert i2c_add_mux_adapter() to return ERR_PTRs instead of NULL, so we can distinguish the "disabled" case * that would mean to fix all existing users That's all not groundbreaking stuff, but needs caution and thoroughness. There might be some gory details left, though... Regards, Wolfram
On 23.03.2015 19:32, Wolfram Sang wrote: >> If modifying i2c-mux-pinctrl to respect the sub-bus status property is >> such a big issue, I'd rather leave the driver as is and expose all >> sub-busses to userspace. > > Well, dunno what 'big issue' is in your book :) What definately needs to > be done is: Wolfram, I had a look at the code in question again and prepared some patches to return ERR_PTRs from i2c_add_mux_adapter(), rework users to check for IS_ERR() and finally skip i2c_add_adapter for the OF disabled case. I have them compile-tested but I don't have any hardware available right now. Anyway, I still have some questions. > * handle "status" at mux-core level, not mux-driver I get that.. but: > * that probably needs us to convert i2c_add_mux_adapter() to return > ERR_PTRs instead of NULL, so we can distinguish the "disabled" case What do you want to return from i2c_add_mux_adapter() if you find an OF disabled child node? AFAIKS, there is no explicit errno for a disabled device (node) so all I can imagine here is to return either the &priv->adap before actually calling i2c_add_adapter on it or NULL now that we explicitly check for errors. > * that would mean to fix all existing users If we choose to return NULL, those users who can deal with a missing/disabled sub-bus on the mux can check with IS_ERR() the others should check with IS_ERR_OR_NULL(). We could also choose to return some other errno (-ENODEV maybe) but still we'd have to double-check that return value on i2c-mux-pinctrl and the others too if we don't care that i2c_add_adapter() wasn't called for a mux. > That's all not groundbreaking stuff, but needs caution and thoroughness. > There might be some gory details left, though... As I said before, the intention was not disable a possible i2c bus that can be dynamically added/removed on that specific Dove SBC/CM-A510 board but to have a single i2c-mux-pinctrl in the SoC dtsi just because the SoC has the optional i2c bus muxing. While thinking about it (and I still think of it as a 'big issue' compared to the intention of the initial patch) I came to the conclusion that I should maybe just go for a board-specific i2c-mux-pinctrl node instead of adding it to the SoC dtsi. That will also avoid doubled i2c busses on boards with just the default i2c option. Sebastian
> While thinking about it (and I still think of it as a 'big issue' > compared to the intention of the initial patch) I came to the > conclusion that I should maybe just go for a board-specific > i2c-mux-pinctrl node instead of adding it to the SoC dtsi. That will > also avoid doubled i2c busses on boards with just the default i2c > option. Ehrm, then please let me know what you decided on. If you chose the above road, then I don't need to think about the other questions :)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index fe80f85896e267..d9a3ad2149332e 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -1305,8 +1305,8 @@ static void of_i2c_register_devices(struct i2c_adapter *adap) { struct device_node *node; - /* Only register child devices if the adapter has a node pointer set */ - if (!adap->dev.of_node) + /* Only register childs if adapter has a node pointer with enabled status */ + if (!adap->dev.of_node || !of_device_is_available(adap->dev.of_node)) return; dev_dbg(&adap->dev, "of_i2c: walking child nodes\n");