| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACPK8Xf1Y31+HBHb0ErpnZ4xVM+bzfHV-khj-kz8Unk1cTx6gw@mail.gmail.com>
Date: Wed, 10 May 2017 17:00:32 +0930
From: Joel Stanley <joel@....id.au>
To: Christopher Bostic <cbostic@...ux.vnet.ibm.com>
Cc: Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Russell King <linux@...linux.org.uk>, rostedt@...dmis.org,
mingo@...hat.com, Greg KH <gregkh@...uxfoundation.org>,
devicetree <devicetree@...r.kernel.org>,
linux-arm-kernel@...ts.infradead.org,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Andrew Jeffery <andrew@...id.au>,
Alistair Popple <alistair@...ple.id.au>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
"Edward A . James" <eajames@...ibm.com>,
Jeremy Kerr <jk@...abs.org>
Subject: Re: [PATCH v6 19/23] drivers/fsi: Add GPIO based FSI master
Hi Chris,
On Tue, Apr 11, 2017 at 5:17 AM, Christopher Bostic
<cbostic@...ux.vnet.ibm.com> wrote:
> From: Chris Bostic <cbostic@...ux.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@...ibm.com> and Jeremy
> Kerr <jk@...abs.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@...ibm.com>
> Signed-off-by: Jeremy Kerr <jk@...abs.org>
> Signed-off-by: Chris Bostic <cbostic@...ux.vnet.ibm.com>
> Signed-off-by: Joel Stanley <joel@....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
>
Powered by blists - more mailing lists