linux-kernel - RE: [EXT] Re: [v6] i2c: imx: support slave mode for imx I2C driver

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <DB7PR04MB44909B54E4760B88071C5B128FE50@DB7PR04MB4490.eurprd04.prod.outlook.com>
Date:   Wed, 4 Mar 2020 06:54:08 +0000
From:   Biwen Li <biwen.li@....com>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
CC:     Sascha Hauer <s.hauer@...gutronix.de>,
        Aisheng Dong <aisheng.dong@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "wsa@...-dreams.de" <wsa@...-dreams.de>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        Xiaobo Xie <xiaobo.xie@....com>, Leo Li <leoyang.li@....com>,
        Clark Wang <xiaoning.wang@....com>,
        dl-linux-imx <linux-imx@....com>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        Jiafei Pan <jiafei.pan@....com>,
        Laurentiu Tudor <laurentiu.tudor@....com>,
        "festevam@...il.com" <festevam@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: RE: [EXT] Re: [v6] i2c: imx: support slave mode for imx I2C driver

> 
> Caution: EXT Email
> 
> Hi Biwen,
> 
> On Wed, Mar 04, 2020 at 04:13:03AM +0000, Biwen Li wrote:
> > Hi Wolfram,
> >
> > Any comments?
> 
> Wolfram is waiting until you react to our comments. Until this is not happened,
> I will not ACK this patch and Wolfram will not take it.
Hi Oleksij,

Okay, got it. Thank you for your reply.
Please look into my reply for you and Sascha.

Best Regards,
Biwen Li
> 
> regards,
> Oleksij
> 
> > Best Regards,
> > Biwen Li
> >
> > >
> > > Hi,
> > >
> > > On 04.12.19 11:00, Sascha Hauer wrote:
> > > > Hi,
> > > >
> > > > The patch looks ok to me now, but I still do not like the
> > > > #ifdeffery around CONFIG_I2C_SLAVE. With the patch I just sent
> > > > (You are on Cc:) we could apply the following on your patch which
> > > > makes it more readable and increases compile coverage.
> > > >
> > > > Wolfram, Biwen, what do you think?
Hi Sascha,

Your patch is ok. But I agree with RCAR.(Keep I2C_SLAVE).
In first version I get a build error, so I add the option in second version patch.
If you want to remove the option I2C_SLAVE, firstly you should
delete it in <linux kernel source code>/include/linux/i2c.h. Then
build error will be gone and it's safe to remove option I2C_SLAVE.

Best Regards,
Biwen Li
> > >
> > >
> > > RCAR depends on slave:
> > > config I2C_RCAR
> > >
> > >          tristate "Renesas R-Car I2C Controller"
> > >
> > >          depends on ARCH_RENESAS || COMPILE_TEST
> > >
> > >          select I2C_SLAVE
> > > see:
> > > drivers/i2c/busses/i2c-rcar.c
> > >
> > > So, I would suggest to do the same in imx.
> > >
> > > So far, I tested this patch on iMX6S. It works in one board
> > > (i2c-gpio + i2c-imx) and two board (i2c-imx + i2c-imx) configuration.
> > > Tested-by: Oleksij Rempel <o.rempel@...gutronix.de>
Hi Oleksij,

Okay, thank you so much.
I agree with your suggestions.

Best Regards,
Biwen Li
> > >
> > >
> > > >
> > > > Sascha
> > > >
> > > > ---------------------------8<------------------------------
> > > >
> > > >  From 52f7c2bf59db61d4b27b59ca6404136e3ed77310 Mon Sep 17
> > > 00:00:00
> > > > 2001
> > > > From: Sascha Hauer <s.hauer@...gutronix.de>
> > > > Date: Wed, 4 Dec 2019 10:56:34 +0100
> > > > Subject: [PATCH] fixup! i2c: imx: support slave mode for imx I2C
> > > > driver
> > > >
> > > > ---
> > > >   drivers/i2c/busses/i2c-imx.c | 21 ++++++++++-----------
> > > >   1 file changed, 10 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-imx.c
> > > > b/drivers/i2c/busses/i2c-imx.c index 40ccfca600bf..c5d9ae8226cd
> > > > 100644
> > > > --- a/drivers/i2c/busses/i2c-imx.c
> > > > +++ b/drivers/i2c/busses/i2c-imx.c
> > > > @@ -203,9 +203,7 @@ struct imx_i2c_struct {
> > > >       struct pinctrl_state *pinctrl_pins_gpio;
> > > >
> > > >       struct imx_i2c_dma      *dma;
> > > > -#if IS_ENABLED(CONFIG_I2C_SLAVE)
> > > >       struct i2c_client       *slave;
> > > > -#endif
> > > >   };
> > > >
> > > >   static const struct imx_i2c_hwdata imx1_i2c_hwdata = { @@
> > > > -913,12
> > > > +911,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
> > > >
> > > >       dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
> > > >
> > > > -#if IS_ENABLED(CONFIG_I2C_SLAVE)
> > > >       if (i2c_imx->slave) {
> > > >               dev_err(&i2c_imx->adapter.dev, "Please not do
> > > > operations
> > > of master mode in slave mode");
> > > >               return -EBUSY;
> > > >       }
> > > > -#endif
> > > >
> > > >       result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent);
> > > >       if (result < 0)
> > > > @@ -1068,7 +1064,6 @@ static u32 i2c_imx_func(struct i2c_adapter
> > > *adapter)
> > > >               | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> > > >   }
> > > >
> > > > -#if IS_ENABLED(CONFIG_I2C_SLAVE)
> > > >   static int i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx)
> > > >   {
> > > >       int temp;
> > > > @@ -1159,6 +1154,10 @@ static int i2c_imx_reg_slave(struct
> > > > i2c_client
> > > *client)
> > > >   {
> > > >       struct imx_i2c_struct *i2c_imx =
> i2c_get_adapdata(client->adapter);
> > > >       int ret;
> > > > +
> > > > +     if (!IS_ENABLED(CONFIG_I2C_SLAVE))
> > > > +             return -EINVAL;
> > > > +
> > > >       if (i2c_imx->slave)
> > > >               return -EBUSY;
> > > >
> > > > @@ -1173,6 +1172,9 @@ static int i2c_imx_unreg_slave(struct
> > > > i2c_client
> > > *client)
> > > >   {
> > > >       struct imx_i2c_struct *i2c_imx =
> > > > i2c_get_adapdata(client->adapter);
> > > >
> > > > +     if (!IS_ENABLED(CONFIG_I2C_SLAVE))
> > > > +             return -EINVAL;
> > > > +
> > > >       if (!i2c_imx->slave)
> > > >               return -EINVAL;
> > > >
> > > > @@ -1188,15 +1190,12 @@ static int i2c_imx_unreg_slave(struct
> > > > i2c_client *client)
> > > >
> > > >       return 0;
> > > >   }
> > > > -#endif
> > > >
> > > >   static const struct i2c_algorithm i2c_imx_algo = {
> > > >       .master_xfer    = i2c_imx_xfer,
> > > >       .functionality  = i2c_imx_func, -#if
> > > > IS_ENABLED(CONFIG_I2C_SLAVE)
> > > >       .reg_slave      = i2c_imx_reg_slave,
> > > >       .unreg_slave    = i2c_imx_unreg_slave,
> > > > -#endif
> > > >   };
> > > >
> > > >   static irqreturn_t i2c_imx_isr(int irq, void *dev_id) @@
> > > > -1208,10
> > > > +1207,10 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
> > > >
> > > >       if (status & I2SR_IIF) {
> > > >               i2c_imx_clr_if_bit(status, i2c_imx); -#if
> > > > IS_ENABLED(CONFIG_I2C_SLAVE)
> > > > -             if (i2c_imx->slave)
> > > > +
> > > > +             if (IS_ENABLED(CONFIG_I2C_SLAVE) && i2c_imx->slave)
> > > >                       return i2c_imx_slave_isr(i2c_imx); -#endif
> > > > +
> > > >               i2c_imx->i2csr = status;
> > > >               return i2c_imx_master_isr(i2c_imx);
> > > >       }
> > > >
> > >
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&amp;data=02%7C01%7Cbiwen.li%40nxp.com%7Ce5b72
> 9bd982449714e7708d7bffc0f7f%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C637188961657171765&amp;sdata=6NGHA8U89taUe48JY7
> GT23wYNrvvTVXQ%2BOfrT3sZmDk%3D&amp;reserved=0  |
> 31137 Hildesheim, Germany                  | Phone:
> +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> +49-5121-206917-5555 |