[U-Boot] [PATCH v2 1/9] dm: Use dm.h header when driver mode is used

[Simon Glass <sjg@chromium.org>]

Hi Masahiro,

On 16 May 2017 at 03:56, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Simon,
>
> 2017-05-13 10:11 GMT+09:00 Simon Glass <sjg@chromium.org>:
>> Hi Masahiro,
>>
>> On 10 May 2017 at 20:12, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>> Hi Simon,
>>>
>>>
>>> 2017-05-11 6:43 GMT+09:00 Tom Rini <trini@konsulko.com>:
>>>> On Mon, May 01, 2017 at 09:18:44AM -0600, Simon Glass wrote:
>>>>
>>>>> This header includes things that are needed to make driver build. Adjust
>>>>> existing users to include that always, even if other dm/ includes are
>>>>> present
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>>
>>>> Reviewed-by: Tom Rini <trini@konsulko.com>
>>>>
>>>
>>> I'd say this is a bad idea.
>>> I believe .c files should include headers that are really necessary.
>>>
>>> Mostly, drivers need only dm/device.h, but this commit
>>> requires additional parse of dm/uclass.h and dm/platdata.h.
>>>
>>> Rather, it is better to deprecate dm.h.
>>>
>>> Its concept is DM common header that you force drivers to include
>>> where some in them may not be necessary.
>>
>> I did consider this right at the start but I think it is too painful
>> for users. There are only a few files that we pull in so the overhead
>> is not great. It avoids having to add new headers because some other
>> function is used.
>>
>> One option might be to define all the structs in one header, since
>> those are the things that are really painful to figure out. We could
>> then make the function name prefixes fully consistent with the header
>> file name (mostly they are, but see lists.h and root.h). That would
>> make it easier.
>
> Still I do not get your point.
>
> include/dm.h includes 3 headers and they are used for different purposes.
>
> Most of drivers need only <dm/device.h>.
> <dm/platdata.h> is mostly used in board files.
> <dm/uclass.h> is used for core frameworks,
> (and when getting access to a different uclass).
>
>
> Each file just includes only needed headers, and
> nothing confusing.
>
>
> I do not see any benefit in this patch.

See the next patch which moves some functions from device.h to
fdtaddr.h. The intent is to make it clear that these functions do not
support livetree. The next series sets up support for dev_read_...()
which is what each driver should use. Eventually we could make
fdtaddr.h private and only include it when needed (i.e. when not using
a live tree). I certainly don't want that inclusion logic to bleed
into drivers.

The implication of what you are asking is that I need to include
dm/fdtaddr.h in those drivers that don't include dm.h. Then in a later
patch I need to change this to dm/read.h.

In fact most people don't want to worry about which header to include
to get the common DM functions. For this sort of refactoring effort it
makes the job that much more painful if every C file does things its
own way.

I regard the actual dm/... headers as internal to driver model. I
don't want to expose particular headers in general, for the very
reason that it causes these sorts of problems.

I think once livetree is complete and the header files are stable then
we could look at the benefits of requiring files to include dm/read.h
since that will become the only file most drivers want. But it does
involve higher maintenance cost.

Right now I am really feeling the pain of the maintenance cost.

So while I understand the disadvantages, I'd like to leave this patch
as is. It is 74 files. I have a second series to get through and then
a third to enable on a real board. It is a lot of work even without
this sort of problem.

If you are really upset about it I am willing to come back when they
are converted and minimise the headers on these files.

>
>
>
>>>
>>> It is a similar idea to include/common.h,
>>> which is one of the biggest design mistakes in U-Boot.
>>
>> We have been slowing pulling things out of common.h - see for example
>> mapmem.h and vsprint.h. We also have a lot of files in include/ which
>> really should be arch-specific.
>>
>> But in any case I think common.h is useful just to include the
>> configuration and some common declarations (like global_data). The
>> problem is that it has too much in it.
>
> The concept of common.h itself is wrong.
>
> If global_data is needed, <asm-generic/global_data.h> should be included.
> If printf() is needed, it should be declared in include/stdio.h or somewhere.

It's all very well to talk about this problem, but we need to change
it. I think the best way is to remove things from common.h until it
contains nothing.

I'll send a series to pare this down a bit more
(u-boot-dm/common-working). If you have time, please help!

Regards
Simon