[U-Boot,v3,01/12] drivers: core: device: add support to check dt compatible for a device/machine

Provide an api to check whether the given device or machine is
compatible with the given compat string which helps in making
decisions in drivers based on device or machine compatible.

Idea taken from Linux.

Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
 drivers/core/device.c | 14 ++++++++++++++
 include/dm/device.h   | 23 +++++++++++++++++++++++
 2 files changed, 37 insertions(+)

Hi Mugunthan,

On 28 April 2016 at 04:06, Mugunthan V N <mugunthanvnm@ti.com> wrote:
> Provide an api to check whether the given device or machine is
> compatible with the given compat string which helps in making
> decisions in drivers based on device or machine compatible.
>
> Idea taken from Linux.
>
> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
> Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
> ---
>  drivers/core/device.c | 14 ++++++++++++++
>  include/dm/device.h   | 23 +++++++++++++++++++++++
>  2 files changed, 37 insertions(+)
>
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 1322991..8fdd193 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -715,3 +715,17 @@ int device_set_name(struct udevice *dev, const char *name)
>
>         return 0;
>  }
> +
> +bool of_device_is_compatible(struct udevice *dev, const char *compat)

This function is in device.h, so I think device_is_compatible() is a
better name. Where does compat come from? Is it another device, or
something else entirely?

> +{
> +       const void *fdt = gd->fdt_blob;
> +
> +       return !fdt_node_check_compatible(fdt, dev->of_offset, compat);
> +}
> +
> +bool of_machine_is_compatible(const char *compat)

This should go in fdtdec.h I think. It doesn't have anything to do
with devices. So fdtdec_machine_is_compatible().

> +{
> +       const void *fdt = gd->fdt_blob;
> +
> +       return !fdt_node_check_compatible(fdt, 0, compat);
> +}
> diff --git a/include/dm/device.h b/include/dm/device.h
> index 8970fc0..cd18e82 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -532,6 +532,29 @@ bool device_is_last_sibling(struct udevice *dev);
>  int device_set_name(struct udevice *dev, const char *name);
>
>  /**
> + * of_device_is_compatible() - check if the device is compatible with the compat
> + *
> + * This allows to check whether the device is comaptible with the compat.
> + *
> + * @dev:       udevice pointer for which compatible needs to be verified.
> + * @compat:    Compatible string which needs to verified in the given
> + *             device
> + * @return true if OK, false if the compatible is not found

Does this mean false if @compat is not found in the device's
compatible string list? Can you beef up the comment a bit to be
specific?

> + */
> +bool of_device_is_compatible(struct udevice *dev, const char *compat);
> +
> +/**
> + * of_machine_is_compatible() - check if the machine is compatible with
> + *                             the compat
> + *
> + * This allows to check whether the machine is comaptible with the compat.

Again can you beef up the comment? What is machine? Where does it actually look?

> + *
> + * @compat:    Compatible string which needs to verified
> + * @return true if OK, false if the compatible is not found
> + */
> +bool of_machine_is_compatible(const char *compat);
> +
> +/**
>   * device_is_on_pci_bus - Test if a device is on a PCI bus
>   *
>   * @dev:       device to test
> --
> 2.8.1.339.g3ad15fd
>

Regards,
Simon

On Monday 02 May 2016 12:24 AM, Simon Glass wrote:
> Hi Mugunthan,
> 
> On 28 April 2016 at 04:06, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>> Provide an api to check whether the given device or machine is
>> compatible with the given compat string which helps in making
>> decisions in drivers based on device or machine compatible.
>>
>> Idea taken from Linux.
>>
>> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
>> Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
>> ---
>>  drivers/core/device.c | 14 ++++++++++++++
>>  include/dm/device.h   | 23 +++++++++++++++++++++++
>>  2 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>> index 1322991..8fdd193 100644
>> --- a/drivers/core/device.c
>> +++ b/drivers/core/device.c
>> @@ -715,3 +715,17 @@ int device_set_name(struct udevice *dev, const char *name)
>>
>>         return 0;
>>  }
>> +
>> +bool of_device_is_compatible(struct udevice *dev, const char *compat)
> 
> This function is in device.h, so I think device_is_compatible() is a
> better name. Where does compat come from? Is it another device, or
> something else entirely?

I have used the funtion names as is from the kernel so that porting
kernel driver to U-boot will be easier.

The compat comes from the driver which uses the API, compat is the
device compatible string like "ti,am3517-emac"

> 
>> +{
>> +       const void *fdt = gd->fdt_blob;
>> +
>> +       return !fdt_node_check_compatible(fdt, dev->of_offset, compat);
>> +}
>> +
>> +bool of_machine_is_compatible(const char *compat)
> 
> This should go in fdtdec.h I think. It doesn't have anything to do
> with devices. So fdtdec_machine_is_compatible().

I have used the funtion names as is from the kernel so that porting
kernel driver to U-boot will be easier.

> 
>> +{
>> +       const void *fdt = gd->fdt_blob;
>> +
>> +       return !fdt_node_check_compatible(fdt, 0, compat);
>> +}
>> diff --git a/include/dm/device.h b/include/dm/device.h
>> index 8970fc0..cd18e82 100644
>> --- a/include/dm/device.h
>> +++ b/include/dm/device.h
>> @@ -532,6 +532,29 @@ bool device_is_last_sibling(struct udevice *dev);
>>  int device_set_name(struct udevice *dev, const char *name);
>>
>>  /**
>> + * of_device_is_compatible() - check if the device is compatible with the compat
>> + *
>> + * This allows to check whether the device is comaptible with the compat.
>> + *
>> + * @dev:       udevice pointer for which compatible needs to be verified.
>> + * @compat:    Compatible string which needs to verified in the given
>> + *             device
>> + * @return true if OK, false if the compatible is not found
> 
> Does this mean false if @compat is not found in the device's
> compatible string list? Can you beef up the comment a bit to be
> specific?

Yes, return true if compatible is found. Will modify like below.

@return true if the compatible is found, false if the compatible is not
found

> 
>> + */
>> +bool of_device_is_compatible(struct udevice *dev, const char *compat);
>> +
>> +/**
>> + * of_machine_is_compatible() - check if the machine is compatible with
>> + *                             the compat
>> + *
>> + * This allows to check whether the machine is comaptible with the compat.
> 
> Again can you beef up the comment? What is machine? Where does it actually look?
> 

Will do.

>> + *
>> + * @compat:    Compatible string which needs to verified
>> + * @return true if OK, false if the compatible is not found
>> + */
>> +bool of_machine_is_compatible(const char *compat);
>> +
>> +/**
>>   * device_is_on_pci_bus - Test if a device is on a PCI bus
>>   *
>>   * @dev:       device to test
>> --
>> 2.8.1.339.g3ad15fd
>>
> 

Regards
Mugunthan V N

Hi Mugunthan,

https://patchwork.ozlabs.org/patch/616098/ was applied to u-boot-net.git.

Thanks!
-Joe

Hi Mugunthan,

On 3 May 2016 at 09:36, Mugunthan V N <mugunthanvnm@ti.com> wrote:
>
> On Monday 02 May 2016 12:24 AM, Simon Glass wrote:
> > Hi Mugunthan,
> >
> > On 28 April 2016 at 04:06, Mugunthan V N <mugunthanvnm@ti.com> wrote:
> >> Provide an api to check whether the given device or machine is
> >> compatible with the given compat string which helps in making
> >> decisions in drivers based on device or machine compatible.
> >>
> >> Idea taken from Linux.
> >>
> >> Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
> >> Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
> >> ---
> >>  drivers/core/device.c | 14 ++++++++++++++
> >>  include/dm/device.h   | 23 +++++++++++++++++++++++
> >>  2 files changed, 37 insertions(+)
> >>
> >> diff --git a/drivers/core/device.c b/drivers/core/device.c
> >> index 1322991..8fdd193 100644
> >> --- a/drivers/core/device.c
> >> +++ b/drivers/core/device.c
> >> @@ -715,3 +715,17 @@ int device_set_name(struct udevice *dev, const char *name)
> >>
> >>         return 0;
> >>  }
> >> +
> >> +bool of_device_is_compatible(struct udevice *dev, const char *compat)
> >
> > This function is in device.h, so I think device_is_compatible() is a
> > better name. Where does compat come from? Is it another device, or
> > something else entirely?
>
> I have used the funtion names as is from the kernel so that porting
> kernel driver to U-boot will be easier.
>
> The compat comes from the driver which uses the API, compat is the
> device compatible string like "ti,am3517-emac"

OK, please update the function comment. Also what do you think about
adding a new of.h /of.c files to hold this function prototype? I
really want to keep the APIs clean.

>
> >
> >> +{
> >> +       const void *fdt = gd->fdt_blob;
> >> +
> >> +       return !fdt_node_check_compatible(fdt, dev->of_offset, compat);
> >> +}
> >> +
> >> +bool of_machine_is_compatible(const char *compat)
> >
> > This should go in fdtdec.h I think. It doesn't have anything to do
> > with devices. So fdtdec_machine_is_compatible().
>
> I have used the funtion names as is from the kernel so that porting
> kernel driver to U-boot will be easier.

Does one function really matter? In that case, it should still go in
fdtdec, you can keep the of_ prefix. Perhaps we should use that more
generally. My reason for not is that the current fdtdec API is not the
same as of_. It uses (const void *blob, int offset) instead of (struct
of_node *node), etc.

>
> >
> >> +{
> >> +       const void *fdt = gd->fdt_blob;
> >> +
> >> +       return !fdt_node_check_compatible(fdt, 0, compat);
> >> +}
> >> diff --git a/include/dm/device.h b/include/dm/device.h
> >> index 8970fc0..cd18e82 100644
> >> --- a/include/dm/device.h
> >> +++ b/include/dm/device.h
> >> @@ -532,6 +532,29 @@ bool device_is_last_sibling(struct udevice *dev);
> >>  int device_set_name(struct udevice *dev, const char *name);
> >>
> >>  /**
> >> + * of_device_is_compatible() - check if the device is compatible with the compat
> >> + *
> >> + * This allows to check whether the device is comaptible with the compat.
> >> + *
> >> + * @dev:       udevice pointer for which compatible needs to be verified.
> >> + * @compat:    Compatible string which needs to verified in the given
> >> + *             device
> >> + * @return true if OK, false if the compatible is not found
> >
> > Does this mean false if @compat is not found in the device's
> > compatible string list? Can you beef up the comment a bit to be
> > specific?
>
> Yes, return true if compatible is found. Will modify like below.
>
> @return true if the compatible is found, false if the compatible is not
> found
>
> >
> >> + */
> >> +bool of_device_is_compatible(struct udevice *dev, const char *compat);
> >> +
> >> +/**
> >> + * of_machine_is_compatible() - check if the machine is compatible with
> >> + *                             the compat
> >> + *
> >> + * This allows to check whether the machine is comaptible with the compat.
> >
> > Again can you beef up the comment? What is machine? Where does it actually look?
> >
>
> Will do.
>
> >> + *
> >> + * @compat:    Compatible string which needs to verified
> >> + * @return true if OK, false if the compatible is not found
> >> + */
> >> +bool of_machine_is_compatible(const char *compat);
> >> +
> >> +/**
> >>   * device_is_on_pci_bus - Test if a device is on a PCI bus
> >>   *
> >>   * @dev:       device to test
> >> --
> >> 2.8.1.339.g3ad15fd
> >>
> >
>
> Regards
> Mugunthan V N

Regards,
Simon