| Message ID | 20150519013415.GV11598@ld-irv-0074 |
|---|---|
| State | Accepted |
| Headers | show |
On 19 May 2015 at 03:34, Brian Norris <computersforpeace@gmail.com> wrote: > So how about the following patch? It seems like we'll need to be able to > ignore useless 'modalias' values in cases like this: > > // modalias = "shinynewdevice" > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; > > and also if somebody leaves off the entire shinynewdevice string: > > // modalias = "spi-nor" > compatible = "jedec,spi-nor"; > > So we rework the spi-nor library to not reject "bad" names, and just > fall back to autodetection, and we add the .of_match_table to properly > catch all "jedec,spi-nor". That's nice but what about platforms using platform data instead of DT? I would like to use some kind of "spi-nor" (with some prefix *maybe*) for them too.
On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote: > On 19 May 2015 at 03:34, Brian Norris <computersforpeace@gmail.com> wrote: > > So how about the following patch? It seems like we'll need to be able to > > ignore useless 'modalias' values in cases like this: > > > > // modalias = "shinynewdevice" > > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; > > > > and also if somebody leaves off the entire shinynewdevice string: > > > > // modalias = "spi-nor" > > compatible = "jedec,spi-nor"; > > > > So we rework the spi-nor library to not reject "bad" names, and just > > fall back to autodetection, and we add the .of_match_table to properly > > catch all "jedec,spi-nor". > > That's nice but what about platforms using platform data instead of > DT? I would like to use some kind of "spi-nor" (with some prefix > *maybe*) for them too. 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? Or if you can propose other clean solutions, I'm all ears. I'm likely to merge this patch for 4.2 unless a better solution comes up, though. Brian
On 20 May 2015 at 23:35, Brian Norris <computersforpeace@gmail.com> wrote: > On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote: >> On 19 May 2015 at 03:34, Brian Norris <computersforpeace@gmail.com> wrote: >> > So how about the following patch? It seems like we'll need to be able to >> > ignore useless 'modalias' values in cases like this: >> > >> > // modalias = "shinynewdevice" >> > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; >> > >> > and also if somebody leaves off the entire shinynewdevice string: >> > >> > // modalias = "spi-nor" >> > compatible = "jedec,spi-nor"; >> > >> > So we rework the spi-nor library to not reject "bad" names, and just >> > fall back to autodetection, and we add the .of_match_table to properly >> > catch all "jedec,spi-nor". >> >> That's nice but what about platforms using platform data instead of >> DT? I would like to use some kind of "spi-nor" (with some prefix >> *maybe*) for them too. > > 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). Are you seriously not going to allow platform stuff *clearly* request flash model detection (JEDEC RDID OP)? Just because they don't use DT?
(trim CC a bit, as this is no longer a DT binding question) On Thu, May 21, 2015 at 09:12:25AM +0200, Rafał Miłecki wrote: > On 20 May 2015 at 23:35, Brian Norris <computersforpeace@gmail.com> wrote: > > On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote: > >> On 19 May 2015 at 03:34, Brian Norris <computersforpeace@gmail.com> wrote: > >> > So how about the following patch? It seems like we'll need to be able to > >> > ignore useless 'modalias' values in cases like this: > >> > > >> > // modalias = "shinynewdevice" > >> > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; > >> > > >> > and also if somebody leaves off the entire shinynewdevice string: > >> > > >> > // modalias = "spi-nor" > >> > compatible = "jedec,spi-nor"; > >> > > >> > So we rework the spi-nor library to not reject "bad" names, and just > >> > fall back to autodetection, and we add the .of_match_table to properly > >> > catch all "jedec,spi-nor". > >> > >> That's nice but what about platforms using platform data instead of > >> DT? I would like to use some kind of "spi-nor" (with some prefix > >> *maybe*) for them too. > > > > 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? Brian
On 21 May 2015 at 09:25, Brian Norris <computersforpeace@gmail.com> wrote: > (trim CC a bit, as this is no longer a DT binding question) > > On Thu, May 21, 2015 at 09:12:25AM +0200, Rafał Miłecki wrote: >> On 20 May 2015 at 23:35, Brian Norris <computersforpeace@gmail.com> wrote: >> > On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote: >> >> On 19 May 2015 at 03:34, Brian Norris <computersforpeace@gmail.com> wrote: >> >> > So how about the following patch? It seems like we'll need to be able to >> >> > ignore useless 'modalias' values in cases like this: >> >> > >> >> > // modalias = "shinynewdevice" >> >> > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; >> >> > >> >> > and also if somebody leaves off the entire shinynewdevice string: >> >> > >> >> > // modalias = "spi-nor" >> >> > compatible = "jedec,spi-nor"; >> >> > >> >> > So we rework the spi-nor library to not reject "bad" names, and just >> >> > fall back to autodetection, and we add the .of_match_table to properly >> >> > catch all "jedec,spi-nor". >> >> >> >> That's nice but what about platforms using platform data instead of >> >> DT? I would like to use some kind of "spi-nor" (with some prefix >> >> *maybe*) for them too. >> > >> > 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. 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), so I think I have to bounce the question: what alternative do you propose?
On Thu, May 21, 2015 at 10:01:05AM +0200, Rafał Miłecki wrote: > On 21 May 2015 at 09:25, Brian Norris <computersforpeace@gmail.com> wrote: > > (trim CC a bit, as this is no longer a DT binding question) > > > > On Thu, May 21, 2015 at 09:12:25AM +0200, Rafał Miłecki wrote: > >> On 20 May 2015 at 23:35, Brian Norris <computersforpeace@gmail.com> wrote: > >> > On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote: > >> >> On 19 May 2015 at 03:34, Brian Norris <computersforpeace@gmail.com> wrote: > >> >> > So how about the following patch? It seems like we'll need to be able to > >> >> > ignore useless 'modalias' values in cases like this: > >> >> > > >> >> > // modalias = "shinynewdevice" > >> >> > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; > >> >> > > >> >> > and also if somebody leaves off the entire shinynewdevice string: > >> >> > > >> >> > // modalias = "spi-nor" > >> >> > compatible = "jedec,spi-nor"; > >> >> > > >> >> > So we rework the spi-nor library to not reject "bad" names, and just > >> >> > fall back to autodetection, and we add the .of_match_table to properly > >> >> > catch all "jedec,spi-nor". > >> >> > >> >> That's nice but what about platforms using platform data instead of > >> >> DT? I would like to use some kind of "spi-nor" (with some prefix > >> >> *maybe*) for them too. > >> > > >> > 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. > 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. Brian
On 21 May 2015 at 10:15, Brian Norris <computersforpeace@gmail.com> 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@gmail.com> wrote: >> > (trim CC a bit, as this is no longer a DT binding question) >> > >> > On Thu, May 21, 2015 at 09:12:25AM +0200, Rafał Miłecki wrote: >> >> On 20 May 2015 at 23:35, Brian Norris <computersforpeace@gmail.com> wrote: >> >> > On Tue, May 19, 2015 at 09:27:50AM +0200, Rafał Miłecki wrote: >> >> >> On 19 May 2015 at 03:34, Brian Norris <computersforpeace@gmail.com> wrote: >> >> >> > So how about the following patch? It seems like we'll need to be able to >> >> >> > ignore useless 'modalias' values in cases like this: >> >> >> > >> >> >> > // modalias = "shinynewdevice" >> >> >> > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; >> >> >> > >> >> >> > and also if somebody leaves off the entire shinynewdevice string: >> >> >> > >> >> >> > // modalias = "spi-nor" >> >> >> > compatible = "jedec,spi-nor"; >> >> >> > >> >> >> > So we rework the spi-nor library to not reject "bad" names, and just >> >> >> > fall back to autodetection, and we add the .of_match_table to properly >> >> >> > catch all "jedec,spi-nor". >> >> >> >> >> >> That's nice but what about platforms using platform data instead of >> >> >> DT? I would like to use some kind of "spi-nor" (with some prefix >> >> >> *maybe*) for them too. >> >> > >> >> > 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.
Hi Rafal, Brian, On Thu, May 21, 2015 at 10:25 AM, Rafał Miłecki <zajec5@gmail.com> wrote: > On 21 May 2015 at 10:15, Brian Norris <computersforpeace@gmail.com> 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@gmail.com> 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? Non-autodetect platform_devices use flash_platform_data.type anyway, and thus fall under the first "if" clause above, don't they? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 21 May 2015 at 10:39, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Rafal, Brian, > > On Thu, May 21, 2015 at 10:25 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >> On 21 May 2015 at 10:15, Brian Norris <computersforpeace@gmail.com> 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@gmail.com> 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.
Hi Rafal, On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >>>> 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. To force m25p80 flash, you set flash_platform_data.type to "m25p80"? >> 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. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 21 May 2015 at 10:58, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >>>>> 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. > > To force m25p80 flash, you set flash_platform_data.type to "m25p80"? Oh, I think I got lost in the way m25p80 is probed. Is it handled by spi_board_info and its "modalias"? Could I leave flash_platform_data.type set to NULL and still have m25p80 probed?
Hi Rafal, On Thu, May 21, 2015 at 11:31 AM, Rafał Miłecki <zajec5@gmail.com> wrote: > On 21 May 2015 at 10:58, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >>>>>> 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. >> >> To force m25p80 flash, you set flash_platform_data.type to "m25p80"? > > Oh, I think I got lost in the way m25p80 is probed. Is it handled by > spi_board_info and its "modalias"? Indeed. > Could I leave flash_platform_data.type set to NULL and still have m25p80 probed? Yes, that case is the final "else" branch above. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 21 May 2015 at 11:39, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, May 21, 2015 at 11:31 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >> On 21 May 2015 at 10:58, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>> On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >>>>>>> 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. >>> >>> To force m25p80 flash, you set flash_platform_data.type to "m25p80"? >> >> Oh, I think I got lost in the way m25p80 is probed. Is it handled by >> spi_board_info and its "modalias"? > > Indeed. > >> Could I leave flash_platform_data.type set to NULL and still have m25p80 probed? > > Yes, that case is the final "else" branch above. Oh, I feel silly now. So I guess Brian's original patch was correct :|
On 21 May 2015 at 11:39, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, May 21, 2015 at 11:31 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >> On 21 May 2015 at 10:58, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>> On Thu, May 21, 2015 at 10:50 AM, Rafał Miłecki <zajec5@gmail.com> wrote: >>>>>>> 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. >>> >>> To force m25p80 flash, you set flash_platform_data.type to "m25p80"? >> >> Oh, I think I got lost in the way m25p80 is probed. Is it handled by >> spi_board_info and its "modalias"? > > Indeed. > >> Could I leave flash_platform_data.type set to NULL and still have m25p80 probed? > > Yes, that case is the final "else" branch above. Or wait a second, I see the problem. So the last "else" branch does: flash_name = spi->modalias; which will result in flash_name getting "m25p80" which will result in passing "m25p80" model to spi-nor. So some sort of solution is to replace if (data && data->type) flash_name = data->type; with if (data) flash_name = data->type; which will allow platform stuff to *not* specify type and trigger auto (JEDEC RD ID) detection. The only minor problem is we will still need specifying spi_board_info.platform_data, but 99.9% of code does as it's requires for setting partitions anyway. Brian: so I think your initial patch was OK, sorry for all this noise :|
On Mon, May 18, 2015 at 06:34:15PM -0700, Brian Norris wrote: > So how about the following patch? It seems like we'll need to be able to > ignore useless 'modalias' values in cases like this: > > // modalias = "shinynewdevice" > compatible = "myvendor,shinynewdevice", "jedec,spi-nor"; > > and also if somebody leaves off the entire shinynewdevice string: > > // modalias = "spi-nor" > compatible = "jedec,spi-nor"; > > So we rework the spi-nor library to not reject "bad" names, and just > fall back to autodetection, and we add the .of_match_table to properly > catch all "jedec,spi-nor". > > Signed-off-by: Brian Norris <computerfsorpeace@gmail.com> I realized this conversation ended in essentially an ack from Rafal, with no other comments. So I've pushed this (with some extra commentary) to l2-mtd.git. Holler if there are objections. Thanks, Brian > --- > drivers/mtd/devices/m25p80.c | 18 +++++++++++------- > drivers/mtd/spi-nor/spi-nor.c | 8 ++++---- > 2 files changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c > index 3af137f49ac9..30d608775f5a 100644 > --- a/drivers/mtd/devices/m25p80.c > +++ b/drivers/mtd/devices/m25p80.c > @@ -223,8 +223,6 @@ static int m25p_probe(struct spi_device *spi) > */ > 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; > > @@ -301,19 +299,25 @@ static const struct spi_device_id m25p_ids[] = { > {"w25q128"}, {"w25q256"}, {"cat25c11"}, > {"cat25c03"}, {"cat25c09"}, {"cat25c17"}, {"cat25128"}, > > - /* > - * Generic support for SPI NOR that can be identified by the JEDEC READ > - * ID opcode (0x9F). Use this, if possible. > - */ > - {"spi-nor"}, > { }, > }; > MODULE_DEVICE_TABLE(spi, m25p_ids); > > +static const struct of_device_id m25p_of_table[] = { > + /* > + * Generic compatibility for SPI NOR that can be identified by the > + * JEDEC READ ID opcode (0x9F). Use this, if possible. > + */ > + { .compatible = "jedec,spi-nor" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, m25p_of_table); > + > static struct spi_driver m25p80_driver = { > .driver = { > .name = "m25p80", > .owner = THIS_MODULE, > + .of_match_table = m25p_of_table, > }, > .id_table = m25p_ids, > .probe = m25p_probe, > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 14a5d2325dac..390d6fa0a53f 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -1003,11 +1003,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) > if (ret) > return ret; > > - /* Try to auto-detect if chip name wasn't specified */ > - if (!name) > - id = spi_nor_read_id(nor); > - else > + if (name) > id = spi_nor_match_id(name); > + /* Try to auto-detect if chip name wasn't specified or not found */ > + if (!id) > + id = spi_nor_read_id(nor); > if (IS_ERR_OR_NULL(id)) > return -ENOENT; > > -- > 1.9.1 >
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c index 3af137f49ac9..30d608775f5a 100644 --- a/drivers/mtd/devices/m25p80.c +++ b/drivers/mtd/devices/m25p80.c @@ -223,8 +223,6 @@ static int m25p_probe(struct spi_device *spi) */ 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; @@ -301,19 +299,25 @@ static const struct spi_device_id m25p_ids[] = { {"w25q128"}, {"w25q256"}, {"cat25c11"}, {"cat25c03"}, {"cat25c09"}, {"cat25c17"}, {"cat25128"}, - /* - * Generic support for SPI NOR that can be identified by the JEDEC READ - * ID opcode (0x9F). Use this, if possible. - */ - {"spi-nor"}, { }, }; MODULE_DEVICE_TABLE(spi, m25p_ids); +static const struct of_device_id m25p_of_table[] = { + /* + * Generic compatibility for SPI NOR that can be identified by the + * JEDEC READ ID opcode (0x9F). Use this, if possible. + */ + { .compatible = "jedec,spi-nor" }, + {} +}; +MODULE_DEVICE_TABLE(of, m25p_of_table); + static struct spi_driver m25p80_driver = { .driver = { .name = "m25p80", .owner = THIS_MODULE, + .of_match_table = m25p_of_table, }, .id_table = m25p_ids, .probe = m25p_probe, diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 14a5d2325dac..390d6fa0a53f 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -1003,11 +1003,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode) if (ret) return ret; - /* Try to auto-detect if chip name wasn't specified */ - if (!name) - id = spi_nor_read_id(nor); - else + if (name) id = spi_nor_match_id(name); + /* Try to auto-detect if chip name wasn't specified or not found */ + if (!id) + id = spi_nor_read_id(nor); if (IS_ERR_OR_NULL(id)) return -ENOENT;