[PATCH linux v3 18/18] drivers/fsi: Add SCOM FSI client device driver

[Christopher Bostic]

On Tue, Oct 11, 2016 at 7:41 PM, Jeremy Kerr <jk at 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