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

[Heiko Schocher]

Hello Baruch,

Am 07.06.2018 um 09:18 schrieb 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.

I like Stefans idea ... we would prevent the "#if defined..." mess.

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de