* [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions
@ 2021-12-09 15:05 ` Akhil R
0 siblings, 0 replies; 25+ messages in thread
From: Akhil R @ 2021-12-09 15:05 UTC (permalink / raw)
To: andy.shevchenko, christian.koenig, digetx, dri-devel, jonathanh,
ldewangan, linaro-mm-sig, linux-i2c, linux-kernel, linux-media,
linux-tegra, p.zabel, sumit.semwal, thierry.reding, robh+dt,
devicetree
Cc: akhilrajeev
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 {
+ struct i2c_smbus_alert_setup alert_data;
+ struct i2c_client *ara;
+};
+
/**
* 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;
+
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*/
+ 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")) {
+ err = tegra_i2c_setup_smbalert(i2c_dev);
+ if (err)
+ dev_warn(&pdev->dev, "smbus-alert setup failed: %d\n", err);
+ }
+
return 0;
release_rpm:
--
2.7.4
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions
2021-12-09 15:05 ` Akhil R
(?)
@ 2021-12-09 15:27 ` Dmitry Osipenko
2021-12-10 9:38 ` Akhil R
-1 siblings, 1 reply; 25+ messages in thread
From: Dmitry Osipenko @ 2021-12-09 15:27 UTC (permalink / raw)
To: Akhil R, andy.shevchenko, christian.koenig, dri-devel, jonathanh,
ldewangan, linaro-mm-sig, linux-i2c, linux-kernel, linux-media,
linux-tegra, p.zabel, sumit.semwal, thierry.reding, robh+dt,
devicetree
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.
^ permalink raw reply [flat|nested] 25+ messages in thread* RE: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions
2021-12-09 15:27 ` Dmitry Osipenko
@ 2021-12-10 9:38 ` Akhil R
2021-12-11 20:07 ` Dmitry Osipenko
2021-12-11 20:12 ` Dmitry Osipenko
0 siblings, 2 replies; 25+ messages in thread
From: Akhil R @ 2021-12-10 9:38 UTC (permalink / raw)
To: Dmitry Osipenko, andy.shevchenko, christian.koenig,
Jonathan Hunter, Laxman Dewangan, linux-i2c, linux-kernel,
linux-tegra, p.zabel, sumit.semwal, thierry.reding, robh+dt,
devicetree
> 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.
The actual notation, I suppose, is SMBALERT#. If you see it is better to make it
smbus_alert, I can update it. 'smbalert' also looks good to me though.
>
> > + struct i2c_smbus_alert_setup alert_data;
> > + struct i2c_client *ara;
>
> What "ara" stands for? Please use meaningful names, like alert_dev for
> example.
i2c-smbus.c uses the same name to refer to the smbus alert i2c_client.
I thought to use the same for consistency with the core and other drivers.
It stands for "Alert response address".
>
> I don't see where this member is used at all, please remove it.
Wouldn't it be good to have it stored so that it is handy if using any other
function later, like i2c_handle_smbus_alert. Moreover, I thought to keep
the usage consistent with other drivers having similar function.
>
> > +};
> > +
> > /**
> > * 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.
Agree
>
> > 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.
Agree.
>
> > + 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.
Agree.
>
> Although, you shouldn't need it at all, use devm_gpiod_get_optional().
smbus-alert is provided in the i2c dt bindings by default. I felt it is good
to put it into use since the use case is same.
>
> > + 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.
Agree. Also, will check on the option to use interrupt directly instead of
GPIO.
Thanks,
Akhil
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions
2021-12-10 9:38 ` Akhil R
@ 2021-12-11 20:07 ` Dmitry Osipenko
2021-12-11 20:12 ` Dmitry Osipenko
1 sibling, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2021-12-11 20:07 UTC (permalink / raw)
To: Akhil R, andy.shevchenko, christian.koenig, Jonathan Hunter,
Laxman Dewangan, linux-i2c, linux-kernel, linux-tegra, p.zabel,
sumit.semwal, thierry.reding, robh+dt, devicetree
10.12.2021 12:38, Akhil R пишет:
>>> +struct tegra_i2c_smbalert {
>> smbalert isn't a word, should be smbus_alert. Same for the GPIO name and
>> other places.
> The actual notation, I suppose, is SMBALERT#. If you see it is better to make it
> smbus_alert, I can update it. 'smbalert' also looks good to me though.
smbus_alert is better
>>> + struct i2c_smbus_alert_setup alert_data;
>>> + struct i2c_client *ara;
>> What "ara" stands for? Please use meaningful names, like alert_dev for
>> example.
> i2c-smbus.c uses the same name to refer to the smbus alert i2c_client.
> I thought to use the same for consistency with the core and other drivers.
> It stands for "Alert response address".
Please add doc comment for the struct with the explained name then.
>> I don't see where this member is used at all, please remove it.
> Wouldn't it be good to have it stored so that it is handy if using any other
> function later, like i2c_handle_smbus_alert. Moreover, I thought to keep
> the usage consistent with other drivers having similar function.
Please add it only once it will be actually needed.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions
2021-12-10 9:38 ` Akhil R
2021-12-11 20:07 ` Dmitry Osipenko
@ 2021-12-11 20:12 ` Dmitry Osipenko
1 sibling, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2021-12-11 20:12 UTC (permalink / raw)
To: Akhil R, andy.shevchenko, christian.koenig, Jonathan Hunter,
Laxman Dewangan, linux-i2c, linux-kernel, linux-tegra, p.zabel,
sumit.semwal, thierry.reding, robh+dt, devicetree
10.12.2021 12:38, Akhil R пишет:
>> Although, you shouldn't need it at all, use devm_gpiod_get_optional().
> smbus-alert is provided in the i2c dt bindings by default. I felt it is good
> to put it into use since the use case is same.
My point is that you don't need to use device_property_read_bool().
Instead, you could use devm_gpiod_get_optional() and check whether
returned GPIO is ERR or NULL (not present). Which could be a bit nicer
variant.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions
2021-12-09 15:05 ` Akhil R
(?)
(?)
@ 2021-12-09 15:30 ` Dmitry Osipenko
2021-12-09 15:33 ` Andy Shevchenko
-1 siblings, 1 reply; 25+ messages in thread
From: Dmitry Osipenko @ 2021-12-09 15:30 UTC (permalink / raw)
To: Akhil R, andy.shevchenko, christian.koenig, dri-devel, jonathanh,
ldewangan, linaro-mm-sig, linux-i2c, linux-kernel, linux-media,
linux-tegra, p.zabel, sumit.semwal, thierry.reding, robh+dt,
devicetree
09.12.2021 18:05, Akhil R пишет:
> +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;
0 is the error condition.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions
2021-12-09 15:30 ` Dmitry Osipenko
@ 2021-12-09 15:33 ` Andy Shevchenko
0 siblings, 0 replies; 25+ messages in thread
From: Andy Shevchenko @ 2021-12-09 15:33 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Akhil R, Christian Koenig, dri-devel, Jon Hunter,
Laxman Dewangan, linaro-mm-sig, linux-i2c,
Linux Kernel Mailing List, Linux Media Mailing List, linux-tegra,
Philipp Zabel, Sumit Semwal, Thierry Reding, Rob Herring,
devicetree
On Thu, Dec 9, 2021 at 5:30 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> 09.12.2021 18:05, Akhil R пишет:
> > +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;
>
> 0 is the error condition.
I'm not sure what you implied here. gpiod_to_irq() returns 0 if and
only if it goes to the architectures where it might be possible to
have valid vIRQ 0, but this is not the case (at least I never heard of
a such) for GPIO controllers on such platforms. So, looking at the
above code I may tell that the '=' part is redundant.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions
@ 2021-12-09 15:33 ` Andy Shevchenko
0 siblings, 0 replies; 25+ messages in thread
From: Andy Shevchenko @ 2021-12-09 15:33 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: devicetree, Akhil R, Linux Kernel Mailing List, dri-devel,
Jon Hunter, linaro-mm-sig, Rob Herring, Laxman Dewangan,
Thierry Reding, linux-i2c, linux-tegra, Christian Koenig,
Linux Media Mailing List
On Thu, Dec 9, 2021 at 5:30 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> 09.12.2021 18:05, Akhil R пишет:
> > +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;
>
> 0 is the error condition.
I'm not sure what you implied here. gpiod_to_irq() returns 0 if and
only if it goes to the architectures where it might be possible to
have valid vIRQ 0, but this is not the case (at least I never heard of
a such) for GPIO controllers on such platforms. So, looking at the
above code I may tell that the '=' part is redundant.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions
2021-12-09 15:33 ` Andy Shevchenko
@ 2021-12-09 15:42 ` Dmitry Osipenko
-1 siblings, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2021-12-09 15:42 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Akhil R, Christian Koenig, dri-devel, Jon Hunter,
Laxman Dewangan, linaro-mm-sig, linux-i2c,
Linux Kernel Mailing List, Linux Media Mailing List, linux-tegra,
Philipp Zabel, Sumit Semwal, Thierry Reding, Rob Herring,
devicetree
09.12.2021 18:33, Andy Shevchenko пишет:
> On Thu, Dec 9, 2021 at 5:30 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>> 09.12.2021 18:05, Akhil R пишет:
>>> +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;
>>
>> 0 is the error condition.
>
> I'm not sure what you implied here. gpiod_to_irq() returns 0 if and
> only if it goes to the architectures where it might be possible to
> have valid vIRQ 0, but this is not the case (at least I never heard of
> a such) for GPIO controllers on such platforms. So, looking at the
> above code I may tell that the '=' part is redundant.
>
Yes, removal of the '=' should be enough here.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions
@ 2021-12-09 15:42 ` Dmitry Osipenko
0 siblings, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2021-12-09 15:42 UTC (permalink / raw)
To: Andy Shevchenko
Cc: devicetree, Akhil R, Linux Kernel Mailing List, dri-devel,
Jon Hunter, linaro-mm-sig, Rob Herring, Laxman Dewangan,
Thierry Reding, linux-i2c, linux-tegra, Christian Koenig,
Linux Media Mailing List
09.12.2021 18:33, Andy Shevchenko пишет:
> On Thu, Dec 9, 2021 at 5:30 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>> 09.12.2021 18:05, Akhil R пишет:
>>> +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;
>>
>> 0 is the error condition.
>
> I'm not sure what you implied here. gpiod_to_irq() returns 0 if and
> only if it goes to the architectures where it might be possible to
> have valid vIRQ 0, but this is not the case (at least I never heard of
> a such) for GPIO controllers on such platforms. So, looking at the
> above code I may tell that the '=' part is redundant.
>
Yes, removal of the '=' should be enough here.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] i2c: tegra: Add SMBus block read and SMBus alert functions
2021-12-09 15:05 ` Akhil R
` (2 preceding siblings ...)
(?)
@ 2021-12-09 15:43 ` Dmitry Osipenko
-1 siblings, 0 replies; 25+ messages in thread
From: Dmitry Osipenko @ 2021-12-09 15:43 UTC (permalink / raw)
To: Akhil R, andy.shevchenko, christian.koenig, dri-devel, jonathanh,
ldewangan, linaro-mm-sig, linux-i2c, linux-kernel, linux-media,
linux-tegra, p.zabel, sumit.semwal, thierry.reding, robh+dt,
devicetree
09.12.2021 18:05, Akhil R пишет:
> +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;
Why GPIO is needed at all if what's actually needed is the interrupt?
^ permalink raw reply [flat|nested] 25+ messages in thread