Hi Doug On 3/20/2018 9:47 PM, Doug Anderson wrote: > Hi, > > On Tue, Mar 20, 2018 at 3:16 PM, Sagar Dharia <sdharia@xxxxxxxxxxxxxx> wrote: >>>>>> + pinconf { >>>>>> + pins = "gpio55", "gpio56"; >>>>>> + drive-strength = <2>; >>>>>> + bias-disable; >>>>>> + }; >>>>>> + }; >>>>>> + >>>>>> + qup-i2c10-sleep { >>>>>> + pinconf { >>>>>> + pins = "gpio55", "gpio56"; >>>>>> + bias-pull-up; >>>>> >>>>> Are you sure that you want pullups enabled for sleep here? There are >>>>> external pulls on this line (as there are on many i2c busses) so doing >>>>> this will double-enable pulls. It probably won't hurt, but I'm >>>>> curious if there's some sort of reason here. >>>>> >>>> 1. We need the lines to remain high to avoid slaves sensing a false >>>> start-condition (this can happen if the SDA goes down before SCL). >>>> 2. Disclaimer: I'm not a HW expert, but we were told that >>>> tri-state/bias-disabled lines can draw more current. I will find out >>>> more about that. >>> >>> Agreed that they need to remain high, but you've got very strong >>> pullups external to the SoC. Those will keep it high. You don't need >>> the internal ones too. >>> >>> As extra evidence that the external pullups _must_ be present on your >>> board: you specify bias-disable in the active state. That can only >>> work if there are external pullups (or if there were some special >>> extra secret internal pullups that were part of geni). i2c is an >>> open-drain bus and thus there must be pullups on the bus in order to >>> communicate. >>> >> >> You are right, I followed up about the pull-up recommendation and that >> was for a GPIO where there was no external pull-up (GPIO was not used >> for I2C). It's safe to assume I2C will always have external pullup. > > It is even more safe to say that I2C will always have an external > pullup on the SDM845-MTP. Remember that the pullup config is in the > board device tree file, not the SoC one. So even if someone out there > decides that the internal pull is somehow good enough for their own > board and they don't stuff external ones, then it will be up to them > to turn the pull up on in their own board file. > > >> We >> will change sleep-config of I2C GPIOs to no-pull. > > Even better IMHO: don't specify the bias in the sleep config. I don't > believe it's possible for the sleep config to take effect without the > default config since the default config applies at probe time. ...so > you'll always get the default config applied at probe time and you > don't need to touch the bias at sleep time. Good point, we will remove the bias from the sleep config for i2c GPIOs. Thanks Sagar > > >>>>>> + i2c10: i2c@a88000 { >>>>> >>>>> Seems like it might be nice to add all the i2c busses into the main >>>>> sdm845.dtsi file. Sure, most won't be enabled, but it seems like it >>>>> would avoid churn later. >>>>> >>>>> ...if you're sure you want to add only one i2c controller, subject of >>>>> this patch should indicate that. >>>>> >>>> >>>> Yes, we typically have a "platform(sdm845 here)-qupv3.dtsi" defining >>>> most of the serial-bus instances (i2c, spi, and uart with >>>> status=disabled) that we include from the common header. The boards >>>> enable instances they need. >>>> Will that be okay? >>> >>> Unless you really feel the need to put these in a separate file I'd >>> just put them straight in sdm845.dtsi. Yeah, it'll get big, but >>> that's OK by me. I _think_ this matches what Bjorn was suggesting on >>> previous device tree patches, but CCing him just in case. I'm >>> personally OK with whatever Bjorn and other folks with more Qualcomm >>> history would like. >>> >>> ...but yeah, I'm asking for them all to be listed with status="disabled". >>> >> >> Sure, we will change the subject of this patch to indicate that we are >> adding 1 controller as of now. Later we will add all I2C controllers to >> dtsi as another patch since that will need pinctrl settings for GPIOs >> used by those instances and the wrappers devices needed by them. > > Yeah, it's fine to just change the subject of this patch. It would be > nice to add all the other controllers in sooner rather than later, but > it doesn't have to be today. > > > -Doug > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project