Re: [PATCH 06/16] drivers: Introduce dev_set_name()

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Tue, Oct 9, 2018 at 8:41 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Andrey.
>
> Looked through the patch, everythign looked OK.
> One nit.
>
>         Sam
>
> > diff --git a/include/driver.h b/include/driver.h
> > index 1b61f2066..7b83c0a77 100644
> > --- a/include/driver.h
> > +++ b/include/driver.h
> > @@ -23,7 +23,6 @@
> >  #include <linux/ioport.h>
> >  #include <of.h>
> >
> > -#define MAX_DRIVER_NAME              32
> >  #define FORMAT_DRIVER_NAME_ID        "%s%d"
> >
> >  #include <param.h>
> > @@ -41,7 +40,7 @@ struct device_d {
> >       /*! This member (and 'type' described below) is used to match with a
> >        * driver. This is a descriptive name and could be MPC5XXX_ether or
> >        * imx_serial. */
> Add here: The name is set using dev_set_name() or somthing like that.
> To let the reader/user know how to set the name in the correct way.
>

I'll add a mention of it in the documentation, but strictly speaking
dev_set_name() is not mandatory to use. Driver can choose to assign
constant strings to that field and it wouldn't be unreasonable.

> > -     char name[MAX_DRIVER_NAME];
> > +     char *name;
> >       /*! The id is used to uniquely identify a device in the system. The id
> >        * will show up under /dev/ as the device's name. Usually this is
> >        * something like eth0 or nor0. */
> > @@ -177,6 +176,8 @@ static inline const char *dev_name(const struct device_d *dev)
> >       return dev_id(dev);
> >  }
> >
> > +int dev_set_name(struct device_d *dev, const char *fmt, ...);
> > +
> It would be nice with a small comment that tell this is used to set
> device_d.name - and that it shuld not be assigned direct.
>

Same here, device_d.name can be assigned directly. However
dev_set_name() does assume that device_d.name is free()-able, so I'll
mention that in the comment.

Will update both in v2.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox