Re: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions

[Dmitry Osipenko <digetx@gmail.com>]

09.12.2021 18:05, Akhil R пишет:
> Emulate the SMBus block read using ContinueXfer and SMBus using GPIO
> interrupt.
> 
> For SMBus block read, the driver  reads the first byte with ContinueXfer
> set which will help to parse the data count and read the remaining bytes
> without stop condition in between.
> SMBus alert is implemented using external gpio interrupt.
> 
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 54 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index a5be8f0..3b70013 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -14,6 +14,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
> +#include <linux/i2c-smbus.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> @@ -226,6 +227,11 @@ struct tegra_i2c_hw_feature {
>  	bool has_interface_timing_reg;
>  };
>  
> +struct tegra_i2c_smbalert {

smbalert isn't a word, should be smbus_alert. Same for the GPIO name and
other places.

> +	struct i2c_smbus_alert_setup alert_data;
> +	struct i2c_client *ara;

What "ara" stands for? Please use meaningful names, like alert_dev for
example.

I don't see where this member is used at all, please remove it.

> +};
> +
>  /**
>   * struct tegra_i2c_dev - per device I2C context
>   * @dev: device reference for power management
> @@ -280,6 +286,8 @@ struct tegra_i2c_dev {
>  	int msg_err;
>  	u8 *msg_buf;
>  
> +	struct tegra_i2c_smbalert smbalert;

All properties must have doc comment.

>  	struct completion dma_complete;
>  	struct dma_chan *tx_dma_chan;
>  	struct dma_chan *rx_dma_chan;
> @@ -1232,6 +1240,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  		return err;
>  
>  	i2c_dev->msg_buf = msg->buf;
> +
> +	/* The condition true implies smbus block read and len is already read*/

Proper SMBus capitalization in comments. Mussing whitespace in the end
of the comment.

> +	if (msg->flags & I2C_M_RECV_LEN && end_state != MSG_END_CONTINUE)
> +		i2c_dev->msg_buf = msg->buf + 1;
> +
>  	i2c_dev->msg_buf_remaining = msg->len;
>  	i2c_dev->msg_err = I2C_ERR_NONE;
>  	i2c_dev->msg_read = !!(msg->flags & I2C_M_RD);
> @@ -1388,6 +1401,15 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>  			else
>  				end_type = MSG_END_REPEAT_START;
>  		}
> +		/* If M_RECV_LEN use ContinueXfer to read the first byte */
> +		if (msgs[i].flags & I2C_M_RECV_LEN) {
> +			ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i], MSG_END_CONTINUE);
> +			if (ret)
> +				break;
> +			/* Set the read byte as msg len */
> +			msgs[i].len = msgs[i].buf[0];
> +			dev_dbg(i2c_dev->dev, "reading %d bytes\n", msgs[i].len);
> +		}
>  		ret = tegra_i2c_xfer_msg(i2c_dev, &msgs[i], end_type);
>  		if (ret)
>  			break;
> @@ -1415,7 +1437,8 @@ static u32 tegra_i2c_func(struct i2c_adapter *adap)
>  {
>  	struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
>  	u32 ret = I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
> -		  I2C_FUNC_10BIT_ADDR |	I2C_FUNC_PROTOCOL_MANGLING;
> +		  I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_10BIT_ADDR |
> +		  I2C_FUNC_PROTOCOL_MANGLING;
>  
>  	if (i2c_dev->hw->has_continue_xfer_support)
>  		ret |= I2C_FUNC_NOSTART;
> @@ -1727,6 +1750,29 @@ static int tegra_i2c_init_hardware(struct tegra_i2c_dev *i2c_dev)
>  	return ret;
>  }
>  
> +static int tegra_i2c_setup_smbalert(struct tegra_i2c_dev *i2c_dev)
> +{
> +	struct tegra_i2c_smbalert *smbalert = &i2c_dev->smbalert;
> +	struct gpio_desc *alert_gpiod;
> +	struct i2c_client *ara;
> +
> +	alert_gpiod = devm_gpiod_get(i2c_dev->dev, "smbalert", GPIOD_IN);
> +	if (IS_ERR(alert_gpiod))
> +		return PTR_ERR(alert_gpiod);
> +
> +	smbalert->alert_data.irq = gpiod_to_irq(alert_gpiod);
> +	if (smbalert->alert_data.irq <= 0)
> +		return smbalert->alert_data.irq;
> +
> +	ara = i2c_new_smbus_alert_device(&i2c_dev->adapter, &smbalert->alert_data);
> +	if (IS_ERR(ara))
> +		return PTR_ERR(ara);
> +
> +	smbalert->ara = ara;
> +
> +	return 0;
> +}
> +
>  static int tegra_i2c_probe(struct platform_device *pdev)
>  {
>  	struct tegra_i2c_dev *i2c_dev;
> @@ -1821,6 +1867,12 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  	if (err)
>  		goto release_rpm;
>  
> +	if (device_property_read_bool(i2c_dev->dev, "smbus-alert")) {

I'd move this device_property_read_bool() inside of
tegra_i2c_setup_smbus_alert(), for consistency with the rest of the code
in this driver.

Although, you shouldn't need it at all, use devm_gpiod_get_optional().

> +		err = tegra_i2c_setup_smbalert(i2c_dev);
> +		if (err)
> +			dev_warn(&pdev->dev, "smbus-alert setup failed: %d\n", err);

GPIO may probe-defer, must be dev_err_probe() here.