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@xxxxxxxxx>
>> ---
>> 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;
>> };
