Re: [RFC PATCH 1/3] of: base: Introduce of_alias_check_id() to check alias IDs

[Rob Herring]

On Fri, Apr 27, 2018 at 1:10 AM, Michal Simek <michal.simek@xxxxxxxxxx> wrote:
> On 27.4.2018 04:39, Rob Herring wrote:
>> On Thu, Apr 26, 2018 at 9:08 AM, Michal Simek <michal.simek@xxxxxxxxxx> wrote:
>>> The function travers the lookup table to check if the request alias
>>> id is compatible with the device driver match structure.
>>> This function will be used by serial drivers to check if requested alias
>>> is allocated or free to use.
>>>
>>> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
>>> ---
>>>
>>>  drivers/of/base.c  | 49 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/of.h |  2 ++
>>>  2 files changed, 51 insertions(+)
>>>
>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>> index 848f549164cd..382de01acc72 100644
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -1892,6 +1892,55 @@ int of_alias_get_id(struct device_node *np, const char *stem)
>>>  }
>>>  EXPORT_SYMBOL_GPL(of_alias_get_id);
>>>
>>> +/**
>>> + * of_alias_check_id - Check alias id for the give compatibility
>>> + * @matches:   Array of of device match structures to search in
>>> + * @stem:      Alias stem of the given device_node
>>> + * @id:                Alias ID for checking
>>> + *
>>> + * The function travers the lookup table to check if the request alias id
>>> + * is compatible with the device driver match structure
>>> + *
>>> + * Return true if ID is allocated, return false if not
>>> + */
>>> +bool of_alias_check_id(const struct of_device_id *matches, const char *stem,
>>> +                      int id)
>>
>> Wouldn't it be simpler to just return a bitmap of all allocated ids
>> that match rather than trying to build that up 1 bit at a time?
>
> Is alias list stable or can dt overlay change it?

Stable. If you use an id and then an overlay adds that id as an alias,
what should the kernel do? The only choice is ignore the suggestion.

> What should be the expected flow? Find out maximum number of aliases of
> the same kind and allocate bitmap and return it with length.

I think we can assume <32 so just return a bitmap in a unsigned long
or u32. Use that to initialize a bitmap in the driver. For un-aliased
devices, find the first unset bit, use that id and set the bit.

> Anyway if you look at that patches I sent then I call in the driver
> of_alias_get_highest_id("serial") which doesn't take care if alias match
> with actual driver. It means having information about max alias ID which
> match actual driver that would be helpful but I am not quite sure what
> should be the flow.

I'm a bit confused as to why you use of_alias_get_highest_id and
of_alias_check_id. of_alias_get_highest_id is really intended if  you
want to start dynamic allocations above all the alias ids.

I guess the need to match devices is based on each serial driver
having it's own major and /dev name. IMO, we should get rid of those
and every serial driver use ttySn (with the exception of things like
USB serial). Then you'd just need to find all serial aliases. But you
can support both with this function. Just return a bitmap of all
aliases if match is NULL.

> Any link to similar function would be good to understand how the flow is
> supposed to work.

I don't think there's a good model. The pl011 code looks broken to me.
The warning about devices with and without aliases should be fixed.
That is perfectly valid and should be supported. You should really
only need an alias for your console.

Rob