Re: [PATCH 0/5] mt8173 spi multiple devices support
Hi Mark, On Mon, 2015-10-19 at 20:27 +0100, Mark Brown wrote: > On Wed, Oct 14, 2015 at 11:23:30AM +0800, Leilk Liu wrote: > > This series are based on 4.3-rc1 and provide 5 patches to support > > mt8173 spi multiple devices. > > This doesn't apply against current code, please check and resend. Also > note that it looks like you've got the execute bit set on some of the > files. The code itself looks fine. OK, I will fix it. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/5] arm64: dts: spi bus dts support multiple devices
Hi Sascha,
On Wed, 2015-10-14 at 07:58 +0200, Sascha Hauer wrote:
> On Wed, Oct 14, 2015 at 11:23:35AM +0800, Leilk Liu wrote:
> > This patch support multiple devices for MT8173.
>
> The subject of this patch and also the above sentence should contain the
> board name this patch is changing so that the reader knows this is about
> a single board, and not arm64 in general.
>
OK, I'll fix it.
> Sascha
>
> >
> > Signed-off-by: Leilk Liu
> > ---
> > arch/arm64/boot/dts/mediatek/mt8173-evb.dts | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > index 04b38ed..1c8c407 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> > @@ -194,7 +194,7 @@
> > pinmux = ,
> > ,
> > ,
> > - ;
> > + ;
> > };
> > };
> > };
> > @@ -399,6 +399,7 @@
> > &spi {
> > pinctrl-names = "default";
> > pinctrl-0 = <&spi_pins_a>;
> > + cs-gpios = <&pio 72 0>;
> > mediatek,pad-select = <0>;
> > status = "okay";
> > };
> > --
> > 1.8.1.1.dirty
> >
> >
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/3] spis: mediatek: add bindings for Mediatek MT2712 soc platform
On Wed, 2018-09-26 at 17:33 -0500, Rob Herring wrote: > On Mon, Sep 17, 2018 at 10:19:20AM +0800, Leilk Liu wrote: > > This patch adds a DT binding documentation for the MT2712 soc. > > > > Signed-off-by: Leilk Liu > > --- > > .../devicetree/bindings/spi/spi-slave-mt27xx.txt | 32 > > > > 1 file changed, 32 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt > > b/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt > > new file mode 100644 > > index 000..09cb2c4 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/spi/spi-slave-mt27xx.txt > > @@ -0,0 +1,32 @@ > > +Binding for MTK SPI Slave controller > > + > > +Required properties: > > +- compatible: should be one of the following. > > +- mediatek,mt2712-spi-slave: for mt2712 platforms > > +- reg: Address and length of the register set for the device. > > +- interrupts: Should contain spi interrupt. > > +- clocks: phandles to input clocks. > > + It's clock gate, and should be <&infracfg CLK_INFRA_AO_SPI1>. > > +- clock-names: should be "spi" for the clock gate. > > + > > +Optional properties: > > +- assigned-clocks: it's mux clock, should be <&topckgen > > CLK_TOP_SPISLV_SEL>. > > +- assigned-clock-parents: parent of mux clock. > > + It's PLL, and should be on of the following. > > s/on/one/ > > With that fixed, > > Reviewed-by: Rob Herring Yes, it's a mistake, I'll fix it, thanks
Re: [PATCH 1/2] spi: mediatek: Add bindings for mediatek MT7629 soc platform
On Tue, 2018-11-20 at 11:28 +0100, Matthias Brugger wrote: > > On 20/11/2018 09:41, Leilk Liu wrote: > > This patch adds a DT binding documentation for the MT7629 soc. > > > > Signed-off-by: Leilk Liu > > --- > > .../devicetree/bindings/spi/spi-mt65xx.txt |1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > b/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > index 7940940..6cc4e87 100644 > > --- a/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > +++ b/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > @@ -6,6 +6,7 @@ Required properties: > > - mediatek,mt2712-spi: for mt2712 platforms > > - mediatek,mt6589-spi: for mt6589 platforms > > - mediatek,mt7622-spi: for mt7622 platforms > > +- mediatek,mt7629-spi: for mt7629 platforms > > That's ok, as you add support in the driver. As it actually doesn't change > anything in the driver itself you could just describe the binding as > something like: > " mediatek,mt7629-spi", "mediatek,mt7622-spi": for mt7629 platforms > > This way the driver will just probe the driver using the fallback (mt7622) > compatible, but if you realize in the future that you will need to distinguish > between the two SoCs, you can add a mt7629 compatible. > > Just as information for the future :) > Got it, thanks for your advice and information :) > Regards, > Matthias
Re: [PATCH 19/24] dt-bindings: spi: spi-mt65xx: add support for MT8516
On Tue, 2019-04-16 at 09:55 +0200, Matthias Brugger wrote: > > On 23/03/2019 22:16, Fabien Parent wrote: > > Add binding documentation of spi-mt65xx for MT8516 SoC. > > > > Signed-off-by: Fabien Parent > > --- > > Documentation/devicetree/bindings/spi/spi-mt65xx.txt | 1 + > > 1 file changed, 1 insertion(+) > > > > > applied to v5.1-next/dts64 > > Mark let me know if you want to take it through your tree and I drop the > patch. > > > diff --git a/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > b/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > index 69c356767cf8..69ac5976b952 100644 > > --- a/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > +++ b/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > @@ -10,6 +10,7 @@ Required properties: > > - mediatek,mt8135-spi: for mt8135 platforms > > - mediatek,mt8173-spi: for mt8173 platforms > > - mediatek,mt8183-spi: for mt8183 platforms > > +- "mediatek,mt8516-spi", "mediatek,mt2701-spi": for mt8516 platforms Hi Fabien, mt8516 SPI design comes from mt2712 and it's different from mt2701. Here it should compatible with mt2712. > > > > - #address-cells: should be 1. > > > > > > ___ > Linux-mediatek mailing list > linux-media...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
Re: [PATCH 19/24] dt-bindings: spi: spi-mt65xx: add support for MT8516
On Thu, 2019-04-18 at 10:05 +0200, Fabien Parent wrote: > On Tue, Apr 16, 2019 at 10:25 AM lei liu wrote: > > > > On Tue, 2019-04-16 at 09:55 +0200, Matthias Brugger wrote: > > > > > > On 23/03/2019 22:16, Fabien Parent wrote: > > > > Add binding documentation of spi-mt65xx for MT8516 SoC. > > > > > > > > Signed-off-by: Fabien Parent > > > > --- > > > > Documentation/devicetree/bindings/spi/spi-mt65xx.txt | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > > > > applied to v5.1-next/dts64 > > > > > > Mark let me know if you want to take it through your tree and I drop the > > > patch. > > > > > > > diff --git a/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > > > b/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > > > index 69c356767cf8..69ac5976b952 100644 > > > > --- a/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > > > +++ b/Documentation/devicetree/bindings/spi/spi-mt65xx.txt > > > > @@ -10,6 +10,7 @@ Required properties: > > > > - mediatek,mt8135-spi: for mt8135 platforms > > > > - mediatek,mt8173-spi: for mt8173 platforms > > > > - mediatek,mt8183-spi: for mt8183 platforms > > > > +- "mediatek,mt8516-spi", "mediatek,mt2701-spi": for mt8516 > > > > platforms > > Hi Fabien, > > mt8516 SPI design comes from mt2712 and it's different from mt2701. Here > > it should compatible with mt2712. > > Ok, thanks. I will retry with the mt2712 compatible. > Hi, I already send mt8516 spi patch and it's applied by Mark. Please don't do it again. Thanks. > > > > > > > > - #address-cells: should be 1. > > > > > > > > > > > > > > ___ > > > Linux-mediatek mailing list > > > linux-media...@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-mediatek > > > >
Re: [PATCH] spi: mediatek: fix spi incorrect endian usage and remove redundant clock
On Tue, 2015-08-18 at 14:19 +0200, Jonas Gorski wrote:
> Hi,
>
> On Tue, Aug 18, 2015 at 12:53 PM, Leilk Liu wrote:
> > This patch fixes incorrect endian usage, removes redundant
> > clock in prepare_hardware/unprepare_hardware and revises
> > coding styles.
> >
> > Signed-off-by: Leilk Liu
> >
> > ---
> > Change in this patch:
> > 1. fix incorrect endian usage on big-endian system.
> > 2. delete redundant clock in prepare/unprepare_hardware.
> > 3. revise coding styles.
>
> The usual philosophy is to have one change per patch, so this might be
> better as three patches. But this is Mark's call. Since the driver
> isn't yet in Linus' tree, it might be a-ok to mix style improvements
> and actual fixes, but as soon as it landed in Linus' tree you need to
> keep them separate, so fixes can be easily backported.
>
OK, I'll divide this patch to three patches on next version.
> Regarding the content ...
>
> > ---
> > drivers/spi/spi-mt65xx.c | 163
> > +--
> > include/linux/platform_data/spi-mt65xx.h | 2 -
> > 2 files changed, 69 insertions(+), 96 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> > index 519f50c..a9da887 100644
> > --- a/drivers/spi/spi-mt65xx.c
> > +++ b/drivers/spi/spi-mt65xx.c
> > @@ -694,6 +662,7 @@ static int mtk_spi_resume(struct device *dev)
> > if (!pm_runtime_suspended(dev)) {
> > ret = clk_prepare_enable(mdata->spi_clk);
> > if (ret < 0)
> > + dev_err(dev, "failed to enable spi_clk (%d)\n",
> > ret);
> > return ret;
>
> You need to add braces here, else the "return ret" isn't covered by
> the if () anymore and you always return at this place.
>
Yes.I'll fix it.
> > }
> >
> > @@ -720,8 +689,14 @@ static int mtk_spi_runtime_resume(struct device *dev)
> > {
> > struct spi_master *master = dev_get_drvdata(dev);
> > struct mtk_spi *mdata = spi_master_get_devdata(master);
> > + int ret;
> > +
> > + ret = clk_prepare_enable(mdata->spi_clk);
> > + if (ret < 0)
> > + dev_err(dev, "failed to enable spi_clk (%d)\n", ret);
> > + return ret;
>
> Same here. Although at least here it should be harmless, as
> clk_prepare_enable doesn't return > 0.
>
Yes.I'll fix it too.
> >
> > - return clk_prepare_enable(mdata->spi_clk);
> > + return 0;
> > }
> > #endif /* CONFIG_PM */
> >
>
>
> Jonas
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] spi: Mediatek: fix endian warnings
Hello Mark, This patch is applied, should I append a new patch or you rollback it? Thanks. On Wed, 2015-08-12 at 16:45 +0200, Jonas Gorski wrote: > Hi, > > On Tue, Aug 11, 2015 at 12:43 PM, Leilk Liu wrote: > > This patch fixes endian warnings detected by sparse: > > - sparse: incorrect type in argument 1 (different base types) > > expected unsigned int [unsigned] val > > got restricted __le32 [usertype] > > - sparse: incorrect type in argument 1 (different base types) > > expected unsigned int [unsigned] val > > got restricted __le32 [usertype] > > This doesn't "fix" the warning, it only hides the warning and leaves > the actual issue unfixed. > > > > > Signed-off-by: Leilk Liu > > --- > > drivers/spi/spi-mt65xx.c | 6 -- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c > > index 4676b01..ae645fa 100644 > > --- a/drivers/spi/spi-mt65xx.c > > +++ b/drivers/spi/spi-mt65xx.c > > @@ -359,9 +359,11 @@ static void mtk_spi_setup_dma_addr(struct spi_master > > *master, > > struct mtk_spi *mdata = spi_master_get_devdata(master); > > > > if (mdata->tx_sgl) > > - writel(cpu_to_le32(xfer->tx_dma), mdata->base + > > SPI_TX_SRC_REG); > > + writel((__force u32)cpu_to_le32(xfer->tx_dma), > > + mdata->base + SPI_TX_SRC_REG); > > if (mdata->rx_sgl) > > - writel(cpu_to_le32(xfer->rx_dma), mdata->base + > > SPI_RX_DST_REG); > > + writel((__force u32)cpu_to_le32(xfer->rx_dma), > > + mdata->base + SPI_RX_DST_REG); > > The issue here is that writel already does a cpu_to_le32 conversion, > so the extra cpu_to_le32 calls are actually bogus and need to be > removed. Else it will do a double conversion on big endian systems, > resulting in the data being written in big endian. > > > Jonas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 3/3] arm64: dts: Add spi bus dts
> > + spi: spi@1100a000 {
> > + compatible = "mediatek,mt8173-spi";
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + reg = <0 0x1100a000 0 0x1000>;
> > + interrupts = ;
> > + clocks = <&topckgen CLK_TOP_SPI_SEL>, <&topckgen
> > CLK_TOP_SYSPLL3_D2>;
> > + clock-names = "spi-clk", "parent-clk";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&spi_pins_a>;
> > + mediatek,pad-select = <0>;
>
> The pinctl and pad-select fields are board specific.
> Please move to mt8173-evb.dtsi, along with status = "okay";
>
OK, I will fix it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [v5,2/3] spi: mediatek: Add spi bus for Mediatek MT8173
Hi,
> > +#include
> > +#include
> > +#include
> > +#include
>
> Since you are using readl/writel, please import linux/io.h as well (it
> is implicitly imported by spi/spi.h, but better be safe...)
>
OK, I'll fix it.
> > +#include
> > +#include
> > +#include
> > +
> > +#define SPI_CMD_ACT_OFFSET0
> > +#define SPI_CMD_RESUME_OFFSET 1
> > +#define SPI_CMD_CPHA_OFFSET 8
> > +#define SPI_CMD_CPOL_OFFSET 9
> > +#define SPI_CMD_TXMSBF_OFFSET 12
> > +#define SPI_CMD_RXMSBF_OFFSET 13
> > +#define SPI_CMD_RX_ENDIAN_OFFSET 14
> > +#define SPI_CMD_TX_ENDIAN_OFFSET 15
>
> It feels error-prone that you are defining these offsets here, then
> redefining the bits just below.
>
> Looking at the code, I think it's better if you remove these
> SPI_CMD_*_OFFSET defines, and only use the BIT(x) defines below.
>
OK, I will remove those defines.
> > +#define SPI_CMD_RST BIT(2)
> > +#define SPI_CMD_PAUSE_EN BIT(4)
> > +#define SPI_CMD_DEASSERT BIT(5)
> > +#define SPI_CMD_CPHA BIT(8)
> > +#define SPI_CMD_CPOL BIT(9)
> > +#define SPI_CMD_RX_DMA BIT(10)
> > +#define SPI_CMD_TX_DMA BIT(11)
> > +#define SPI_CMD_TXMSBF BIT(12)
> > +#define SPI_CMD_RXMSBF BIT(13)
> > +#define SPI_CMD_RX_ENDIANBIT(14)
> > +#define SPI_CMD_TX_ENDIANBIT(15)
> > +#define SPI_CMD_FINISH_IEBIT(16)
> > +#define SPI_CMD_PAUSE_IE BIT(17)
> > +
> > +
> > +struct mtk_spi_compatible {
> > + u32 need_pad_sel;
> > + u32 must_tx;
>
> Since the quirks are true/false, define these as bool, and remove
> MTK_SPI_QUIRK_PAD_SELECT/MTK_SPI_QUIRK_MUST_TX. Move the comment here
> too.
>
OK. I will fix it.
> > +};
> > +
> > +static const struct mtk_spi_compatible mt6589_compat = {
> > + .need_pad_sel = 0,
> > + .must_tx = 0,
> > +};
> > +
> > +static const struct mtk_spi_compatible mt8135_compat = {
> > + .need_pad_sel = 0,
> > + .must_tx = 0,
> > +};
>
> You don't need to set these explicitly to 0/false.
>
OK, I will fix it.
> > +
> > +static const struct mtk_spi_compatible mt8173_compat = {
> > + .need_pad_sel = MTK_SPI_QUIRK_PAD_SELECT,
> > + .must_tx = MTK_SPI_QUIRK_MUST_TX,
>
> Then you can set those as "true".
>
OK, I will fix it.
> > +};
> > +
> > +/*
> > + * A piece of default chip info unless the platform
> > + * supplies it.
> > + */
> > +static const struct mtk_chip_config mtk_default_chip_info = {
> > + .rx_mlsb = 1,
> > + .tx_mlsb = 1,
> > + .tx_endian = 0,
> > + .rx_endian = 0,
> > +};
> > +
> > +
> > + trans = list_first_entry(&msg->transfers, struct spi_transfer,
> > +transfer_list);
> > + if (trans->cs_change == 0) {
>
> !trans->cs_change
>
OK, I will fix it.
> > + mdata->state = MTK_SPI_IDLE;
> > + mtk_spi_reset(mdata);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int mtk_spi_unprepare_hardware(struct spi_master *master)
> > +{
> > + struct mtk_spi *mdata = spi_master_get_devdata(master);
> > +
> > + clk_disable_unprepare(mdata->spi_clk);
> > +
> > + return 0;
> > +}
>
> In this case, prepare_hardware/unprepare_hardware is redundant with
> pm_runtime (apart from the cs_change check, possibly).
>
> If PM_RUNTIME is disabled, leave the clock enabled all the time, if
> not enable/disable in pm_runtime functions (as you already do)
>
> See
> https://git.kernel.org/cgit/linux/kernel/git/broonie/spi.git/commit/?id=db91841b58f9ad0ecbb81ed0fa496c3a1b67fd63
> "spi/omap100k: Convert to runtime PM" for an example (it's one of the
> last driver that used prepare/unprepare calls, and got converted to
> pm_runtime)
>
OK, I'll fix it.
> > +static int mtk_spi_prepare_message(struct spi_master *master,
> > + struct spi_message *msg)
> > +{
> > + u32 reg_val;
> > + u8 cpha, cpol;
> > + struct mtk_chip_config *chip_config;
> > + struct spi_device *spi = msg->spi;
> > + struct mtk_spi *mdata = spi_master_get_devdata(master);
> > +
> > + cpha = spi->mode & SPI_CPHA ? 1 : 0;
> > + cpol = spi->mode & SPI_CPOL ? 1 : 0;
> > +
> > + reg_val = readl(mdata->base + SPI_CMD_REG);
> > + reg_val &= ~(SPI_CMD_CPHA | SPI_CMD_CPOL);
> > + reg_val |= (cpha << SPI_CMD_CPHA_OFFSET);
> > + reg_val |= (cpol << SPI_CMD_CPOL_OFFSET);
>
> This can actually be simplified a bit by using
> SPI_CMD_CPHA/SPI_CMD_CPOL instead.
>
OK, I will fix it.
> > + writel(reg_val, mdata->base + SPI_CMD_REG);
> > +
> > +
> > +static void mtk_spi_prepare_transfer(struct spi_master *master,
> > +struct spi_transfer *xfer)
> > +{
> > + u32 spi_clk_hz, div, high_time, low_time, holdtime,
Re: [PATCH v2] spi: mediatek: single device does not require cs_gpios
On Mon, 2015-11-09 at 12:14 +0800, Nicolas Boichat wrote:
> When only one device is present, it is not necessary to specify
> cs_gpios, as the CS line can be controlled by the hardware
> module.
>
> Without this patch, older device tree bindings used before
> 37457607 "spi: mediatek: mt8173 spi multiple devices support"
> would cause a panic on boot. This fixes the crash, and
> re-introduces backward compatibility.
>
> Signed-off-by: Nicolas Boichat
Acked-by: Leilk Liu
> ---
>
> v2: Use gpio_is_valid()
>
> Applies on top of broonie/spi.git/for-next.
>
> drivers/spi/spi-mt65xx.c | 26 ++
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> index 563954a..7840067 100644
> --- a/drivers/spi/spi-mt65xx.c
> +++ b/drivers/spi/spi-mt65xx.c
> @@ -410,7 +410,7 @@ static int mtk_spi_setup(struct spi_device *spi)
> if (!spi->controller_data)
> spi->controller_data = (void *)&mtk_default_chip_info;
>
> - if (mdata->dev_comp->need_pad_sel)
> + if (mdata->dev_comp->need_pad_sel && gpio_is_valid(spi->cs_gpio))
> gpio_direction_output(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
>
> return 0;
> @@ -632,13 +632,23 @@ static int mtk_spi_probe(struct platform_device *pdev)
> goto err_put_master;
> }
>
> - for (i = 0; i < master->num_chipselect; i++) {
> - ret = devm_gpio_request(&pdev->dev, master->cs_gpios[i],
> - dev_name(&pdev->dev));
> - if (ret) {
> - dev_err(&pdev->dev,
> - "can't get CS GPIO %i\n", i);
> - goto err_put_master;
> + if (!master->cs_gpios && master->num_chipselect > 1) {
> + dev_err(&pdev->dev,
> + "cs_gpios not specified and num_chipselect >
> 1\n");
> + ret = -EINVAL;
> + goto err_put_master;
> + }
> +
> + if (master->cs_gpios) {
> + for (i = 0; i < master->num_chipselect; i++) {
> + ret = devm_gpio_request(&pdev->dev,
> + master->cs_gpios[i],
> + dev_name(&pdev->dev));
> + if (ret) {
> + dev_err(&pdev->dev,
> + "can't get CS GPIO %i\n", i);
> + goto err_put_master;
> + }
> }
> }
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] spi: mediatek: revise mtk_spi_probe() failure flow
On Tue, 2015-11-24 at 19:12 +0100, Matthias Brugger wrote:
>
> On 24/11/15 03:38, Leilk Liu wrote:
> > This patch revises failure flow while pm_runtime_enable().
>
> Please write a proper commit message explaining what this patch does.
>
OK, thanks!
> >
> > Signed-off-by: Leilk Liu
> > ---
> > drivers/spi/spi-mt65xx.c | 15 ---
> > 1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> > index 6c1a96e..00a36da 100644
> > --- a/drivers/spi/spi-mt65xx.c
> > +++ b/drivers/spi/spi-mt65xx.c
> > @@ -607,7 +607,8 @@ static int mtk_spi_probe(struct platform_device *pdev)
> > ret = clk_set_parent(mdata->sel_clk, mdata->parent_clk);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "failed to clk_set_parent (%d)\n", ret);
> > - goto err_disable_clk;
> > + clk_disable_unprepare(mdata->spi_clk);
> > + goto err_put_master;
> > }
> >
> > clk_disable_unprepare(mdata->spi_clk);
> > @@ -617,7 +618,7 @@ static int mtk_spi_probe(struct platform_device *pdev)
> > ret = devm_spi_register_master(&pdev->dev, master);
> > if (ret) {
> > dev_err(&pdev->dev, "failed to register master (%d)\n", ret);
> > - goto err_put_master;
> > + goto err_disable_runtime_pm;
> > }
> >
> > if (mdata->dev_comp->need_pad_sel) {
> > @@ -626,14 +627,14 @@ static int mtk_spi_probe(struct platform_device *pdev)
> > "pad_num does not match num_chipselect(%d !=
> > %d)\n",
> > mdata->pad_num, master->num_chipselect);
> > ret = -EINVAL;
> > - goto err_put_master;
> > + goto err_disable_runtime_pm;
> > }
> >
> > if (!master->cs_gpios && master->num_chipselect > 1) {
> > dev_err(&pdev->dev,
> > "cs_gpios not specified and num_chipselect >
> > 1\n");
> > ret = -EINVAL;
> > - goto err_put_master;
> > + goto err_disable_runtime_pm;
> > }
> >
> > if (master->cs_gpios) {
> > @@ -644,7 +645,7 @@ static int mtk_spi_probe(struct platform_device *pdev)
> > if (ret) {
> > dev_err(&pdev->dev,
> > "can't get CS GPIO %i\n", i);
> > - goto err_put_master;
> > + goto err_disable_runtime_pm;
> > }
> > }
> > }
> > @@ -652,8 +653,8 @@ static int mtk_spi_probe(struct platform_device *pdev)
> >
> > return 0;
> >
> > -err_disable_clk:
> > - clk_disable_unprepare(mdata->spi_clk);
> > +err_disable_runtime_pm:
> > + pm_runtime_disable(&pdev->dev);
> > err_put_master:
> > spi_master_put(master);
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] spi: mediatek: revise mtk_spi_probe() failure flow
On Tue, 2015-11-24 at 13:04 +, Mark Brown wrote: > On Tue, Nov 24, 2015 at 10:33:24AM +0800, lei liu wrote: > > On Sat, 2015-11-21 at 13:39 +, Mark Brown wrote: > > > On Fri, Nov 20, 2015 at 10:21:19AM +0800, Leilk Liu wrote: > > > > > This patch revises failure flow while pm_runtime_enable(). > > > > Why? This also doesn't apply against current code, please check and > > > resend. > > > I don't know. I can git am this patch to spi/for-next. I will resend it, > > please help to apply it again, thanks. > > If you don't know why we should make this change then we may as well > just not make it? OK. I will write a proper commit message to explain what this patch does. Thanks. > _ > __ > Linux-mediatek mailing list > linux-media...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/4] spi: mediatek: revise mtk_spi_probe() failure flow
On Sat, 2015-11-21 at 13:39 +, Mark Brown wrote:
> On Fri, Nov 20, 2015 at 10:21:19AM +0800, Leilk Liu wrote:
> > This patch revises failure flow while pm_runtime_enable().
>
> Why? This also doesn't apply against current code, please check and
> resend.
I don't know. I can git am this patch to spi/for-next. I will resend it,
please help to apply it again, thanks.
> >
> > Signed-off-by: Leilk Liu
> > ---
> > drivers/spi/spi-mt65xx.c | 15 ---
> > 1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/spi/spi-mt65xx.c b/drivers/spi/spi-mt65xx.c
> > index 6c1a96e..00a36da 100644
> > --- a/drivers/spi/spi-mt65xx.c
> > +++ b/drivers/spi/spi-mt65xx.c
> > @@ -607,7 +607,8 @@ static int mtk_spi_probe(struct platform_device *pdev)
> > ret = clk_set_parent(mdata->sel_clk, mdata->parent_clk);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "failed to clk_set_parent (%d)\n", ret);
> > - goto err_disable_clk;
> > + clk_disable_unprepare(mdata->spi_clk);
> > + goto err_put_master;
> > }
> >
> > clk_disable_unprepare(mdata->spi_clk);
> > @@ -617,7 +618,7 @@ static int mtk_spi_probe(struct platform_device *pdev)
> > ret = devm_spi_register_master(&pdev->dev, master);
> > if (ret) {
> > dev_err(&pdev->dev, "failed to register master (%d)\n", ret);
> > - goto err_put_master;
> > + goto err_disable_runtime_pm;
> > }
> >
> > if (mdata->dev_comp->need_pad_sel) {
> > @@ -626,14 +627,14 @@ static int mtk_spi_probe(struct platform_device *pdev)
> > "pad_num does not match num_chipselect(%d !=
> > %d)\n",
> > mdata->pad_num, master->num_chipselect);
> > ret = -EINVAL;
> > - goto err_put_master;
> > + goto err_disable_runtime_pm;
> > }
> >
> > if (!master->cs_gpios && master->num_chipselect > 1) {
> > dev_err(&pdev->dev,
> > "cs_gpios not specified and num_chipselect >
> > 1\n");
> > ret = -EINVAL;
> > - goto err_put_master;
> > + goto err_disable_runtime_pm;
> > }
> >
> > if (master->cs_gpios) {
> > @@ -644,7 +645,7 @@ static int mtk_spi_probe(struct platform_device *pdev)
> > if (ret) {
> > dev_err(&pdev->dev,
> > "can't get CS GPIO %i\n", i);
> > - goto err_put_master;
> > + goto err_disable_runtime_pm;
> > }
> > }
> > }
> > @@ -652,8 +653,8 @@ static int mtk_spi_probe(struct platform_device *pdev)
> >
> > return 0;
> >
> > -err_disable_clk:
> > - clk_disable_unprepare(mdata->spi_clk);
> > +err_disable_runtime_pm:
> > + pm_runtime_disable(&pdev->dev);
> > err_put_master:
> > spi_master_put(master);
> >
> > --
> > 1.7.9.5
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/3] spis: mediatek: add spi slave for Mediatek MT2712
Hi Mark,
Thanks for your comments.
On Tue, 2018-09-18 at 09:30 -0700, Mark Brown wrote:
> On Mon, Sep 17, 2018 at 10:19:21AM +0800, Leilk Liu wrote:
>
> This looks overall pretty good, a few smallish comments below:
>
> Please use subject lines matching the style for the subsystem. This
> makes it easier for people to identify relevant patches.
>
OK, I'll fix it, thanks.
> > if SPI_SLAVE
> > +config SPI_SLAVE_MT27XX
> > + tristate "MediaTek SPI slave device"
> > + depends on ARCH_MEDIATEK || COMPILE_TEST
> > + help
> > + This selects the MediaTek(R) SPI slave device driver.
> > + If you want to use MediaTek(R) SPI slave interface,
> > + say Y or M here.If you are not sure, say N.
> > + SPI slave drivers for Mediatek MT27XX series ARM SoCs.
> >
> > config SPI_SLAVE_TIME
>
> This is a driver not a slave implementation, it should be with the other
> drivers in the Kconfig. The slave implementations are for the
> functionality that uses the drivers, not the drivers themselves.
>
OK, I'll fix it, thanks.
> > + /* set the tx/rx endian */
> > +#ifdef __LITTLE_ENDIAN
> > + reg_val &= ~SPIS_TX_ENDIAN;
> > + reg_val &= ~SPIS_RX_ENDIAN;
> > +#else
> > + reg_val |= SPIS_TX_ENDIAN;
> > + reg_val |= SPIS_RX_ENDIAN;
> > +#endif
> > + writel(reg_val, mdata->base + SPIS_CFG_REG);
>
> This seems weird - it looks like it's changing the endianess of the
> protocol based on the endianness the processor is running in. What's
> going on here? I'd expect the driver to be just treating everything as
> a byte stream and letting the protocol driver handle the endianness
> issues, otherwise we might end up with double converions.
>
OK, I'll set the endian of SPI the same with the processor. Thanks.
> > + xfer->tx_dma = dma_map_single(dev, (void *)xfer->tx_buf,
> > + xfer->len, DMA_TO_DEVICE);
>
> Why is there a cast to void here? That's usually a sign that there's a
> type safety issue, the whole point with void is that it's compatible
> with any other pointer.
>
tx_buf is a const void*, here it need a void * for the dma mapping.
And I'll remove void * from dma_map_single((void *)rx_buf).
Thanks.
> > +static irqreturn_t mtk_spi_slave_interrupt(int irq, void *dev_id)
> > +{
> > + struct spi_controller *ctlr = dev_id;
> > + struct mtk_spi_slave *mdata = spi_controller_get_devdata(ctlr);
> > + struct spi_transfer *trans = mdata->cur_transfer;
> > + u32 int_status, reg_val, cnt, remainder;
> > +
> > + int_status = readl(mdata->base + SPIS_IRQ_ST_REG);
> > + writel(int_status, mdata->base + SPIS_IRQ_CLR_REG);
> > +
> > + if (!trans)
> > + return IRQ_HANDLED;
>
> Are you sure that this is the right thing to do if we get a spurious
> interrupt - the normal thing would be to return IRQ_NONE, possibly
> logging a warning as well?
>
OK, it should reture IRQ_NONE here, thanks.
> > + if (int_status & CMD_INVALID_ST)
> > + dev_err(&ctlr->dev, "cmd invalid\n");
>
> If there's an interrupt that doesn't have any of the interrupt status
> flags set I'd expect to see a warning and probably IRQ_NONE being
> returned. That way if the interrupt line is shared we work correctly
> and if something goes wrong and the interrupt gets stuck on then the
> core interrupt code's error handling can kick in.
OK, I'll fix it, thanks.
Re: [spi:topic/mtk 2/2] drivers/spi/spi-mt65xx.c:362:24: sparse: incorrect type in argument 1 (different base types)
Hello Mark,
On Fri, 2015-08-07 at 22:33 +0800, kbuild test robot wrote:
> tree: git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi topic/mtk
> head: a568231f463225eb31593f71446a267a03ae0528
> commit: a568231f463225eb31593f71446a267a03ae0528 [2/2] spi: mediatek: Add spi
> bus for Mediatek MT8173
> reproduce:
> # apt-get install sparse
> git checkout a568231f463225eb31593f71446a267a03ae0528
> make ARCH=x86_64 allmodconfig
> make C=1 CF=-D__CHECK_ENDIAN__
>
>
I use these commands and also find another waring:
../drivers/spi/spi-mt65xx.c:589:25: warning: incorrect type in argument
1 (different address spaces)
../drivers/spi/spi-mt65xx.c:589:25: expected void const *ptr
../drivers/spi/spi-mt65xx.c:589:25: got void [noderef] *base
../drivers/spi/spi-mt65xx.c:590:36: warning: incorrect type in argument
1 (different address spaces)
../drivers/spi/spi-mt65xx.c:590:36: expected void const *ptr
../drivers/spi/spi-mt65xx.c:590:36: got void [noderef] *base
vi drivers/spi/spi-mt65xx.c +589
588 mdata->base = devm_ioremap_resource(&pdev->dev, res);
589 if (IS_ERR(mdata->base)) {
590 ret = PTR_ERR(mdata->base);
591 goto err_put_master;
592 }
should I also fix them?
> sparse warnings: (new ones prefixed by >>)
>
> >> drivers/spi/spi-mt65xx.c:362:24: sparse: incorrect type in argument 1
> >> (different base types)
>drivers/spi/spi-mt65xx.c:362:24:expected unsigned int [unsigned] val
>drivers/spi/spi-mt65xx.c:362:24:got restricted __le32 [usertype]
>
>drivers/spi/spi-mt65xx.c:364:24: sparse: incorrect type in argument 1
> (different base types)
>drivers/spi/spi-mt65xx.c:364:24:expected unsigned int [unsigned] val
>drivers/spi/spi-mt65xx.c:364:24:got restricted __le32 [usertype]
>
> >> drivers/spi/spi-mt65xx.c:734:24: sparse: symbol 'mtk_spi_driver' was not
> >> declared. Should it be static?
>
> Please review and possibly fold the followup patch.
>
> vim +362 drivers/spi/spi-mt65xx.c
>
>346mult_delta =
> mtk_spi_get_mult_delta(mdata->tx_sgl_len);
>347mdata->xfer_len = mdata->tx_sgl_len -
> mult_delta;
>348mdata->tx_sgl_len = mult_delta;
>349} else if (mdata->rx_sgl_len) {
>350mult_delta =
> mtk_spi_get_mult_delta(mdata->rx_sgl_len);
>351mdata->xfer_len = mdata->rx_sgl_len -
> mult_delta;
>352mdata->rx_sgl_len = mult_delta;
>353}
>354}
>355
>356static void mtk_spi_setup_dma_addr(struct spi_master *master,
>357 struct spi_transfer *xfer)
>358{
>359struct mtk_spi *mdata = spi_master_get_devdata(master);
>360
>361if (mdata->tx_sgl)
> > 362writel(cpu_to_le32(xfer->tx_dma), mdata->base +
> SPI_TX_SRC_REG);
>363if (mdata->rx_sgl)
>364writel(cpu_to_le32(xfer->rx_dma), mdata->base +
> SPI_RX_DST_REG);
>365}
>366
>367static int mtk_spi_fifo_transfer(struct spi_master *master,
>368 struct spi_device *spi,
>369 struct spi_transfer *xfer)
>370{
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] dt-bindings: spi: update bindings for MT8192 SoC
On Tue, 2020-07-21 at 10:48 +0100, Mark Brown wrote: > On Tue, Jul 21, 2020 at 10:48:19AM +0800, Leilk Liu wrote: > > From: "leilk.liu" > > > > Add a DT binding documentation for the MT8192 soc. > > I'd expect to see a matching driver patch. OK,I'll send patch v2. Thanks.
Re: [PATCH 2/3] spi: mediatek: support adjust register define
On Tue, 2017-06-06 at 19:57 +0100, Mark Brown wrote: > On Fri, Jun 02, 2017 at 03:18:42PM +0800, Leilk Liu wrote: > > > + /* some IC design adjust register define */ > > + bool adjust_reg; > > Can we have a name that's more specific to the particular quirk please? > The current name will get confusing if some future chip also needs > slightly different register settings. > thanks for your suggestion, I'll fix it. > Otherwise this looks good.
