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

[Simon Glass]

Hi Mugunthan,

On 3 May 2016 at 09:36, Mugunthan V N <mugunthanvnm at 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 at 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 at ti.com>
> >> Reviewed-by: Joe Hershberger <joe.hershberger at 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