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

[Peter Rosin <peda@axentia.se>]

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?

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