On Tue, 30 Jul 2024 at 11:27, Maxime Ripard <mripard@xxxxxxxxxx> wrote: > > On Wed, Jul 24, 2024 at 07:59:21PM GMT, Marc Gonzalez wrote: > > On 15/07/2024 16:40, Maxime Ripard wrote: > > > On Thu, Jul 04, 2024 at 07:04:41PM GMT, Marc Gonzalez wrote: > > >> On 01/07/2024 15:50, Maxime Ripard wrote: > > >> > > >>> The i2c register access (and the whole behaviour of the device) is > > >>> constrained on the I2C_EN pin status, and you can't read it from the > > >>> device, so it's also something we need to have in the DT. > > >> > > >> I think the purpose of the I2C_EN pin might have been misunderstood. > > >> > > >> I2C_EN is not meant to be toggled, ever, by anyone from this planet. > > > > > > Toggled, probably not. Connected to a GPIO and the kernel has to assert > > > a level at boot, I've seen worse hardware design already. > > > > > >> I2C_EN is a layout-time setting, decided by a board manufacturer: > > >> > > >> - If the TDP158 is fully configured once-and-for-all at layout-time, > > >> then no I2C bus is required, and I2C_EN is pulled down forever. > > >> > > >> - If the board manufacturer wants to keep open the possibility > > >> to adjust some parameters at run-time, then they must connect > > >> the device to an I2C bus, and I2C_EN is pulled up forever. > > > > > > How do you express both cases in your current binding? > > > > It's not that I'm ignoring your question. > > > > It's that I don't understand what you're asking. > > And that's fine, you just need to say so. > > Generally speaking, you're focusing on the driver. The driver is not the > issue here. You can do whatever you want in the driver for all I care, > we can change that later on as we wish. > > The binding however cannot change, so it *has* to ideally cover all > possible situations the hardware can be used in, or at a minimum leave > the door open to support those without a compatibility breakage. > > That's why I've been asking those questions, because so far the only > thing you've claimed is that "I can't test the driver for anything > else", but, again, whether there's a driver or not, or if it's > functional, is completely missing the point. > > > SITUATION 1 > > tdp158 is pin strapped. > > Device node is child of root node. > > Properties in proposed binding are valid (regulators and power-on pin) > > Can be supported via module_platform_driver. > > > > SITUATION 2 > > tdp158 is sitting on I2C bus. > > Device node is child of i2c bus node. > > (robh said missing reg prop would be flagged by the compiler) > > Properties in proposed binding are valid (regulators and power-on pin) > > Supported via module_i2c_driver. > > > > If some settings-specific properties are added later, like skew, > > they would only be valid for the I2C programmable mode, obviously. > > I think there's a couple more combinations: > > - The device is connected on an I2C bus, but I2C_EN is tied low No, this is not possible. I2C pins are repurposed if I2C_EN is low. You can not call that an i2c bus anymore. > - The device is connected on an I2C bus, but I2C_EN is connected to a > GPIO and the kernel needs to assert its state at boot. This is a pretty strange configuration. The I2C_EN pin isn't supposed to be toggled dynamically. Anyway, if that happens, I'd use pinctrl / hog to control the pin. > > The GPIO case can be easily dealt with later on using an optional GPIO > in the binding, but the current binding infers the I2C_EN level from the > bus it's connected to, and I think we don't have a good way to deal with > cases that would break that assumption. > > So I think we need an extra property to report the state of the i2c_en > pin (and would be mutually exclusive with the GPIO if we ever have to > support it). -- With best wishes Dmitry
