| Message ID | af12825d-76b1-e1b8-216e-03761b265a6e@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: gpio: support write-only sda | expand |
On 16.01.2023 08:18, Peter Rosin wrote: > Hi! > > 2023-01-15 at 11:18, Heiner Kallweit wrote: >> There are slave devices that understand I2C but have read-only >> SDA and SCL. Examples are FD650 7-segment LED controller and >> its derivatives. Typical board designs don't even have a >> pull-up for both pins. Therefore don't enforce open-drain >> if SDA and SCL both are unidirectional. This patch makes >> i2c-gpio usable with such devices, based on new DT property >> i2c-gpio,sda-output-only. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> v3: >> - improve description of attribute sda_is_output_only >> --- >> drivers/i2c/busses/i2c-gpio.c | 14 +++++++++++--- >> include/linux/platform_data/i2c-gpio.h | 3 +++ >> 2 files changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c >> index 0e4385a9b..ea108d7e4 100644 >> --- a/drivers/i2c/busses/i2c-gpio.c >> +++ b/drivers/i2c/busses/i2c-gpio.c >> @@ -316,6 +316,8 @@ static void of_i2c_gpio_get_props(struct device_node *np, >> of_property_read_bool(np, "i2c-gpio,scl-open-drain"); >> pdata->scl_is_output_only = >> of_property_read_bool(np, "i2c-gpio,scl-output-only"); >> + pdata->sda_is_output_only = >> + of_property_read_bool(np, "i2c-gpio,sda-output-only"); >> } >> >> static struct gpio_desc *i2c_gpio_get_desc(struct device *dev, >> @@ -363,6 +365,7 @@ static int i2c_gpio_probe(struct platform_device *pdev) >> struct device *dev = &pdev->dev; >> struct device_node *np = dev->of_node; >> enum gpiod_flags gflags; >> + bool sda_scl_output_only; >> int ret; >> >> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> @@ -391,8 +394,12 @@ static int i2c_gpio_probe(struct platform_device *pdev) >> * marking these lines to be handled as open drain, and we should just >> * handle them as we handle any other output. Else we enforce open >> * drain as this is required for an I2C bus. >> + * If SCL/SDA both are write-only, then this indicates I2C-like slaves >> + * with read-only SCL/SDA. Such slaves don't need open-drain, and partially >> + * don't even work with open-drain. >> */ >> - if (pdata->sda_is_open_drain) >> + sda_scl_output_only = pdata->sda_is_output_only && pdata->scl_is_output_only; >> + if (pdata->sda_is_open_drain || sda_scl_output_only) > > I have not looked closely, but I see no strong reason to tie the SCL > output-only property to the flags of SDA? > Maybe I best start with explaining my use case. It's about a slave device that has read-only SDA/SCL and is connected to SDA/SCL GPIO's w/o pull-up resistor. If the GPIO's are configured as open-drain the slave device doesn't work. Maybe the slave device SDA/SCL pins have an internal pull-low, I don't know. So I have to ensure that GPIOD_OUT_HIGH is used for both GPIO's. I have some doubt that it's safe to use the following. The master may pull SCL high whilst a clock stretching capable slave pulls SCL low. if (pdata->scl_is_open_drain || pdata->scl_is_output_only) gflags = GPIOD_OUT_HIGH; else gflags = GPIOD_OUT_HIGH_OPEN_DRAIN; I could use the i2c-gpio,scl-open-drain property that does what I need, but just by chance and it would be misleading to (mis-)use it. Maybe add a property i2c-gpio,scl-no-pullup that de-facto is an alias for i2c-gpio,scl-open-drain? My patch was based on the assumption that if both, SDA and SCL, are write-only, then only slaves with read-only SDA/SCL are attached. I can't imagine a meaningful setup where this assumption isn't true. >> gflags = GPIOD_OUT_HIGH; >> else >> gflags = GPIOD_OUT_HIGH_OPEN_DRAIN; >> @@ -400,7 +407,7 @@ static int i2c_gpio_probe(struct platform_device *pdev) >> if (IS_ERR(priv->sda)) >> return PTR_ERR(priv->sda); >> >> - if (pdata->scl_is_open_drain) >> + if (pdata->scl_is_open_drain || sda_scl_output_only) > > Same here, why tie the SDA output-only property to the flags of SCL? > > Cheers, > Peter > >> gflags = GPIOD_OUT_HIGH; >> else >> gflags = GPIOD_OUT_HIGH_OPEN_DRAIN; >> @@ -418,7 +425,8 @@ static int i2c_gpio_probe(struct platform_device *pdev) >> >> if (!pdata->scl_is_output_only) >> bit_data->getscl = i2c_gpio_getscl; >> - bit_data->getsda = i2c_gpio_getsda; >> + if (!pdata->sda_is_output_only) >> + bit_data->getsda = i2c_gpio_getsda; >> >> if (pdata->udelay) >> bit_data->udelay = pdata->udelay; >> diff --git a/include/linux/platform_data/i2c-gpio.h b/include/linux/platform_data/i2c-gpio.h >> index a907774fd..e9536c078 100644 >> --- a/include/linux/platform_data/i2c-gpio.h >> +++ b/include/linux/platform_data/i2c-gpio.h >> @@ -16,6 +16,8 @@ >> * isn't actively driven high when setting the output value high. >> * gpio_get_value() must return the actual pin state even if the >> * pin is configured as an output. >> + * @sda_is_output_only: SDA output drivers can't be turned off. >> + * This is for clients that can only read SDA/SCL. >> * @scl_is_open_drain: SCL is set up as open drain. Same requirements >> * as for sda_is_open_drain apply. >> * @scl_is_output_only: SCL output drivers cannot be turned off. >> @@ -24,6 +26,7 @@ struct i2c_gpio_platform_data { >> int udelay; >> int timeout; >> unsigned int sda_is_open_drain:1; >> + unsigned int sda_is_output_only:1; >> unsigned int scl_is_open_drain:1; >> unsigned int scl_is_output_only:1; >> };
Hi! 2023-01-16 at 22:59, Heiner Kallweit wrote: > On 16.01.2023 08:18, Peter Rosin wrote: >> Hi! >> >> 2023-01-15 at 11:18, Heiner Kallweit wrote: >>> There are slave devices that understand I2C but have read-only >>> SDA and SCL. Examples are FD650 7-segment LED controller and >>> its derivatives. Typical board designs don't even have a >>> pull-up for both pins. Therefore don't enforce open-drain >>> if SDA and SCL both are unidirectional. This patch makes >>> i2c-gpio usable with such devices, based on new DT property >>> i2c-gpio,sda-output-only. >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> v3: >>> - improve description of attribute sda_is_output_only >>> --- >>> drivers/i2c/busses/i2c-gpio.c | 14 +++++++++++--- >>> include/linux/platform_data/i2c-gpio.h | 3 +++ >>> 2 files changed, 14 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c >>> index 0e4385a9b..ea108d7e4 100644 >>> --- a/drivers/i2c/busses/i2c-gpio.c >>> +++ b/drivers/i2c/busses/i2c-gpio.c >>> @@ -316,6 +316,8 @@ static void of_i2c_gpio_get_props(struct device_node *np, >>> of_property_read_bool(np, "i2c-gpio,scl-open-drain"); >>> pdata->scl_is_output_only = >>> of_property_read_bool(np, "i2c-gpio,scl-output-only"); >>> + pdata->sda_is_output_only = >>> + of_property_read_bool(np, "i2c-gpio,sda-output-only"); >>> } >>> >>> static struct gpio_desc *i2c_gpio_get_desc(struct device *dev, >>> @@ -363,6 +365,7 @@ static int i2c_gpio_probe(struct platform_device *pdev) >>> struct device *dev = &pdev->dev; >>> struct device_node *np = dev->of_node; >>> enum gpiod_flags gflags; >>> + bool sda_scl_output_only; >>> int ret; >>> >>> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >>> @@ -391,8 +394,12 @@ static int i2c_gpio_probe(struct platform_device *pdev) >>> * marking these lines to be handled as open drain, and we should just >>> * handle them as we handle any other output. Else we enforce open >>> * drain as this is required for an I2C bus. >>> + * If SCL/SDA both are write-only, then this indicates I2C-like slaves >>> + * with read-only SCL/SDA. Such slaves don't need open-drain, and partially >>> + * don't even work with open-drain. >>> */ >>> - if (pdata->sda_is_open_drain) >>> + sda_scl_output_only = pdata->sda_is_output_only && pdata->scl_is_output_only; >>> + if (pdata->sda_is_open_drain || sda_scl_output_only) >> >> I have not looked closely, but I see no strong reason to tie the SCL >> output-only property to the flags of SDA? >> > > Maybe I best start with explaining my use case. It's about a slave device that has > read-only SDA/SCL and is connected to SDA/SCL GPIO's w/o pull-up resistor. > If the GPIO's are configured as open-drain the slave device doesn't work. > Maybe the slave device SDA/SCL pins have an internal pull-low, I don't know. > So I have to ensure that GPIOD_OUT_HIGH is used for both GPIO's. > > I have some doubt that it's safe to use the following. The master may pull SCL > high whilst a clock stretching capable slave pulls SCL low. > > if (pdata->scl_is_open_drain || pdata->scl_is_output_only) > gflags = GPIOD_OUT_HIGH; > else > gflags = GPIOD_OUT_HIGH_OPEN_DRAIN; > > I could use the i2c-gpio,scl-open-drain property that does what I need, > but just by chance and it would be misleading to (mis-)use it. > > Maybe add a property i2c-gpio,scl-no-pullup that de-facto is an alias for > i2c-gpio,scl-open-drain? > > My patch was based on the assumption that if both, SDA and SCL, are write-only, > then only slaves with read-only SDA/SCL are attached. > I can't imagine a meaningful setup where this assumption isn't true. Oh. I misread the patch, sorry for the confusion. But now that I see that you avoid open-drain and use push-pull simply because *both* SCL and SDA are output-only, I have to object to the mixing of these issues. It may fit your case, but imagine a case where pull-ups are used to provide a lower voltage level on the I2C bus than what is normal for the master. That could be useful, even if something is also making SDA/SCL write- only. Perhaps exactly that low bus level make reading buggy? That case would perhaps not be possible if the master then pushed SCL and SDA higher than intended by the pull-ups? So. Yes, I do agree that i2c-gpio,scl-open-drain and i2c-gpio,sda-open-drain look very wrong when they are used to get rid of open-drain and am personally for an alias. i2c-gpio,scl-no-pullup is perhaps not the best name as it does not describe the hardware, but instead the hardware that isn't there? IDK, perhaps i2c-gpio,scl-push-pull ? Is that too specific? Are there other methods to drive a GPIO pin? Hmm, whatever, I'm ok with i2c-gpio,scl-no-pullup too... Mixing i2c-gpio,scl-open-drain with whatever new alias should probably be forbidden even if they trigger the same code in the driver, since it makes no sense to have neither open-drain AND push-pull, nor open-drain AND no-pullup. Cheers, Peter >>> gflags = GPIOD_OUT_HIGH; >>> else >>> gflags = GPIOD_OUT_HIGH_OPEN_DRAIN; >>> @@ -400,7 +407,7 @@ static int i2c_gpio_probe(struct platform_device *pdev) >>> if (IS_ERR(priv->sda)) >>> return PTR_ERR(priv->sda); >>> >>> - if (pdata->scl_is_open_drain) >>> + if (pdata->scl_is_open_drain || sda_scl_output_only) >> >> Same here, why tie the SDA output-only property to the flags of SCL? >> >> Cheers, >> Peter >> >>> gflags = GPIOD_OUT_HIGH; >>> else >>> gflags = GPIOD_OUT_HIGH_OPEN_DRAIN; >>> @@ -418,7 +425,8 @@ static int i2c_gpio_probe(struct platform_device *pdev) >>> >>> if (!pdata->scl_is_output_only) >>> bit_data->getscl = i2c_gpio_getscl; >>> - bit_data->getsda = i2c_gpio_getsda; >>> + if (!pdata->sda_is_output_only) >>> + bit_data->getsda = i2c_gpio_getsda; >>> >>> if (pdata->udelay) >>> bit_data->udelay = pdata->udelay; >>> diff --git a/include/linux/platform_data/i2c-gpio.h b/include/linux/platform_data/i2c-gpio.h >>> index a907774fd..e9536c078 100644 >>> --- a/include/linux/platform_data/i2c-gpio.h >>> +++ b/include/linux/platform_data/i2c-gpio.h >>> @@ -16,6 +16,8 @@ >>> * isn't actively driven high when setting the output value high. >>> * gpio_get_value() must return the actual pin state even if the >>> * pin is configured as an output. >>> + * @sda_is_output_only: SDA output drivers can't be turned off. >>> + * This is for clients that can only read SDA/SCL. >>> * @scl_is_open_drain: SCL is set up as open drain. Same requirements >>> * as for sda_is_open_drain apply. >>> * @scl_is_output_only: SCL output drivers cannot be turned off. >>> @@ -24,6 +26,7 @@ struct i2c_gpio_platform_data { >>> int udelay; >>> int timeout; >>> unsigned int sda_is_open_drain:1; >>> + unsigned int sda_is_output_only:1; >>> unsigned int scl_is_open_drain:1; >>> unsigned int scl_is_output_only:1; >>> }; >
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c index 0e4385a9b..ea108d7e4 100644 --- a/drivers/i2c/busses/i2c-gpio.c +++ b/drivers/i2c/busses/i2c-gpio.c @@ -316,6 +316,8 @@ static void of_i2c_gpio_get_props(struct device_node *np, of_property_read_bool(np, "i2c-gpio,scl-open-drain"); pdata->scl_is_output_only = of_property_read_bool(np, "i2c-gpio,scl-output-only"); + pdata->sda_is_output_only = + of_property_read_bool(np, "i2c-gpio,sda-output-only"); } static struct gpio_desc *i2c_gpio_get_desc(struct device *dev, @@ -363,6 +365,7 @@ static int i2c_gpio_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; enum gpiod_flags gflags; + bool sda_scl_output_only; int ret; priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); @@ -391,8 +394,12 @@ static int i2c_gpio_probe(struct platform_device *pdev) * marking these lines to be handled as open drain, and we should just * handle them as we handle any other output. Else we enforce open * drain as this is required for an I2C bus. + * If SCL/SDA both are write-only, then this indicates I2C-like slaves + * with read-only SCL/SDA. Such slaves don't need open-drain, and partially + * don't even work with open-drain. */ - if (pdata->sda_is_open_drain) + sda_scl_output_only = pdata->sda_is_output_only && pdata->scl_is_output_only; + if (pdata->sda_is_open_drain || sda_scl_output_only) gflags = GPIOD_OUT_HIGH; else gflags = GPIOD_OUT_HIGH_OPEN_DRAIN; @@ -400,7 +407,7 @@ static int i2c_gpio_probe(struct platform_device *pdev) if (IS_ERR(priv->sda)) return PTR_ERR(priv->sda); - if (pdata->scl_is_open_drain) + if (pdata->scl_is_open_drain || sda_scl_output_only) gflags = GPIOD_OUT_HIGH; else gflags = GPIOD_OUT_HIGH_OPEN_DRAIN; @@ -418,7 +425,8 @@ static int i2c_gpio_probe(struct platform_device *pdev) if (!pdata->scl_is_output_only) bit_data->getscl = i2c_gpio_getscl; - bit_data->getsda = i2c_gpio_getsda; + if (!pdata->sda_is_output_only) + bit_data->getsda = i2c_gpio_getsda; if (pdata->udelay) bit_data->udelay = pdata->udelay; diff --git a/include/linux/platform_data/i2c-gpio.h b/include/linux/platform_data/i2c-gpio.h index a907774fd..e9536c078 100644 --- a/include/linux/platform_data/i2c-gpio.h +++ b/include/linux/platform_data/i2c-gpio.h @@ -16,6 +16,8 @@ * isn't actively driven high when setting the output value high. * gpio_get_value() must return the actual pin state even if the * pin is configured as an output. + * @sda_is_output_only: SDA output drivers can't be turned off. + * This is for clients that can only read SDA/SCL. * @scl_is_open_drain: SCL is set up as open drain. Same requirements * as for sda_is_open_drain apply. * @scl_is_output_only: SCL output drivers cannot be turned off. @@ -24,6 +26,7 @@ struct i2c_gpio_platform_data { int udelay; int timeout; unsigned int sda_is_open_drain:1; + unsigned int sda_is_output_only:1; unsigned int scl_is_open_drain:1; unsigned int scl_is_output_only:1; };
There are slave devices that understand I2C but have read-only SDA and SCL. Examples are FD650 7-segment LED controller and its derivatives. Typical board designs don't even have a pull-up for both pins. Therefore don't enforce open-drain if SDA and SCL both are unidirectional. This patch makes i2c-gpio usable with such devices, based on new DT property i2c-gpio,sda-output-only. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- v3: - improve description of attribute sda_is_output_only --- drivers/i2c/busses/i2c-gpio.c | 14 +++++++++++--- include/linux/platform_data/i2c-gpio.h | 3 +++ 2 files changed, 14 insertions(+), 3 deletions(-)