Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 17/10/17 12:04, Ladislav Michl wrote:
> On Tue, Oct 17, 2017 at 11:46:51AM +0300, Roger Quadros wrote:
>> Hi,
>>
>>
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
>> On 16/10/17 02:20, Ladislav Michl wrote:
>>> Generic probe function can deal with OneNAND node, remove
>>> now useless gpmc_probe_onenand_child function.
>>>
>>> Signed-off-by: Ladislav Michl <ladis@xxxxxxxxxxxxxx>
>>> ---
>>> drivers/memory/omap-gpmc.c | 111 ++++++++++++++++++++++++++++-----------------
>>> 1 file changed, 69 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
>>> index f2aef0b87bc6..fc7c3c2a0b04 100644
>>> --- a/drivers/memory/omap-gpmc.c
>>> +++ b/drivers/memory/omap-gpmc.c
>>> @@ -1921,52 +1921,22 @@ static void __maybe_unused gpmc_read_timings_dt(struct device_node *np,
>>> of_property_read_bool(np, "gpmc,time-para-granularity");
>>> }
>>>
>>> -#if IS_ENABLED(CONFIG_MTD_ONENAND)
>>> -static int gpmc_probe_onenand_child(struct platform_device *pdev,
>>> - struct device_node *child)
>>> -{
>>> - u32 val;
>>> - struct omap_onenand_platform_data *gpmc_onenand_data;
>>> -
>>> - if (of_property_read_u32(child, "reg", &val) < 0) {
>>> - dev_err(&pdev->dev, "%pOF has no 'reg' property\n",
>>> - child);
>>> - return -ENODEV;
>>> - }
>>> -
>>> - gpmc_onenand_data = devm_kzalloc(&pdev->dev, sizeof(*gpmc_onenand_data),
>>> - GFP_KERNEL);
>>> - if (!gpmc_onenand_data)
>>> - return -ENOMEM;
>>> -
>>> - gpmc_onenand_data->cs = val;
>>> - gpmc_onenand_data->of_node = child;
>>> - gpmc_onenand_data->dma_channel = -1;
>>> -
>>> - if (!of_property_read_u32(child, "dma-channel", &val))
>>> - gpmc_onenand_data->dma_channel = val;
>>> -
>>> - return gpmc_onenand_init(gpmc_onenand_data);
>>> -}
>>> -#else
>>> -static int gpmc_probe_onenand_child(struct platform_device *pdev,
>>> - struct device_node *child)
>>> -{
>>> - return 0;
>>> -}
>>> -#endif
>>> +extern int
>>> +gpmc_omap_onenand_init(struct omap_onenand_platform_data *_onenand_data);
>>>
>>> /**
>>> - * gpmc_probe_generic_child - configures the gpmc for a child device
>>> + * gpmc_probe_child - configures the gpmc for a child device
>>
>> Let's not do this name change it is not really required.
>
> But it is nice, right? :) Will drop that change.>
:)
>>> * @pdev: pointer to gpmc platform device
>>> * @child: pointer to device-tree node for child device
>>> *
>>> * Allocates and configures a GPMC chip-select for a child device.
>>> * Returns 0 on success and appropriate negative error code on failure.
>>> */
>>> -static int gpmc_probe_generic_child(struct platform_device *pdev,
>>> +static int gpmc_probe_child(struct platform_device *pdev,
>>> struct device_node *child)
>>> {
>>> + struct omap_onenand_platform_data *gpmc_onenand_data;
>>> + struct platform_device *child_dev;
>>> struct gpmc_settings gpmc_s;
>>> struct gpmc_timings gpmc_t;
>>> struct resource res;
>>> @@ -2058,6 +2028,33 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>>> }
>>> }
>>>
>>> + if (of_node_cmp(child->name, "onenand") == 0) {
>>> + /*
>>> + * Warn about older DT blobs with no compatible property
>>> + * and fix them for now
>>> + */
>>> + if (!of_property_read_bool(child, "compatible")) {
>>> + struct property *prop;
>>> +
>>> + dev_warn(&pdev->dev,
>>> + "Incompatible OneNAND node, fixing.");
>>> +
>>> + prop = kzalloc(sizeof(*prop), GFP_KERNEL);
>>> + if (!prop) {
>>> + ret = -ENOMEM;
>>> + goto err;
>>> + }
>>> +
>>> + prop->name = "compatible";
>>> + /* See FIXME about device-tree migration bellow */
>>> + prop->value = (gpmc_capability & GPMC_HAS_WR_ACCESS) ?
>>> + "ti,omap3-onenand" : "ti,omap2-onenand";
>>> + prop->length = 14;
>>> +
>>> + of_update_property(child, prop);
>>> + }
>>
>> I don't want to fixup broken DTs like this as they will go silently without being fixed.
>>
>> Let's instead just error out after the dev_warn().
>> This will force us to fix all DT nodes.
>
> Is it really right thing to do? A lot of people scream loudly when you break
> their boot.
>
And it is our job to fix it if it does :).
IMO this is way better than things going unfixed for years together.
>>> + }
>>> +
>>> if (of_device_is_compatible(child, "ti,omap2-nand")) {
>>> /* NAND specific setup */
>>> val = 8;
>>> @@ -2079,6 +2076,30 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>>> /* disable write protect */
>>> gpmc_configure(GPMC_CONFIG_WP, 0);
>>> gpmc_s.device_nand = true;
>>> + } else if (of_device_is_compatible(child, "ti,omap2-onenand") ||
>>> + of_device_is_compatible(child, "ti,omap3-onenand")) {
>>
>> Could just be if() instead of else if() as we don't have any dependency on nand.
>
> It could, but as node is eithrt nand or onenand, this saves few cycles.
>
I don't think the few cycles matter that much. I'd choose an option which
has better readability. Choice is yours :)
>>> + gpmc_onenand_data = devm_kzalloc(&pdev->dev,
>>> + sizeof(*gpmc_onenand_data),
>>> + GFP_KERNEL);
>>> + if (!gpmc_onenand_data) {
>>> + ret = -ENOMEM;
>>> + goto err;
>>> + }
>>> +
>>> + gpmc_onenand_data->cs = cs;
>>> + gpmc_onenand_data->of_node = child;
>>> + if (!of_property_read_u32(child, "dma-channel", &val))
>>> + gpmc_onenand_data->dma_channel = val;
>>> + else
>>> + gpmc_onenand_data->dma_channel = -1;
>>> +
>>> + ret = gpmc_omap_onenand_init(gpmc_onenand_data);
>>> + if (ret)
>>> + goto err;
>>
>> this gpmc_onenand_data should go away if we squash patch 10 here.
>
> But driver will stop working then as setup function will be NULL.
>
Which is fine. It will start working again when mtd parts get merged via mtd tree.
>>> +
>>> + /* OneNAND driver handles timings on its own */
>>> + gpmc_cs_enable_mem(cs);
>>> + goto no_timings;
>>
>> Actually ASYNC timings should be handled here just as the other children.
>> Only SYNC timings will be triggered by the OneNAND driver.
>
> It is handled here, it just cannot be done in one patch as MTD driver
> needs to be modified.
>
>>> } else {
>>> ret = of_property_read_u32(child, "bank-width",
>>> &gpmc_s.device_width);
>>> @@ -2126,9 +2147,19 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>>> no_timings:
>>>
>>> /* create platform device, NULL on error or when disabled */
>>> - if (!of_platform_device_create(child, NULL, &pdev->dev))
>>> + child_dev = of_platform_device_create(child, NULL, &pdev->dev);
>>> + if (!child_dev)
>>> goto err_child_fail;
>>>
>>> + /* Use platform data until OneNAND driver is DT aware */
>>> + if (gpmc_onenand_data) {
>>> + child_dev->name = "omap2-onenand";
>>> + child_dev->dev.platform_data = gpmc_onenand_data;
>>> + ret = platform_device_add_resources(child_dev, &res, 1);
>>> + if (ret)
>>> + goto err_child_fail;
>>> + }
>>> +
>>
>> This bit will go away on squashing patch 10.
>
> I'm not sure how to do this and merge changes using two separate trees.
- make sure build doesn't break in each tree. I'm OK with oneNAND functionality broken in
individual trees for this case. As there is no other way out and make your life easier.
- make sure everything works after trees are merged.
>
>>> /* is child a common bus? */
>>> if (of_match_node(of_default_bus_match_table, child))
>>> /* create children and other common bus children */
>>> @@ -2193,11 +2224,7 @@ static void gpmc_probe_dt_children(struct platform_device *pdev)
>>> if (!child->name)
>>> continue;
>>>
>>> - if (of_node_cmp(child->name, "onenand") == 0)
>>> - ret = gpmc_probe_onenand_child(pdev, child);
>>> - else
>>> - ret = gpmc_probe_generic_child(pdev, child);
>>> -
>>> + ret = gpmc_probe_child(pdev, child);
>>> if (ret) {
>>> dev_err(&pdev->dev, "failed to probe DT child '%s': %d\n",
>>> child->name, ret);
>>>
>>
--
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
