Message ID | 1476222003-99697-19-git-send-email-christopher.lee.bostic@gmail.com |
---|---|
State | Changes Requested, archived |
Headers | show |
Hi Chris, > Create a bare bones FSI client device driver that registers > with the FSI core. Cool! > diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile > index 2021ce5..3e31d9a 100644 > --- a/drivers/fsi/Makefile > +++ b/drivers/fsi/Makefile > @@ -2,3 +2,4 @@ > obj-$(CONFIG_FSI) += fsi-core.o > obj-$(CONFIG_FSI_MASTER_FAKE) += fsi-master-fake.o > obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o > +obj-$(CONFIG_FSI_SCOM) += fsi-scom.o > diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c > index c5e7441..15d8635 100644 > --- a/drivers/fsi/fsi-core.c > +++ b/drivers/fsi/fsi-core.c > @@ -391,6 +391,27 @@ static int fsi_bus_match(struct device *dev, struct device_driver *drv) > return 0; > } > > +int fsidrv_register(struct fsi_driver *fsi_drv) > +{ > + int rc = 0; > + > + if (!fsi_drv) > + return -EINVAL; > + if (!fsi_drv->id_table) > + return -EINVAL; > + > + rc = driver_register(&fsi_drv->drv); > + > + return rc; > +} > +EXPORT_SYMBOL_GPL(fsidrv_register); > + > +void fsidrv_unregister(struct fsi_driver *fsi_drv) > +{ > + driver_unregister(&fsi_drv->drv); > +} > +EXPORT_SYMBOL_GPL(fsidrv_unregister); > + These aren't part of the SCOM engine device driver, they're changes to the core. Can you do them as a separate patch? > .name = "fsi", > .match = fsi_bus_match, > > diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c > new file mode 100644 > index 0000000..1600f3b > --- /dev/null > +++ b/drivers/fsi/fsi-scom.c > @@ -0,0 +1,62 @@ > +/* > + * SCOM FSI Client device driver > + * > + * Copyright (C) IBM Corporation 2016 > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/fsi.h> > +#include <linux/module.h> > + > +#define FSI_ENGID_SCOM 5 > + > +static int scom_probe(struct device *); > + > +struct fsi_device_id scom_ids[] = { > + { > + .engine_type = FSI_ENGID_SCOM, > + .version = FSI_VERSION_ANY, > + }, > + { > + .engine_type = 0, > + } You can just use { 0 } for the end sentinel: struct fsi_device_id scom_ids[] = { { .engine_type = FSI_ENGID_SCOM, .version = FSI_VERSION_ANY, }, { 0 } } - which makes it a *tiny* bit more obvious that you're only matching one fsi_device_id. > +static int scom_init(void) > +{ > + int rc; > + > + rc = fsidrv_register(&scom_drv); > + > + return rc; > +} You have this pattern a few times in your series. It's pretty minor, but a little neater if you just return the value directly in cases like that. For example: static int scom_init(void) { return fsidrv_register(&scom_drv); } Cheers, Jeremy
On Tue, Oct 11, 2016 at 7:41 PM, Jeremy Kerr <jk@ozlabs.org> wrote: > Hi Chris, > >> Create a bare bones FSI client device driver that registers >> with the FSI core. > > Cool! > >> diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile >> index 2021ce5..3e31d9a 100644 >> --- a/drivers/fsi/Makefile >> +++ b/drivers/fsi/Makefile >> @@ -2,3 +2,4 @@ >> obj-$(CONFIG_FSI) += fsi-core.o >> obj-$(CONFIG_FSI_MASTER_FAKE) += fsi-master-fake.o >> obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o >> +obj-$(CONFIG_FSI_SCOM) += fsi-scom.o >> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c >> index c5e7441..15d8635 100644 >> --- a/drivers/fsi/fsi-core.c >> +++ b/drivers/fsi/fsi-core.c >> @@ -391,6 +391,27 @@ static int fsi_bus_match(struct device *dev, struct device_driver *drv) >> return 0; >> } >> >> +int fsidrv_register(struct fsi_driver *fsi_drv) >> +{ >> + int rc = 0; >> + >> + if (!fsi_drv) >> + return -EINVAL; >> + if (!fsi_drv->id_table) >> + return -EINVAL; >> + >> + rc = driver_register(&fsi_drv->drv); >> + >> + return rc; >> +} >> +EXPORT_SYMBOL_GPL(fsidrv_register); >> + >> +void fsidrv_unregister(struct fsi_driver *fsi_drv) >> +{ >> + driver_unregister(&fsi_drv->drv); >> +} >> +EXPORT_SYMBOL_GPL(fsidrv_unregister); >> + > > These aren't part of the SCOM engine device driver, they're changes to > the core. Can you do them as a separate patch? > OK, will break these up into separate patches. > >> .name = "fsi", >> .match = fsi_bus_match, >> >> diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c >> new file mode 100644 >> index 0000000..1600f3b >> --- /dev/null >> +++ b/drivers/fsi/fsi-scom.c >> @@ -0,0 +1,62 @@ >> +/* >> + * SCOM FSI Client device driver >> + * >> + * Copyright (C) IBM Corporation 2016 >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#include <linux/fsi.h> >> +#include <linux/module.h> >> + >> +#define FSI_ENGID_SCOM 5 >> + >> +static int scom_probe(struct device *); >> + >> +struct fsi_device_id scom_ids[] = { >> + { >> + .engine_type = FSI_ENGID_SCOM, >> + .version = FSI_VERSION_ANY, >> + }, >> + { >> + .engine_type = 0, >> + } > > You can just use { 0 } for the end sentinel: > > struct fsi_device_id scom_ids[] = { > { > .engine_type = FSI_ENGID_SCOM, > .version = FSI_VERSION_ANY, > }, > { 0 } > } > > - which makes it a *tiny* bit more obvious that you're only matching one > fsi_device_id. > Will change. >> +static int scom_init(void) >> +{ >> + int rc; >> + >> + rc = fsidrv_register(&scom_drv); >> + >> + return rc; >> +} > > You have this pattern a few times in your series. It's pretty minor, but > a little neater if you just return the value directly in cases like > that. For example: > > static int scom_init(void) > { > return fsidrv_register(&scom_drv); > } Ok will revise that. Thanks, Chris > > Cheers, > > > Jeremy
diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig index 69e7ee8..35a8e12 100644 --- a/drivers/fsi/Kconfig +++ b/drivers/fsi/Kconfig @@ -24,6 +24,13 @@ config FSI_MASTER_GPIO select GPIO_DEVRES ---help--- This option enables a FSI master driver, using GPIO lines directly. + +config FSI_SCOM + tristate "SCOM FSI client" + depends on FSI + ---help--- + This option enables the SCOM FSI client device driver. + endif endmenu diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile index 2021ce5..3e31d9a 100644 --- a/drivers/fsi/Makefile +++ b/drivers/fsi/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_FSI) += fsi-core.o obj-$(CONFIG_FSI_MASTER_FAKE) += fsi-master-fake.o obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o +obj-$(CONFIG_FSI_SCOM) += fsi-scom.o diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c index c5e7441..15d8635 100644 --- a/drivers/fsi/fsi-core.c +++ b/drivers/fsi/fsi-core.c @@ -391,6 +391,27 @@ static int fsi_bus_match(struct device *dev, struct device_driver *drv) return 0; } +int fsidrv_register(struct fsi_driver *fsi_drv) +{ + int rc = 0; + + if (!fsi_drv) + return -EINVAL; + if (!fsi_drv->id_table) + return -EINVAL; + + rc = driver_register(&fsi_drv->drv); + + return rc; +} +EXPORT_SYMBOL_GPL(fsidrv_register); + +void fsidrv_unregister(struct fsi_driver *fsi_drv) +{ + driver_unregister(&fsi_drv->drv); +} +EXPORT_SYMBOL_GPL(fsidrv_unregister); + struct bus_type fsi_bus_type = { .name = "fsi", .match = fsi_bus_match, diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c new file mode 100644 index 0000000..1600f3b --- /dev/null +++ b/drivers/fsi/fsi-scom.c @@ -0,0 +1,62 @@ +/* + * SCOM FSI Client device driver + * + * Copyright (C) IBM Corporation 2016 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/fsi.h> +#include <linux/module.h> + +#define FSI_ENGID_SCOM 5 + +static int scom_probe(struct device *); + +struct fsi_device_id scom_ids[] = { + { + .engine_type = FSI_ENGID_SCOM, + .version = FSI_VERSION_ANY, + }, + { + .engine_type = 0, + } +}; + +struct fsi_driver scom_drv = { + .id_table = scom_ids, + .drv = { + .name = "scom", + .bus = &fsi_bus_type, + .probe = scom_probe, + } +}; + +static int scom_probe(struct device *dev) +{ + return 0; +} + +static int scom_init(void) +{ + int rc; + + rc = fsidrv_register(&scom_drv); + + return rc; +} + +static void scom_exit(void) +{ + fsidrv_unregister(&scom_drv); +} + +module_init(scom_init); +module_exit(scom_exit); diff --git a/include/linux/fsi.h b/include/linux/fsi.h index 47af0af..09670b7 100644 --- a/include/linux/fsi.h +++ b/include/linux/fsi.h @@ -55,6 +55,9 @@ struct fsi_driver { #define to_fsi_dev(devp) container_of(devp, struct fsi_device, dev) #define to_fsi_drv(drvp) container_of(drvp, struct fsi_driver, drv) +extern int fsidrv_register(struct fsi_driver *); +extern void fsidrv_unregister(struct fsi_driver *); + extern struct bus_type fsi_bus_type; #endif /* LINUX_FSI_H */