On Tue, Oct 9, 2018 at 8:41 AM Sam Ravnborg <sam@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
