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

[Stefan Roese]

Hi Baruch,

On 07.06.2018 08:59, Baruch Siach wrote:
> Hi Stefan,
> 
> 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.

Thanks,
Stefan