diff mbox series

[-next,1/2] spi: introduce devm_spi_alloc_controller()

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

Commit Message

Yang Yingliang Sept. 26, 2022, 2:29 p.m. UTC
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(+)

Comments

Lukas Wunner Sept. 27, 2022, 3:45 a.m. UTC | #1
[+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
Geert Uytterhoeven Sept. 27, 2022, 7:11 a.m. UTC | #2
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
Mark Brown Sept. 27, 2022, 11:21 a.m. UTC | #3
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.
Yang Yingliang Sept. 27, 2022, 11:57 a.m. UTC | #4
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
Lukas Wunner Sept. 27, 2022, 1:31 p.m. UTC | #5
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
Mark Brown Sept. 27, 2022, 5:01 p.m. UTC | #6
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.
Lukas Wunner Sept. 27, 2022, 8:19 p.m. UTC | #7
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
Mark Brown Sept. 27, 2022, 8:22 p.m. UTC | #8
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.
Geert Uytterhoeven Sept. 27, 2022, 8:43 p.m. UTC | #9
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
Yang Yingliang Sept. 28, 2022, 6:34 a.m. UTC | #10
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
Mark Brown Sept. 28, 2022, 11:10 a.m. UTC | #11
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.
Mark Brown Sept. 28, 2022, 11:15 a.m. UTC | #12
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?
Yang Yingliang Sept. 28, 2022, 3:01 p.m. UTC | #13
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
Lukas Wunner Sept. 28, 2022, 3:11 p.m. UTC | #14
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
Mark Brown Sept. 28, 2022, 3:58 p.m. UTC | #15
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.
Mark Brown Sept. 28, 2022, 4:08 p.m. UTC | #16
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.
Geert Uytterhoeven Sept. 28, 2022, 5:14 p.m. UTC | #17
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 mbox series

Patch

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);