Re: [PATCH 2/3] soc: samsung: Add a driver for Samsung SPEEDY host controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Christophe,

On 12/14/24 5:52 PM, Christophe JAILLET wrote:
Le 12/12/2024 à 22:09, Markuss Broks a écrit :
Add a driver for Samsung SPEEDY serial bus host controller.
SPEEDY is a proprietary 1 wire serial bus used by Samsung
in various devices (usually mobile), like Samsung Galaxy
phones. It is usually used for connecting PMIC or various
other peripherals, like audio codecs or RF components.

This bus can address at most 1MiB (4 bit device address,
8 bit registers per device, 8 bit wide registers:
256*256*16 = 1MiB of address space.

...

+static int _speedy_read(struct speedy_controller *speedy, u32 reg, u32 addr, u32 *val)
+{
+    int ret;
+    u32 cmd, int_ctl, int_status;
+
+    mutex_lock(&speedy->io_lock);

All error handling paths fail to release the mutex.
guard(mutex) would help here.

True, I didn't know that such a thing existed, thanks for the tip! :)



+
+    ret = speedy_fifo_reset(speedy);
+    if (ret)
+        return ret;
+
+    ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
+                  SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
+    if (ret)
+        return ret;
+
+    cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_READ |
+          SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
+
+    int_ctl = SPEEDY_TRANSFER_DONE_EN | SPEEDY_FIFO_RX_ALMOST_FULL_EN |
+          SPEEDY_RX_FIFO_INT_TRAILER_EN | SPEEDY_RX_MODEBIT_ERR_EN |
+          SPEEDY_RX_GLITCH_ERR_EN | SPEEDY_RX_ENDBIT_ERR_EN |
+          SPEEDY_REMOTE_RESET_REQ_EN;
+
+    ret = speedy_int_clear(speedy);
+    if (ret)
+        return ret;
+
+    ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
+    if (ret)
+        return ret;
+
+    ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
+    if (ret)
+        return ret;
+
+    /* Wait for xfer done */
+    ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
+                       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
+    if (ret)
+        return ret;
+
+    ret = regmap_read(speedy->map, SPEEDY_RX_DATA, val);
+    if (ret)
+        return ret;
+
+    ret = speedy_int_clear(speedy);
+
+    mutex_unlock(&speedy->io_lock);
+
+    return ret;
+}

...

+static int _speedy_write(struct speedy_controller *speedy, u32 reg, u32 addr, u32 val)
+{
+    int ret;
+    u32 cmd, int_ctl, int_status;
+
+    mutex_lock(&speedy->io_lock);
+
+    ret = speedy_fifo_reset(speedy);
+    if (ret)
+        return ret;

All error handling paths fail to release the mutex.
guard(mutex) would help here.

+
+    ret = regmap_set_bits(speedy->map, SPEEDY_FIFO_CTRL,
+                  SPEEDY_RX_LENGTH(1) | SPEEDY_TX_LENGTH(1));
+    if (ret)
+        return ret;
+
+    cmd = SPEEDY_ACCESS_RANDOM | SPEEDY_DIRECTION_WRITE |
+          SPEEDY_DEVICE(reg) | SPEEDY_ADDRESS(addr);
+
+    int_ctl = (SPEEDY_TRANSFER_DONE_EN |
+           SPEEDY_FIFO_TX_ALMOST_EMPTY_EN |
+           SPEEDY_TX_LINE_BUSY_ERR_EN |
+           SPEEDY_TX_STOPBIT_ERR_EN |
+           SPEEDY_REMOTE_RESET_REQ_EN);
+
+    ret = speedy_int_clear(speedy);
+    if (ret)
+        return ret;
+
+    ret = regmap_write(speedy->map, SPEEDY_INT_ENABLE, int_ctl);
+    if (ret)
+        return ret;
+
+    ret = regmap_write(speedy->map, SPEEDY_CMD, cmd);
+    if (ret)
+        return ret;
+
+    ret = regmap_write(speedy->map, SPEEDY_TX_DATA, val);
+    if (ret)
+        return ret;
+
+    /* Wait for xfer done */
+    ret = regmap_read_poll_timeout(speedy->map, SPEEDY_INT_STATUS, int_status,
+                       int_status & SPEEDY_TRANSFER_DONE, 5000, 50000);
+    if (ret)
+        return ret;
+
+    speedy_int_clear(speedy);
+
+    mutex_unlock(&speedy->io_lock);
+
+    return 0;
+}

...

+/**
+ * speedy_get_by_phandle() - internal get speedy device handle
+ * @np:    pointer to OF device node of device
+ *
+ * Return: 0 on success, -errno otherwise

On success, a handle is returned, not 0.

+ */
+static const struct speedy_device *speedy_get_device(struct device_node *np)
+{
...

+out:
+    of_node_put(speedy_np);
+    return handle;
+}

...

+static int speedy_probe(struct platform_device *pdev)
+{
+    struct device *dev = &pdev->dev;
+    struct speedy_controller *speedy;
+    void __iomem *mem;
+    int ret;
+
+    speedy = devm_kzalloc(dev, sizeof(struct speedy_controller), GFP_KERNEL);
+    if (!speedy)
+        return -ENOMEM;
+
+    platform_set_drvdata(pdev, speedy);
+    speedy->pdev = pdev;
+
+    mutex_init(&speedy->io_lock);
+
+    mem = devm_platform_ioremap_resource(pdev, 0);
+    if (IS_ERR(mem))
+        return dev_err_probe(dev, PTR_ERR(mem), "Failed to ioremap memory\n");
+
+    speedy->map = devm_regmap_init_mmio(dev, mem, &speedy_map_cfg);
+    if (IS_ERR(speedy->map))
+        return dev_err_probe(dev, PTR_ERR(speedy->map), "Failed to init the regmap\n");
+
+    /* Clear any interrupt status remaining */
+    ret = speedy_int_clear(speedy);
+    if (ret)
+        return ret;
+
+    /* Reset the controller */
+    ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_SW_RST);
+    if (ret)
+        return ret;
+
+    msleep(20);
+
+    /* Enable the hw */
+    ret = regmap_set_bits(speedy->map, SPEEDY_CTRL, SPEEDY_ENABLE);
+    if (ret)
+        return ret;
+
+    msleep(20);
+
+    /* Probe child devices */
+    ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, dev);
+    if (ret)
+        dev_err(dev, "Failed to populate child devices: %d\n", ret);

Could be dev_err_probe() as well, at least for consistency.


I agree, will fix in the next revision.



+
+    return ret;
+}

...

CJ


- Markuss





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux