[U-Boot] [PATCH v3 4/8] dm: Expand the uclass for Platform Controller Hubs (PCH)

[Simon Glass]

Hi Bin,

On 21 December 2015 at 02:16, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Sun, Dec 20, 2015 at 6:42 AM, Simon Glass <sjg at chromium.org> wrote:
>> A Platform Controller Hub is an Intel concept - it is like the peripherals
>> on an SoC and is often in a separate chip from the CPU. The chip is typically
>> found on the first PCI bus and integrates multiple devices.
>>
>> We have a very simple uclass to support PCHs. Add a few operations, such as
>> setting up the devices on the PCH and finding the SPI controller base
>> address. Also move it into drivers/pch/ since we will be adding a few PCH
>> drivers.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>> Changes in v3:
>> - Add a PCH method to enable/disable SPI flash protection
>> - Drop the pch_init() call
>>
>> Changes in v2:
>> - Update the commit message and header file comments
>> - Use an enum for the PCH version
>> - Replace SBASE with SPI base
>> - Add a TODO to check if the init() method can be removed later
>>
>>  arch/x86/lib/Makefile                      |  1 -
>>  drivers/Makefile                           |  1 +
>>  drivers/pch/Makefile                       |  5 ++
>>  {arch/x86/lib => drivers/pch}/pch-uclass.c | 32 +++++++++++++
>>  include/pch.h                              | 74 ++++++++++++++++++++++++++++++
>>  5 files changed, 112 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/pch/Makefile
>>  rename {arch/x86/lib => drivers/pch}/pch-uclass.c (50%)
>>  create mode 100644 include/pch.h
>>
>> diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
>> index cd5ecb6..43792bc 100644
>> --- a/arch/x86/lib/Makefile
>> +++ b/arch/x86/lib/Makefile
>> @@ -24,7 +24,6 @@ obj-$(CONFIG_I8254_TIMER) += i8254.o
>>  ifndef CONFIG_DM_PCI
>>  obj-$(CONFIG_PCI) += pci_type1.o
>>  endif
>> -obj-y  += pch-uclass.o
>>  obj-y  += pirq_routing.o
>>  obj-y  += relocate.o
>>  obj-y += physmem.o
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index c9031f2..acc6af9 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -51,6 +51,7 @@ obj-y += hwmon/
>>  obj-y += misc/
>>  obj-y += pcmcia/
>>  obj-y += dfu/
>> +obj-$(CONFIG_X86) += pch/
>>  obj-y += rtc/
>>  obj-y += sound/
>>  obj-y += timer/
>> diff --git a/drivers/pch/Makefile b/drivers/pch/Makefile
>> new file mode 100644
>> index 0000000..d69a99c
>> --- /dev/null
>> +++ b/drivers/pch/Makefile
>> @@ -0,0 +1,5 @@
>> +#
>> +# SPDX-License-Identifier:     GPL-2.0+
>> +#
>> +
>> +obj-y += pch-uclass.o
>> diff --git a/arch/x86/lib/pch-uclass.c b/drivers/pch/pch-uclass.c
>> similarity index 50%
>> rename from arch/x86/lib/pch-uclass.c
>> rename to drivers/pch/pch-uclass.c
>> index 20dfa81..4579ed1 100644
>> --- a/arch/x86/lib/pch-uclass.c
>> +++ b/drivers/pch/pch-uclass.c
>> @@ -7,10 +7,42 @@
>>
>>  #include <common.h>
>>  #include <dm.h>
>> +#include <pch.h>
>>  #include <dm/root.h>
>>
>>  DECLARE_GLOBAL_DATA_PTR;
>>
>> +int pch_get_sbase(struct udevice *dev, ulong *sbasep)
>> +{
>> +       struct pch_ops *ops = pch_get_ops(dev);
>> +
>> +       *sbasep = 0;
>> +       if (!ops->get_sbase)
>> +               return -ENOSYS;
>> +
>> +       return ops->get_sbase(dev, sbasep);
>> +}
>> +
>> +enum pch_version pch_get_version(struct udevice *dev)
>
> Should the return value be int given it can return -ENOSYS which is not enum?

An (unpacked) enum is just an int so I think this is OK. Callers
typically check against enum members. The nice thing is that it makes
the return type really clear. I'm happy to change it if you think it
is dodgy.,

[snip]

Regards,
Simon