diff mbox series

[v3,3/3] i2c: gpio: support write-only sda

Message ID af12825d-76b1-e1b8-216e-03761b265a6e@gmail.com
State New
Headers show
Series i2c: gpio: support write-only sda | expand

Commit Message

Heiner Kallweit Jan. 15, 2023, 10:18 a.m. UTC
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(-)

Comments

Heiner Kallweit Jan. 16, 2023, 9:59 p.m. UTC | #1
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;
>>  };
Peter Rosin Jan. 16, 2023, 11:15 p.m. UTC | #2
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 mbox series

Patch

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