[U-Boot] [PATCH v3 1/2] i2c: mvtwsi: disable i2c slave on Armada 38x

[Baruch Siach]

Hi Stefan,

On Thu, Jun 07, 2018 at 09:09:34AM +0200, Stefan Roese wrote:
> On 07.06.2018 08:59, Baruch Siach wrote:
> > On Thu, Jun 07, 2018 at 08:53:34AM +0200, Stefan Roese wrote:
> > > On 06.06.2018 16:05, Baruch Siach wrote:
> > > > Equivalent code that disables the hidden i2c0 slave already exists in
> > > > the Turris Omnia platform specific code. But this hidden i2c0 slave that
> > > > interferes the i2c bus is not board specific. Armada 38x SoCs and at
> > > > least some Kirkwood variants are affected as well. Add code to disable
> > > > this slave to the i2c bus driver to make it work on all affected
> > > > hardware.
> > > > 
> > > > Use the bind callback because we want this to always run at boot,
> > > > regardless of whether U-Boot uses the i2c bus.
> > > > 
> > > > Cc: Rabeeh Khoury <rabeeh at solid-run.com>
> > > > Cc: Chris Packham <judge.packham at gmail.com>
> > > > Reviewed-by: Stefan Roese <sr at denx.de>
> > > > Reviewed-by: Heiko Schocher <hs at denx.de>
> > > > Signed-off-by: Baruch Siach <baruch at tkos.co.il>
> > > > ---
> > > > v3:
> > > >     * Fix build for SUNXI (Heiko Schocher)
> > > > 
> > > > v2:
> > > >     * Use clrbits_le32 (Stefan Roese)
> > > > 
> > > >     * Apply to Kirkwood (Chris Packham)
> > > > 
> > > >     * Add review tags from Stefan and Heiko
> > > > ---
> > > >    drivers/i2c/mvtwsi.c | 25 ++++++++++++++++++++++++-
> > > >    1 file changed, 24 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
> > > > index f9822e56b894..c4f387992b5d 100644
> > > > --- a/drivers/i2c/mvtwsi.c
> > > > +++ b/drivers/i2c/mvtwsi.c
> > > > @@ -11,6 +11,7 @@
> > > >    #include <i2c.h>
> > > >    #include <linux/errno.h>
> > > >    #include <asm/io.h>
> > > > +#include <linux/bitops.h>
> > > >    #include <linux/compat.h>
> > > >    #ifdef CONFIG_DM_I2C
> > > >    #include <dm.h>
> > > > @@ -70,8 +71,10 @@ struct  mvtwsi_registers {
> > > >    		u32 baudrate;	/* When writing */
> > > >    	};
> > > >    	u32 xtnd_slave_addr;
> > > > -	u32 reserved[2];
> > > > +	u32 reserved0[2];
> > > >    	u32 soft_reset;
> > > > +	u32 reserved1[27];
> > > > +	u32 debug;
> > > >    };
> > > >    #endif
> > > > @@ -795,6 +798,25 @@ static int mvtwsi_i2c_ofdata_to_platdata(struct udevice *bus)
> > > >    	return 0;
> > > >    }
> > > > +static void twsi_disable_i2c_slave(struct mvtwsi_registers *twsi)
> > > > +{
> > > > +#if (defined(CONFIG_ARMADA_38X) || defined(CONFIG_KIRKWOOD))
> > > > +	clrbits_le32(&twsi->debug, BIT(18));
> > > > +#endif
> > > > +}
> > > 
> > > I would prefer this version:
> > > 
> > > #if (defined(CONFIG_ARMADA_38X) || defined(CONFIG_KIRKWOOD))
> > > static void twsi_disable_i2c_slave(struct mvtwsi_registers *twsi)
> > > {
> > > 	clrbits_le32(&twsi->debug, BIT(18));
> > > }
> > > #else
> > > static inline void twsi_disable_i2c_slave(struct mvtwsi_registers *twsi)
> > > {
> > > }
> > > #endif
> > 
> > Should I drop the IS_ENABLED condition from the bind routine, then?
> 
> Thinking a bit more about this, perhaps its better to also add this
> debug register to the SUNXI register struct (with a comment, that
> its not used - only to enable compilation without errors) and drop
> the #ifdef completely.

I don't like this idea. It creates a non-existing register definition, and 
makes the code harder to comprehend.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -