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

[Markuss Broks <markuss.broks@gmail.com>]

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