On 30/07/2024 11:30, Maxime Ripard wrote: > On Tue, Jul 30, 2024 at 11:46:24AM GMT, Dmitry Baryshkov wrote: >> 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. > > ACK. I still believe it would be valuable, but I don't really want to be > part of that conversation anymore. Marc, do whatever you want. OK. I'll send the v4 sitting in my queue. Regards
