Add GPIO support for mcp251x driver

[Schlüßler, Timo <t.schluessler@xxxxxxxxx>]

Hello,

what do you think would be the right way to control the GPIOs of the MCP251x/25625 from userland? Is this something that should be done using a SIOCDEVPRIVATE ioctl? Where should this get documented?
I could imagine something like this, but am not sure whether this is the right way to go.

diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 12358f0..2a14d5d 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -77,6 +77,8 @@


 /* MPC251x registers */
+#define BFPCTRL                        0x0c
+#define TXRTSCTRL              0x0d
 #define CANSTAT              0x0e
 #define CANCTRL              0x0f
 #  define CANCTRL_REQOP_MASK       0xe0
@@ -971,11 +973,48 @@ static int mcp251x_open(struct net_device *net)
        return ret;
 }

+static int mcp251x_gpio_ctrl(struct net_device *net, struct ifreq *rq)
+{
+       struct mcp251x_priv *priv = netdev_priv(net);
+       struct spi_device *spi = priv->spi;
+       struct mcp251x_gpio_ctrl ctrl;
+       bool copy_back = false;
+
+       if (copy_from_user(&ctrl, rq->ifr_data, sizeof(ctrl)))
+               return -EFAULT;
+
+       mutex_lock(&priv->mcp_lock);
+       if (ctrl.txnrts) { // only do SPI transfer if necessary
+               ctrl.txnrts = mcp251x_read_reg(spi, TXRTSCTRL);
+               copy_back = true;
+       }
+       if (ctrl.rxnbf_mask) // only do SPI transfer if necessary
+               mcp251x_write_bits(spi, BFPCTRL, ctrl.rxnbf_mask, ctrl.rxnbf);
+       mutex_unlock(&priv->mcp_lock);
+
+       if (copy_back &&
+           copy_to_user(rq->ifr_data, &ctrl, sizeof(struct mcp251x_gpio_ctrl)))
+               return -EFAULT;
+
+       return 0;
+}
+
+static int mcp251x_ioctl(struct net_device *net, struct ifreq *rq, int cmd)
+{
+       switch (cmd) {
+       case SIOCGPIOCTRL:
+               return mcp251x_gpio_ctrl(net, rq);
+       default:
+               return -EOPNOTSUPP;
+       }
+}
+
 static const struct net_device_ops mcp251x_netdev_ops = {
        .ndo_open = mcp251x_open,
        .ndo_stop = mcp251x_stop,
        .ndo_start_xmit = mcp251x_hard_start_xmit,
        .ndo_change_mtu = can_change_mtu,
+       .ndo_do_ioctl = mcp251x_ioctl,
 };

 static const struct of_device_id mcp251x_of_match[] = {
diff --git a/include/uapi/linux/can/mcp251x.h b/include/uapi/linux/can/mcp251x.h
index e69de29..9528ef3 100755
--- a/include/uapi/linux/can/mcp251x.h
+++ b/include/uapi/linux/can/mcp251x.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
+/*
+ * linux/can/mcp251x.h
+ *
+ * Definitions for MCP251x GPIO control
+ */
+
+#ifndef _UAPI_CAN_MCP251X_H
+#define _UAPI_CAN_MCP251X_H
+
+#define SIOCGPIOCTRL SIOCDEVPRIVATE
+
+#define MCP251X_RX0BF_MASK 0x15
+#define MCP251X_RX0BF_LOW 0x04
+#define MCP251X_RX0BF_HIGH 0x14
+#define MCP251X_RX0BF_BUFFER_FULL 0x05
+#define MCP251X_RX0BF_DISABLED 0x0
+
+#define MCP251X_RX1BF_MASK 0x2A
+#define MCP251X_RX1BF_LOW 0x08
+#define MCP251X_RX1BF_HIGH 0x28
+#define MCP251X_RX1BF_BUFFER_FULL 0x0A
+#define MCP251X_RX1BF_DISABLED 0x0
+
+#define MCP251X_TX0RTS_SET 0x08
+#define MCP251X_TX1RTS_SET 0x10
+#define MCP251X_TX2RTS_SET 0x20
+
+struct mcp251x_gpio_ctrl {
+       __u8 txnrts;
+
+       __u8 rxnbf;
+       __u8 rxnbf_mask;
+};
+
+#endif /* !_UAPI_CAN_MCP251X_H */

Also would you prefer using the workqueue instead of accesing the SPI bus directly in the ioctl call?

The usercode would look like this to set RX0BF high:

#include <linux/can/mcp251x.h>
...
struct mcp251x_gpio_ctrl ctrl;
ctrl.txnrts = 0; // don't readout inputs
ctrl.rxnbf = MCP251X_RX0BF_HIGH;
ctrl.rxnbf_mask = MCP251X_RX0BF_MASK;
ifr.ifr_data = &ctrl;
ioctl(sock, SIOCGPIOCTRL, &ifr);

Thanks for your comments.

Regards
Timo Schlüßler