| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <72ea6030-5f2d-4f51-3a42-8386c3638c65@gmail.com>
Date: Thu, 9 Dec 2021 18:27:42 +0300
From: Dmitry Osipenko <digetx@...il.com>
To: Akhil R <akhilrajeev@...dia.com>, andy.shevchenko@...il.com,
christian.koenig@....com, dri-devel@...ts.freedesktop.org,
jonathanh@...dia.com, ldewangan@...dia.com,
linaro-mm-sig@...ts.linaro.org, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
linux-tegra@...r.kernel.org, p.zabel@...gutronix.de,
sumit.semwal@...aro.org, thierry.reding@...il.com,
robh+dt@...nel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert
functions
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@...dia.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.
Powered by blists - more mailing lists