[RFC v2 2/2] mmc: meson-mx-sdio: Add a driver for the Amlogic Meson8 and Meson8b SoCs

[Ulf Hansson]

[...]

> +#define MESON_MX_SDIO_MAX_SLOTS                                        3
> +
> +struct meson_mx_mmc_slot {
> +       struct device                   *dev;
> +       struct mmc_host                 *mmc;
> +       struct meson_mx_mmc_host        *host;
> +
> +       struct mmc_request              *mrq;
> +       struct mmc_command              *cmd;
> +       int                             error;
> +
> +       unsigned int                    id;
> +};

If I recall correctly, we agreed on postponing the multiple slot
support for now as to start simple.
Therefore, can you please remove all that code, and instead only use
the struct meson_mx_mmc_host for the data needed.

Just to be clear, I am fine having using the mmc slot bindings, as we
agreed. However as only one slot is supported in this stage, we don't
need a separate struct for that.

> +
> +struct meson_mx_mmc_host {
> +       struct device                   *dev;
> +
> +       struct clk                      *parent_clk;
> +       struct clk                      *core_clk;
> +       struct clk_divider              cfg_div;
> +       struct clk                      *cfg_div_clk;
> +       struct clk_fixed_factor         fixed_factor;
> +       struct clk                      *fixed_factor_clk;
> +
> +       void __iomem                    *base;
> +       int                             irq;
> +       spinlock_t                      irq_lock;
> +
> +       struct timer_list               cmd_timeout;
> +
> +       struct meson_mx_mmc_slot        *slot;
> +};
> +

[...]

> +static int meson_mx_mmc_slot_probe(struct device *slot_dev,
> +                                  struct meson_mx_mmc_host *host)
> +{
> +       struct meson_mx_mmc_slot *slot;
> +       struct mmc_host *mmc;
> +       unsigned int id;
> +       int ret;
> +
> +       if (of_property_read_u32(slot_dev->of_node, "reg", &id)) {
> +               dev_err(slot_dev, "missing 'reg' property\n");
> +               return -EINVAL;
> +       }
> +
> +       if (id >= MESON_MX_SDIO_MAX_SLOTS) {
> +               dev_err(slot_dev, "invalid 'reg' property value %d\n", id);
> +               return -EINVAL;
> +       }
> +
> +       mmc = mmc_alloc_host(sizeof(*slot), slot_dev);
> +       if (!mmc)
> +               return -ENOMEM;
> +
> +       slot = mmc_priv(mmc);
> +       slot->dev = slot_dev;
> +       slot->mmc = mmc;
> +       slot->id = id;
> +       slot->host = host;
> +
> +       host->slot = slot;
> +
> +       /* Get regulators and the supported OCR mask */
> +       ret = mmc_regulator_get_supply(mmc);
> +       if (ret == -EPROBE_DEFER)
> +               goto error_free_host;
> +
> +       mmc->max_req_size = MESON_MX_SDIO_BOUNCE_REQ_SIZE;
> +       mmc->max_seg_size = mmc->max_req_size;
> +       mmc->max_blk_count =
> +               FIELD_GET(MESON_MX_SDIO_SEND_REPEAT_PACKAGE_TIMES_MASK,
> +                         0xffffffff);
> +       mmc->max_blk_size = FIELD_GET(MESON_MX_SDIO_EXT_DATA_RW_NUMBER_MASK,
> +                                     0xffffffff);
> +       mmc->max_blk_size -= (4 * MESON_MX_SDIO_RESPONSE_CRC16_BITS);
> +       mmc->max_blk_size /= BITS_PER_BYTE;
> +
> +       /* Get the min and max supported clock rates */
> +       mmc->f_min = clk_round_rate(host->cfg_div_clk, 1);
> +       mmc->f_max = clk_round_rate(host->cfg_div_clk,
> +                                   clk_get_rate(host->parent_clk));
> +
> +       mmc->caps |= MMC_CAP_ERASE | MMC_CAP_CMD23;
> +       mmc->ops = &meson_mx_mmc_ops;
> +
> +       ret = mmc_of_parse(mmc);
> +       if (ret)
> +               goto error_free_host;
> +
> +       ret = mmc_add_host(mmc);
> +       if (ret)
> +               goto error_free_host;
> +
> +       return 0;
> +
> +error_free_host:
> +       mmc_free_host(mmc);
> +       return ret;
> +}
> +
> +static int meson_mx_mmc_probe_slots(struct meson_mx_mmc_host *host)
> +{
> +       struct device_node *slot_node;
> +       struct platform_device *slot_pdev;
> +       int ret;
> +
> +       for_each_child_of_node(host->dev->of_node, slot_node) {
> +               if (!of_device_is_compatible(slot_node, "mmc-slot"))
> +                       continue;
> +
> +               /*
> +                * TODO: the MMC core framework currently does not support
> +                * controllers with multiple slots properly.
> +                */
> +               if (host->slot) {
> +                       dev_warn(host->dev,
> +                                "skipping slot other than the first\n");
> +                       continue;
> +               }
> +
> +               slot_pdev = of_platform_device_create(slot_node, NULL,
> +                                                     host->dev);
> +               if (!slot_pdev)
> +                       continue;
> +
> +               ret = meson_mx_mmc_slot_probe(&slot_pdev->dev, host);
> +               if (ret) {
> +                       of_platform_device_destroy(&slot_pdev->dev, NULL);
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}

For the above two functions, please simplify the code by limiting them
to deal with only one valid mmc-slot.

[...]

Besides the comments on the multiple slot support, this looks good to me.

Kind regards
Uffe