[PATCH v6 19/23] drivers/fsi: Add GPIO based FSI master

Joel Stanley joel at jms.id.au
Wed May 10 00:30:32 PDT 2017


Hi Chris,

On Tue, Apr 11, 2017 at 5:17 AM, Christopher Bostic
<cbostic at linux.vnet.ibm.com> wrote:

> From: Chris Bostic <cbostic at linux.vnet.ibm.com>
>
> Implement a FSI master using GPIO.  Will generate FSI protocol for
> read and write commands to particular addresses.  Sends master command
> and waits for and decodes a slave response.
>
> Includes changes from Edward A. James <eajames at us.ibm.com> and Jeremy
> Kerr <jk at ozlabs.org>.

I think the series is looking good. I've done a bunch of testing on
some machines, and it worked for me.

I've got a few comments and things to be clarified below.

>
> Signed-off-by: Edward A. James <eajames at us.ibm.com>
> Signed-off-by: Jeremy Kerr <jk at ozlabs.org>
> Signed-off-by: Chris Bostic <cbostic at linux.vnet.ibm.com>
> Signed-off-by: Joel Stanley <joel at jms.id.au>
> ---
>  drivers/fsi/Kconfig           |  11 +
>  drivers/fsi/Makefile          |   1 +
>  drivers/fsi/fsi-master-gpio.c | 610 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 622 insertions(+)
>  create mode 100644 drivers/fsi/fsi-master-gpio.c
>
> diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
> index 04c1a0e..448bc3b 100644
> --- a/drivers/fsi/Kconfig
> +++ b/drivers/fsi/Kconfig
> @@ -9,4 +9,15 @@ config FSI
>         ---help---
>           FSI - the FRU Support Interface - is a simple bus for low-level
>           access to POWER-based hardware.
> +
> +if FSI
> +
> +config FSI_MASTER_GPIO
> +       tristate "GPIO-based FSI master"
> +       depends on GPIOLIB
> +       ---help---
> +       This option enables a FSI master driver using GPIO lines.
> +
> +endif
> +
>  endmenu
> diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
> index db0e5e7..ed28ac0 100644
> --- a/drivers/fsi/Makefile
> +++ b/drivers/fsi/Makefile
> @@ -1,2 +1,3 @@
>
>  obj-$(CONFIG_FSI) += fsi-core.o
> +obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> new file mode 100644
> index 0000000..9fedfaf
> --- /dev/null
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -0,0 +1,610 @@
> +/*
> + * A FSI master controller, using a simple GPIO bit-banging interface
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/fsi.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "fsi-master.h"
> +
> +#define        FSI_GPIO_STD_DLY        1       /* Standard pin delay in nS */
> +#define        FSI_ECHO_DELAY_CLOCKS   16      /* Number clocks for echo delay */
> +#define        FSI_PRE_BREAK_CLOCKS    50      /* Number clocks to prep for break */
> +#define        FSI_BREAK_CLOCKS        256     /* Number of clocks to issue break */
> +#define        FSI_POST_BREAK_CLOCKS   16000   /* Number clocks to set up cfam */
> +#define        FSI_INIT_CLOCKS         5000    /* Clock out any old data */
> +#define        FSI_GPIO_STD_DELAY      10      /* Standard GPIO delay in nS */
> +                                       /* todo: adjust down as low as */
> +                                       /* possible or eliminate */
> +#define        FSI_GPIO_CMD_DPOLL      0x2
> +#define        FSI_GPIO_CMD_TERM       0x3f
> +#define FSI_GPIO_CMD_ABS_AR    0x4
> +
> +#define        FSI_GPIO_DPOLL_CLOCKS   100      /* < 21 will cause slave to hang */
> +
> +/* Bus errors */
> +#define        FSI_GPIO_ERR_BUSY       1       /* Slave stuck in busy state */
> +#define        FSI_GPIO_RESP_ERRA      2       /* Any (misc) Error */
> +#define        FSI_GPIO_RESP_ERRC      3       /* Slave reports master CRC error */
> +#define        FSI_GPIO_MTOE           4       /* Master time out error */
> +#define        FSI_GPIO_CRC_INVAL      5       /* Master reports slave CRC error */
> +
> +/* Normal slave responses */
> +#define        FSI_GPIO_RESP_BUSY      1
> +#define        FSI_GPIO_RESP_ACK       0
> +#define        FSI_GPIO_RESP_ACKD      4
> +
> +#define        FSI_GPIO_MAX_BUSY       100
> +#define        FSI_GPIO_MTOE_COUNT     1000
> +#define        FSI_GPIO_DRAIN_BITS     20
> +#define        FSI_GPIO_CRC_SIZE       4
> +#define        FSI_GPIO_MSG_ID_SIZE            2
> +#define        FSI_GPIO_MSG_RESPID_SIZE        2
> +#define        FSI_GPIO_PRIME_SLAVE_CLOCKS     100
> +
> +static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);     /* lock around fsi commands */

Should this be per-master?

> +
> +struct fsi_master_gpio {
> +       struct fsi_master       master;
> +       struct device           *dev;
> +       struct gpio_desc        *gpio_clk;
> +       struct gpio_desc        *gpio_data;
> +       struct gpio_desc        *gpio_trans;    /* Voltage translator */
> +       struct gpio_desc        *gpio_enable;   /* FSI enable */
> +       struct gpio_desc        *gpio_mux;      /* Mux control */
> +};
> +
> +#define to_fsi_master_gpio(m) container_of(m, struct fsi_master_gpio, master)
> +
> +struct fsi_gpio_msg {
> +       uint64_t        msg;
> +       uint8_t         bits;
> +};
> +
> +static void clock_toggle(struct fsi_master_gpio *master, int count)
> +{
> +       int i;
> +
> +       for (i = 0; i < count; i++) {
> +               ndelay(FSI_GPIO_STD_DLY);
> +               gpiod_set_value(master->gpio_clk, 0);
> +               ndelay(FSI_GPIO_STD_DLY);
> +               gpiod_set_value(master->gpio_clk, 1);
> +       }
> +}
> +
> +static int sda_in(struct fsi_master_gpio *master)
> +{
> +       int in;
> +
> +       ndelay(FSI_GPIO_STD_DLY);
> +       in = gpiod_get_value(master->gpio_data);
> +       return in ? 1 : 0;
> +}
> +
> +static void sda_out(struct fsi_master_gpio *master, int value)
> +{
> +       gpiod_set_value(master->gpio_data, value);
> +}
> +
> +static void set_sda_input(struct fsi_master_gpio *master)
> +{
> +       gpiod_direction_input(master->gpio_data);
> +       if (master->gpio_trans)
> +               gpiod_set_value(master->gpio_trans, 0);

gpiod_set_value checks for the first argument being NULL. You could
drop your check here.

> +}
> +
> +static void set_sda_output(struct fsi_master_gpio *master, int value)
> +{
> +       if (master->gpio_trans)
> +               gpiod_set_value(master->gpio_trans, 1);
> +       gpiod_direction_output(master->gpio_data, value);
> +}
> +
> +static void clock_zeros(struct fsi_master_gpio *master, int count)
> +{
> +       set_sda_output(master, 1);
> +       clock_toggle(master, count);
> +}
> +
> +static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *msg,
> +                       uint8_t num_bits)
> +{
> +       uint8_t bit, in_bit;
> +
> +       set_sda_input(master);
> +
> +       for (bit = 0; bit < num_bits; bit++) {
> +               clock_toggle(master, 1);
> +               in_bit = sda_in(master);
> +               msg->msg <<= 1;
> +               msg->msg |= ~in_bit & 0x1;      /* Data is negative active */

"Active low" makes more sense than negative active.

> +       }
> +       msg->bits += num_bits;
> +}
> +
> +static void serial_out(struct fsi_master_gpio *master,
> +                       const struct fsi_gpio_msg *cmd)
> +{
> +       uint8_t bit;
> +       uint64_t msg = ~cmd->msg;       /* Data is negative active */
> +       uint64_t sda_mask = 0x1ULL << (cmd->bits - 1);
> +       uint64_t last_bit = ~0;
> +       int next_bit;
> +
> +       if (!cmd->bits) {
> +               dev_warn(master->dev, "trying to output 0 bits\n");
> +               return;
> +       }
> +       set_sda_output(master, 0);
> +
> +       /* Send the start bit */
> +       sda_out(master, 0);
> +       clock_toggle(master, 1);
> +
> +       /* Send the message */
> +       for (bit = 0; bit < cmd->bits; bit++) {
> +               next_bit = (msg & sda_mask) >> (cmd->bits - 1);
> +               if (last_bit ^ next_bit) {
> +                       sda_out(master, next_bit);
> +                       last_bit = next_bit;
> +               }
> +               clock_toggle(master, 1);
> +               msg <<= 1;
> +       }
> +}
> +
> +static void msg_push_bits(struct fsi_gpio_msg *msg, uint64_t data, int bits)
> +{
> +       msg->msg <<= bits;
> +       msg->msg |= data & ((1ull << bits) - 1);
> +       msg->bits += bits;
> +}
> +
> +static void msg_push_crc(struct fsi_gpio_msg *msg)
> +{
> +       uint8_t crc;
> +       int top;
> +
> +       top = msg->bits & 0x3;
> +
> +       /* start bit, and any non-aligned top bits */
> +       crc = fsi_crc4(0,
> +                       1 << top | msg->msg >> (msg->bits - top),
> +                       top + 1);
> +
> +       /* aligned bits */
> +       crc = fsi_crc4(crc, msg->msg, msg->bits - top);
> +
> +       msg_push_bits(msg, crc, 4);
> +}
> +
> +static void build_abs_ar_command(struct fsi_gpio_msg *cmd,
> +               uint8_t id, uint32_t addr, size_t size, const void *data)

What is abs_ar? Perhaps a comment would help.

> +{
> +       bool write = !!data;
> +       uint8_t ds;
> +       int i;
> +
> +       cmd->bits = 0;
> +       cmd->msg = 0;
> +
> +       msg_push_bits(cmd, id, 2);
> +       msg_push_bits(cmd, FSI_GPIO_CMD_ABS_AR, 3);
> +       msg_push_bits(cmd, write ? 0 : 1, 1);
> +
> +       /*
> +        * The read/write size is encoded in the lower bits of the address
> +        * (as it must be naturally-aligned), and the following ds bit.
> +        *
> +        *      size    addr:1  addr:0  ds
> +        *      1       x       x       0
> +        *      2       x       0       1
> +        *      4       0       1       1
> +        *
> +        */
> +       ds = size > 1 ? 1 : 0;
> +       addr &= ~(size - 1);
> +       if (size == 4)
> +               addr |= 1;
> +
> +       msg_push_bits(cmd, addr & ((1 << 21) - 1), 21);
> +       msg_push_bits(cmd, ds, 1);
> +       for (i = 0; write && i < size; i++)
> +               msg_push_bits(cmd, ((uint8_t *)data)[i], 8);
> +
> +       msg_push_crc(cmd);
> +}
> +
> +static void build_dpoll_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
> +{
> +       cmd->bits = 0;
> +       cmd->msg = 0;
> +
> +       msg_push_bits(cmd, slave_id, 2);
> +       msg_push_bits(cmd, FSI_GPIO_CMD_DPOLL, 3);
> +       msg_push_crc(cmd);
> +}
> +
> +static void echo_delay(struct fsi_master_gpio *master)
> +{
> +       set_sda_output(master, 1);
> +       clock_toggle(master, FSI_ECHO_DELAY_CLOCKS);
> +}
> +
> +static void build_term_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
> +{
> +       cmd->bits = 0;
> +       cmd->msg = 0;
> +
> +       msg_push_bits(cmd, slave_id, 2);
> +       msg_push_bits(cmd, FSI_GPIO_CMD_TERM, 6);
> +       msg_push_crc(cmd);
> +}
> +
> +/*
> + * Store information on master errors so handler can detect and clean
> + * up the bus
> + */
> +static void fsi_master_gpio_error(struct fsi_master_gpio *master, int error)
> +{
> +
> +}
> +
> +static int read_one_response(struct fsi_master_gpio *master,
> +               uint8_t data_size, struct fsi_gpio_msg *msgp, uint8_t *tagp)
> +{
> +       struct fsi_gpio_msg msg;
> +       uint8_t id, tag;
> +       uint32_t crc;
> +       int i;
> +
> +       /* wait for the start bit */
> +       for (i = 0; i < FSI_GPIO_MTOE_COUNT; i++) {
> +               msg.bits = 0;
> +               msg.msg = 0;
> +               serial_in(master, &msg, 1);
> +               if (msg.msg)
> +                       break;
> +       }
> +       if (i >= FSI_GPIO_MTOE_COUNT) {

Testing for == will do.

> +               dev_dbg(master->dev,
> +                       "Master time out waiting for response\n");
> +               fsi_master_gpio_error(master, FSI_GPIO_MTOE);
> +               return -EIO;
> +       }
> +
> +       msg.bits = 0;
> +       msg.msg = 0;
> +
> +       /* Read slave ID & response tag */
> +       serial_in(master, &msg, 4);
> +
> +       id = (msg.msg >> FSI_GPIO_MSG_RESPID_SIZE) & 0x3;
> +       tag = msg.msg & 0x3;
> +
> +       /* if we have an ACK, and we're expecting data, clock the
> +        * data in too
> +        */

The comment should fit on one line.

> +       if (tag == FSI_GPIO_RESP_ACK && data_size)
> +               serial_in(master, &msg, data_size * 8);
> +
> +       /* read CRC */
> +       serial_in(master, &msg, FSI_GPIO_CRC_SIZE);
> +
> +       /* we have a whole message now; check CRC */
> +       crc = fsi_crc4(0, 1, 1);
> +       crc = fsi_crc4(crc, msg.msg, msg.bits);
> +       if (crc) {
> +               dev_dbg(master->dev, "ERR response CRC\n");
> +               fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
> +               return -EIO;
> +       }
> +
> +       if (msgp)
> +               *msgp = msg;
> +       if (tagp)
> +               *tagp = tag;
> +
> +       return 0;
> +}
> +
> +static int issue_term(struct fsi_master_gpio *master, uint8_t slave)
> +{
> +       struct fsi_gpio_msg cmd;
> +       uint8_t tag;
> +       int rc;
> +
> +       build_term_command(&cmd, slave);
> +       serial_out(master, &cmd);
> +       echo_delay(master);
> +
> +       rc = read_one_response(master, 0, NULL, &tag);
> +       if (rc) {

 read_one_response returns zero on success and a negative error code
on failure. Do you mean if (rc < 0)?

> +               dev_err(master->dev,
> +                               "TERM failed; lost communication with slave\n");
> +               return -EIO;
> +       } else if (tag != FSI_GPIO_RESP_ACK) {
> +               dev_err(master->dev, "TERM failed; response %d\n", tag);
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static int poll_for_response(struct fsi_master_gpio *master,
> +               uint8_t slave, uint8_t size, void *data)
> +{
> +       struct fsi_gpio_msg response, cmd;
> +       int busy_count = 0, rc, i;
> +       uint8_t tag;
> +
> +retry:
> +       rc = read_one_response(master, size, &response, &tag);
> +       if (rc)
> +               return rc;
> +
> +       switch (tag) {
> +       case FSI_GPIO_RESP_ACK:
> +               if (size && data) {
> +                       uint64_t val = response.msg;
> +                       /* clear crc & mask */
> +                       val >>= 4;
> +                       val &= (1ull << (size * 8)) - 1;

You could write this as a GENMASK. Not sure if it's more readable though.

val &= GENMASK_ULL(size * 8 - 1, 0)

> +
> +                       for (i = 0; i < size; i++) {
> +                               ((uint8_t *)data)[size-i-1] =
> +                                       val & 0xff;
> +                               val >>= 8;

Put this on one line so it's easier to read:

((uint8_t *)data)[size-i-1] = val & 0xff;

You can drop the 0xff; this is happening implicitly when assigning to
the uint_8 array.

I suggest creating a local variable of type uint8_t * so you don't
need the cast.

> +                       }
> +               }
> +               break;
> +       case FSI_GPIO_RESP_BUSY:
> +               /*
> +                * Its necessary to clock slave before issuing
> +                * d-poll, not indicated in the hardware protocol
> +                * spec. < 20 clocks causes slave to hang, 21 ok.
> +                */
> +               clock_zeros(master, FSI_GPIO_DPOLL_CLOCKS);
> +               if (busy_count++ < FSI_GPIO_MAX_BUSY) {
> +                       build_dpoll_command(&cmd, slave);
> +                       serial_out(master, &cmd);
> +                       echo_delay(master);
> +                       goto retry;
> +               }
> +               dev_warn(master->dev,
> +                       "ERR slave is stuck in busy state, issuing TERM\n");
> +               issue_term(master, slave);
> +               rc = -EIO;
> +               break;
> +
> +       case FSI_GPIO_RESP_ERRA:
> +       case FSI_GPIO_RESP_ERRC:
> +               dev_dbg(master->dev, "ERR%c received: 0x%x\n",
> +                       tag == FSI_GPIO_RESP_ERRA ? 'A' : 'C',
> +                       (int)response.msg);
> +               fsi_master_gpio_error(master, response.msg);
> +               rc = -EIO;
> +               break;
> +       }
> +
> +       /* Clock the slave enough to be ready for next operation */
> +       clock_zeros(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);

Do you need to do this after an issue_term?

How about after a fsi_master_gpio_error? I suspect yes, for now, as
this function is empty (?!).

> +       return rc;
> +}
> +
> +static int fsi_master_gpio_xfer(struct fsi_master_gpio *master, uint8_t slave,
> +               struct fsi_gpio_msg *cmd, size_t resp_len, void *resp)
> +{
> +       unsigned long flags;
> +       int rc;
> +
> +       spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
> +       serial_out(master, cmd);
> +       echo_delay(master);
> +       rc = poll_for_response(master, slave, resp_len, resp);
> +       spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
> +
> +       return rc;
> +}
> +
> +static int fsi_master_gpio_read(struct fsi_master *_master, int link,
> +               uint8_t id, uint32_t addr, void *val, size_t size)
> +{
> +       struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
> +       struct fsi_gpio_msg cmd;
> +
> +       if (link != 0)
> +               return -ENODEV;

Is this because we only support one link?

> +
> +       build_abs_ar_command(&cmd, id, addr, size, NULL);
> +       return fsi_master_gpio_xfer(master, id, &cmd, size, val);
> +}
> +
> +static int fsi_master_gpio_write(struct fsi_master *_master, int link,
> +               uint8_t id, uint32_t addr, const void *val, size_t size)
> +{
> +       struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
> +       struct fsi_gpio_msg cmd;
> +
> +       if (link != 0)
> +               return -ENODEV;
> +
> +       build_abs_ar_command(&cmd, id, addr, size, val);
> +       return fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
> +}
> +
> +static int fsi_master_gpio_term(struct fsi_master *_master,
> +               int link, uint8_t id)
> +{
> +       struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
> +       struct fsi_gpio_msg cmd;
> +
> +       if (link != 0)
> +               return -ENODEV;
> +
> +       build_term_command(&cmd, id);
> +       return fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
> +}
> +
> +/*
> + * Issue a break command on link
> + */

Redundant comment, remove it.

> +static int fsi_master_gpio_break(struct fsi_master *_master, int link)
> +{
> +       struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
> +
> +       if (link != 0)
> +               return -ENODEV;
> +
> +       set_sda_output(master, 1);
> +       sda_out(master, 1);
> +       clock_toggle(master, FSI_PRE_BREAK_CLOCKS);
> +       sda_out(master, 0);
> +       clock_toggle(master, FSI_BREAK_CLOCKS);
> +       echo_delay(master);
> +       sda_out(master, 1);
> +       clock_toggle(master, FSI_POST_BREAK_CLOCKS);
> +
> +       /* Wait for logic reset to take effect */
> +       udelay(200);
> +
> +       return 0;
> +}
> +
> +static void fsi_master_gpio_init(struct fsi_master_gpio *master)
> +{
> +       if (master->gpio_mux)
> +               gpiod_direction_output(master->gpio_mux, 1);
> +       if (master->gpio_trans)
> +               gpiod_direction_output(master->gpio_trans, 1);
> +       if (master->gpio_enable)
> +               gpiod_direction_output(master->gpio_enable, 1);
> +       gpiod_direction_output(master->gpio_clk, 1);
> +       gpiod_direction_output(master->gpio_data, 1);
> +
> +       /* todo: evaluate if clocks can be reduced */
> +       clock_zeros(master, FSI_INIT_CLOCKS);
> +}
> +
> +static int fsi_master_gpio_link_enable(struct fsi_master *_master, int link)
> +{
> +       struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
> +
> +       if (link != 0)
> +               return -ENODEV;
> +       if (master->gpio_enable)
> +               gpiod_set_value(master->gpio_enable, 1);
> +
> +       return 0;
> +}
> +
> +static void fsi_master_gpio_release(struct device *dev)
> +{
> +}

Empty function?

> +
> +static int fsi_master_gpio_probe(struct platform_device *pdev)
> +{
> +       struct fsi_master_gpio *master;
> +       struct gpio_desc *gpio;
> +
> +       master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
> +       if (!master)
> +               return -ENOMEM;
> +
> +       master->dev = &pdev->dev;
> +       master->master.dev.parent = master->dev;
> +       master->master.dev.release = fsi_master_gpio_release;
> +
> +       gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
> +       if (IS_ERR(gpio)) {
> +               dev_err(&pdev->dev, "failed to get clock gpio\n");
> +               return PTR_ERR(gpio);
> +       }
> +       master->gpio_clk = gpio;
> +
> +       gpio = devm_gpiod_get(&pdev->dev, "data", 0);
> +       if (IS_ERR(gpio)) {
> +               dev_err(&pdev->dev, "failed to get data gpio\n");
> +               return PTR_ERR(gpio);
> +       }
> +       master->gpio_data = gpio;
> +
> +       /* Optional GPIOs */
> +       gpio = devm_gpiod_get_optional(&pdev->dev, "trans", 0);
> +       if (IS_ERR(gpio)) {
> +               dev_err(&pdev->dev, "failed to get trans gpio\n");
> +               return PTR_ERR(gpio);
> +       }
> +       master->gpio_trans = gpio;
> +
> +       gpio = devm_gpiod_get_optional(&pdev->dev, "enable", 0);
> +       if (IS_ERR(gpio)) {
> +               dev_err(&pdev->dev, "failed to get enable gpio\n");
> +               return PTR_ERR(gpio);
> +       }
> +       master->gpio_enable = gpio;
> +
> +       gpio = devm_gpiod_get_optional(&pdev->dev, "mux", 0);
> +       if (IS_ERR(gpio)) {
> +               dev_err(&pdev->dev, "failed to get mux gpio\n");
> +               return PTR_ERR(gpio);
> +       }
> +       master->gpio_mux = gpio;
> +
> +       master->master.n_links = 1;
> +       master->master.read = fsi_master_gpio_read;
> +       master->master.write = fsi_master_gpio_write;
> +       master->master.term = fsi_master_gpio_term;
> +       master->master.send_break = fsi_master_gpio_break;
> +       master->master.link_enable = fsi_master_gpio_link_enable;
> +       platform_set_drvdata(pdev, master);
> +
> +       fsi_master_gpio_init(master);
> +
> +       fsi_master_register(&master->master);

This function can return an error.

> +
> +       return 0;
> +}
> +
> +
> +static int fsi_master_gpio_remove(struct platform_device *pdev)
> +{
> +       struct fsi_master_gpio *master = platform_get_drvdata(pdev);
> +
> +       devm_gpiod_put(&pdev->dev, master->gpio_clk);
> +       devm_gpiod_put(&pdev->dev, master->gpio_data);
> +       if (master->gpio_trans)
> +               devm_gpiod_put(&pdev->dev, master->gpio_trans);
> +       if (master->gpio_enable)
> +               devm_gpiod_put(&pdev->dev, master->gpio_enable);
> +       if (master->gpio_mux)
> +               devm_gpiod_put(&pdev->dev, master->gpio_mux);

Given you're removing the driver here, can you omit the devm_gpiod_put calls?

> +       fsi_master_unregister(&master->master);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id fsi_master_gpio_match[] = {
> +       { .compatible = "fsi-master-gpio" },
> +       { },
> +};
> +
> +static struct platform_driver fsi_master_gpio_driver = {
> +       .driver = {
> +               .name           = "fsi-master-gpio",
> +               .of_match_table = fsi_master_gpio_match,
> +       },
> +       .probe  = fsi_master_gpio_probe,
> +       .remove = fsi_master_gpio_remove,
> +};
> +
> +module_platform_driver(fsi_master_gpio_driver);
> +MODULE_LICENSE("GPL");
> --
> 1.8.2.2
>



More information about the linux-arm-kernel mailing list