Re: [PATCH] Documentation: dt: mtd: replace "nor-jedec" binding with "jedec,spi-nor"

[Rafał Miłecki <zajec5@xxxxxxxxx>]

On 21 May 2015 at 10:39, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> Hi Rafal, Brian,
>
> On Thu, May 21, 2015 at 10:25 AM, Rafał Miłecki <zajec5@xxxxxxxxx> wrote:
>> On 21 May 2015 at 10:15, Brian Norris <computersforpeace@xxxxxxxxx> wrote:
>>> On Thu, May 21, 2015 at 10:01:05AM +0200, Rafał Miłecki wrote:
>>>> On 21 May 2015 at 09:25, Brian Norris <computersforpeace@xxxxxxxxx> wrote:
>>>> >> > For platform devices, you might as well just use the name of the driver,
>>>> >> > which is 'm25p80'. Isn't that how most platform devices are matched with
>>>> >> > drivers?
>>>> >>
>>>> >> Yes and I think it's ugly because it keeps causing the warning about
>>>> >> read flash model not matching specified one (m25p80).
>>>> >
>>>> > Sure, I agree.
>>>> >
>>>> >> Are you
>>>> >> seriously not going to allow platform stuff *clearly* request flash
>>>> >> model detection (JEDEC RDID OP)? Just because they don't use DT?
>>>> >
>>>> > No, this isn't about "allowing" anything. It's just that my primary
>>>> > concern was to get the DT binding straightened out properly. Linus'
>>>> > current tree now has the proper binding, but the m25p80.c code doesn't
>>>> > quite bind properly. It will work if "jedec,spi-nor" is the first
>>>> > entry in the compatible property (and so it becomes the 'modalias', but
>>>> > not second, third, etc. So my patch fixes that properly.
>>>> >
>>>> > Now, the secondary concern is that you want platform devices to specify
>>>> > something generic, and that doesn't yield a "found X, expected Y"
>>>> > message. I'm perfectly fine with fixing that too, if you have a patch
>>>> > for it. What do you propose?
>>>>
>>>> Maybe I wasn't clear enough. I was going to start using struct
>>>> flash_platform_data with
>>>> .type = "spi-nor",
>>>> but your proposed patch removes support for such name.
>>>
>>> Ah, OK. So that's the part I was overlooking.
>>>
>>>> While I like matching DT *clearly* against the whole "jedec,spi-nor"
>>>> string (really, I'm all for it), I'm confused what I should use for
>>>> platform stuff now. I don't have any proposal as my initial plan was
>>>> exactly to use this "spi-nor".
>>>> I guess I don't want to re-add support for "spi-nor" (as you just
>>>> proposed to remove it),
>>>
>>> I wasn't really trying to remove "spi-nor", that was mostly a side
>>> effect.
>>
>> OK, I think we understand each other now :)
>>
>>>> so I think I have to bounce the question: what
>>>> alternative do you propose?
>>>
>>> I think your comments suggest that I shouldn't be removing "spi-nor"
>>> from m25p_ids[] nor from this block:
>>>
>>>         if (data && data->type)
>>>                 flash_name = data->type;
>>>         else if (!strcmp(spi->modalias, "spi-nor"))
>>>                 flash_name = NULL; /* auto-detect */
>>>         else
>>>                 flash_name = spi->modalias;
>>>
>>> So it stays in both m25p_ids[] and .of_match_table.
>>>
>>> I suppose that can work. It then allows people to do weird stuff like:
>>>
>>>         compatible = "idontknowwhatimdoing,spi-nor";
>>>
>>> in their device tree. But other than that, there's not much downside I don't
>>> think.
>>
>> It sounds like a reasonable solution. I guess there isn't a perfect
>> one. Even if we decide to go for sth like "jedec-spi-nor", there
>> always will be a chance of someone using
>> compatible = "idontknowwhatimdoing,jedec-spi-nor";
>> So if you rework your patch to leave "spi-nor" support in m25p_ids and
>> conditions block, it should be OK.
>
> Typically platform devices just use the driver's name. Hence IMHO there's
> no need to add a shiny new spi-nor device name.
>
> So what's wrong with using "m25p80", and treating that as auto-detect iff
> !spi->dev.of_node?

Treating "m25p80" as auto-detect triggering string won't allow
platform to *force* "m25p80" flash type if there ever appears to be
needed. Maybe it's unlikely, but it still sounds like a bit bad design
for me.


> Non-autodetect platform_devices use flash_platform_data.type anyway,
> and thus fall under the first "if" clause above, don't they?

They do, but I don't see the point.

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html