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

[Matt Redfearn <matt.redfearn@imgtec.com>]

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@caviumnetworks.com>
>>
>> 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@iki.fi>
>> Signed-off-by: Chandrakala Chavva<cchavva@caviumnetworks.com>
>> Signed-off-by: David Daney<david.daney@cavium.com>
>> Signed-off-by: Aleksey Makarov<aleksey.makarov@auriga.com>
>> Signed-off-by: Leonid Rosenboim<lrosenboim@caviumnetworks.com>
>> Signed-off-by: Peter Swain<pswain@cavium.com>
>> Signed-off-by: Aaron Williams<aaron.williams@cavium.com>
>> Signed-off-by: Matt Redfearn<matt.redfearn@imgtec.com>
>> ---
>> 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;
>> +}
>> +
>