Re: [PATCH v5] mmc: OCTEON: Add host driver for OCTEON MMC controller

[Matt Redfearn]

Hi David

On 10/02/16 18:02, David Daney wrote:
> On 02/10/2016 09:36 AM, Matt Redfearn wrote:
> > From: Aleksey Makarov<aleksey.makarov@xxxxxxxxxxxxxxxxxx>
> >
> > The OCTEON MMC controller is currently found on cn61XX and cnf71XX
> > devices.  Device parameters are configured from device tree data.
> >
> > eMMC, MMC and SD devices are supported.
> >
> > Tested-by: Aaro Koskinen<aaro.koskinen@xxxxxx>
> > Signed-off-by: Chandrakala Chavva<cchavva@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: David Daney<david.daney@xxxxxxxxxx>
> > Signed-off-by: Aleksey Makarov<aleksey.makarov@xxxxxxxxxx>
> > Signed-off-by: Leonid Rosenboim<lrosenboim@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Peter Swain<pswain@xxxxxxxxxx>
> > Signed-off-by: Aaron Williams<aaron.williams@xxxxxxxxxx>
> > Signed-off-by: Matt Redfearn<matt.redfearn@xxxxxxxxxx>
> > ---
> > v5:
> > Incoroprate comments from review
> > http://patchwork.linux-mips.org/patch/9558/
> > - Use standard <bus-width> property instead of <cavium,bus-max-width>.
> > - Use standard <max-frequency> property instead of <spi-max-frequency>.
> > - Add octeon_mmc_of_parse_legacy function to deal with the above
> >    properties, since many devices have shipped with those properties
> >    embedded in firmware.
> > - Allow the <vmmc-supply> binding in addition to the legacy
> >    <gpios-power>.
> > - Remove the secondary driver for each slot.
> > - Use core gpio cd/wp handling
> [...]
>
> > +static int octeon_mmc_of_copy_legacy_u32(struct device_node *node,
> > +                      const char *legacy_name,
> > +                      const char *new_name)
> > +{
> > +    u32 value;
> > +    int ret;
> > +
> > +    ret = of_property_read_u32(node, legacy_name, &value);
> > +    if (!ret) {
> > +        /* Found legacy - set generic property */
> > +        struct property *new_p;
> > +        u32 *new_v;
> > +
> > +        pr_warn(FW_WARN "%s: Legacy property '%s'. Please remove\n",
> > +            node->full_name, legacy_name);
> > +
>
> I don't like this warning message.
>
> The vast majority of people that see it will not be able to change 
> their firmware.  So it will be forever cluttering up their boot logs.
>
> We are not ever planning on removing support for legacy firmware 
> properties, so alarming people is really all this message does.
>
> If you insist on a message then make it something like pr_info("This 
> is working properly, but please consider using modern device tree 
> properties...")

Fair enough - I was just following what the PHY driver does when it 
encounters a whitelisted compatible string, e.g. when my board boots:

[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@0: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@1: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@2: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@3: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@4: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@5: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@6: Whitelisted 
compatible string. Please remove
[Firmware Warn]: /soc@0/mdio@1180000001800/ethernet-phy@7: Whitelisted 
compatible string. Please remove

But I can see why it would be preferable to avoid this kind of message. 
I don't think it's essential so could be removed.

Thanks,
Matt
> > +        new_p = kzalloc(sizeof(*new_p), GFP_KERNEL);
> > +        new_v = kzalloc(sizeof(u32), GFP_KERNEL);
> > +        if (!new_p || !new_v)
> > +            return -ENOMEM;
> > +
> > +        *new_v = value;
> > +        new_p->name = kstrdup(new_name, GFP_KERNEL);
> > +        new_p->length = sizeof(u32);
> > +        new_p->value = new_v;
> > +
> > +        of_update_property(node, new_p);
> > +    }
> > +    return 0;
> > +}
> > +
> > +/*
> > + * This function parses the legacy device tree that may be found in 
> > devices
> > + * shipped before the driver was upstreamed. Future devices should 
> > not require
> > + * it as standard bindings should be used
> > + */
> > +static int octeon_mmc_of_parse_legacy(struct device *dev,
> > +                      struct device_node *node,
> > +                      struct octeon_mmc_slot *slot)
> > +{
> > +    int ret;
> > +
> > +    ret = octeon_mmc_of_copy_legacy_u32(node, "cavium,bus-max-width",
> > +                        "bus-width");
> > +    if (ret)
> > +        return ret;
> > +
> > +    ret = octeon_mmc_of_copy_legacy_u32(node, "spi-max-frequency",
> > +                        "max-frequency");
> > +    if (ret)
> > +        return ret;
> > +
> > +    slot->pwr_gpiod = devm_gpiod_get_optional(dev, "power", 
> > GPIOD_OUT_LOW);
> > +    if (!IS_ERR(slot->pwr_gpiod)) {
> > +        pr_warn(FW_WARN "%s: Legacy property '%s'. Please remove\n",
> > +            node->full_name, "gpios-power");
> > +    }
> > +
> > +    return 0;
> > +}
> > +