Re: [PATCH v1 1/3] spi: geni-qcom: Improve target mode allocation by using proper allocation functions

[Praveen Talari]

Hi Konrad,

On 1/29/2026 5:12 PM, Konrad Dybcio wrote:
> On 1/28/26 5:32 PM, Praveen Talari wrote:
> > Hi Konrad
> >
> > On 1/27/2026 6:45 PM, Konrad Dybcio wrote:
> > > On 1/22/26 4:10 PM, Praveen Talari wrote:
> > > > The current implementation always allocates a host controller and sets the
> > > > target flag later when the "spi-slave" device tree property is present.
> > > > This approach is suboptimal as it doesn't utilize the dedicated allocation
> > > > functions designed for target mode.
> > > >
> > > > Use devm_spi_alloc_target() when "spi-slave" device tree property is
> > > > present, otherwise use devm_spi_alloc_host(). This replaces the previous
> > > > approach of always allocating a host controller and setting target flag
> > > > later.
> > > >
> > > > Signed-off-by: Praveen Talari <praveen.talari@xxxxxxxxxxxxxxxx>
> > > > ---
> > > >    drivers/spi/spi-geni-qcom.c | 15 ++++++++-------
> > > >    1 file changed, 8 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> > > > index 0e5fd9df1a8f..f5d05025b196 100644
> > > > --- a/drivers/spi/spi-geni-qcom.c
> > > > +++ b/drivers/spi/spi-geni-qcom.c
> > > > @@ -1017,6 +1017,14 @@ static int spi_geni_probe(struct platform_device *pdev)
> > > >        struct clk *clk;
> > > >        struct device *dev = &pdev->dev;
> > > >    +    if (device_property_read_bool(dev, "spi-slave"))
> > > > +        spi = devm_spi_alloc_target(dev, sizeof(*mas));
> > > > +    else
> > > > +        spi = devm_spi_alloc_host(dev, sizeof(*mas));
> > > > +
> > > > +    if (!spi)
> > > > +        return -ENOMEM;
> > > > +
> > > >        irq = platform_get_irq(pdev, 0);
> > > >        if (irq < 0)
> > > >            return irq;
> > > > @@ -1033,10 +1041,6 @@ static int spi_geni_probe(struct platform_device *pdev)
> > > >        if (IS_ERR(clk))
> > > >            return PTR_ERR(clk);
> > > >    -    spi = devm_spi_alloc_host(dev, sizeof(*mas));
> > > > -    if (!spi)
> > > > -        return -ENOMEM;
> > >
> > > Is there a reason you're moving this code to the top of the function?
> >
> > When CONFIG_SPI_SLAVE is disabled, the call returns NULL; therefore, I placed this check at the start of the probe() function.
> >
> > ref:
> > static inline struct spi_controller *devm_spi_alloc_target(struct device *dev, unsigned int size)
> > {
> >      if (!IS_ENABLED(CONFIG_SPI_SLAVE))
> >          return NULL;
> >
> >      return __devm_spi_alloc_controller(dev, size, true);
> > }
>
> That doesn't really matter since spi is not accessed beforehand
> and it'd return a NULL if it failed to allocate either way
I agree. I had also reviewed other SPI drivers as a reference for this 
implementation.

Do you want me to keep the change where earlier the host allocation was 
present, or is the current modification acceptable?

Please help on this.

Thanks,
Praveen
> I'm not sure this is a concern nowadays with fw_devlink and
> friends, but today the allocation happens after we get a clock
> reference, which could throw an eprobe_defer, which I think would
> cause the memory to be de-allocated again, wasting cycles
>
> Konrad