| Message ID | 20220926142933.2299460-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [-next,1/2] spi: introduce devm_spi_alloc_controller() | expand |
[+cc Geert, who originally came up with "spi_controller"] On Mon, Sep 26, 2022 at 10:29:32PM +0800, Yang Yingliang wrote: > Introduce devm_spi_alloc_controller() to wrap __devm_spi_alloc_controller(), > with this wrapper, the drivers can use it to update to the modern naming. [...] > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -778,6 +778,12 @@ static inline struct spi_controller *devm_spi_alloc_slave(struct device *dev, > return __devm_spi_alloc_controller(dev, size, true); > } > > +static inline struct spi_controller *devm_spi_alloc_controller(struct device *dev, > + unsigned int size) > +{ > + return __devm_spi_alloc_controller(dev, size, false); > +} > + > extern int spi_register_controller(struct spi_controller *ctlr); > extern int devm_spi_register_controller(struct device *dev, > struct spi_controller *ctlr); This doesn't really make sense I'm afraid. The umbrella term "spi_controller" can refer to either a master or a slave. One has to specify on allocation which of the two is desired. An API which purports to allow allocation of the umbrella term but defaults to a master behind the scenes seems misleading to me. Thanks, Lukas
Hi Lukas, On Tue, Sep 27, 2022 at 5:45 AM Lukas Wunner <lukas@wunner.de> wrote: > [+cc Geert, who originally came up with "spi_controller"] > > On Mon, Sep 26, 2022 at 10:29:32PM +0800, Yang Yingliang wrote: > > Introduce devm_spi_alloc_controller() to wrap __devm_spi_alloc_controller(), > > with this wrapper, the drivers can use it to update to the modern naming. > [...] > > --- a/include/linux/spi/spi.h > > +++ b/include/linux/spi/spi.h > > @@ -778,6 +778,12 @@ static inline struct spi_controller *devm_spi_alloc_slave(struct device *dev, > > return __devm_spi_alloc_controller(dev, size, true); > > } > > > > +static inline struct spi_controller *devm_spi_alloc_controller(struct device *dev, > > + unsigned int size) > > +{ > > + return __devm_spi_alloc_controller(dev, size, false); > > +} > > + > > extern int spi_register_controller(struct spi_controller *ctlr); > > extern int devm_spi_register_controller(struct device *dev, > > struct spi_controller *ctlr); > > This doesn't really make sense I'm afraid. The umbrella term > "spi_controller" can refer to either a master or a slave. > One has to specify on allocation which of the two is desired. > > An API which purports to allow allocation of the umbrella term > but defaults to a master behind the scenes seems misleading to me. Moreover, you already added devm_spi_alloc_master() and devm_spi_alloc_slave() in commit 5e844cc37a5cbaa4 ("spi: Introduce device-managed SPI controller allocation") in v5.10 ;-) 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 Tue, Sep 27, 2022 at 05:45:25AM +0200, Lukas Wunner wrote: > On Mon, Sep 26, 2022 at 10:29:32PM +0800, Yang Yingliang wrote: > > extern int devm_spi_register_controller(struct device *dev, > > struct spi_controller *ctlr); > This doesn't really make sense I'm afraid. The umbrella term > "spi_controller" can refer to either a master or a slave. > One has to specify on allocation which of the two is desired. > An API which purports to allow allocation of the umbrella term > but defaults to a master behind the scenes seems misleading to me. Yes, we'd need to either have two wrappers using some more appropriate terminology than master/slave or have a parameter which specifies the role.
Hi Mark, On 2022/9/27 19:21, Mark Brown wrote: > On Tue, Sep 27, 2022 at 05:45:25AM +0200, Lukas Wunner wrote: >> On Mon, Sep 26, 2022 at 10:29:32PM +0800, Yang Yingliang wrote: >>> extern int devm_spi_register_controller(struct device *dev, >>> struct spi_controller *ctlr); >> This doesn't really make sense I'm afraid. The umbrella term >> "spi_controller" can refer to either a master or a slave. >> One has to specify on allocation which of the two is desired. >> An API which purports to allow allocation of the umbrella term >> but defaults to a master behind the scenes seems misleading to me. > Yes, we'd need to either have two wrappers using some more appropriate > terminology than master/slave or have a parameter which specifies the > role. Do you mean to introduce two more proper wrappers to instead of devm_spi_alloc_master/slave() ? Thanks, Yang
On Tue, Sep 27, 2022 at 07:57:05PM +0800, Yang Yingliang wrote: > On 2022/9/27 19:21, Mark Brown wrote: > > On Tue, Sep 27, 2022 at 05:45:25AM +0200, Lukas Wunner wrote: > > > On Mon, Sep 26, 2022 at 10:29:32PM +0800, Yang Yingliang wrote: > > > > extern int devm_spi_register_controller(struct device *dev, > > > > struct spi_controller *ctlr); > > > This doesn't really make sense I'm afraid. The umbrella term > > > "spi_controller" can refer to either a master or a slave. > > > One has to specify on allocation which of the two is desired. > > > An API which purports to allow allocation of the umbrella term > > > but defaults to a master behind the scenes seems misleading to me. > > Yes, we'd need to either have two wrappers using some more appropriate > > terminology than master/slave or have a parameter which specifies the > > role. > Do you mean to introduce two more proper wrappers to instead of > devm_spi_alloc_master/slave() ? Honestly I don't think there's room for (or a need for) improvement here. Thanks, Lukas
On Tue, Sep 27, 2022 at 03:31:29PM +0200, Lukas Wunner wrote: > On Tue, Sep 27, 2022 at 07:57:05PM +0800, Yang Yingliang wrote: > > Do you mean to introduce two more proper wrappers to instead of > > devm_spi_alloc_master/slave() ? > Honestly I don't think there's room for (or a need for) improvement here. The issue here is that we're trying to get rid of the master/slave terminology.
On Tue, Sep 27, 2022 at 06:01:39PM +0100, Mark Brown wrote: > On Tue, Sep 27, 2022 at 03:31:29PM +0200, Lukas Wunner wrote: > > On Tue, Sep 27, 2022 at 07:57:05PM +0800, Yang Yingliang wrote: > > > Do you mean to introduce two more proper wrappers to instead of > > > devm_spi_alloc_master/slave() ? > > > Honestly I don't think there's room for (or a need for) improvement here. > > The issue here is that we're trying to get rid of the master/slave > terminology. Converting drivers to use spi_controller everywhere in lieu of spi_master is fine, but drivers need to specify whether the spi_controller is a master or a slave and Geert's design is to specify that on allocation. Which makes sense because that's the moment the spi_controller comes to life, there's no earlier moment where one could specify the type. Thanks, Lukas
On Tue, Sep 27, 2022 at 10:19:01PM +0200, Lukas Wunner wrote: > On Tue, Sep 27, 2022 at 06:01:39PM +0100, Mark Brown wrote: > > On Tue, Sep 27, 2022 at 03:31:29PM +0200, Lukas Wunner wrote: > > > On Tue, Sep 27, 2022 at 07:57:05PM +0800, Yang Yingliang wrote: > > > > Do you mean to introduce two more proper wrappers to instead of > > > > devm_spi_alloc_master/slave() ? > > > Honestly I don't think there's room for (or a need for) improvement here. > > The issue here is that we're trying to get rid of the master/slave > > terminology. > Converting drivers to use spi_controller everywhere in lieu of > spi_master is fine, but drivers need to specify whether the > spi_controller is a master or a slave and Geert's design is > to specify that on allocation. Which makes sense because > that's the moment the spi_controller comes to life, there's > no earlier moment where one could specify the type. Sure, but since the current wrappers use the legacy names this means that we need new wrappers with more modern names hence there is something to improve here.
Hi Mark, On Tue, Sep 27, 2022 at 10:24 PM Mark Brown <broonie@kernel.org> wrote: > On Tue, Sep 27, 2022 at 10:19:01PM +0200, Lukas Wunner wrote: > > On Tue, Sep 27, 2022 at 06:01:39PM +0100, Mark Brown wrote: > > > On Tue, Sep 27, 2022 at 03:31:29PM +0200, Lukas Wunner wrote: > > > > On Tue, Sep 27, 2022 at 07:57:05PM +0800, Yang Yingliang wrote: > > > > > > Do you mean to introduce two more proper wrappers to instead of > > > > > devm_spi_alloc_master/slave() ? > > > > > Honestly I don't think there's room for (or a need for) improvement here. > > > > The issue here is that we're trying to get rid of the master/slave > > > terminology. > > > Converting drivers to use spi_controller everywhere in lieu of > > spi_master is fine, but drivers need to specify whether the > > spi_controller is a master or a slave and Geert's design is > > to specify that on allocation. Which makes sense because > > that's the moment the spi_controller comes to life, there's > > no earlier moment where one could specify the type. > > Sure, but since the current wrappers use the legacy names this means > that we need new wrappers with more modern names hence there is > something to improve here. So what are the more modern names? 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 2022/9/28 4:22, Mark Brown wrote: > On Tue, Sep 27, 2022 at 10:19:01PM +0200, Lukas Wunner wrote: >> On Tue, Sep 27, 2022 at 06:01:39PM +0100, Mark Brown wrote: >>> On Tue, Sep 27, 2022 at 03:31:29PM +0200, Lukas Wunner wrote: >>>> On Tue, Sep 27, 2022 at 07:57:05PM +0800, Yang Yingliang wrote: >>>>> Do you mean to introduce two more proper wrappers to instead of >>>>> devm_spi_alloc_master/slave() ? >>>> Honestly I don't think there's room for (or a need for) improvement here. >>> The issue here is that we're trying to get rid of the master/slave >>> terminology. >> Converting drivers to use spi_controller everywhere in lieu of >> spi_master is fine, but drivers need to specify whether the >> spi_controller is a master or a slave and Geert's design is >> to specify that on allocation. Which makes sense because >> that's the moment the spi_controller comes to life, there's >> no earlier moment where one could specify the type. > Sure, but since the current wrappers use the legacy names this means > that we need new wrappers with more modern names hence there is > something to improve here. How about using primary/secondary, introduce two wrappers like this: diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 6ea889df0813..c41654fb069b 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -778,6 +778,21 @@ static inline struct spi_controller *devm_spi_alloc_slave(struct device *dev, return __devm_spi_alloc_controller(dev, size, true); } +static inline struct spi_controller *devm_spi_alloc_primary(struct device *dev, + unsigned int size) +{ + return __devm_spi_alloc_controller(dev, size, false); +} + +static inline struct spi_controller *devm_spi_alloc_secondary(struct device *dev, + unsigned int size) +{ + if (!IS_ENABLED(CONFIG_SPI_SLAVE)) + return NULL; + + return __devm_spi_alloc_controller(dev, size, true); +} + extern int spi_register_controller(struct spi_controller *ctlr); extern int devm_spi_register_controller(struct device *dev, struct spi_controller *ctlr); Thanks, Yang
On Wed, Sep 28, 2022 at 02:34:17PM +0800, Yang Yingliang wrote: > On 2022/9/28 4:22, Mark Brown wrote: > > Sure, but since the current wrappers use the legacy names this means > > that we need new wrappers with more modern names hence there is > > something to improve here. > How about using primary/secondary, introduce two wrappers like this: That's not going to be clear enough I think.
On Tue, Sep 27, 2022 at 10:43:08PM +0200, Geert Uytterhoeven wrote: > On Tue, Sep 27, 2022 at 10:24 PM Mark Brown <broonie@kernel.org> wrote: > > Sure, but since the current wrappers use the legacy names this means > > that we need new wrappers with more modern names hence there is > > something to improve here. > So what are the more modern names? It's unfortunately not 100% clear, and our use of controller for the generic thing gets in the way a bit. There was some stuff from one of the open source hardware groups recently that tried to propose new names but I'm not immediately finding it. "host" and "target" would probably do the trick?
On 2022/9/28 19:15, Mark Brown wrote: > On Tue, Sep 27, 2022 at 10:43:08PM +0200, Geert Uytterhoeven wrote: >> On Tue, Sep 27, 2022 at 10:24 PM Mark Brown <broonie@kernel.org> wrote: >>> Sure, but since the current wrappers use the legacy names this means >>> that we need new wrappers with more modern names hence there is >>> something to improve here. >> So what are the more modern names? > It's unfortunately not 100% clear, and our use of controller for the > generic thing gets in the way a bit. There was some stuff from one of > the open source hardware groups recently that tried to propose new names > but I'm not immediately finding it. "host" and "target" would probably > do the trick? So shall we use host/target to do wrappers. Thanks, Yang
On Wed, Sep 28, 2022 at 12:15:15PM +0100, Mark Brown wrote: > On Tue, Sep 27, 2022 at 10:43:08PM +0200, Geert Uytterhoeven wrote: > > On Tue, Sep 27, 2022 at 10:24 PM Mark Brown <broonie@kernel.org> wrote: > > > > Sure, but since the current wrappers use the legacy names this means > > > that we need new wrappers with more modern names hence there is > > > something to improve here. > > > So what are the more modern names? > > It's unfortunately not 100% clear, and our use of controller for the > generic thing gets in the way a bit. There was some stuff from one of > the open source hardware groups recently that tried to propose new names > but I'm not immediately finding it. "host" and "target" would probably > do the trick? Perhaps you mean that one? https://www.oshwa.org/a-resolution-to-redefine-spi-signal-names/ Looks like they're replacing master with controller and slave with peripheral. Pity, we're using controller as an umbrella term for either one of them. Renaming that will lead to an awful lot of churn. :( Thanks, Lukas
On Wed, Sep 28, 2022 at 05:11:16PM +0200, Lukas Wunner wrote: > On Wed, Sep 28, 2022 at 12:15:15PM +0100, Mark Brown wrote: > > It's unfortunately not 100% clear, and our use of controller for the > > generic thing gets in the way a bit. There was some stuff from one of > > the open source hardware groups recently that tried to propose new names > > but I'm not immediately finding it. "host" and "target" would probably > > do the trick? > Perhaps you mean that one? > https://www.oshwa.org/a-resolution-to-redefine-spi-signal-names/ > Looks like they're replacing master with controller and > slave with peripheral. Pity, we're using controller as > an umbrella term for either one of them. > Renaming that will lead to an awful lot of churn. :( Ah, that's the one - I knew there was some reason I didn't do anything with it when I saw it. I'm not sure the churn would be worth it.
On Wed, Sep 28, 2022 at 11:01:00PM +0800, Yang Yingliang wrote: > On 2022/9/28 19:15, Mark Brown wrote: > > It's unfortunately not 100% clear, and our use of controller for the > > generic thing gets in the way a bit. There was some stuff from one of > > the open source hardware groups recently that tried to propose new names > > but I'm not immediately finding it. "host" and "target" would probably > > do the trick? > So shall we use host/target to do wrappers. Yes, let's try that and see how it looks.
On Wed, Sep 28, 2022 at 5:11 PM Lukas Wunner <lukas@wunner.de> wrote: > On Wed, Sep 28, 2022 at 12:15:15PM +0100, Mark Brown wrote: > > On Tue, Sep 27, 2022 at 10:43:08PM +0200, Geert Uytterhoeven wrote: > > > On Tue, Sep 27, 2022 at 10:24 PM Mark Brown <broonie@kernel.org> wrote: > > > > Sure, but since the current wrappers use the legacy names this means > > > > that we need new wrappers with more modern names hence there is > > > > something to improve here. > > > > > So what are the more modern names? > > > > It's unfortunately not 100% clear, and our use of controller for the > > generic thing gets in the way a bit. There was some stuff from one of > > the open source hardware groups recently that tried to propose new names > > but I'm not immediately finding it. "host" and "target" would probably > > do the trick? > > Perhaps you mean that one? > > https://www.oshwa.org/a-resolution-to-redefine-spi-signal-names/ > > Looks like they're replacing master with controller and > slave with peripheral. In an inconsistent way: there is no peripheral select, but a chip select. > Pity, we're using controller as > an umbrella term for either one of them. So we have a controller controller, and a peripheral controller ;-) 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
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 6ea889df0813..32dd736ebe2e 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -778,6 +778,12 @@ static inline struct spi_controller *devm_spi_alloc_slave(struct device *dev, return __devm_spi_alloc_controller(dev, size, true); } +static inline struct spi_controller *devm_spi_alloc_controller(struct device *dev, + unsigned int size) +{ + return __devm_spi_alloc_controller(dev, size, false); +} + extern int spi_register_controller(struct spi_controller *ctlr); extern int devm_spi_register_controller(struct device *dev, struct spi_controller *ctlr);
Introduce devm_spi_alloc_controller() to wrap __devm_spi_alloc_controller(), with this wrapper, the drivers can use it to update to the modern naming. Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- include/linux/spi/spi.h | 6 ++++++ 1 file changed, 6 insertions(+)