by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] SPI: Add SPI driver for Sunplus SP7021

On Wed, Nov 10, 2021 at 02:42:01AM +0000, Lh Kuo 郭力豪 wrote:

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

> > This is *still* open coding a GPIO chip select, to repeat what I said last time
> > this is not OK - use the core facilities to avoid introducing bugs like double
> > application of SPI_CS_HIGH you have here.

> I try to find some function can replay this part.
> EX:
> Spi.c -> spi_set_cs but it is not EXPORT_SYMBOL_GPL and it looks not fit in the driver
>
> Spi-gpio.c -> spi_gpio_chipselect it looks not fit in the driver.
>
> Sorry maybe i misunderstood this issue
>
> Or the problem is gpiod_set_value_cansleep can't be used in here ?
>
> Which function can I use for GPIO_CS?

The same way other controllers do: by setting use_gpio_descriptors in
the controller. The core will then request the GPIOs for the driver
using the standard binding, this requires no further work on the part of
the driver.

> > > +// spi slave irq handler
> > > +static irqreturn_t sp7021_spi_sla_irq(int _irq, void *_dev) {
> > > + struct sp7021_spi_ctlr *pspim = (struct sp7021_spi_ctlr *)_dev;

> > If you need this cast something is very wrong, do you need it?

> So the vold* should be struct * spi_controller ??

No, interrupt handlers need to take an int and a void *. There should
be no cast.

> > > + if (RW_phase == SP7021_SLA_WRITE) {

> > This looks like a switch statement, though given how little code is shared it's
> > not clear why this is all in one function.

> It is easy to check the flow and setting for me

It's contributing to making the code hard for other people to follow.

> > > + if (_t->tx_dma == pspim->tx_dma_phy_base)
> > > + memcpy(pspim->tx_dma_vir_base, _t->tx_buf, _t->len);

> > Why are we copying data into a DMA transfer buffer, doesn't this defeat the
> > point of DMA? I'd expect to DMA data directly. I'd also expect some
> > synchronisation operations to ensure that everything has a consistent view of
> > the memory.

> It only happens when tx_dma = pspim->tx_dma_phy_base
> And if it can't get dma-addr or wrong case. I will set tx_dma = pspim->tx_dma_phy_base.

What does that mean at a higher level - what is tx_dma here? Why does
not being able to get an address to DMA mean that we need to memcpy()
things? I can't see any reason for the memcpy() at all.

> > > +// spi master irq handler
> > > +static irqreturn_t sp7021_spi_mas_irq_dma(int _irq, void *_dev) {
> > > + struct sp7021_spi_ctlr *pspim = (struct sp7021_spi_ctlr *)_dev;

> > > + spin_lock_irq(&pspim->lock);

> > Why are we using spin_lock_irq() when we're already in an interrupt handler?

> Yes it is in interrupt handler

Yes, I know it's an interrupt handler - to repeat my question why are we
we using spin_lock_irq() in that case?

> > > + return IRQ_HANDLED;
> > > +}

> > This unconditionally reports IRQ_HANDLED even if we didn't actually see any
> > interrupt status flagged, this will break shared interrupts and reduce the ability
> > of the interrupt core to handle errors.

> Sorry I'm confuse. What should i do in this issue

Report IRQ_NONE if there was no interrupts reported by the hardware.

> > This is still copying all data for no apparent reason as highlighted last time.

> It is difference case. It is in master mode and and can only be transmitted through FIFO

> And It is transmitting for one message and I need collect the all tx data first.

For what reason do you need to collect all the tx data? It really is
not at all apparent from the code and seems especially unusual in the
PIO case.

> > Is the device full duplex or half duplex? The code immediately above this
> > treats both transmit and recieve buffers as optional. If the device does
> > actually need to be full duplex then the driver should flag it as such.

> You mean set the flsg of should be struct * spi_controller in probe function

> Ctlr-flags = SPI_CONTROLLER_FULL_DUPLEX right ?

Yes.

> > > +// called when child device is registering on the bus static int
> > > +sp7021_spi_dev_setup(struct spi_device *_s) {
> > > + struct sp7021_spi_ctlr *pspim =
> > spi_controller_get_devdata(_s->controller);
> > > + int ret;
> > > +
> > > + ret = pm_runtime_get_sync(pspim->dev);
> > > + if (ret < 0)
> > > + return 0;
> > > +
> > > + pm_runtime_get_sync(pspim->dev);;
> > > +
> > > + return 0;
> > > +
> > > +}

> > This function does nothing except bounce the power on the device, this is
> > obviously not useful and should be removed.

> You mean set the auto_runtime_pm of should be struct * spi_controller in probe function
> And remove pm_runtime_get_sync(pspim->dev);

> pm_runtime_get_sync(pspim->dev);

> even the pm_runtime in the probe should be remove . right ?

You should only take a runtime reference for the period that it's
actually needed. Taking one in probe and then not dropping it before
the end of probe would defeat the point of having runtime PM.

> > > +static size_t sp7021_spi_max_length(struct spi_device *spi) {
> > > + return SP7021_SPI_MSG_SIZE;
> > > +}

> > Is there any actual limit in the hardware? This looks very much like it's a
> > completely artificial limit in the driver for no obvious reason.

> The limit of the hardware is only 255 bytes . But more user need more than the limit.
> So I try to extend by software that is one reason use one message transfer and use CS-GPIO

As ever *please* use the core features rather than open coding - if you
specify a maximum transfer size the core already supports using a
software controllable chip select to handle messages with transfers of
arbatrary lengths. There is no need for the driver to do anything here
other than providing a length, but that needs to be per transfer and not
per message.

In general if you're doing something that doesn't interact directly with
the hardware it shouldn't be in the driver, it's a pure software thing
which will also apply to any other similar hardware and should therefore
be done in the core.

> > > + pm_runtime_get_sync(pspim->dev);

> > To repeat the feedback from last time do not open code runtime PM, use the
> > core support.

> Only set set the auto_runtime_pm of should be struct * spi_controller in probe function right ?

Yes.

> > > + list_for_each_entry(xfer, &m->transfers, transfer_list) {
> > > + if (!first_xfer)
> > > + first_xfer = xfer;
> > > + total_len += xfer->len;

> > To further repeat my prior feedback I can't see any reason why this driver
> > doesn't just use transfer_one().

> The FIFO only 16bytes for one action. I need push tx_data and pull rx_data as soon as possible.
> Use one message I can collect the data first and start transmitting
> It is more efficient than transfer_one at real case.

That doesn't mean it's a good idea to just duplicate the core code, that
means that you should look into improving the performance of the core
code so any optimisation benefits everyone and the driver doesn't end
up with some combination of bugs, missing features and reduced
performance in the open coded version. Having small FIFOs isn't at all
unusual so it seems unlikely that there's any need for anything here to
be device specific, and any benefits from copying all the data into a
linear buffer have got to be application specific, indeed it'll clearly
make things worse in some common cases. For example with something like
flash where we have frequent use of large transfers for data payloads
the data is already mostly in large buffers the overhead of copying
those buffers is going to be very noticable compared to any saving,
especially given that there's only two transfers. On the other end of
things when something like regmap is already linearising small writes
into single transfers then the copy is just pure overhead.

There's definitely scope for improving things here, the main one being
to pull advancing to the next transfer into spi_finalize_current_transfer()
which would benefit DMA as well, that don't introduce these overheads
and don't involve all this code duplication.

Attachments:

(No filename) (8.07 kB)
signature.asc (488.00 B)
Download all attachments

2021-11-11 13:41:50

On Wed, Nov 17, 2021 at 09:11:08AM +0000, Lh Kuo 郭力豪 wrote:

> The main function are as follows
>
> The sp7021_spi_mas_transfer_one is replace the transfer_one_message function.
>
> static int sp7021_spi_mas_transfer_one(struct spi_controller *ctlr,
> struct spi_device *spi, struct spi_transfer *xfer)
> {
> struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);
> u32 reg_temp = 0;
> unsigned long timeout = msecs_to_jiffies(1000);

I'm still not clear why this needs to be transfer_one_message() and not
just transfer_one()? The whole thing with copying everything into a
buffer is a bit confusing to me.

> The probe function is as follows.
>
> static int sp7021_spi_controller_probe(struct platform_device *pdev)
> {
> int ret;
> int mode;
> struct spi_controller *ctlr;
> struct sp7021_spi_ctlr *pspim;

This looks fine.

Attachments:

(No filename) (853.00 B)
signature.asc (488.00 B)
Download all attachments

2021-11-19 13:06:33

On Tue, Nov 23, 2021 at 12:09:54AM +0200, Andy Shevchenko wrote:
> On Mon, Nov 22, 2021 at 4:34 AM LH.Kuo <[email protected]> wrote:

> > +// slave only. usually called on driver remove

> Why is it so?
> Also find use of proper English grammar (capitalization, periods, etc.
> Ditto for all your comments.

Please don't go overboard on changes you're requesting, the important
thing with comments is that they're intelligible. People have different
levels of skill with English and that's totally fine, it's much better
that people feel able to write comments than that they stop doing so
because they are concerned about issues with their foreign language
skills.

> > + unsigned long flags;
> > + struct sp7021_spi_ctlr *pspim = dev;
> > + u32 fd_status = 0;
> > + unsigned int tx_len, rx_cnt, tx_cnt, total_len;
> > + bool isrdone = false;

> Reversed xmas tree order here and everywhere else.

Again, please don't go overboard - this isn't a general requirement for
kernel code, some parts of the kernel do want it but outside of those
areas it's not something that we should be asking for (and TBH even when
it is required you should explain what it is, it's not as easy to search
for as it could be). I certainly don't care.

> > + if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
> > + mode = SP7021_SLAVE_MODE;

> There is no need to check of_node for this call.

OTOH if we are checking it anyway it doesn't hurt to have all the
property reads in the check for of_node. Either way it doesn't
fundamentally matter.

Attachments:

(No filename) (1.57 kB)
signature.asc (488.00 B)
Download all attachments

2021-11-23 14:36:10

On Fri, Nov 26, 2021 at 11:36:29AM +0100, Philipp Zabel wrote:

> > pm_runtime_disable(&pdev->dev);

> I'm not sure if the SPI framework requires the spi_controller to be
> unregistered before hardware is powered off, maybe it is enough to call
> spi_controller_suspend() in the right place?

It would *probably* do the right thing but the expectation really is
that you'll unregister before making the controller stop working, that
should be much more robust..

Attachments:

(No filename) (463.00 B)
signature.asc (488.00 B)
Download all attachments

2021-11-29 00:37:41

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] devicetree: bindings SPI Add bindings doc for Sunplus SP7021

On Mon, Nov 22, 2021 at 10:33:33AM +0800, LH.Kuo wrote:
> Add devicetree bindings SPI Add bindings doc for Sunplus SP7021
>
> Signed-off-by: LH.Kuo <[email protected]>

Again, From and S-o-b must match.

> ---
> Changes in v3:
> - Addressed all comments from Mr. Mark Brown
> - Addressed all comments from Mr. Philipp Zabel
> - Addressed all comments from Mr. Rob Herring
> - remove spi transfer_one_message
>
> .../bindings/spi/spi-sunplus-sp7021.yaml | 95 ++++++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 96 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml b/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
> new file mode 100644
> index 0000000..5502f15
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (C) Sunplus Co., Ltd. 2021
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/spi-sunplus-sp7021.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sunplus sp7021 SPI controller
> +
> +allOf:
> + - $ref: "spi-controller.yaml"
> +
> +maintainers:
> + - lh.kuo <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - sunplus,sp7021-spi-controller

I think you can drop '-controller'.

> +
> + reg:
> + items:
> + - description: Base address and length of the SPI master registers
> + - description: Base address and length of the SPI slave registers

Drop 'Base address and length of '. The rest is sufficient.

> +
> + reg-names:
> + items:
> + - const: spi_master
> + - const: spi_slave

Drop 'spi_'.

> +
> + interrupt-names:
> + items:
> + - const: dma_w_intr
> + - const: mas_risc_intr
> + - const: slave_risc_intr

Drop '_intr', it's redundant.

> +
> + interrupts:
> + minItems: 3
> +
> + clocks:
> + maxItems: 1
> +
> + clocks-names:
> + items:
> + - const: sys_pll
> +
> + resets:
> + maxItems: 1
> +
> + pinctrl-names:
> + description:
> + A pinctrl state named "default" must be defined.
> + const: default
> +
> + pinctrl-0:
> + description:
> + A phandle to the default pinctrl state.
> +
> +required:
> + - compatible
> + - reg
> + - reg-names
> + - interrupts
> + - interrupt-names
> + - clocks
> + - clocks-names
> + - resets
> + - pinctrl-names
> + - pinctrl-0
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/sp-sp7021.h>
> + #include <dt-bindings/reset/sp-sp7021.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> + spi@9C002D80 {
> + compatible = "sunplus,sp7021-spi-controller";
> + reg = <0x9C002D80 0x80>, <0x9C002E00 0x80>;
> + reg-names = "spi_master", "spi_slave";
> + interrupt-parent = <&intc>;
> + interrupt-names = "dma_w_intr",
> + "mas_risc_intr",
> + "slave_risc_intr";
> + interrupts = <144 IRQ_TYPE_LEVEL_HIGH>,
> + <146 IRQ_TYPE_LEVEL_HIGH>,
> + <145 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&clkc SPI_COMBO_0>;
> + clocks-names = "sys_pll";
> + resets = <&rstc RST_SPI_COMBO_0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pins_spi0>;
> + };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 75fa7d5..88f3747 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18220,6 +18220,7 @@ SUNPLUS SPI CONTROLLER INTERFACE DRIVER
> M: LH Kuo <[email protected]>
> L: [email protected]
> S: Maintained
> +F: Documentation/devicetree/bindings/spi/spi-sunplus-sp7021.yaml
> F: drivers/spi/spi-sunplus-sp7021.c
>
> SUPERH
> --
> 2.7.4
>
>

2021-11-29 14:39:31

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

On Mon, Nov 29, 2021 at 8:20 AM Lh Kuo 郭力豪 <[email protected]> wrote:

> Feel sorry. I haven't found any devm PM API use in the SPI driver, and I didn't realize that PM function also has devm API. So I was confused before. I will move the pm_runtime_enable() after the devm_spi_register_controller() . I have rewritten the Probe and Remove functions as shown below.

Neither you found APIs for clock and reset, Try to grep for
devm_add_action_or_reset().

So, for PM it is probably good to leave it last, but you still have the issue.

> And sp7021_spi_controller driver is modified and the code cleaned more than -50 LOCs. If the Probe and Remove functions is OK I will start next submission.

No, it's not okay.. yet, but we are closer. See my comments above and below.

> Thanks for all comments
>
> static int sp7021_spi_controller_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct sp7021_spi_ctlr *pspim;
> struct spi_controller *ctlr;
> int mode;
> int ret;
>
> dev_info(dev, "sp7021_spi_controller_probe\n");
>
> mode = SP7021_MASTER_MODE;
> pdev->id = of_alias_get_id(pdev->dev.of_node, "sp_spi");
>
> if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
> mode = SP7021_SLAVE_MODE;
>
> if (mode == SP7021_SLAVE_MODE)
> ctlr = devm_spi_alloc_slave(dev, sizeof(*pspim));
> else
> ctlr = devm_spi_alloc_master(dev, sizeof(*pspim));
> if (!ctlr)
> return -ENOMEM;
>

> ctlr->dev.of_node = pdev->dev.of_node;

device_set_node()

> ctlr->bus_num = pdev->id;
> ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LSB_FIRST;
> ctlr->auto_runtime_pm = true;
> ctlr->prepare_message = sp7021_spi_controller_prepare_message;
> if (mode == SP7021_SLAVE_MODE) {
> ctlr->transfer_one = sp7021_spi_sla_transfer_one;
> ctlr->slave_abort = sp7021_spi_sla_abort;
> ctlr->flags = SPI_CONTROLLER_HALF_DUPLEX;
> } else {
> ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
> ctlr->min_speed_hz = 40000;
> ctlr->max_speed_hz = 25000000;
> ctlr->use_gpio_descriptors = true;
> ctlr->flags = SPI_CONTROLLER_MUST_RX | SPI_CONTROLLER_MUST_TX;
> ctlr->transfer_one = sp7021_spi_mas_transfer_one;
> }
> platform_set_drvdata(pdev, ctlr);
> pspim = spi_controller_get_devdata(ctlr);
> pspim->ctlr = ctlr;
> pspim->dev = dev;
> spin_lock_init(&pspim->lock);
> mutex_init(&pspim->buf_lock);
> init_completion(&pspim->isr_done);
> init_completion(&pspim->sla_isr);

> pspim->mas_base = devm_platform_ioremap_resource_byname(pdev, "master");
> pspim->sla_base = devm_platform_ioremap_resource_byname(pdev, "slave");

Where are the error checks?

> pspim->mas_irq = platform_get_irq_byname(pdev, "mas_risc");
> if (pspim->mas_irq < 0)
> return pspim->mas_irq;
>
> pspim->sla_irq = platform_get_irq_byname(pdev, "slave_risc");
> if (pspim->sla_irq < 0)
> return pspim->sla_irq;
>
> ret = devm_request_irq(dev, pspim->mas_irq, sp7021_spi_mas_irq,
> IRQF_TRIGGER_RISING, pdev->name, pspim);
> if (ret)
> return ret;
>
> ret = devm_request_irq(dev, pspim->sla_irq, sp7021_spi_sla_irq,
> IRQF_TRIGGER_RISING, pdev->name, pspim);
> if (ret)
> return ret;
>
> pspim->spi_clk = devm_clk_get(dev, NULL);
> if (IS_ERR(pspim->spi_clk))
> return dev_err_probe(dev, PTR_ERR(pspim->spi_clk), "clk get fail\n");
>
> pspim->rstc = devm_reset_control_get_exclusive(dev, NULL);
> if (IS_ERR(pspim->rstc))
> return dev_err_probe(dev, PTR_ERR(pspim->rstc), "rst get fail\n");

> ret = clk_prepare_enable(pspim->spi_clk);
> if (ret)
> return dev_err_probe(dev, ret, "failed to enable clk\n");
>
> ret = reset_control_deassert(pspim->rstc);
> if (ret) {
> dev_err_probe(dev, ret, "failed to deassert reset\n");
> goto err_free_reset;
> }

These two need to be wrapped as I explained above.

> ret = devm_spi_register_controller(dev, ctlr);
> pm_runtime_enable(dev);
> if (ret) {

> dev_err(dev, "spi_register_master fail\n");
> goto err_disable_pm_runtime;

pm_runtime_disable();
return dev_err_probe();

> }
>
> return ret;
>
> err_disable_pm_runtime:
> pm_runtime_disable(dev);
> err_free_reset:
> reset_control_assert(pspim->rstc);
> clk_disable_unprepare(pspim->spi_clk);
>
> return ret;
> }
>
> static int sp7021_spi_controller_remove(struct platform_device *pdev)
> {
> struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
> struct sp7021_spi_ctlr *pspim = spi_master_get_devdata(ctlr);

> spi_unregister_controller(ctlr);

Lukas already explained to you why this is wrong.

> pm_runtime_disable(&pdev->dev);
> pm_runtime_set_suspended(&pdev->dev);

> reset_control_assert(pspim->rstc);
> clk_disable_unprepare(pspim->spi_clk);

This has the same ordering issue as already discussed.

> return 0;
> }

--
With Best Regards,
Andy Shevchenko

2021-12-01 19:30:22

by Andy Shevchenko

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

On Tue, Nov 30, 2021 at 10:32 AM Lh Kuo 郭力豪 <[email protected]> wrote:

> > > ctlr->dev.of_node = pdev->dev.of_node;
> >
> > device_set_node()
>
> Is this funciton set as follows?
> device_set_node(&ctlr->dev, of_fwnode_handle(pdev->dev.fwnode));

Can you please do something?
For example, figuring out yourself (Elixir is a very good service for
that): https://elixir.bootlin.com/linux/latest/A/ident/device_set_node

...

> > > pspim->mas_base = devm_platform_ioremap_resource_byname(pdev, "master");
> > > pspim->sla_base = devm_platform_ioremap_resource_byname(pdev,
> > > "slave");
> >
> > Where are the error checks?

> The changes are as follows? is this correct?

Almost, but not enough. Please run checkpatch.

> pspim->mas_base = devm_platform_ioremap_resource_byname(pdev, "master");
> if (IS_ERR(pspim->mas_base)) {
> return dev_err_probe(dev, PTR_ERR(pspim->mas_base), "mas_base get fail\n");
> }
>
> pspim->sla_base = devm_platform_ioremap_resource_byname(pdev, "slave");
> if (IS_ERR(pspim->sla_base)) {
> return dev_err_probe(dev, PTR_ERR(pspim->sla_base), "sla_base get fail\n");
> }

...

> > > if (ret) {
> > > dev_err_probe(dev, ret, "failed to deassert reset\n");
> > > goto err_free_reset;
> > > }
> >
> > These two need to be wrapped as I explained above.
>
> I think these changes are depend on remove-function.

No, it's the other way around: ->remove() implementation depends on
these changes.

> These settings are as follows? is this correct?

No.

> pspim->spi_clk = devm_clk_get(dev, NULL);
> if (IS_ERR(pspim->spi_clk))
> return dev_err_probe(dev, PTR_ERR(pspim->spi_clk), "clk get fail\n");
>
> pspim->rstc = devm_reset_control_get_exclusive(dev, NULL);
> if (IS_ERR(pspim->rstc))
> return dev_err_probe(dev, PTR_ERR(pspim->rstc), "rst get fail\n");
>
> ret = clk_prepare_enable(pspim->spi_clk);
> if (ret)
> return dev_err_probe(dev, ret, "failed to enable clk\n");
>
> devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare,
> pspim->spi_clk);

Please, find other drivers as examples of how to do that and take care
about possible errors.

> ret = reset_control_deassert(pspim->rstc);
> if (ret)
> return dev_err_probe(dev, ret, "failed to deassert reset\n");
>
> devm_add_action_or_reset(dev, (void(*)(void *))reset_control_assert,
> pspim->rstc);

Ditto.

> ret = spi_register_controller(ctlr);

Read what Lukas said.

> pm_runtime_enable(dev);
> if (ret) {
> pm_runtime_disable(dev);
> return dev_err_probe(dev, ret, "spi_register_master fail\n");
> }
>
> return ret;
>
> }
>
> static int sp7021_spi_controller_remove(struct platform_device *pdev)
> {
> struct spi_controller *ctlr = dev_get_drvdata(&pdev->dev);
>
> pm_runtime_disable(&pdev->dev);
> pm_runtime_set_suspended(&pdev->dev);
>
> return 0;
> }

--
With Best Regards,
Andy Shevchenko