Re: [PATCH 05/14] hw/i2c/Kconfig: Add an entry for the SMBus - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: cminyard@mvista.com
Cc: Fam Zheng <fam@euphon.net>, Eduardo Habkost <ehabkost@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Jason Wang <jasowang@redhat.com>,
	Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>,
	qemu-devel@nongnu.org, Artyom Tarasenko <atar4qemu@gmail.com>,
	Aleksandar Markovic <amarkovic@wavecomp.com>,
	qemu-ppc@nongnu.org, Gerd Hoffmann <kraxel@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>,
	David Gibson <david@gibson.dropbear.id.au>,
	Aurelien Jarno <aurelien@aurel32.net>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH 05/14] hw/i2c/Kconfig: Add an entry for the SMBus
Date: Wed, 1 Jan 2020 17:21:04 +0100	[thread overview]
Message-ID: <e171b2b3-22de-d4d5-1b1d-c988dbe3744c@redhat.com> (raw)
In-Reply-To: <20200101161558.GC6454@minyard.net>

On 1/1/20 5:15 PM, Corey Minyard wrote:
> On Wed, Jan 01, 2020 at 11:25:42AM +0100, Philippe Mathieu-Daudé wrote:
>> On 12/31/19 8:16 PM, Corey Minyard wrote:
>>> On Tue, Dec 31, 2019 at 07:32:07PM +0100, Philippe Mathieu-Daudé wrote:
>>>> The System Management Bus is more or less a derivative of the I2C
>>>> bus, thus the Kconfig entry depends of I2C.
>>>> Not all boards providing an I2C bus support SMBus.
>>>> Use two different Kconfig entries to be able to select I2C without
>>>> selecting SMBus.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>> Cc: Corey Minyard <cminyard@mvista.com>
>>>> ---
>>>>    default-configs/mips-softmmu-common.mak | 1 +
>>>>    hw/i2c/Kconfig                          | 8 ++++++--
>>>>    hw/i2c/Makefile.objs                    | 3 ++-
>>>>    3 files changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/default-configs/mips-softmmu-common.mak b/default-configs/mips-softmmu-common.mak
>>>> index da29c6c0b2..ac76d944b8 100644
>>>> --- a/default-configs/mips-softmmu-common.mak
>>>> +++ b/default-configs/mips-softmmu-common.mak
>>>> @@ -37,6 +37,7 @@ CONFIG_R4K=y
>>>>    CONFIG_MALTA=y
>>>>    CONFIG_PCNET_PCI=y
>>>>    CONFIG_MIPSSIM=y
>>>> +CONFIG_SMBUS=y
>>>
>>> Why is the above necessary?  Wouldn't CONFIG_ACPI_SMBUS=y below cause
>>> this to be done?
>>
>> Yes you are correct!
>>
>>>
>>>>    CONFIG_ACPI_SMBUS=y
>>>>    CONFIG_SMBUS_EEPROM=y
>>>>    CONFIG_TEST_DEVICES=y
>>>> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
>>>> index 2bbd395813..09642a6dcb 100644
>>>> --- a/hw/i2c/Kconfig
>>>> +++ b/hw/i2c/Kconfig
>>>> @@ -1,9 +1,13 @@
>>>>    config I2C
>>>>        bool
>>>> +config SMBUS
>>>> +    bool
>>>> +    select I2C
>>>> +
>>>>    config SMBUS_EEPROM
>>>>        bool
>>>> -    depends on I2C
>>>> +    select SMBUS
>>>>    config VERSATILE_I2C
>>>>        bool
>>>> @@ -11,7 +15,7 @@ config VERSATILE_I2C
>>>>    config ACPI_SMBUS
>>>>        bool
>>>> -    select I2C
>>>> +    select SMBUS
>>>>    config BITBANG_I2C
>>>>        bool
>>>> diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
>>>> index d7073a401f..cbbc8507a3 100644
>>>> --- a/hw/i2c/Makefile.objs
>>>> +++ b/hw/i2c/Makefile.objs
>>>> @@ -1,4 +1,5 @@
>>>> -common-obj-$(CONFIG_I2C) += core.o smbus_slave.o smbus_master.o
>>>> +common-obj-$(CONFIG_I2C) += core.o
>>>> +common-obj-$(CONFIG_SMBUS) += smbus_slave.o smbus_master.o
>>>>    common-obj-$(CONFIG_SMBUS_EEPROM) += smbus_eeprom.o
>>>>    common-obj-$(CONFIG_VERSATILE_I2C) += versatile_i2c.o
>>>>    common-obj-$(CONFIG_ACPI_X86_ICH) += smbus_ich9.o
>>>
>>> There is some messiness with ICH, but it appears that everything will
>>> work correctly there and this patch is not the right place to fix it.
>>
>> I did some cleaning with the ICH9 chipset last year, I need to find some
>> time to refresh it.
>>
>>> This looks fine and avoids including smbus code when it is not
>>> necessary.  With the MIPS config item removed (assuming it is not
>>> necessary):
>>>
>>> Reviewed-by: Corey Minyard <cminyard@mvista.com>
>>
>> Thanks!
>>
>>>
>>> I can take it into my tree if you like.
>>
>> Sure, do you mind cleaning default-configs/mips-softmmu-common.mak or you
>> prefer a respin of this single patch?
> 
> I removed that piece and have it queued.

Thanks!

Phil.