Re: [PATCH v5 14/24] clk: mediatek: Add MT8192 imp i2c wrapper clock support

[Yingjoe Chen]

On Mon, 2020-11-09 at 10:03 +0800, Weiyi Lu wrote:
> Add MT8192 imp i2c wrapper clock provider
> 
> Signed-off-by: Weiyi Lu 
> ---
>  drivers/clk/mediatek/Kconfig   |   6 ++
>  drivers/clk/mediatek/Makefile  |   1 +
>  drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c | 119 
> +
>  3 files changed, 126 insertions(+)
>  create mode 100644 drivers/clk/mediatek/clk-mt8192-imp_iic_wrap.c
> 
> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
> index eb549f8..8acc7d6 100644
> --- a/drivers/clk/mediatek/Kconfig
> +++ b/drivers/clk/mediatek/Kconfig
> @@ -515,6 +515,12 @@ config COMMON_CLK_MT8192_IMGSYS
>   help
> This driver supports MediaTek MT8192 imgsys and imgsys2 clocks.
>  
> +config COMMON_CLK_MT8192_IMP_IIC_WRAP
> + bool "Clock driver for MediaTek MT8192 imp_iic_wrap"
> + depends on COMMON_CLK_MT8192
> + help
> +   This driver supports MediaTek MT8192 imp_iic_wrap clocks.
> +
>  config COMMON_CLK_MT8516
>   bool "Clock driver for MediaTek MT8516"
>   depends on ARCH_MEDIATEK || COMPILE_TEST

<...>

> +
> +static struct platform_driver clk_mt8192_imp_iic_wrap_drv = {
> + .probe = mtk_clk_simple_probe,

Good to have this generic probe function. Now several mtk clk drivers
are just a few data.

But this series still add >10 configs for mt8192 clock drivers. Why do
we need separate configs for clocks of different domain? I don't think
they need lots of resource. We should review the rationale and reduce
the numbers. 


Joe.C

Re: [PATCH v4 19/34] clk: mediatek: Add MT8192 imp i2c wrapper c clock support

[Yingjoe Chen]

On Thu, 2020-10-22 at 20:37 +0800, Weiyi Lu wrote:
> Add MT8192 imp i2c wrapper c clock provider
> 
> Signed-off-by: Weiyi Lu 
> ---
>  drivers/clk/mediatek/Kconfig |  6 +++
>  drivers/clk/mediatek/Makefile|  1 +
>  drivers/clk/mediatek/clk-mt8192-imp_iic_wrap_c.c | 62 
> 
>  3 files changed, 69 insertions(+)
>  create mode 100644 drivers/clk/mediatek/clk-mt8192-imp_iic_wrap_c.c
> 
> diff --git a/drivers/clk/mediatek/Kconfig b/drivers/clk/mediatek/Kconfig
> index 99b0168..a0eb76d 100644
> --- a/drivers/clk/mediatek/Kconfig
> +++ b/drivers/clk/mediatek/Kconfig
> @@ -491,6 +491,12 @@ config COMMON_CLK_MT8192_IMGSYS2
>   help
> This driver supports MediaTek MT8192 imgsys2 clocks.
>  
> +config COMMON_CLK_MT8192_IMP_IIC_WRAP_C
> + bool "Clock driver for MediaTek MT8192 imp_iic_wrap_c"
> + depends on COMMON_CLK_MT8192
> + help
> +   This driver supports MediaTek MT8192 imp_iic_wrap_c clocks.
> +
>  config COMMON_CLK_MT8516
>   bool "Clock driver for MediaTek MT8516"
>   depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index 012a01a..8aac821 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -69,5 +69,6 @@ obj-$(CONFIG_COMMON_CLK_MT8192_CAMSYS_RAWB) += 
> clk-mt8192-cam_rawb.o
>  obj-$(CONFIG_COMMON_CLK_MT8192_CAMSYS_RAWC) += clk-mt8192-cam_rawc.o
>  obj-$(CONFIG_COMMON_CLK_MT8192_IMGSYS) += clk-mt8192-img.o
>  obj-$(CONFIG_COMMON_CLK_MT8192_IMGSYS2) += clk-mt8192-img2.o
> +obj-$(CONFIG_COMMON_CLK_MT8192_IMP_IIC_WRAP_C) += clk-mt8192-imp_iic_wrap_c.o
>  obj-$(CONFIG_COMMON_CLK_MT8516) += clk-mt8516.o
>  obj-$(CONFIG_COMMON_CLK_MT8516_AUDSYS) += clk-mt8516-aud.o
> diff --git a/drivers/clk/mediatek/clk-mt8192-imp_iic_wrap_c.c 
> b/drivers/clk/mediatek/clk-mt8192-imp_iic_wrap_c.c
> new file mode 100644
> index 000..e7a0033
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-mt8192-imp_iic_wrap_c.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (c) 2020 MediaTek Inc.
> +// Author: Weiyi Lu 
> +
> +#include 
> +#include 
> +
> +#include "clk-mtk.h"
> +#include "clk-gate.h"
> +
> +#include 
> +
> +static const struct mtk_gate_regs imp_iic_wrap_c_cg_regs = {
> + .set_ofs = 0xe08,
> + .clr_ofs = 0xe04,
> + .sta_ofs = 0xe00,
> +};
> +
> +#define GATE_IMP_IIC_WRAP_C(_id, _name, _parent, _shift) 
> \
> + GATE_MTK_FLAGS(_id, _name, _parent, _iic_wrap_c_cg_regs, _shift,
> \
> + _clk_gate_ops_setclr, CLK_OPS_PARENT_ENABLE)
> +
> +static const struct mtk_gate imp_iic_wrap_c_clks[] = {
> + GATE_IMP_IIC_WRAP_C(CLK_IMP_IIC_WRAP_C_I2C10, "imp_iic_wrap_c_i2c10", 
> "infra_i2c0", 0),
> + GATE_IMP_IIC_WRAP_C(CLK_IMP_IIC_WRAP_C_I2C11, "imp_iic_wrap_c_i2c11", 
> "infra_i2c0", 1),
> + GATE_IMP_IIC_WRAP_C(CLK_IMP_IIC_WRAP_C_I2C12, "imp_iic_wrap_c_i2c12", 
> "infra_i2c0", 2),
> + GATE_IMP_IIC_WRAP_C(CLK_IMP_IIC_WRAP_C_I2C13, "imp_iic_wrap_c_i2c13", 
> "infra_i2c0", 3),
> +};
> +
> +static int clk_mt8192_imp_iic_wrap_c_probe(struct platform_device *pdev)
> +{
> + struct clk_onecell_data *clk_data;
> + struct device_node *node = pdev->dev.of_node;
> + int r;
> +
> + clk_data = mtk_alloc_clk_data(CLK_IMP_IIC_WRAP_C_NR_CLK);
> + if (!clk_data)
> + return -ENOMEM;
> +
> + r = mtk_clk_register_gates(node, imp_iic_wrap_c_clks, 
> ARRAY_SIZE(imp_iic_wrap_c_clks),
> + clk_data);
> + if (r)
> + return r;
> +
> + return of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
> +}
> +
> +static const struct of_device_id of_match_clk_mt8192_imp_iic_wrap_c[] = {
> + { .compatible = "mediatek,mt8192-imp_iic_wrap_c", },
> + {}
> +};

It seems these mt8192-imp_iic_wrap_* drivers are very similar.
I think it make more sense to use 1 single driver to provide them, ie:

+static const struct of_device_id of_match_clk_mt8192_imp_iic_wrap[] = {
+   { .compatible = "mediatek,mt8192-imp_iic_wrap_c", imp_iic_wrap_c_clks},
+   { .compatible = "mediatek,mt8192-imp_iic_wrap_e", imp_iic_wrap_e_clks},
+   { .compatible = "mediatek,mt8192-imp_iic_wrap_n", imp_iic_wrap_n_clks},

+   {}
+};

Maybe other clk drivers can be merged to the same driverl.

Joe.C

Re: [PATCH v3 5/6] spi: spi-mtk-nor: support 36bit dma addressing

[Yingjoe Chen]

On Fri, 2020-09-25 at 14:54 +0800, Ikjoon Jang wrote:
> This patch enables 36bit dma address support to spi-mtk-nor.
> Currently this is enabled only for mt8192-nor.
> 
> Signed-off-by: Ikjoon Jang 
> ---
> 
> (no changes since v1)
> 
>  drivers/spi/spi-mtk-nor.c | 18 +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 8dbafee7f431..35205635ed42 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -78,6 +78,8 @@
>  #define MTK_NOR_REG_DMA_FADR 0x71c
>  #define MTK_NOR_REG_DMA_DADR 0x720
>  #define MTK_NOR_REG_DMA_END_DADR 0x724
> +#define MTK_NOR_REG_DMA_DADR_HB  0x738
> +#define MTK_NOR_REG_DMA_END_DADR_HB  0x73c
>  
>  /* maximum bytes of TX in PRG mode */
>  #define MTK_NOR_PRG_MAX_SIZE 6
> @@ -106,6 +108,7 @@ struct mtk_nor {
>   unsigned int spi_freq;
>   bool wbuf_en;
>   bool has_irq;
> + bool high_dma;
>   struct completion op_done;
>  };
>  
> @@ -305,6 +308,11 @@ static int mtk_nor_dma_exec(struct mtk_nor *sp, u32 
> from, unsigned int length,
>   writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
>   writel(dma_addr + length, sp->base + MTK_NOR_REG_DMA_END_DADR);
>  
> + if (sp->high_dma) {
> + writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
> + writel((dma_addr + length) >> 32, sp->base + 
> MTK_NOR_REG_DMA_END_DADR_HB);
> + }
> +

Maybe use upper_32_bits() ?


>   if (sp->has_irq) {
>   reinit_completion(>op_done);
>   mtk_nor_rmw(sp, MTK_NOR_REG_IRQ_EN, MTK_NOR_IRQ_DMA, 0);
> @@ -635,7 +643,8 @@ static const struct spi_controller_mem_ops 
> mtk_nor_mem_ops = {
>  };
>  
>  static const struct of_device_id mtk_nor_match[] = {
> - { .compatible = "mediatek,mt8173-nor" },
> + { .compatible = "mediatek,mt8192-nor", .data = (void *)36 },
> + { .compatible = "mediatek,mt8173-nor", .data = (void *)32 },
>   { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, mtk_nor_match);
> @@ -647,6 +656,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
>   void __iomem *base;
>   struct clk *spi_clk, *ctlr_clk;
>   int ret, irq;
> + unsigned long dma_bits;
>  
>   base = devm_platform_ioremap_resource(pdev, 0);
>   if (IS_ERR(base))
> @@ -660,6 +670,12 @@ static int mtk_nor_probe(struct platform_device *pdev)
>   if (IS_ERR(ctlr_clk))
>   return PTR_ERR(ctlr_clk);
>  
> + dma_bits = (unsigned long)of_device_get_match_data(>dev);
> + if (dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(dma_bits))) {
> + dev_err(>dev, "failed to set dma mask(%lu)\n", dma_bits);
> + return -EINVAL;
> + }
> +

As said in previous version. I don't see any place enable high_dma, so I
think this patch won't set >32bits for anychip. We need something like:

sp->hidh_dma = dma_bits > 32;

Am I missing anything?

Joe.C

Re: [PATCH v2 4/5] spi: spi-mtk-nor: support 36bit dma addressing to mediatek

[Yingjoe Chen]

On Fri, 2020-09-18 at 16:31 +0800, Ikjoon Jang wrote:
> This patch enables 36bit dma address support to spi-mtk-nor.
> Currently 36bit dma addressing is enabled only for mt8192-nor.
> 
> Signed-off-by: Ikjoon Jang 
> ---
> 
> (no changes since v1)
> 
>  drivers/spi/spi-mtk-nor.c | 19 ++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index e14798a6e7d0..99dd5dca744e 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -78,6 +78,8 @@
>  #define MTK_NOR_REG_DMA_FADR 0x71c
>  #define MTK_NOR_REG_DMA_DADR 0x720
>  #define MTK_NOR_REG_DMA_END_DADR 0x724
> +#define MTK_NOR_REG_DMA_DADR_HB  0x738
> +#define MTK_NOR_REG_DMA_END_DADR_HB  0x73c
>  
>  #define MTK_NOR_PRG_MAX_SIZE 6
>  // Reading DMA src/dst addresses have to be 16-byte aligned
> @@ -102,6 +104,7 @@ struct mtk_nor {
>   unsigned int spi_freq;
>   bool wbuf_en;
>   bool has_irq;
> + bool high_dma;
>   struct completion op_done;
>  };
>  
> @@ -291,6 +294,11 @@ static int read_dma(struct mtk_nor *sp, u32 from, 
> unsigned int length,
>   writel(dma_addr, sp->base + MTK_NOR_REG_DMA_DADR);
>   writel(dma_addr + length, sp->base + MTK_NOR_REG_DMA_END_DADR);
>  
> + if (sp->high_dma) {
> + writel(dma_addr >> 32, sp->base + MTK_NOR_REG_DMA_DADR_HB);
> + writel((dma_addr + length) >> 32, sp->base + 
> MTK_NOR_REG_DMA_END_DADR_HB);
> + }
> +
>   if (sp->has_irq) {
>   reinit_completion(>op_done);
>   mtk_nor_rmw(sp, MTK_NOR_REG_IRQ_EN, MTK_NOR_IRQ_DMA, 0);
> @@ -594,7 +602,8 @@ static const struct spi_controller_mem_ops 
> mtk_nor_mem_ops = {
>  };
>  
>  static const struct of_device_id mtk_nor_match[] = {
> - { .compatible = "mediatek,mt8173-nor" },
> + { .compatible = "mediatek,mt8192-nor", .data = (void *)36 },
> + { .compatible = "mediatek,mt8173-nor", .data = (void *)32 },
>   { /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, mtk_nor_match);
> @@ -607,6 +616,7 @@ static int mtk_nor_probe(struct platform_device *pdev)
>   u8 *buffer;
>   struct clk *spi_clk, *ctlr_clk;
>   int ret, irq;
> + unsigned long dma_bits;
>  
>   base = devm_platform_ioremap_resource(pdev, 0);
>   if (IS_ERR(base))
> @@ -623,6 +633,13 @@ static int mtk_nor_probe(struct platform_device *pdev)
>   buffer = devm_kmalloc(>dev,
> MTK_NOR_BOUNCE_BUF_SIZE + MTK_NOR_DMA_ALIGN,
> GFP_KERNEL);
> +
> + dma_bits = (unsigned long)of_device_get_match_data(>dev);
> + if (dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(dma_bits))) {
> + dev_err(>dev, "failed to set dma mask(%lu)\n", dma_bits);
> + return -EINVAL;
> + }
> +

Do we need to set sp->high_dma when we have >32bits DMA?

Joe.C

Re: [PATCH] i2c: mediatek: Fix generic definitions for bus frequencies

[Yingjoe Chen]

On Sat, 2020-09-12 at 13:57 +0800, qii.w...@mediatek.com wrote:
> From: Qii Wang 
> 
> The master code needs to being sent when the speed is more than
> I2C_MAX_FAST_MODE_PLUS_FREQ instead of
> I2C_MAX_HIGH_SPEED_MODE_FREQ. Fix it.

This was introduced by "i2c: drivers: Use generic definitions for bus
frequencies". You should have
Fixes: 90224e6468e1 ("i2c: drivers: Use generic definitions for bus
frequencies")

You can have my reviewed by after you add fixes.
Reviewed-by: Yingjoe Chen 

Joe.C

> 
> Signed-off-by: Qii Wang 
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index efc1404..0cbdfbe 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -681,8 +681,8 @@ static int mtk_i2c_calculate_speed(struct mtk_i2c *i2c, 
> unsigned int clk_src,
>   unsigned int cnt_mul;
>   int ret = -EINVAL;
>  
> - if (target_speed > I2C_MAX_FAST_MODE_PLUS_FREQ)
> - target_speed = I2C_MAX_FAST_MODE_PLUS_FREQ;
> + if (target_speed > I2C_MAX_HIGH_SPEED_MODE_FREQ)
> + target_speed = I2C_MAX_HIGH_SPEED_MODE_FREQ;
>  
>   max_step_cnt = mtk_i2c_max_step_cnt(target_speed);
>   base_step_cnt = max_step_cnt;
> @@ -759,7 +759,7 @@ static int mtk_i2c_set_speed(struct mtk_i2c *i2c, 
> unsigned int parent_clk)
>   for (clk_div = 1; clk_div <= max_clk_div; clk_div++) {
>   clk_src = parent_clk / clk_div;
>  
> - if (target_speed > I2C_MAX_FAST_MODE_FREQ) {
> + if (target_speed > I2C_MAX_FAST_MODE_PLUS_FREQ) {
>   /* Set master code speed register */
>   ret = mtk_i2c_calculate_speed(i2c, clk_src,
> I2C_MAX_FAST_MODE_FREQ,

Re: [PATCH v17 1/3] dt-bindings: Add bindings for Mediatek matrix keypad

[Yingjoe Chen]

On Wed, 2020-08-12 at 15:13 -0700, Dmitry Torokhov wrote:
> Hi,
> 
> On Tue, Aug 11, 2020 at 09:47:23AM +0800, Yingjoe Chen wrote:
> > Hi,
> > 
> > 
> > On Mon, 2020-08-10 at 14:40 +0800, Fengping Yu wrote:
> > > From: "fengping.yu" 
> > > 
> > > This patch add devicetree bindings for Mediatek matrix keypad driver.
> > > 
> > > Signed-off-by: fengping.yu 
> > > ---
> > >  .../devicetree/bindings/input/mtk-kpd.yaml| 87 +++
> > >  1 file changed, 87 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.yaml 
> > > b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> > > new file mode 100644
> > > index ..d74dd8a6fbde
> > 
> > <...>
> > 
> > 
> > > +  keypad,num-columns:
> > > +description: Number of column lines connected to the keypad 
> > > controller,
> > > +it is not equal to PCB columns number, instead you should add 
> > > required value
> > > +for each IC. If not specified, the default value is 1.
> > > +
> > > +  keypad,num-rows:
> > > +description: Number of row lines connected to the keypad controller, 
> > > it is
> > > +not equal to PCB rows number, instead you should add required value 
> > > for each IC.
> > > +If not specified, the default value is 1.
> > 
> > Your source code can't really handle dts without rows/columns
> > properties. Also, the default value doesn't make any sense. No IC will
> > have rows or columns set to 1.
> > 
> > Since these are IC specified, not board specified, I think you should
> > just have the correct numbers in driver.
> 
> It is actually property of board to decide how many keys it wants to
> wire up. In extreme case it will be a single key, i.e. number of rows
> and columns will indeed be 1.
> 
> Thanks.
> 

From the binding "it is not equal to PCB columns number, instead you
should add required value for each IC."
Driver code use this to calculate bit position in register, which is IC
dependent.

Joe.C

Re: [PATCH v17 1/3] dt-bindings: Add bindings for Mediatek matrix keypad

[Yingjoe Chen]

Hi,


On Mon, 2020-08-10 at 14:40 +0800, Fengping Yu wrote:
> From: "fengping.yu" 
> 
> This patch add devicetree bindings for Mediatek matrix keypad driver.
> 
> Signed-off-by: fengping.yu 
> ---
>  .../devicetree/bindings/input/mtk-kpd.yaml| 87 +++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.yaml 
> b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> new file mode 100644
> index ..d74dd8a6fbde

<...>


> +  keypad,num-columns:
> +description: Number of column lines connected to the keypad controller,
> +it is not equal to PCB columns number, instead you should add required 
> value
> +for each IC. If not specified, the default value is 1.
> +
> +  keypad,num-rows:
> +description: Number of row lines connected to the keypad controller, it 
> is
> +not equal to PCB rows number, instead you should add required value for 
> each IC.
> +If not specified, the default value is 1.

Your source code can't really handle dts without rows/columns
properties. Also, the default value doesn't make any sense. No IC will
have rows or columns set to 1.

Since these are IC specified, not board specified, I think you should
just have the correct numbers in driver.

Joe.C

Re: [v2,5/6] reset-controller: ti: Introduce force-update method

[Yingjoe Chen]

On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> Introduce force-update method for assert and deassert interface,
> which force the write operation in case the read already happens
> to return the correct value.
> 
> Signed-off-by: Crystal Guo 
> ---
>  drivers/reset/reset-ti-syscon.c | 15 +--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index 1c74bcb9a6c3..f4baf78afd14 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -57,6 +57,7 @@ struct ti_syscon_reset_data {
>   struct ti_syscon_reset_control *controls;
>   unsigned int nr_controls;
>   bool assert_deassert_together;
> + bool update_force;
>  };
>  
>  #define to_ti_syscon_reset_data(rcdev)   \
> @@ -90,7 +91,10 @@ static int ti_syscon_reset_assert(struct 
> reset_controller_dev *rcdev,
>   mask = BIT(control->assert_bit);
>   value = (control->flags & ASSERT_SET) ? mask : 0x0;
>  
> - return regmap_update_bits(data->regmap, control->assert_offset, mask, 
> value);
> + if (data->update_force)
> + return regmap_write_bits(data->regmap, control->assert_offset, 
> mask, value);
> + else
> + return regmap_update_bits(data->regmap, control->assert_offset, 
> mask, value);
>  }
>  
>  /**
> @@ -121,7 +125,10 @@ static int ti_syscon_reset_deassert(struct 
> reset_controller_dev *rcdev,
>   mask = BIT(control->deassert_bit);
>   value = (control->flags & DEASSERT_SET) ? mask : 0x0;
>  
> - return regmap_update_bits(data->regmap, control->deassert_offset, mask, 
> value);
> + if (data->update_force)
> + return regmap_write_bits(data->regmap, 
> control->deassert_offset, mask, value);
> + else
> + return regmap_update_bits(data->regmap, 
> control->deassert_offset, mask, value);
>  }
>  
>  /**
> @@ -223,6 +230,10 @@ static int ti_syscon_reset_probe(struct platform_device 
> *pdev)
>   data->assert_deassert_together = true;
>   else
>   data->assert_deassert_together = false;
> + if (of_property_read_bool(np, "update-force"))
> + data->update_force = true;
> + else
> + data->update_force = false;

You are using 'force-update' in commit message, and I think that a
better one.
Please change it if we still need this one

Joe.C

Re: [v2,3/6] dt-binding: reset-controller: ti: add generic-reset to compatible

[Yingjoe Chen]

On Tue, 2020-08-04 at 10:15 +0200, Philipp Zabel wrote:
> On Mon, 2020-08-03 at 14:15 +0800, Crystal Guo wrote:
> > The TI syscon reset controller provides a common reset management,
> > and should be suitable for other SOCs. Add compatible "generic-reset",
> > which denotes to use a common reset-controller driver.
> > 
> > Signed-off-by: Crystal Guo 
> > ---
> >  Documentation/devicetree/bindings/reset/ti-syscon-reset.txt | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt 
> > b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > index d551161ae785..e36d3631eab2 100644
> > --- a/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > +++ b/Documentation/devicetree/bindings/reset/ti-syscon-reset.txt
> > @@ -25,6 +25,7 @@ Required properties:
> > "ti,k2l-pscrst"
> > "ti,k2hk-pscrst"
> > "ti,syscon-reset"
> > +   "generic-reset", "ti,syscon-reset"
> >   - #reset-cells: Should be 1. Please see the reset consumer 
> > node below
> >   for usage details
> >   - ti,reset-bits   : Contains the reset control register information
> > -- 
> > 2.18.0
> 
> My understanding is that it would be better to add a mtk specific
> compatible instead of adding this "generic-reset", especially since we
> can't guarantee this binding will be considered generic in the future.
> I think there is nothing wrong with specifying
>   compatible = "mtk,your-compatible", "ti,syscon-reset";
> in your device tree if your hardware is indeed compatible with the
> specified "ti,syscon-reset" binding, but I may be wrong: Therefore,
> please add devicet...@vger.kernel.org to Cc: for binding changes.
> 

Hi Philipp,

This would work.
But having "ti" in mtk dts raise alarm for some people inside and
outside of MTK. It would save us some explanation if we could use a more
generic name.

Joe.C

Re: [PATCH v16 1/3] dt-bindings: Add keypad devicetree documentation

[Yingjoe Chen]

Hi,

Summary should specified this patch is for MediaTek:

dt-bindings: add MediaTek keypad devicetree documentation


On Mon, 2020-07-27 at 19:45 +0800, Fengping Yu wrote:
> From: "fengping.yu" 

> Add Mediatek matrix keypad dt-bindings doc as yaml schema.
> 
> Signed-off-by: fengping.yu 
> ---
>  .../devicetree/bindings/input/mtk-kpd.yaml| 96 +++
>  1 file changed, 96 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/mtk-kpd.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/mtk-kpd.yaml 
> b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> new file mode 100644
> index ..3bf09e7395d0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/mtk-kpd.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +version: 1
> +
> +$id: http://devicetree.org/schemas/input/mtk-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek's Keypad Controller device tree bindings
> +
> +maintainer:
> +  - Fengping Yu 
> +
> +description: |
> +  Mediatek's Keypad controller is used to interface a SoC with a matrix-type
> +  keypad device. The keypad controller supports multiple row and column 
> lines.
> +  A key can be placed at each intersection of a unique row and a unique 
> column.
> +  The keypad controller can sense a key-press and key-release and report the
> +  event using a interrupt to the cpu.
> +
> +properties:
> +  compatible:
> +oneOf:
> +  - const: "mediatek,mt6779-keypad"
> +  - const: "mediatek,mt6873-keypad"
> +
> +  clock-names:
> +description: Names of the clocks listed in clocks property in the same 
> order
> +maxItems: 1

Please list the clock-names required. In this case, 'kpd'


> +
> +  clocks:
> +description: Must contain one entry, for the module clock
> +refs: devicetree/bindings/clocks/clock-bindings.txt for details.
> +
> +  interrupts:
> +description: A single interrupt specifier
> +maxItems: 1
> +
> +  linux,keymap:
> +description: The keymap for keys as described in the binding document
> +refs: devicetree/bindings/input/matrix-keymap.txt
> +minItems: 1
> +maxItems: 16

Why is this max at 16?


> +
> +  pinctrl-0:
> +description: Specify pin control groups used for this controller
> +refs: devicetree/bindings/pinctrl/pinctrl-bindings.txt
> +
> +  pinctrl-names:
> +description: Names for optional pin modes
> +maxItems: 1

I'm not sure we should list pinctrl here. 


> +
> +  reg:
> +description: The base address of the Keypad register bank
> +maxItems: 1
> +
> +  wakeup-source:
> +description: use any event on keypad as wakeup event
> +type: boolean
> +
> +  keypad,num-columns:
> +description: Number of column lines connected to the keypad controller,
> +it is not equal to PCB columns number, instead you should add required 
> value
> +for each IC
> +
> +  keypad,num-rows:
> +description: Number of row lines connected to the keypad controller, it 
> is
> +not equal to PCB rows number, instead you should add required value for 
> each IC

So the values depend on IC HW. These are not listed as required. Can
your driver works without them? Default value?

> +
> +  mediatek,debounce-us:
> +description: Debounce interval in microseconds, if not specified, the 
> default
> +value is 16000
> +maximum: 256000
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - linux,keymap
> +  - pinctrl
> +  - clocks
> +  - clock-names
> +
> +examples:
> +  - |
> +
> +  keypad: kp@1001 {

This should be 'keypad' or instead of kp.

Joe.C

> +compatible = "mediatek,mt6779-keypad";
> +reg = <0 0x1001 0 0x1000>;
> +linux,keymap = < MATRIX_KEY(0x00, 0x00, KEY_VOLUMEDOWN) >;
> +interrupts = ;
> +clocks = <>;
> +clock-names = "kpd";
> +pinctrl-names = "default";
> +pinctrl-0 = <_gpios_def_cfg>;
> +  };

Re: [PATCH v2 2/4] i2c: mediatek: Add access to more than 8GB dram in i2c driver

[Yingjoe Chen]

On Tue, 2020-07-28 at 20:30 +0800, Qii Wang wrote:
> Newer MTK chip support more than 8GB of dram. Replace support_33bits
> with more general dma_max_support and remove mtk_i2c_set_4g_mode.
> 
> Signed-off-by: Qii Wang 

Qii,

After you remove I2C_DMA_4G_MODE Matthias mentioned, you can have:

Reviewed-by: Yingjoe Chen 

Joe.C

Re: [PATCH 1/4] i2c: mediatek: Add apdma sync in i2c driver

[Yingjoe Chen]

On Wed, 2020-07-22 at 20:31 +0800, Qii Wang wrote:
> With the apdma remove hand-shake signal, it need to keep i2c and
> apdma in sync manually.
> 

Looks good to me,

Reviewed-by: Yingjoe Chen 


Just a reminder, we have another patch 'i2c: mediatek: Add to support
continuous mode' under review now. Please remember to update OFFSET_CON
access code in that patch.

Joe.C



> Signed-off-by: Qii Wang 
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 23 ---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index deef69e..e6b984a 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -48,6 +48,9 @@
>  
>  #define I2C_DMA_CON_TX   0x
>  #define I2C_DMA_CON_RX   0x0001
> +#define I2C_DMA_ASYNC_MODE   0x0004
> +#define I2C_DMA_SKIP_CONFIG  0x0010
> +#define I2C_DMA_DIR_CHANGE   0x0200
>  #define I2C_DMA_START_EN 0x0001
>  #define I2C_DMA_INT_FLAG_NONE0x
>  #define I2C_DMA_CLR_FLAG 0x
> @@ -205,6 +208,7 @@ struct mtk_i2c_compatible {
>   unsigned char timing_adjust: 1;
>   unsigned char dma_sync: 1;
>   unsigned char ltiming_adjust: 1;
> + unsigned char apdma_sync: 1;
>  };
>  
>  struct mtk_i2c_ac_timing {
> @@ -311,6 +315,7 @@ struct i2c_spec_values {
>   .timing_adjust = 1,
>   .dma_sync = 0,
>   .ltiming_adjust = 0,
> + .apdma_sync = 0,
>  };
>  
>  static const struct mtk_i2c_compatible mt6577_compat = {
> @@ -324,6 +329,7 @@ struct i2c_spec_values {
>   .timing_adjust = 0,
>   .dma_sync = 0,
>   .ltiming_adjust = 0,
> + .apdma_sync = 0,
>  };
>  
>  static const struct mtk_i2c_compatible mt6589_compat = {
> @@ -337,6 +343,7 @@ struct i2c_spec_values {
>   .timing_adjust = 0,
>   .dma_sync = 0,
>   .ltiming_adjust = 0,
> + .apdma_sync = 0,
>  };
>  
>  static const struct mtk_i2c_compatible mt7622_compat = {
> @@ -350,6 +357,7 @@ struct i2c_spec_values {
>   .timing_adjust = 0,
>   .dma_sync = 0,
>   .ltiming_adjust = 0,
> + .apdma_sync = 0,
>  };
>  
>  static const struct mtk_i2c_compatible mt8173_compat = {
> @@ -362,6 +370,7 @@ struct i2c_spec_values {
>   .timing_adjust = 0,
>   .dma_sync = 0,
>   .ltiming_adjust = 0,
> + .apdma_sync = 0,
>  };
>  
>  static const struct mtk_i2c_compatible mt8183_compat = {
> @@ -375,6 +384,7 @@ struct i2c_spec_values {
>   .timing_adjust = 1,
>   .dma_sync = 1,
>   .ltiming_adjust = 1,
> + .apdma_sync = 0,
>  };
>  
>  static const struct of_device_id mtk_i2c_of_match[] = {
> @@ -798,6 +808,7 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, 
> struct i2c_msg *msgs,
>   u16 start_reg;
>   u16 control_reg;
>   u16 restart_flag = 0;
> + u16 dma_sync = 0;
>   u32 reg_4g_mode;
>   u8 *dma_rd_buf = NULL;
>   u8 *dma_wr_buf = NULL;
> @@ -851,10 +862,16 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, 
> struct i2c_msg *msgs,
>   mtk_i2c_writew(i2c, num, OFFSET_TRANSAC_LEN);
>   }
>  
> + if (i2c->dev_comp->apdma_sync) {
> + dma_sync = I2C_DMA_SKIP_CONFIG | I2C_DMA_ASYNC_MODE;
> + if (i2c->op == I2C_MASTER_WRRD)
> + dma_sync |= I2C_DMA_DIR_CHANGE;
> + }
> +
>   /* Prepare buffer data to start transfer */
>   if (i2c->op == I2C_MASTER_RD) {
>   writel(I2C_DMA_INT_FLAG_NONE, i2c->pdmabase + OFFSET_INT_FLAG);
> - writel(I2C_DMA_CON_RX, i2c->pdmabase + OFFSET_CON);
> + writel(I2C_DMA_CON_RX | dma_sync, i2c->pdmabase + OFFSET_CON);
>  
>   dma_rd_buf = i2c_get_dma_safe_msg_buf(msgs, 1);
>   if (!dma_rd_buf)
> @@ -877,7 +894,7 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, 
> struct i2c_msg *msgs,
>   writel(msgs->len, i2c->pdmabase + OFFSET_RX_LEN);
>   } else if (i2c->op == I2C_MASTER_WR) {
>   writel(I2C_DMA_INT_FLAG_NONE, i2c->pdmabase + OFFSET_INT_FLAG);
> - writel(I2C_DMA_CON_TX, i2c->pdmabase + OFFSET_CON);
> + writel(I2C_DMA_CON_TX | dma_sync, i2c->pdmabase + OFFSET_CON);
>  
>   dma_wr_buf = i2c_get_dma_safe_msg_buf(msgs, 1);
>   if (!dma_wr_buf)
> @@ -900,7 +917,7 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, 
> struct i2c_msg *msgs,
>   writel(msgs->len, i2c->pdmabase + OFFSET_TX_LEN);
>   } else {
>   writel(I2C_DMA_CLR_FLAG, i2c->pdmabase + OFFSET_INT_FLAG);
> - writel(I2C_DMA_CLR_FLAG, i2c->pdmabase + OFFSET_CON);
> + writel(I2C_DMA_CLR_FLAG | dma_sync, i2c->pdmabase + OFFSET_CON);
>  
>   dma_wr_buf = i2c_get_dma_safe_msg_buf(msgs, 1);
>   if (!dma_wr_buf)

Re: [PATCH 2/4] i2c: mediatek: Support DMA mask range over 33-bits

[Yingjoe Chen]

On Wed, 2020-07-22 at 20:31 +0800, Qii Wang wrote:
> Replace 'support_33bits with 'dma_max_support' for DMA mask
> operation, and replace 'mtk_i2c_set_4g_mode' with 'upper_32_bits'.

This doesn't explain why we need this patch. How about:

Newer MTK chip support more than 8GB of dram. Replace support_33bits
with more general dma_max_support.


> 
> Signed-off-by: Qii Wang 
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 37 +
>  1 file changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index e6b984a..e475877 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -209,6 +209,7 @@ struct mtk_i2c_compatible {
>   unsigned char dma_sync: 1;
>   unsigned char ltiming_adjust: 1;
>   unsigned char apdma_sync: 1;
> + unsigned char max_dma_support;

support_33bits is no longer used. Please remove it.

Joe.C

Re: [PATCH v6 03/10] iommu/mediatek: Use a u32 flags to describe different HW features

[Yingjoe Chen]

On Fri, 2020-07-03 at 12:41 +0800, Chao Hao wrote:
> Given the fact that we are adding more and more plat_data bool values,
> it would make sense to use a u32 flags register and add the appropriate
> macro definitions to set and check for a flag present.
> No functional change.
> 
> Cc: Yong Wu 
> Suggested-by: Matthias Brugger 
> Signed-off-by: Chao Hao 
> Reviewed-by: Matthias Brugger 
> ---
>  drivers/iommu/mtk_iommu.c | 28 +---
>  drivers/iommu/mtk_iommu.h |  7 +--
>  2 files changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 88d3df5b91c2..40ca564d97af 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -100,6 +100,15 @@
>  #define MTK_M4U_TO_LARB(id)  (((id) >> 5) & 0xf)
>  #define MTK_M4U_TO_PORT(id)  ((id) & 0x1f)
>  
> +#define HAS_4GB_MODE BIT(0)
> +/* HW will use the EMI clock if there isn't the "bclk". */
> +#define HAS_BCLK BIT(1)
> +#define HAS_VLD_PA_RNG   BIT(2)
> +#define RESET_AXIBIT(3)
> +
> +#define MTK_IOMMU_HAS_FLAG(pdata, _x) \
> + pdata)->flags) & (_x)) == (_x))
> +
>  struct mtk_iommu_domain {
>   struct io_pgtable_cfg   cfg;
>   struct io_pgtable_ops   *iop;
> @@ -563,7 +572,8 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data 
> *data)
>upper_32_bits(data->protect_base);
>   writel_relaxed(regval, data->base + REG_MMU_IVRP_PADDR);
>  
> - if (data->enable_4GB && data->plat_data->has_vld_pa_rng) {
> + if (data->enable_4GB &&
> + MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_VLD_PA_RNG)) {
>   /*
>* If 4GB mode is enabled, the validate PA range is from
>* 0x1__ to 0x1__. here record bit[32:30].
> @@ -573,7 +583,7 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data 
> *data)
>   }
>   writel_relaxed(0, data->base + REG_MMU_DCM_DIS);
>  
> - if (data->plat_data->reset_axi) {
> + if (MTK_IOMMU_HAS_FLAG(data->plat_data, RESET_AXI)) {
>   /* The register is called STANDARD_AXI_MODE in this case */
>   writel_relaxed(0, data->base + REG_MMU_MISC_CTRL);
>   }
> @@ -618,7 +628,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>  
>   /* Whether the current dram is over 4GB */
>   data->enable_4GB = !!(max_pfn > (BIT_ULL(32) >> PAGE_SHIFT));
> - if (!data->plat_data->has_4gb_mode)
> + if (!MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE))
>   data->enable_4GB = false;
>  
>   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -631,7 +641,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>   if (data->irq < 0)
>   return data->irq;
>  
> - if (data->plat_data->has_bclk) {
> + if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_BCLK)) {
>   data->bclk = devm_clk_get(dev, "bclk");
>   if (IS_ERR(data->bclk))
>   return PTR_ERR(data->bclk);
> @@ -763,23 +773,19 @@ static const struct dev_pm_ops mtk_iommu_pm_ops = {
>  
>  static const struct mtk_iommu_plat_data mt2712_data = {
>   .m4u_plat = M4U_MT2712,
> - .has_4gb_mode = true,
> - .has_bclk = true,
> - .has_vld_pa_rng   = true,
> + .flags= HAS_4GB_MODE | HAS_BCLK | HAS_VLD_PA_RNG,
>   .larbid_remap = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9},
>  };
>  
>  static const struct mtk_iommu_plat_data mt8173_data = {
>   .m4u_plat = M4U_MT8173,
> - .has_4gb_mode = true,
> - .has_bclk = true,
> - .reset_axi= true,
> + .flags= HAS_4GB_MODE | HAS_BCLK | RESET_AXI,
>   .larbid_remap = {0, 1, 2, 3, 4, 5}, /* Linear mapping. */
>  };
>  
>  static const struct mtk_iommu_plat_data mt8183_data = {
>   .m4u_plat = M4U_MT8183,
> - .reset_axi= true,
> + .flags= RESET_AXI,
>   .larbid_remap = {0, 4, 5, 6, 7, 2, 3, 1},
>  };
>  
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index 7212e6fcf982..5225a9170aaa 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -39,12 +39,7 @@ enum mtk_iommu_plat {
>  
>  struct mtk_iommu_plat_data {
>   enum mtk_iommu_plat m4u_plat;
> - boolhas_4gb_mode;
> -
> - /* HW will use the EMI clock if there isn't the "bclk". */
> - boolhas_bclk;
> - boolhas_vld_pa_rng;
> - boolreset_axi;
> + u32 flags;


How about using bit field instead? eg

  u32 has_bclk:1;

In this way, we don't need to change code.

Joe.C

Re: [PATCH] i2c: mediatek: Add to support continuous mode

[Yingjoe Chen]

Hi Qiangming,

When you send new version, you should

- Add version number in subject, so we know this is a new one.
- List what's changed in this patch after ---,  so reviewer knows where
we should check. 


On Tue, 2020-06-23 at 15:42 +0800, Qiangming Xia wrote:
> From: "qiangming.xia" 
> 
> Mediatek i2c controller support for continuous mode,
> it allow to transfer once multiple writing messages of equal
> length.
> A i2c slave sometimes need write a serial of non-continuous
> offset range in chip,e.g. camera sensor imx586,imx576. It need
> transfer 294 writing messages when initiate setting. It can use
> this mode to improve performance.
> 
> Change-Id: If473d96d2b76e9d51f20741a9380db4fcad15dbd

Remove this.


> Signed-off-by: Qiangming Xia 
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 66 +
>  1 file changed, 66 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index deef69e56906..108bca1a4042 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -97,6 +97,7 @@ enum mtk_trans_op {
>   I2C_MASTER_WR = 1,
>   I2C_MASTER_RD,
>   I2C_MASTER_WRRD,
> + I2C_MASTER_CONTINUOUS_WR,
>  };
>  
>  enum I2C_REGS_OFFSET {
> @@ -846,6 +847,9 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, 
> struct i2c_msg *msgs,
>   OFFSET_TRANSFER_LEN);
>   }
>   mtk_i2c_writew(i2c, I2C_WRRD_TRANAC_VALUE, OFFSET_TRANSAC_LEN);
> + } else if (i2c->op == I2C_MASTER_CONTINUOUS_WR) {
> + mtk_i2c_writew(i2c, msgs->len / num, OFFSET_TRANSFER_LEN);
> + mtk_i2c_writew(i2c, num, OFFSET_TRANSAC_LEN);
>   } else {
>   mtk_i2c_writew(i2c, msgs->len, OFFSET_TRANSFER_LEN);
>   mtk_i2c_writew(i2c, num, OFFSET_TRANSAC_LEN);
> @@ -896,6 +900,23 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, 
> struct i2c_msg *msgs,
>   writel(reg_4g_mode, i2c->pdmabase + OFFSET_TX_4G_MODE);
>   }
>  
> + writel((u32)wpaddr, i2c->pdmabase + OFFSET_TX_MEM_ADDR);
> + writel(msgs->len, i2c->pdmabase + OFFSET_TX_LEN);
> + } else if (i2c->op == I2C_MASTER_CONTINUOUS_WR) {
> + writel(I2C_DMA_INT_FLAG_NONE, i2c->pdmabase + OFFSET_INT_FLAG);
> + writel(I2C_DMA_CON_TX, i2c->pdmabase + OFFSET_CON);
> + wpaddr = dma_map_single(i2c->dev, msgs->buf,
> + msgs->len, DMA_TO_DEVICE);
> + if (dma_mapping_error(i2c->dev, wpaddr)) {
> + kfree(msgs->buf);
> + return -ENOMEM;
> + }
> +
> + if (i2c->dev_comp->support_33bits) {
> + reg_4g_mode = mtk_i2c_set_4g_mode(wpaddr);
> + writel(reg_4g_mode, i2c->pdmabase + OFFSET_TX_4G_MODE);
> + }
> +
>   writel((u32)wpaddr, i2c->pdmabase + OFFSET_TX_MEM_ADDR);
>   writel(msgs->len, i2c->pdmabase + OFFSET_TX_LEN);
>   } else {
> @@ -979,6 +1000,11 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, 
> struct i2c_msg *msgs,
>msgs->len, DMA_FROM_DEVICE);
>  
>   i2c_put_dma_safe_msg_buf(dma_rd_buf, msgs, true);
> + } else if (i2c->op == I2C_MASTER_CONTINUOUS_WR) {
> + dma_unmap_single(i2c->dev, wpaddr,
> +  msgs->len, DMA_TO_DEVICE);
> +
> + kfree(msgs->buf);
>   } else {
>   dma_unmap_single(i2c->dev, wpaddr, msgs->len,
>DMA_TO_DEVICE);
> @@ -1009,6 +1035,9 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
>  {
>   int ret;
>   int left_num = num;
> + int i, j;
> + u8 *dma_multi_wr_buf;
> + struct i2c_msg multi_msg[1];
>   struct mtk_i2c *i2c = i2c_get_adapdata(adap);
>  
>   ret = mtk_i2c_clock_enable(i2c);
> @@ -1025,6 +1054,43 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
>   }
>   }
>  
> + if (num > 1 && !(msgs[0].flags & I2C_M_RD)) {
> + for (i = 0; i < num - 1; i++) {
> + if (!(msgs[i+1].flags & I2C_M_RD) &&
> + (msgs[i].addr == msgs[i+1].addr)
> + && (msgs[i].len == msgs[i+1].len)) {
> + continue;

Don't need () for addr/len check.
When wrap, please put operator at end of previous line.
The 2nd line of if is at the same indent level with continue, that
doesn't look good. Let's use 4 spaces as indent for addr and len check, 
so this should be:

if (num > 1 && !(msgs[0].flags & I2C_M_RD)) {
for (i = 0; i < num - 1; i++) {
if (!(msgs[i+1].flags & I2C_M_RD) && 
msgs[i].addr == msgs[i+1].addr &&
msgs[i].len == msgs[i+1].len) {


Joe.C

Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support

[Yingjoe Chen]

Sorry for late review.


On Thu, 2020-05-14 at 21:09 +0800, Qii Wang wrote:
> This patch adds a algorithm to calculate some ac-timing parameters
> which can fully meet I2C Spec.
> 
> Signed-off-by: Qii Wang 
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 328 
> +---
>  1 file changed, 277 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index 0ca6c38a..7020618 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c

<...>

> @@ -948,9 +1177,6 @@ static int mtk_i2c_probe(struct platform_device *pdev)
>   if (ret)
>   return -EINVAL;
>  
> - if (i2c->dev_comp->timing_adjust)
> - i2c->clk_src_div *= I2C_DEFAULT_CLK_DIV;
> -

After this patch, the 'clock-div' property in device tree is no longer
used for platform with timing_adjust ability.
Please change the binding, so we don't need to provide 'clock-div' for
these platform.

Joe.C

>   if (i2c->have_pmic && !i2c->dev_comp->pmic_i2c)
>   return -EINVAL;
>  

Re: [PATCH] i2c: mediatek: Add to support continuous mode

[Yingjoe Chen]

On Fri, 2020-06-19 at 16:06 +0800, Qiangming Xia wrote:
> From: "qiangming.xia" 

Please make 'From:' the same to Signed-off-by.



> Mediatek i2c controller support for continuous mode,
> it allow to transfer once multiple writing messages of equal length.

So the limitations are writing to same address, all in same length.
I think this is strict limitation. Do we have many this kind of usage?
How about change this to:

MediaTek i2c controller support continuous mode. This allows to write
multiple same length messages to single address with only one setup.


> For example, a slave need write a serial of non-continuous
> offset range in chip,e.g. writing offset 0,offset 2 and offset 4.
> Normally, it need three times i2c write operation. However,it can
> use once transfer to finish it by using continuous mode.
> 
> Change-Id: If06991e3fd32867bdeaacf15bb24864d5c5904d0

Please drop Change-Id:


> Signed-off-by: Qiangming Xia 
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 67 +
>  1 file changed, 67 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index deef69e56906..76ec65d869f6 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -97,6 +97,7 @@ enum mtk_trans_op {
>   I2C_MASTER_WR = 1,
>   I2C_MASTER_RD,
>   I2C_MASTER_WRRD,
> + I2C_MASTER_CONTINUOUS_WR,
>  };
>  
>  enum I2C_REGS_OFFSET {
> @@ -846,6 +847,9 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, 
> struct i2c_msg *msgs,
>   OFFSET_TRANSFER_LEN);
>   }
>   mtk_i2c_writew(i2c, I2C_WRRD_TRANAC_VALUE, OFFSET_TRANSAC_LEN);
> + } else if (i2c->op == I2C_MASTER_CONTINUOUS_WR) {
> + mtk_i2c_writew(i2c, msgs->len / num, OFFSET_TRANSFER_LEN);
> + mtk_i2c_writew(i2c, num, OFFSET_TRANSAC_LEN);
>   } else {
>   mtk_i2c_writew(i2c, msgs->len, OFFSET_TRANSFER_LEN);
>   mtk_i2c_writew(i2c, num, OFFSET_TRANSAC_LEN);
> @@ -896,6 +900,23 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, 
> struct i2c_msg *msgs,
>   writel(reg_4g_mode, i2c->pdmabase + OFFSET_TX_4G_MODE);
>   }
>  
> + writel((u32)wpaddr, i2c->pdmabase + OFFSET_TX_MEM_ADDR);
> + writel(msgs->len, i2c->pdmabase + OFFSET_TX_LEN);
> + } else if (i2c->op == I2C_MASTER_CONTINUOUS_WR) {
> + writel(I2C_DMA_INT_FLAG_NONE, i2c->pdmabase + OFFSET_INT_FLAG);
> + writel(I2C_DMA_CON_TX, i2c->pdmabase + OFFSET_CON);
> + wpaddr = dma_map_single(i2c->dev, msgs->buf,
> + msgs->len, DMA_TO_DEVICE);
> + if (dma_mapping_error(i2c->dev, wpaddr)) {
> + kfree(msgs->buf);
> + return -ENOMEM;
> + }
> +
> + if (i2c->dev_comp->support_33bits) {
> + reg_4g_mode = mtk_i2c_set_4g_mode(wpaddr);
> + writel(reg_4g_mode, i2c->pdmabase + OFFSET_TX_4G_MODE);
> + }
> +
>   writel((u32)wpaddr, i2c->pdmabase + OFFSET_TX_MEM_ADDR);
>   writel(msgs->len, i2c->pdmabase + OFFSET_TX_LEN);
>   } else {
> @@ -979,6 +1000,11 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, 
> struct i2c_msg *msgs,
>msgs->len, DMA_FROM_DEVICE);
>  
>   i2c_put_dma_safe_msg_buf(dma_rd_buf, msgs, true);
> + } else if (i2c->op == I2C_MASTER_CONTINUOUS_WR) {
> + dma_unmap_single(i2c->dev, wpaddr,
> +  msgs->len, DMA_TO_DEVICE);
> +
> + kfree(msgs->buf);
>   } else {
>   dma_unmap_single(i2c->dev, wpaddr, msgs->len,
>DMA_TO_DEVICE);
> @@ -1009,6 +1035,9 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
>  {
>   int ret;
>   int left_num = num;
> + int i, j;
> + u8 *dma_multi_wr_buf;
> + struct i2c_msg multi_msg[1];
>   struct mtk_i2c *i2c = i2c_get_adapdata(adap);
>  
>   ret = mtk_i2c_clock_enable(i2c);
> @@ -1025,6 +1054,44 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
>   }
>   }
>  
> + if (num > 1) {
> + for (i = 0; i < num - 1; i++) {
> + if (!(msgs[i].flags & I2C_M_RD) && !(msgs[i+1].flags &
> + I2C_M_RD) && (msgs[i].addr == msgs[i+1].addr)
> + && (msgs[i].len == msgs[i+1].len)) {

Since this block is all for continuous mode check, we could move first
flags check out and made the if in for loop simpler to read:

if (num > 1 && !(msgs[0].flags & I2C_M_RD)) {
for (i = 0; i < num - 1; i++) {
if (!(msgs[i+1].flags & I2C_M_RD) && 
msgs[i].addr == msgs[i+1].addr &&
msgs[i].len == msgs[i+1].len) 

Re: [PATCH v8 2/3] drivers: input: keyboard: Add mtk keypad driver

[Yingjoe Chen]

On Fri, 2020-05-15 at 14:40 +0300, Andy Shevchenko wrote:
> On Fri, May 15, 2020 at 11:30:16AM +0200, Marco Felsch wrote:
> > On 20-05-15 14:20, Fengping Yu wrote:
> 
> ...
> 
> > > + depends on OF && HAVE_CLK
> > 
> > Please drop those deps and instead use:
> 
> +1
> 
> > depends on ARCH_MEDIATEK && ARM64
> 
> I would go even further
>   depends on (ARCH_MEDIATEK && ARM64) || COMPILE_TEST

Please drop ARM64. This works for 32bits SoC as well.

Joe.C

Re: [PATCH RESEND] i2c: mediatek: disable zero-length transfers for mt8183

[Yingjoe Chen]

On Thu, 2019-08-22 at 13:57 +0800, Hsin-Yi Wang wrote:
> When doing i2cdetect quick write mode, we would get transfer
> error ENOMEM, and i2cdetect shows there's no device at the address.
> Quoting from mt8183 datasheet, the number of transfers to be
> transferred in one transaction should be set to bigger than 1,
> so we should forbid zero-length transfer and update functionality.


<...>

> @@ -933,8 +942,8 @@ static int mtk_i2c_probe(struct platform_device *pdev)
>   i2c->dev = >dev;
>   i2c->adap.dev.parent = >dev;
>   i2c->adap.owner = THIS_MODULE;
> - i2c->adap.algo = _i2c_algorithm;
>   i2c->adap.quirks = i2c->dev_comp->quirks;
> + i2c->adap.algo = _i2c_algorithm;
>   i2c->adap.timeout = 2 * HZ;
>   i2c->adap.retries = 1;
>  

Why do you need to change this part?

Joe.C

Re: [PATCH v5 08/13] dt-bindings: pwm: update bindings for MT7628 SoC

[Yingjoe Chen]

On Thu, 2019-08-22 at 14:58 +0800, Sam Shih wrote:
> This updates bindings for MT7628 pwm controller.
> 
> Signed-off-by: Sam Shih 
> ---
>  Documentation/devicetree/bindings/pwm/pwm-mediatek.txt | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt 
> b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> index ea95b490a913..93980e3da261 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
> @@ -21,6 +21,10 @@ Required properties:
> See pinctrl/pinctrl-bindings.txt for details of the property values.
>   - num-pwms: the number of PWM channels.
> +
> + Optional properties:
> + - clock-frequency: fix clock frequency, this is only used in MT7628 SoC
> +for period calculation. This SoC has no complex clock 
> tree.

I'm sorry if this has been discussed before. 

Would it be simpler if you just provide a fixed-clock as clock in device
tree? This way you don't need this optional properties and don't need to
special handle it in driver code. 

After all, the hw is still connected to a simple clock tree.

Joe.C

Re: [PATCH] checkpatch: avoid default n

[Yingjoe Chen]

On Wed, 2019-07-03 at 01:42 -0700, Joe Perches wrote:
> On Wed, 2019-07-03 at 16:30 +0800, Miles Chen wrote:
> > This change reports a warning when "default n" is used.
> > 
> > I have seen several "remove default n" patches, so I think
> > it might be helpful to add this test in checkpatch.
> > 
> > tested:
> > WARNING: 'default n' is the default value, no need to write it explicitly.
> > +   default n
> 
> I don't think this is reasonable as there are
> several uses like:
> 
>   default y
>   default n if 
> 
> For instance:
> 
> arch/alpha/Kconfig-config ALPHA_WTINT
> arch/alpha/Kconfig- bool "Use WTINT" if ALPHA_SRM || ALPHA_GENERIC
> arch/alpha/Kconfig- default y if ALPHA_QEMU
> arch/alpha/Kconfig: default n if ALPHA_EV5 || ALPHA_EV56 || (ALPHA_EV4 && 
> !ALPHA_LCA)
> arch/alpha/Kconfig: default n if !ALPHA_SRM && !ALPHA_GENERIC

Hi,


I've sent similar patch in 2016, my version won't complain about these.

https://lkml.org/lkml/2016/4/22/580

Joe.C

Re: [PATCH] checkpatch: avoid default n

[Yingjoe Chen]

On Wed, 2019-07-03 at 01:42 -0700, Joe Perches wrote:
> On Wed, 2019-07-03 at 16:30 +0800, Miles Chen wrote:
> > This change reports a warning when "default n" is used.
> > 
> > I have seen several "remove default n" patches, so I think
> > it might be helpful to add this test in checkpatch.
> > 
> > tested:
> > WARNING: 'default n' is the default value, no need to write it explicitly.
> > +   default n
> 
> I don't think this is reasonable as there are
> several uses like:
> 
>   default y
>   default n if 
> 
> For instance:
> 
> arch/alpha/Kconfig-config ALPHA_WTINT
> arch/alpha/Kconfig- bool "Use WTINT" if ALPHA_SRM || ALPHA_GENERIC
> arch/alpha/Kconfig- default y if ALPHA_QEMU
> arch/alpha/Kconfig: default n if ALPHA_EV5 || ALPHA_EV56 || (ALPHA_EV4 && 
> !ALPHA_LCA)
> arch/alpha/Kconfig: default n if !ALPHA_SRM && !ALPHA_GENERIC

I've sent similar patch in 2016, my version won't complain about these.

https://lkml.org/lkml/2016/4/22/580

Joe.C

[PATCH] i2c: dev: fix potential memory leak in i2cdev_ioctl_rdwr

[Yingjoe Chen]

If I2C_M_RECV_LEN check failed, msgs[i].buf allocated by memdup_user
will not be freed. Pump index up so it will be freed.

Fixes: 838bfa6049fb ("i2c-dev: Add support for I2C_M_RECV_LEN")
Signed-off-by: Yingjoe Chen 
---
Only check arm64 defconfig build pass.
I haven't test it since it just fix memleak for error cases.
---
 drivers/i2c/i2c-dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 3f7b9af..776f366 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -283,6 +283,7 @@ static noinline int i2cdev_ioctl_rdwr(struct i2c_client 
*client,
msgs[i].len < 1 || msgs[i].buf[0] < 1 ||
msgs[i].len < msgs[i].buf[0] +
 I2C_SMBUS_BLOCK_MAX) {
+   i++;
res = -EINVAL;
break;
}
-- 
1.9.1

Re: [PATCH v3 10/10] rtc: Add support for the MediaTek MT6358 RTC

[Yingjoe Chen]

On Fri, 2019-05-03 at 17:31 +0800, Hsin-Hsiung Wang wrote:
> From: Ran Bi 
> 
> This add support for the MediaTek MT6358 RTC. Driver using
> compatible data to store different RTC_WRTGR address offset.
> 
> Signed-off-by: Ran Bi 
> ---
>  drivers/rtc/rtc-mt6397.c | 43 
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index f85f1fc29e32..3476e29db87c 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -27,7 +28,8 @@
>  #define RTC_BBPU 0x
>  #define RTC_BBPU_CBUSY   BIT(6)
>  
> -#define RTC_WRTGR0x003c
> +#define RTC_WRTGR_MT6358 0x3a
> +#define RTC_WRTGR_MT6397 0x3c
>  
>  #define RTC_IRQ_STA  0x0002
>  #define RTC_IRQ_STA_AL   BIT(0)
> @@ -71,6 +73,10 @@
>  #define RTC_NUM_YEARS128
>  #define RTC_MIN_YEAR_OFFSET  (RTC_MIN_YEAR - RTC_BASE_YEAR)
>  
> +struct mtk_rtc_compatible {
> + u32 wrtgr_addr;
> +};
> +
>  struct mt6397_rtc {
>   struct device   *dev;
>   struct rtc_device   *rtc_dev;
> @@ -78,7 +84,25 @@ struct mt6397_rtc {
>   struct regmap   *regmap;
>   int irq;
>   u32 addr_base;
> + const struct mtk_rtc_compatible *dev_comp;
> +};
> +
> +static const struct mtk_rtc_compatible mt6358_rtc_compat = {
> + .wrtgr_addr = RTC_WRTGR_MT6358,
> +};
> +
> +static const struct mtk_rtc_compatible mt6397_rtc_compat = {
> + .wrtgr_addr = RTC_WRTGR_MT6397,
> +};
> +
> +static const struct of_device_id mt6397_rtc_of_match[] = {
> + { .compatible = "mediatek,mt6358-rtc",
> + .data = (void *)_rtc_compat, },
> + { .compatible = "mediatek,mt6397-rtc",
> + .data = (void *)_rtc_compat, },
> + {}
>  };
> +MODULE_DEVICE_TABLE(of, mt6397_rtc_of_match);
>  
>  static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
>  {
> @@ -86,7 +110,8 @@ static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
>   int ret;
>   u32 data;
>  
> - ret = regmap_write(rtc->regmap, rtc->addr_base + RTC_WRTGR, 1);
> + ret = regmap_write(rtc->regmap,
> +rtc->addr_base + rtc->dev_comp->wrtgr_addr, 1);
>   if (ret < 0)
>   return ret;
>  
> @@ -332,6 +357,7 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>   struct resource *res;
>   struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
>   struct mt6397_rtc *rtc;
> + const struct of_device_id *of_id;
>   int ret;
>  
>   rtc = devm_kzalloc(>dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> @@ -341,6 +367,13 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   rtc->addr_base = res->start;
>  
> + of_id = of_match_device(mt6397_rtc_of_match, >dev);
> + if (!of_id) {

This will never happens, but I'm fine with it.

Review-by: Yingjoe Chen 

Joe.C

Re: [PATCH 1/2] pinctrl: mediatek: Add mtk_eint_pm_ops to common-v2

[Yingjoe Chen]

On Mon, 2019-04-29 at 11:25 +0800, Nicolas Boichat wrote:
> pinctrl variants that include pinctrl-mtk-common-v2.h (and not
> pinctrl-mtk-common.h) also need to use mtk_eint_pm_ops to setup
> wake mask properly, so copy over the pm_ops to v2.
> 
> It is not easy to merge the 2 copies (or move
> mtk_eint_suspend/resume to mtk-eint.c), as we need to
> dereference pctrl->eint, and struct mtk_pinctrl *pctl has a
> different structure definition for v1 and v2.
> 
> Signed-off-by: Nicolas Boichat 
> Reviewed-by: Chuanjia Liu 
> ---
>  .../pinctrl/mediatek/pinctrl-mtk-common-v2.c  | 19 +++
>  .../pinctrl/mediatek/pinctrl-mtk-common-v2.h  |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c 
> b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> index 20e1c890e73b30c..7e19b5a4748eafe 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> @@ -723,3 +723,22 @@ int mtk_pinconf_adv_drive_get(struct mtk_pinctrl *hw,
>  
>   return 0;
>  }
> +
> +static int mtk_eint_suspend(struct device *device)
> +{
> + struct mtk_pinctrl *pctl = dev_get_drvdata(device);
> +
> + return mtk_eint_do_suspend(pctl->eint);
> +}
> +
> +static int mtk_eint_resume(struct device *device)
> +{
> + struct mtk_pinctrl *pctl = dev_get_drvdata(device);
> +
> + return mtk_eint_do_resume(pctl->eint);
> +}
> +
> +const struct dev_pm_ops mtk_eint_pm_ops = {
> + .suspend_noirq = mtk_eint_suspend,
> + .resume_noirq = mtk_eint_resume,
> +};

This is identical to the one in pinctrl-mtk-common.c and will have name
clash if both pinctrl-mtk-common.c and pinctrl-mtk-common-v2.c are
built.

It would be better if we try to merge both version into mtk-eint.c, this
way we could also remove some global functions.

Joe.C

Re: [PATCH 09/24] mmc: mtk-sd: check for valid optional memory resource

[Yingjoe Chen]

On Sat, 2019-03-23 at 22:15 +0100, Fabien Parent wrote:
> 'top_base' memory region is optional. Check that the resource is valid
> before using it. This avoid getting a "invalid resource" error message
> printed by the kernel.
> 
> Signed-off-by: Fabien Parent 
> ---
>  drivers/mmc/host/mtk-sd.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 833ef0590af8..573aa127d00b 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -2123,9 +2123,11 @@ static int msdc_drv_probe(struct platform_device *pdev)
>   }
>  
>   res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> - host->top_base = devm_ioremap_resource(>dev, res);
> - if (IS_ERR(host->top_base))
> - host->top_base = NULL;
> + if (ret) {

This should be res?

> + host->top_base = devm_ioremap_resource(>dev, res);
> + if (IS_ERR(host->top_base))
> + host->top_base = NULL;
> + }
>  
>   ret = mmc_regulator_get_supply(mmc);
>   if (ret)

Re: [PATCH v2 9/9] rtc: Add support for the MediaTek MT6358 RTC

[Yingjoe Chen]

Hi,


Should use 'rtc: mt6397: ' as prefix for this patch.


On Mon, 2019-03-11 at 11:46 +0800, Hsin-Hsiung Wang wrote:
> From: Ran Bi 
> 
> This add support for the MediaTek MT6358 RTC. MT6397 mfd will pass
> RTC_WRTGR address offset to RTC driver.
> 
> Signed-off-by: Ran Bi 
> ---
>  drivers/rtc/rtc-mt6397.c | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index f85f1fc..c8a0090 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -27,7 +27,7 @@
>  #define RTC_BBPU 0x
>  #define RTC_BBPU_CBUSY   BIT(6)
>  
> -#define RTC_WRTGR0x003c
> +#define RTC_WRTGR_DEFAULT0x003c
>  
>  #define RTC_IRQ_STA  0x0002
>  #define RTC_IRQ_STA_AL   BIT(0)
> @@ -78,6 +78,7 @@ struct mt6397_rtc {
>   struct regmap   *regmap;
>   int irq;
>   u32 addr_base;
> + u32 wrtgr_offset;
>  };
>  
>  static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
> @@ -86,7 +87,8 @@ static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
>   int ret;
>   u32 data;
>  
> - ret = regmap_write(rtc->regmap, rtc->addr_base + RTC_WRTGR, 1);
> + ret = regmap_write(rtc->regmap,
> +rtc->addr_base + rtc->wrtgr_offset, 1);
>   if (ret < 0)
>   return ret;
>  
> @@ -341,6 +343,15 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   rtc->addr_base = res->start;
>  
> + res = platform_get_resource(pdev, IORESOURCE_REG, 0);
> + if (res) {
> + rtc->wrtgr_offset = res->start;
> + dev_info(>dev, "register offset:%d\n", rtc->wrtgr_offset);
> + } else {
> + rtc->wrtgr_offset = RTC_WRTGR_DEFAULT;
> + dev_err(>dev, "Failed to get register offset\n");
> + }
> +

Since this will be passed by MFD, do we still need to keep the DEFAULT?
Any case this platform_get_resource will failed?

It's too bad HW changed this offset, but I'm not sure about passing this
information from MFD. We have 1 register that have different offset now,
and might have others for future chips, adding each one by
IORESOURCE_IRQ doesn't looks like a good solution. Keeping this
information in RTC driver only also looks better.


Joe.C

Re: [PATCH] eint: add gpio vritual number select

[Yingjoe Chen]

On Mon, 2018-12-17 at 11:15 +0800, Chuanjia Liu wrote:
> On Thu, 2018-12-13 at 11:33 -0800, Sean Wang wrote:
> > On Thu, Dec 13, 2018 at 1:36 AM  wrote:
> > >
> > > From: Chuanjia Liu 
> > >
> > > This patch add gpio vritual number select,avoid virtual gpio set SMT.
> > 
> > s/gpio/GPIO/
> > s/vritual/virtual/
> > 
> > Virtual GPIOs you said here that means these pins only used inside SoC
> > and not being exported to outside SoC, right? It seems this kind of
> > pins doesn't need SMT.
> > 
> Yes,virtual gpio only used inside SOC and these pins doesn't need set
> SMT

Hi,

I don't see full patch in linux-mediatek archive. Maybe you are not
subscribed so it is rejected?

Please add this description to commit message and/or code comment.
I think 'internal GPIO' might be a better name for this. Does the name
'virtual GPIO' come from datasheet?


> > >
> > > Signed-off-by: Chuanjia Liu 
> > > ---
> > >  drivers/pinctrl/mediatek/mtk-eint.h  |1 +
> > >  drivers/pinctrl/mediatek/pinctrl-mt8183.c|1 +
> > >  drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c |9 ++---
> > >  3 files changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.h 
> > > b/drivers/pinctrl/mediatek/mtk-eint.h
> > > index 48468d0..c16beaf 100644
> > > --- a/drivers/pinctrl/mediatek/mtk-eint.h
> > > +++ b/drivers/pinctrl/mediatek/mtk-eint.h
> > > @@ -37,6 +37,7 @@ struct mtk_eint_hw {
> > > u8  ports;
> > > unsigned intap_num;
> > > unsigned intdb_cnt;
> > > +   unsigned intvir_start;


Since it is about GPIO and SMT, maybe it should be added to mtk_pin_soc
instead of mtk_eint_hw ?

Joe.C

> > >  };
> > >
> > >  struct mtk_eint;
> > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mt8183.c 
> > > b/drivers/pinctrl/mediatek/pinctrl-mt8183.c
> > > index 6262fd3..bbeafd3 100644
> > > --- a/drivers/pinctrl/mediatek/pinctrl-mt8183.c
> > > +++ b/drivers/pinctrl/mediatek/pinctrl-mt8183.c
> > > @@ -497,6 +497,7 @@
> > > .ports = 6,
> > > .ap_num= 212,
> > > .db_cnt= 13,
> > > +   .vir_start = 180,
> > >  };
> > >
> > >  static const struct mtk_pin_soc mt8183_data = {
> > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c 
> > > b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> > > index 4a9e0d4..ca3bae1 100644
> > > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> > > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> > > @@ -289,9 +289,12 @@ static int mtk_xt_set_gpio_as_eint(void *data, 
> > > unsigned long eint_n)
> > > if (err)
> > > return err;
> > >
> > > -   err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_SMT, MTK_ENABLE);
> > > -   if (err)
> > > -   return err;
> > > +   if (gpio_n < hw->eint->hw->vir_start) {
> > > +   err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_SMT,
> > > +  MTK_ENABLE);
> > > +   if (err)
> > > +   return err;
> > > +   }
> > 
> > The changes will break these SoCs without a properly configured vir_start.
> > 
> > If SMT seems unnecessary for these kinds of virtual GPIOs pin in the
> > path, we can do it as
> > 
> > err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_SMT,
> > MTK_ENABLE);
> > /* please add comments for the exclusion condition */
> > if (err && err != -ENOTSUPP)
> > return err;
> > 
> > If there is getting much special on certain pins between SoCs, and
> > then we can consider creating a desc->flag to split logic.
> 
> Yes,SMT unnecessary for these kinds of virtual GPIOS pin in the path,if
> do it as
>   err = mtk_hw_set_value(hw, desc, PINCTRL_PIN_REG_SMT,
> MTK_ENABLE);
>   if (err && err != -ENOTSUPP)
> return err;
> I wonder if system will lose -ENOTSUPP information when smt was not
> successfully set by real gpio?
> > 
> > >
> > > return 0;
> > >  }
> > > --
> > > 1.7.9.5
> 
> 
> 
> ___
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v5 2/2] arm: dts: mt2712: add uart APDMA to device tree

[Yingjoe Chen]

On Tue, 2018-12-11 at 13:37 +0800, Long Cheng wrote:
> 1. add uart APDMA controller device node
> 2. add uart 0/1/2/3/4/5 DMA function
> 
> Signed-off-by: Long Cheng 
> ---
>  arch/arm64/boot/dts/mediatek/mt2712e.dtsi |   50 
> +
>  1 file changed, 50 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi 
> b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> index 976d92a..a59728b 100644
> --- a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> @@ -300,6 +300,9 @@
>   interrupts = ;
>   clocks = <_clk>, <_clk>;
>   clock-names = "baud", "bus";
> + dmas = < 10
> +  11>;
> + dma-names = "tx", "rx";
>   status = "disabled";
>   };
>  
> @@ -378,6 +381,38 @@
>   status = "disabled";
>   };
>  
> + apdma: dma-controller@11000400 {
> + compatible = "mediatek,mt2712-uart-dma",
> +  "mediatek,mt6577-uart-dma";

Sorting, please make sure this is before 

auxadc: adc@11001000 {


> + reg = <0 0x11000400 0 0x80>,
> +   <0 0x11000480 0 0x80>,
> +   <0 0x11000500 0 0x80>,
> +   <0 0x11000580 0 0x80>,
> +   <0 0x11000600 0 0x80>,
> +   <0 0x11000680 0 0x80>,
> +   <0 0x11000700 0 0x80>,
> +   <0 0x11000780 0 0x80>,
> +   <0 0x11000800 0 0x80>,
> +   <0 0x11000880 0 0x80>,
> +   <0 0x11000900 0 0x80>,
> +   <0 0x11000980 0 0x80>;
> + interrupts = ,
> +  ,
> +  ,
> +  ,
> +  ,
> +  ,
> +  ,
> +  ,
> +  ,
> +  ,
> +  ,
> +  ;
> + clocks = < CLK_PERI_AP_DMA>;
> + clock-names = "apdma";
> + #dma-cells = <1>;
> + };
> +
>   uart0: serial@11002000 {
>   compatible = "mediatek,mt2712-uart",
>"mediatek,mt6577-uart";
> @@ -385,6 +420,9 @@


...deleted

Re: [PATCH v3 1/4] dt-bindings: pinctrl: Add devicetree bindings for MT6797 SoC Pinctrl

[Yingjoe Chen]

On Wed, 2018-11-07 at 23:18 +0530, Manivannan Sadhasivam wrote:
> Add devicetree bindings for Mediatek MT6797 SoC Pin Controller.
> 
> Signed-off-by: Manivannan Sadhasivam 
> ---
>  .../bindings/pinctrl/pinctrl-mt6797.txt   |   83 +
>  include/dt-bindings/pinctrl/mt6797-pinfunc.h  | 1368 +
>  2 files changed, 1451 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/pinctrl/pinctrl-mt6797.txt
>  create mode 100644 include/dt-bindings/pinctrl/mt6797-pinfunc.h
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mt6797.txt 
> b/Documentation/devicetree/bindings/pinctrl/pinctrl-mt6797.txt
> new file mode 100644
> index ..bd83401e6179
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mt6797.txt

Sorry if this is discussed.

What's the difference between this and
Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt?
Should this be added to pinctrl-mt65xx.txt?

Joe.C

Re: [PATCH v3 1/4] dt-bindings: pinctrl: Add devicetree bindings for MT6797 SoC Pinctrl

[Yingjoe Chen]

On Wed, 2018-11-07 at 23:18 +0530, Manivannan Sadhasivam wrote:
> Add devicetree bindings for Mediatek MT6797 SoC Pin Controller.
> 
> Signed-off-by: Manivannan Sadhasivam 
> ---
>  .../bindings/pinctrl/pinctrl-mt6797.txt   |   83 +
>  include/dt-bindings/pinctrl/mt6797-pinfunc.h  | 1368 +
>  2 files changed, 1451 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/pinctrl/pinctrl-mt6797.txt
>  create mode 100644 include/dt-bindings/pinctrl/mt6797-pinfunc.h
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mt6797.txt 
> b/Documentation/devicetree/bindings/pinctrl/pinctrl-mt6797.txt
> new file mode 100644
> index ..bd83401e6179
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mt6797.txt

Sorry if this is discussed.

What's the difference between this and
Documentation/devicetree/bindings/pinctrl/pinctrl-mt65xx.txt?
Should this be added to pinctrl-mt65xx.txt?

Joe.C

Re: [PATCH v2 2/3] i2c: Add helper to ease DMA handling

[Yingjoe Chen]

On Wed, 2018-08-08 at 22:57 +0200, Wolfram Sang wrote:
> On Sat, Jul 07, 2018 at 05:29:54PM +0800, Jun Gao wrote:
> > From: Jun Gao 
> > 
> > This function is needed by i2c_get_dma_safe_msg_buf() potentially.
> > It is used to free DMA safe buffer when DMA operation fails.
> > 
> > Signed-off-by: Jun Gao 
> 
> Right, we need something like this. This leaks in the sh_mobile driver,
> too :(
> 
> I am still thinking if there is a nice way to put this functionality
> into i2c_release_dma_safe_msg_buf() itself somehow...

Wolfram,

I have second thought on these API now. Recently, we have saw similar
issue for spi device driver.

I believe the reason for these api is because some arch changed to can
not do DMA on stack recently. Maybe we should have dma_mapping to bounce
buffer like it bounce un-dma-able address for those arch? or we should
have a common driver API for this, not just for i2c?

Joe.C

Re: [PATCH v2 2/3] i2c: Add helper to ease DMA handling

[Yingjoe Chen]

On Wed, 2018-08-08 at 22:57 +0200, Wolfram Sang wrote:
> On Sat, Jul 07, 2018 at 05:29:54PM +0800, Jun Gao wrote:
> > From: Jun Gao 
> > 
> > This function is needed by i2c_get_dma_safe_msg_buf() potentially.
> > It is used to free DMA safe buffer when DMA operation fails.
> > 
> > Signed-off-by: Jun Gao 
> 
> Right, we need something like this. This leaks in the sh_mobile driver,
> too :(
> 
> I am still thinking if there is a nice way to put this functionality
> into i2c_release_dma_safe_msg_buf() itself somehow...

Wolfram,

I have second thought on these API now. Recently, we have saw similar
issue for spi device driver.

I believe the reason for these api is because some arch changed to can
not do DMA on stack recently. Maybe we should have dma_mapping to bounce
buffer like it bounce un-dma-able address for those arch? or we should
have a common driver API for this, not just for i2c?

Joe.C

Re: [PATCH v3 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC

[Yingjoe Chen]

Sean,

Looks good to me. You can add if you want:

Reviewed-by: Yingjoe Chen <yingjoe.c...@mediatek.com>

Joe.C


On Mon, 2017-10-23 at 15:16 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang <sean.w...@mediatek.com>
> 
> This patch introduces the driver for the RTC on MT7622 SoC.
> 
> Signed-off-by: Sean Wang <sean.w...@mediatek.com>
> ---
>  drivers/rtc/Kconfig  |  10 ++
>  drivers/rtc/Makefile |   1 +
>  drivers/rtc/rtc-mt7622.c | 422 
> +++
>  3 files changed, 433 insertions(+)
>  create mode 100644 drivers/rtc/rtc-mt7622.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e0e58f3..fd9b590 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1705,6 +1705,16 @@ config RTC_DRV_MOXART
>  This driver can also be built as a module. If so, the module
>  will be called rtc-moxart
>  
> +config RTC_DRV_MT7622
> + tristate "MediaTek SoC based RTC"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + help
> +   This enables support for the real time clock built in the MediaTek
> +   SoCs.
> +
> +   This drive can also be built as a module. If so, the module
> +   will be called rtc-mt7622.
> +
>  config RTC_DRV_MT6397
>   tristate "Mediatek Real Time Clock driver"
>   depends on MFD_MT6397 || (COMPILE_TEST && IRQ_DOMAIN)
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 7230014..eb47f8c 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_RTC_DRV_MOXART)  += rtc-moxart.o
>  obj-$(CONFIG_RTC_DRV_MPC5121)+= rtc-mpc5121.o
>  obj-$(CONFIG_RTC_DRV_VRTC)   += rtc-mrst.o
>  obj-$(CONFIG_RTC_DRV_MSM6242)+= rtc-msm6242.o
> +obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o
>  obj-$(CONFIG_RTC_DRV_MT6397) += rtc-mt6397.o
>  obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o
>  obj-$(CONFIG_RTC_DRV_MXC)+= rtc-mxc.o
> diff --git a/drivers/rtc/rtc-mt7622.c b/drivers/rtc/rtc-mt7622.c
> new file mode 100644
> index 000..d79b9ae
> --- /dev/null
> +++ b/drivers/rtc/rtc-mt7622.c
> @@ -0,0 +1,422 @@
> +/*
> + * Driver for MediaTek SoC based RTC
> + *
> + * Copyright (C) 2017 Sean Wang <sean.w...@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define MTK_RTC_DEV KBUILD_MODNAME
> +
> +#define MTK_RTC_PWRCHK1  0x4
> +#define  RTC_PWRCHK1_MAGIC   0xc6
> +
> +#define MTK_RTC_PWRCHK2  0x8
> +#define  RTC_PWRCHK2_MAGIC   0x9a
> +
> +#define MTK_RTC_KEY  0xc
> +#define  RTC_KEY_MAGIC   0x59
> +
> +#define MTK_RTC_PROT10x10
> +#define  RTC_PROT1_MAGIC 0xa3
> +
> +#define MTK_RTC_PROT20x14
> +#define  RTC_PROT2_MAGIC 0x57
> +
> +#define MTK_RTC_PROT30x18
> +#define  RTC_PROT3_MAGIC 0x67
> +
> +#define MTK_RTC_PROT40x1c
> +#define  RTC_PROT4_MAGIC 0xd2
> +
> +#define MTK_RTC_CTL  0x20
> +#define  RTC_RC_STOP BIT(0)
> +
> +#define MTK_RTC_DEBNCE   0x2c
> +#define  RTC_DEBNCE_MASK GENMASK(2, 0)
> +
> +#define MTK_RTC_INT  0x30
> +#define RTC_INT_AL_STA   BIT(4)
> +
> +/*
> + * Ranges from 0x40 to 0x78 provide RTC time setup for year, month,
> + * day of month, day of week, hour, minute and second.
> + */
> +#define MTK_RTC_TREG(_t, _f) (0x40 + (0x4 * (_f)) + ((_t) * 0x20))
> +
> +#define MTK_RTC_AL_CTL   0x7c
> +#define  RTC_AL_EN   BIT(0)
> +#define  RTC_AL_ALL  GENMASK(7, 0)
> +
> +/*
> + * The offset is used in the translation for the year between in struct
> + * rtc_time and in hardware register MTK_RTC_TREG(x,MTK_YEA)
> + */
> +#define MTK_RTC_TM_YR_OFFSET 100
> +
> +/*
> + * The lowest value for the valid tm_year. RTC hardware would take 
> incorrectly
> + * tm_year 100 as not a leap year and thus it is also requir

Re: [PATCH v3 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC

[Yingjoe Chen]

Sean,

Looks good to me. You can add if you want:

Reviewed-by: Yingjoe Chen 

Joe.C


On Mon, 2017-10-23 at 15:16 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> This patch introduces the driver for the RTC on MT7622 SoC.
> 
> Signed-off-by: Sean Wang 
> ---
>  drivers/rtc/Kconfig  |  10 ++
>  drivers/rtc/Makefile |   1 +
>  drivers/rtc/rtc-mt7622.c | 422 
> +++
>  3 files changed, 433 insertions(+)
>  create mode 100644 drivers/rtc/rtc-mt7622.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e0e58f3..fd9b590 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1705,6 +1705,16 @@ config RTC_DRV_MOXART
>  This driver can also be built as a module. If so, the module
>  will be called rtc-moxart
>  
> +config RTC_DRV_MT7622
> + tristate "MediaTek SoC based RTC"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + help
> +   This enables support for the real time clock built in the MediaTek
> +   SoCs.
> +
> +   This drive can also be built as a module. If so, the module
> +   will be called rtc-mt7622.
> +
>  config RTC_DRV_MT6397
>   tristate "Mediatek Real Time Clock driver"
>   depends on MFD_MT6397 || (COMPILE_TEST && IRQ_DOMAIN)
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 7230014..eb47f8c 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_RTC_DRV_MOXART)  += rtc-moxart.o
>  obj-$(CONFIG_RTC_DRV_MPC5121)+= rtc-mpc5121.o
>  obj-$(CONFIG_RTC_DRV_VRTC)   += rtc-mrst.o
>  obj-$(CONFIG_RTC_DRV_MSM6242)+= rtc-msm6242.o
> +obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o
>  obj-$(CONFIG_RTC_DRV_MT6397) += rtc-mt6397.o
>  obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o
>  obj-$(CONFIG_RTC_DRV_MXC)+= rtc-mxc.o
> diff --git a/drivers/rtc/rtc-mt7622.c b/drivers/rtc/rtc-mt7622.c
> new file mode 100644
> index 000..d79b9ae
> --- /dev/null
> +++ b/drivers/rtc/rtc-mt7622.c
> @@ -0,0 +1,422 @@
> +/*
> + * Driver for MediaTek SoC based RTC
> + *
> + * Copyright (C) 2017 Sean Wang 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define MTK_RTC_DEV KBUILD_MODNAME
> +
> +#define MTK_RTC_PWRCHK1  0x4
> +#define  RTC_PWRCHK1_MAGIC   0xc6
> +
> +#define MTK_RTC_PWRCHK2  0x8
> +#define  RTC_PWRCHK2_MAGIC   0x9a
> +
> +#define MTK_RTC_KEY  0xc
> +#define  RTC_KEY_MAGIC   0x59
> +
> +#define MTK_RTC_PROT10x10
> +#define  RTC_PROT1_MAGIC 0xa3
> +
> +#define MTK_RTC_PROT20x14
> +#define  RTC_PROT2_MAGIC 0x57
> +
> +#define MTK_RTC_PROT30x18
> +#define  RTC_PROT3_MAGIC 0x67
> +
> +#define MTK_RTC_PROT40x1c
> +#define  RTC_PROT4_MAGIC 0xd2
> +
> +#define MTK_RTC_CTL  0x20
> +#define  RTC_RC_STOP BIT(0)
> +
> +#define MTK_RTC_DEBNCE   0x2c
> +#define  RTC_DEBNCE_MASK GENMASK(2, 0)
> +
> +#define MTK_RTC_INT  0x30
> +#define RTC_INT_AL_STA   BIT(4)
> +
> +/*
> + * Ranges from 0x40 to 0x78 provide RTC time setup for year, month,
> + * day of month, day of week, hour, minute and second.
> + */
> +#define MTK_RTC_TREG(_t, _f) (0x40 + (0x4 * (_f)) + ((_t) * 0x20))
> +
> +#define MTK_RTC_AL_CTL   0x7c
> +#define  RTC_AL_EN   BIT(0)
> +#define  RTC_AL_ALL  GENMASK(7, 0)
> +
> +/*
> + * The offset is used in the translation for the year between in struct
> + * rtc_time and in hardware register MTK_RTC_TREG(x,MTK_YEA)
> + */
> +#define MTK_RTC_TM_YR_OFFSET 100
> +
> +/*
> + * The lowest value for the valid tm_year. RTC hardware would take 
> incorrectly
> + * tm_year 100 as not a leap year and thus it is also required being excluded
> + * from the valid options.
> + */
> +#define MTK_RTC_TM_YR_L  (MTK_RTC_TM_YR_

Re: [PATCH v2 1/4] dt-bindings: rtc: mediatek: add bindings for MediaTek SoC based RTC

[Yingjoe Chen]

On Tue, 2017-10-17 at 17:40 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> Add device-tree binding for MediaTek SoC based RTC
> 
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Sean Wang 
> Acked-by: Rob Herring 
> ---
>  .../devicetree/bindings/rtc/rtc-mediatek.txt| 21 
> +
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mediatek.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-mediatek.txt 
> b/Documentation/devicetree/bindings/rtc/rtc-mediatek.txt
> new file mode 100644
> index 000..09fe8f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-mediatek.txt


How about change this filename to match driver filename change?

Joe.C

Re: [PATCH v2 1/4] dt-bindings: rtc: mediatek: add bindings for MediaTek SoC based RTC

[Yingjoe Chen]

On Tue, 2017-10-17 at 17:40 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> Add device-tree binding for MediaTek SoC based RTC
> 
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Sean Wang 
> Acked-by: Rob Herring 
> ---
>  .../devicetree/bindings/rtc/rtc-mediatek.txt| 21 
> +
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mediatek.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-mediatek.txt 
> b/Documentation/devicetree/bindings/rtc/rtc-mediatek.txt
> new file mode 100644
> index 000..09fe8f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-mediatek.txt


How about change this filename to match driver filename change?

Joe.C

Re: [PATCH v2 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC

[Yingjoe Chen]

On Tue, 2017-10-17 at 17:40 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> This patch introduces the driver for the RTC on MT7622 SoC.
> 
> Signed-off-by: Sean Wang 
> ---
>  drivers/rtc/Kconfig  |  10 ++
>  drivers/rtc/Makefile |   1 +
>  drivers/rtc/rtc-mt7622.c | 418 
> +++
>  3 files changed, 429 insertions(+)
>  create mode 100644 drivers/rtc/rtc-mt7622.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e0e58f3..4226295 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1705,6 +1705,16 @@ config RTC_DRV_MOXART
>  This driver can also be built as a module. If so, the module
>  will be called rtc-moxart
>  
> +config RTC_DRV_MEDIATEK

How about changing this to RTC_DRV_MT7622 or RTC_DRV_MEDIATEK_SOC?
It is confusing to have both RTC_DRV_MEDIATEK & RTC_DRV_MT6397 here.

> + tristate "MediaTek SoC based RTC"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + help
> +   This enables support for the real time clock built in the MediaTek
> +   SoCs.
> +
> +   This drive can also be built as a module. If so, the module
> +   will be called rtc-mediatek.
> +
>  config RTC_DRV_MT6397
>   tristate "Mediatek Real Time Clock driver"
>   depends on MFD_MT6397 || (COMPILE_TEST && IRQ_DOMAIN)
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 7230014..593a02c 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_RTC_DRV_MOXART)  += rtc-moxart.o
>  obj-$(CONFIG_RTC_DRV_MPC5121)+= rtc-mpc5121.o
>  obj-$(CONFIG_RTC_DRV_VRTC)   += rtc-mrst.o
>  obj-$(CONFIG_RTC_DRV_MSM6242)+= rtc-msm6242.o
> +obj-$(CONFIG_RTC_DRV_MEDIATEK)   += rtc-mt7622.o
>  obj-$(CONFIG_RTC_DRV_MT6397) += rtc-mt6397.o
>  obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o
>  obj-$(CONFIG_RTC_DRV_MXC)+= rtc-mxc.o
> diff --git a/drivers/rtc/rtc-mt7622.c b/drivers/rtc/rtc-mt7622.c
> new file mode 100644
> index 000..1f00494
> --- /dev/null
> +++ b/drivers/rtc/rtc-mt7622.c
> @@ -0,0 +1,418 @@
> +/*
> + * Driver for MediaTek SoC based RTC
> + *
> + * Copyright (C) 2017 Sean Wang 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define MTK_RTC_DEV KBUILD_MODNAME
> +
> +#define MTK_RTC_PWRCHK1  0x4
> +#define  RTC_PWRCHK1_MAGIC   0xc6
> +
> +#define MTK_RTC_PWRCHK2  0x8
> +#define  RTC_PWRCHK2_MAGIC   0x9a
> +
> +#define MTK_RTC_KEY  0xc
> +#define  RTC_KEY_MAGIC   0x59
> +
> +#define MTK_RTC_PROT10x10
> +#define  RTC_PROT1_MAGIC 0xa3
> +
> +#define MTK_RTC_PROT20x14
> +#define  RTC_PROT2_MAGIC 0x57
> +
> +#define MTK_RTC_PROT30x18
> +#define  RTC_PROT3_MAGIC 0x67
> +
> +#define MTK_RTC_PROT40x1c
> +#define  RTC_PROT4_MAGIC 0xd2
> +
> +#define MTK_RTC_CTL  0x20
> +#define  RTC_RC_STOP BIT(0)
> +
> +#define MTK_RTC_DEBNCE   0x2c
> +#define  RTC_DEBNCE_MASK GENMASK(2, 0)
> +
> +#define MTK_RTC_INT  0x30
> +#define RTC_INT_AL_STA   BIT(4)
> +
> +/* Ranges from 0x40 to 0x78 provide RTC time setup for year, month,
> + * day of month, day of week, hour, minute and second.
> + */
> +#define MTK_RTC_TREG(_t, _f) (0x40 + (0x4 * (_f)) + ((_t) * 0x20))
> +
> +#define MTK_RTC_AL_CTL   0x7c
> +#define  RTC_AL_EN   BIT(0)
> +#define  RTC_AL_ALL  GENMASK(7, 0)
> +
> +#define MTK_RTC_TM_YR_L  100
> +
> +/* The maximum years the RTC can support is 99, For MT7622 */
> +#define MTK_RTC_HW_YR_LIMIT  100
> +
> +/* Types of the function the RTC provides are time counter and alarm. */
> +enum {
> + MTK_TC,
> + MTK_AL,
> + MTK_TYPE_MAX,
> +};
> +
> +/* Indexes are used for the pointer to relevant registers in MTK_RTC_TREG */
> +enum {
> + MTK_YEA,
> + MTK_MON,
> + MTK_DOM,
> + MTK_DOW,
> + MTK_HOU,
> + MTK_MIN,
> + MTK_SEC
> +};
> +
> +struct mtk_rtc {
> + struct rtc_device *rtc;
> + void __iomem *base;
> + int irq;
> + struct clk *clk;
> + u32 yr_base[MTK_TYPE_MAX];
> +};
> +
> +static void mtk_w32(struct mtk_rtc *rtc, u32 reg, u32 val)
> +{
> +  

Re: [PATCH v2 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC

[Yingjoe Chen]

On Tue, 2017-10-17 at 17:40 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> This patch introduces the driver for the RTC on MT7622 SoC.
> 
> Signed-off-by: Sean Wang 
> ---
>  drivers/rtc/Kconfig  |  10 ++
>  drivers/rtc/Makefile |   1 +
>  drivers/rtc/rtc-mt7622.c | 418 
> +++
>  3 files changed, 429 insertions(+)
>  create mode 100644 drivers/rtc/rtc-mt7622.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e0e58f3..4226295 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1705,6 +1705,16 @@ config RTC_DRV_MOXART
>  This driver can also be built as a module. If so, the module
>  will be called rtc-moxart
>  
> +config RTC_DRV_MEDIATEK

How about changing this to RTC_DRV_MT7622 or RTC_DRV_MEDIATEK_SOC?
It is confusing to have both RTC_DRV_MEDIATEK & RTC_DRV_MT6397 here.

> + tristate "MediaTek SoC based RTC"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + help
> +   This enables support for the real time clock built in the MediaTek
> +   SoCs.
> +
> +   This drive can also be built as a module. If so, the module
> +   will be called rtc-mediatek.
> +
>  config RTC_DRV_MT6397
>   tristate "Mediatek Real Time Clock driver"
>   depends on MFD_MT6397 || (COMPILE_TEST && IRQ_DOMAIN)
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 7230014..593a02c 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_RTC_DRV_MOXART)  += rtc-moxart.o
>  obj-$(CONFIG_RTC_DRV_MPC5121)+= rtc-mpc5121.o
>  obj-$(CONFIG_RTC_DRV_VRTC)   += rtc-mrst.o
>  obj-$(CONFIG_RTC_DRV_MSM6242)+= rtc-msm6242.o
> +obj-$(CONFIG_RTC_DRV_MEDIATEK)   += rtc-mt7622.o
>  obj-$(CONFIG_RTC_DRV_MT6397) += rtc-mt6397.o
>  obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o
>  obj-$(CONFIG_RTC_DRV_MXC)+= rtc-mxc.o
> diff --git a/drivers/rtc/rtc-mt7622.c b/drivers/rtc/rtc-mt7622.c
> new file mode 100644
> index 000..1f00494
> --- /dev/null
> +++ b/drivers/rtc/rtc-mt7622.c
> @@ -0,0 +1,418 @@
> +/*
> + * Driver for MediaTek SoC based RTC
> + *
> + * Copyright (C) 2017 Sean Wang 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define MTK_RTC_DEV KBUILD_MODNAME
> +
> +#define MTK_RTC_PWRCHK1  0x4
> +#define  RTC_PWRCHK1_MAGIC   0xc6
> +
> +#define MTK_RTC_PWRCHK2  0x8
> +#define  RTC_PWRCHK2_MAGIC   0x9a
> +
> +#define MTK_RTC_KEY  0xc
> +#define  RTC_KEY_MAGIC   0x59
> +
> +#define MTK_RTC_PROT10x10
> +#define  RTC_PROT1_MAGIC 0xa3
> +
> +#define MTK_RTC_PROT20x14
> +#define  RTC_PROT2_MAGIC 0x57
> +
> +#define MTK_RTC_PROT30x18
> +#define  RTC_PROT3_MAGIC 0x67
> +
> +#define MTK_RTC_PROT40x1c
> +#define  RTC_PROT4_MAGIC 0xd2
> +
> +#define MTK_RTC_CTL  0x20
> +#define  RTC_RC_STOP BIT(0)
> +
> +#define MTK_RTC_DEBNCE   0x2c
> +#define  RTC_DEBNCE_MASK GENMASK(2, 0)
> +
> +#define MTK_RTC_INT  0x30
> +#define RTC_INT_AL_STA   BIT(4)
> +
> +/* Ranges from 0x40 to 0x78 provide RTC time setup for year, month,
> + * day of month, day of week, hour, minute and second.
> + */
> +#define MTK_RTC_TREG(_t, _f) (0x40 + (0x4 * (_f)) + ((_t) * 0x20))
> +
> +#define MTK_RTC_AL_CTL   0x7c
> +#define  RTC_AL_EN   BIT(0)
> +#define  RTC_AL_ALL  GENMASK(7, 0)
> +
> +#define MTK_RTC_TM_YR_L  100
> +
> +/* The maximum years the RTC can support is 99, For MT7622 */
> +#define MTK_RTC_HW_YR_LIMIT  100
> +
> +/* Types of the function the RTC provides are time counter and alarm. */
> +enum {
> + MTK_TC,
> + MTK_AL,
> + MTK_TYPE_MAX,
> +};
> +
> +/* Indexes are used for the pointer to relevant registers in MTK_RTC_TREG */
> +enum {
> + MTK_YEA,
> + MTK_MON,
> + MTK_DOM,
> + MTK_DOW,
> + MTK_HOU,
> + MTK_MIN,
> + MTK_SEC
> +};
> +
> +struct mtk_rtc {
> + struct rtc_device *rtc;
> + void __iomem *base;
> + int irq;
> + struct clk *clk;
> + u32 yr_base[MTK_TYPE_MAX];
> +};
> +
> +static void mtk_w32(struct mtk_rtc *rtc, u32 reg, u32 val)
> +{
> + writel_relaxed(val, rtc->base + reg);
> +}
> +
> +static u32 

Re: [PATCH v2 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC

[Yingjoe Chen]

On Tue, 2017-10-17 at 17:40 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> This patch introduces the driver for the RTC on MT7622 SoC.
> 
> Signed-off-by: Sean Wang 
> ---
>  drivers/rtc/Kconfig  |  10 ++
>  drivers/rtc/Makefile |   1 +
>  drivers/rtc/rtc-mt7622.c | 418 
> +++
>  3 files changed, 429 insertions(+)
>  create mode 100644 drivers/rtc/rtc-mt7622.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e0e58f3..4226295 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1705,6 +1705,16 @@ config RTC_DRV_MOXART
>  This driver can also be built as a module. If so, the module
>  will be called rtc-moxart
>  
> +config RTC_DRV_MEDIATEK
> + tristate "MediaTek SoC based RTC"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + help
> +   This enables support for the real time clock built in the MediaTek
> +   SoCs.
> +
> +   This drive can also be built as a module. If so, the module
> +   will be called rtc-mediatek.
> +
>  config RTC_DRV_MT6397
>   tristate "Mediatek Real Time Clock driver"
>   depends on MFD_MT6397 || (COMPILE_TEST && IRQ_DOMAIN)
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 7230014..593a02c 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_RTC_DRV_MOXART)  += rtc-moxart.o
>  obj-$(CONFIG_RTC_DRV_MPC5121)+= rtc-mpc5121.o
>  obj-$(CONFIG_RTC_DRV_VRTC)   += rtc-mrst.o
>  obj-$(CONFIG_RTC_DRV_MSM6242)+= rtc-msm6242.o
> +obj-$(CONFIG_RTC_DRV_MEDIATEK)   += rtc-mt7622.o
>  obj-$(CONFIG_RTC_DRV_MT6397) += rtc-mt6397.o
>  obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o
>  obj-$(CONFIG_RTC_DRV_MXC)+= rtc-mxc.o
> diff --git a/drivers/rtc/rtc-mt7622.c b/drivers/rtc/rtc-mt7622.c
> new file mode 100644
> index 000..1f00494
> --- /dev/null
> +++ b/drivers/rtc/rtc-mt7622.c
> @@ -0,0 +1,418 @@
> +/*
> + * Driver for MediaTek SoC based RTC
> + *
> + * Copyright (C) 2017 Sean Wang 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define MTK_RTC_DEV KBUILD_MODNAME
> +
> +#define MTK_RTC_PWRCHK1  0x4
> +#define  RTC_PWRCHK1_MAGIC   0xc6
> +
> +#define MTK_RTC_PWRCHK2  0x8
> +#define  RTC_PWRCHK2_MAGIC   0x9a
> +
> +#define MTK_RTC_KEY  0xc
> +#define  RTC_KEY_MAGIC   0x59
> +
> +#define MTK_RTC_PROT10x10
> +#define  RTC_PROT1_MAGIC 0xa3
> +
> +#define MTK_RTC_PROT20x14
> +#define  RTC_PROT2_MAGIC 0x57
> +
> +#define MTK_RTC_PROT30x18
> +#define  RTC_PROT3_MAGIC 0x67
> +
> +#define MTK_RTC_PROT40x1c
> +#define  RTC_PROT4_MAGIC 0xd2
> +
> +#define MTK_RTC_CTL  0x20
> +#define  RTC_RC_STOP BIT(0)
> +
> +#define MTK_RTC_DEBNCE   0x2c
> +#define  RTC_DEBNCE_MASK GENMASK(2, 0)
> +
> +#define MTK_RTC_INT  0x30
> +#define RTC_INT_AL_STA   BIT(4)
> +
> +/* Ranges from 0x40 to 0x78 provide RTC time setup for year, month,
> + * day of month, day of week, hour, minute and second.
> + */
> +#define MTK_RTC_TREG(_t, _f) (0x40 + (0x4 * (_f)) + ((_t) * 0x20))
> +
> +#define MTK_RTC_AL_CTL   0x7c
> +#define  RTC_AL_EN   BIT(0)
> +#define  RTC_AL_ALL  GENMASK(7, 0)
> +
> +#define MTK_RTC_TM_YR_L  100
> +
> +/* The maximum years the RTC can support is 99, For MT7622 */
> +#define MTK_RTC_HW_YR_LIMIT  100
> +
> +/* Types of the function the RTC provides are time counter and alarm. */
> +enum {
> + MTK_TC,
> + MTK_AL,
> + MTK_TYPE_MAX,
> +};
> +
> +/* Indexes are used for the pointer to relevant registers in MTK_RTC_TREG */
> +enum {
> + MTK_YEA,
> + MTK_MON,
> + MTK_DOM,
> + MTK_DOW,
> + MTK_HOU,
> + MTK_MIN,
> + MTK_SEC
> +};
> +
> +struct mtk_rtc {
> + struct rtc_device *rtc;
> + void __iomem *base;
> + int irq;
> + struct clk *clk;
> + u32 yr_base[MTK_TYPE_MAX];
> +};
> +
> +static void mtk_w32(struct mtk_rtc *rtc, u32 reg, u32 val)
> +{
> + writel_relaxed(val, rtc->base + reg);
> +}
> +
> +static u32 mtk_r32(struct mtk_rtc *rtc, u32 reg)
> +{
> + return 

Re: [PATCH v2 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC

[Yingjoe Chen]

On Tue, 2017-10-17 at 17:40 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> This patch introduces the driver for the RTC on MT7622 SoC.
> 
> Signed-off-by: Sean Wang 
> ---
>  drivers/rtc/Kconfig  |  10 ++
>  drivers/rtc/Makefile |   1 +
>  drivers/rtc/rtc-mt7622.c | 418 
> +++
>  3 files changed, 429 insertions(+)
>  create mode 100644 drivers/rtc/rtc-mt7622.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e0e58f3..4226295 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1705,6 +1705,16 @@ config RTC_DRV_MOXART
>  This driver can also be built as a module. If so, the module
>  will be called rtc-moxart
>  
> +config RTC_DRV_MEDIATEK
> + tristate "MediaTek SoC based RTC"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + help
> +   This enables support for the real time clock built in the MediaTek
> +   SoCs.
> +
> +   This drive can also be built as a module. If so, the module
> +   will be called rtc-mediatek.
> +
>  config RTC_DRV_MT6397
>   tristate "Mediatek Real Time Clock driver"
>   depends on MFD_MT6397 || (COMPILE_TEST && IRQ_DOMAIN)
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 7230014..593a02c 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -101,6 +101,7 @@ obj-$(CONFIG_RTC_DRV_MOXART)  += rtc-moxart.o
>  obj-$(CONFIG_RTC_DRV_MPC5121)+= rtc-mpc5121.o
>  obj-$(CONFIG_RTC_DRV_VRTC)   += rtc-mrst.o
>  obj-$(CONFIG_RTC_DRV_MSM6242)+= rtc-msm6242.o
> +obj-$(CONFIG_RTC_DRV_MEDIATEK)   += rtc-mt7622.o
>  obj-$(CONFIG_RTC_DRV_MT6397) += rtc-mt6397.o
>  obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o
>  obj-$(CONFIG_RTC_DRV_MXC)+= rtc-mxc.o
> diff --git a/drivers/rtc/rtc-mt7622.c b/drivers/rtc/rtc-mt7622.c
> new file mode 100644
> index 000..1f00494
> --- /dev/null
> +++ b/drivers/rtc/rtc-mt7622.c
> @@ -0,0 +1,418 @@
> +/*
> + * Driver for MediaTek SoC based RTC
> + *
> + * Copyright (C) 2017 Sean Wang 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define MTK_RTC_DEV KBUILD_MODNAME
> +
> +#define MTK_RTC_PWRCHK1  0x4
> +#define  RTC_PWRCHK1_MAGIC   0xc6
> +
> +#define MTK_RTC_PWRCHK2  0x8
> +#define  RTC_PWRCHK2_MAGIC   0x9a
> +
> +#define MTK_RTC_KEY  0xc
> +#define  RTC_KEY_MAGIC   0x59
> +
> +#define MTK_RTC_PROT10x10
> +#define  RTC_PROT1_MAGIC 0xa3
> +
> +#define MTK_RTC_PROT20x14
> +#define  RTC_PROT2_MAGIC 0x57
> +
> +#define MTK_RTC_PROT30x18
> +#define  RTC_PROT3_MAGIC 0x67
> +
> +#define MTK_RTC_PROT40x1c
> +#define  RTC_PROT4_MAGIC 0xd2
> +
> +#define MTK_RTC_CTL  0x20
> +#define  RTC_RC_STOP BIT(0)
> +
> +#define MTK_RTC_DEBNCE   0x2c
> +#define  RTC_DEBNCE_MASK GENMASK(2, 0)
> +
> +#define MTK_RTC_INT  0x30
> +#define RTC_INT_AL_STA   BIT(4)
> +
> +/* Ranges from 0x40 to 0x78 provide RTC time setup for year, month,
> + * day of month, day of week, hour, minute and second.
> + */
> +#define MTK_RTC_TREG(_t, _f) (0x40 + (0x4 * (_f)) + ((_t) * 0x20))
> +
> +#define MTK_RTC_AL_CTL   0x7c
> +#define  RTC_AL_EN   BIT(0)
> +#define  RTC_AL_ALL  GENMASK(7, 0)
> +
> +#define MTK_RTC_TM_YR_L  100
> +
> +/* The maximum years the RTC can support is 99, For MT7622 */
> +#define MTK_RTC_HW_YR_LIMIT  100
> +
> +/* Types of the function the RTC provides are time counter and alarm. */
> +enum {
> + MTK_TC,
> + MTK_AL,
> + MTK_TYPE_MAX,
> +};
> +
> +/* Indexes are used for the pointer to relevant registers in MTK_RTC_TREG */
> +enum {
> + MTK_YEA,
> + MTK_MON,
> + MTK_DOM,
> + MTK_DOW,
> + MTK_HOU,
> + MTK_MIN,
> + MTK_SEC
> +};
> +
> +struct mtk_rtc {
> + struct rtc_device *rtc;
> + void __iomem *base;
> + int irq;
> + struct clk *clk;
> + u32 yr_base[MTK_TYPE_MAX];
> +};
> +
> +static void mtk_w32(struct mtk_rtc *rtc, u32 reg, u32 val)
> +{
> + writel_relaxed(val, rtc->base + reg);
> +}
> +
> +static u32 mtk_r32(struct mtk_rtc *rtc, u32 reg)
> +{
> + return readl_relaxed(rtc->base + reg);
> +}
> +
> +static void mtk_rmw(struct mtk_rtc *rtc, 

Re: [PATCH 01/12] mmc: dt-bindings: update Mediatek MMC bindings

[Yingjoe Chen]

On Fri, 2017-09-29 at 09:56 +0800, Chaotian Jing wrote:
> On Wed, 2017-09-27 at 09:18 +0800, Chaotian Jing wrote:
> > On Wed, 2017-09-27 at 00:33 +0200, Ulf Hansson wrote:
> > > On 14 September 2017 at 04:10, Chaotian Jing  
> > > wrote:
> > > > On Wed, 2017-09-13 at 09:10 -0500, Rob Herring wrote:
> > > >> On Tue, Sep 12, 2017 at 05:07:41PM +0800, Chaotian Jing wrote:
> > > >> > Change the comptiable for support of multi-platform
> > > >> > Add description for reg
> > > >> > Add description for source_cg
> > > >> > Add description for mediatek,latch-ck
> > > >>
> > > >> This is at least the 3rd patch with exactly the same vague subject.
> > > >> Please make the subject somewhat unique.
> > > >>
> > > > Thx, will change the subject at next version
> > > >> >
> > > >> > Signed-off-by: Chaotian Jing 
> > > >> > ---
> > > >> >  Documentation/devicetree/bindings/mmc/mtk-sd.txt | 13 ++---
> > > >> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > >> >
> > > >> > diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.txt 
> > > >> > b/Documentation/devicetree/bindings/mmc/mtk-sd.txt
> > > >> > index 4182ea3..405cd06 100644
> > > >> > --- a/Documentation/devicetree/bindings/mmc/mtk-sd.txt
> > > >> > +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.txt
> > > >> > @@ -7,10 +7,15 @@ This file documents differences between the core 
> > > >> > properties in mmc.txt
> > > >> >  and the properties used by the msdc driver.
> > > >> >
> > > >> >  Required properties:
> > > >> > -- compatible: Should be "mediatek,mt8173-mmc","mediatek,mt8135-mmc"
> > > >> > +- compatible: value should be either of the following.
> > > >> > +   "mediatek,mt8135-mmc": for mmc host ip compatible with mt8135
> > > >> > +   "mediatek,mt8173-mmc": for mmc host ip compatible with mt8173
> > > >> > +   "mediatek,mt2701-mmc": for mmc host ip compatible with mt2701
> > > >> > +   "mediatek,mt2712-mmc": for mmc host ip compatible with mt2712
> > > >> > +- reg: physical base address of the controller and length
> > > >> >  - interrupts: Should contain MSDC interrupt number
> > > >> > -- clocks: MSDC source clock, HCLK
> > > >> > -- clock-names: "source", "hclk"
> > > >> > +- clocks: MSDC source clock, HCLK, source_cg
> > > >> > +- clock-names: "source", "hclk", "source_cg"
> > > >>
> > > >> All chips support source_cg? That's not backwards compatible for
> > > >> existing compatible strings if the driver requires it.
> > > > Not all chips support source_cg, for chips which do not support
> > > > source_cg, no need source_cg here, and the driver will parse it
> > > > to know if current chip support it.
> > > 
> > > In such case you must not add add a required binding for it. I think
> > > that is what Rob is trying to point out for you.
> > > 
> > > [...]
> > > 
> > > Kind regards
> > > Uffe
> > The source_cg is required(MUST) at MT2712 and future SoCs, but not
> > required(do not have it) at previous SoCs, so that put it at required
> > properties, let the driver to handle it.
> 
> Any other comments about it ? still must not add a required binding for
> it ? if add a optional binding for it, how to add it ? as cannot
> duplicate "clocks" & "clock-names" in one node.


Chaotian,

You can follow examples of scpsys [1]:

- clock, clock-names: clocks according to the common clock binding.
  These are clocks which hardware needs to be
  enabled before enabling certain power domains.
Required clocks for MT2701: "mm", "mfg", "ethif"
Required clocks for MT6797: "mm", "mfg", "vdec"
Required clocks for MT8173: "mm", "mfg", "venc", "venc_lt"

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt?h=v4.13.4

Joe.C

Re: [PATCH 01/12] mmc: dt-bindings: update Mediatek MMC bindings

[Yingjoe Chen]

On Fri, 2017-09-29 at 09:56 +0800, Chaotian Jing wrote:
> On Wed, 2017-09-27 at 09:18 +0800, Chaotian Jing wrote:
> > On Wed, 2017-09-27 at 00:33 +0200, Ulf Hansson wrote:
> > > On 14 September 2017 at 04:10, Chaotian Jing  
> > > wrote:
> > > > On Wed, 2017-09-13 at 09:10 -0500, Rob Herring wrote:
> > > >> On Tue, Sep 12, 2017 at 05:07:41PM +0800, Chaotian Jing wrote:
> > > >> > Change the comptiable for support of multi-platform
> > > >> > Add description for reg
> > > >> > Add description for source_cg
> > > >> > Add description for mediatek,latch-ck
> > > >>
> > > >> This is at least the 3rd patch with exactly the same vague subject.
> > > >> Please make the subject somewhat unique.
> > > >>
> > > > Thx, will change the subject at next version
> > > >> >
> > > >> > Signed-off-by: Chaotian Jing 
> > > >> > ---
> > > >> >  Documentation/devicetree/bindings/mmc/mtk-sd.txt | 13 ++---
> > > >> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > > >> >
> > > >> > diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.txt 
> > > >> > b/Documentation/devicetree/bindings/mmc/mtk-sd.txt
> > > >> > index 4182ea3..405cd06 100644
> > > >> > --- a/Documentation/devicetree/bindings/mmc/mtk-sd.txt
> > > >> > +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.txt
> > > >> > @@ -7,10 +7,15 @@ This file documents differences between the core 
> > > >> > properties in mmc.txt
> > > >> >  and the properties used by the msdc driver.
> > > >> >
> > > >> >  Required properties:
> > > >> > -- compatible: Should be "mediatek,mt8173-mmc","mediatek,mt8135-mmc"
> > > >> > +- compatible: value should be either of the following.
> > > >> > +   "mediatek,mt8135-mmc": for mmc host ip compatible with mt8135
> > > >> > +   "mediatek,mt8173-mmc": for mmc host ip compatible with mt8173
> > > >> > +   "mediatek,mt2701-mmc": for mmc host ip compatible with mt2701
> > > >> > +   "mediatek,mt2712-mmc": for mmc host ip compatible with mt2712
> > > >> > +- reg: physical base address of the controller and length
> > > >> >  - interrupts: Should contain MSDC interrupt number
> > > >> > -- clocks: MSDC source clock, HCLK
> > > >> > -- clock-names: "source", "hclk"
> > > >> > +- clocks: MSDC source clock, HCLK, source_cg
> > > >> > +- clock-names: "source", "hclk", "source_cg"
> > > >>
> > > >> All chips support source_cg? That's not backwards compatible for
> > > >> existing compatible strings if the driver requires it.
> > > > Not all chips support source_cg, for chips which do not support
> > > > source_cg, no need source_cg here, and the driver will parse it
> > > > to know if current chip support it.
> > > 
> > > In such case you must not add add a required binding for it. I think
> > > that is what Rob is trying to point out for you.
> > > 
> > > [...]
> > > 
> > > Kind regards
> > > Uffe
> > The source_cg is required(MUST) at MT2712 and future SoCs, but not
> > required(do not have it) at previous SoCs, so that put it at required
> > properties, let the driver to handle it.
> 
> Any other comments about it ? still must not add a required binding for
> it ? if add a optional binding for it, how to add it ? as cannot
> duplicate "clocks" & "clock-names" in one node.


Chaotian,

You can follow examples of scpsys [1]:

- clock, clock-names: clocks according to the common clock binding.
  These are clocks which hardware needs to be
  enabled before enabling certain power domains.
Required clocks for MT2701: "mm", "mfg", "ethif"
Required clocks for MT6797: "mm", "mfg", "vdec"
Required clocks for MT8173: "mm", "mfg", "venc", "venc_lt"

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/Documentation/devicetree/bindings/soc/mediatek/scpsys.txt?h=v4.13.4

Joe.C

Re: [PATCH v2 05/10] arm: dts: mt7623: update pio, usb and crypto nodes

[Yingjoe Chen]

On Tue, 2017-09-26 at 10:02 +0800, Ryder Lee wrote:
> This patch updates pio, usb and crypto nodes to make them be consistent
> with the binding documents.
> 
> Signed-off-by: Ryder Lee 
> ---
>  arch/arm/boot/dts/mt7623.dtsi | 52 
> ++-
>  1 file changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/mt7623.dtsi b/arch/arm/boot/dts/mt7623.dtsi
> index 381843e..9ec3767 100644
> --- a/arch/arm/boot/dts/mt7623.dtsi
> +++ b/arch/arm/boot/dts/mt7623.dtsi
> @@ -226,21 +226,6 @@
>   #reset-cells = <1>;
>   };
>  
> - pio: pinctrl@10005000 {
> - compatible = "mediatek,mt7623-pinctrl",
> -  "mediatek,mt2701-pinctrl";
> - reg = <0 0x1000b000 0 0x1000>;
> - mediatek,pctl-regmap = <_pctl_a>;
> - pins-are-numbered;
> - gpio-controller;
> - #gpio-cells = <2>;
> - interrupt-controller;
> - interrupt-parent = <>;
> - #interrupt-cells = <2>;
> - interrupts = ,
> -  ;
> - };
> -
>   syscfg_pctl_a: syscfg@10005000 {
>   compatible = "mediatek,mt7623-pctl-a-syscfg", "syscon";
>   reg = <0 0x10005000 0 0x1000>;


Hi Ryder,

pio node is special, it have 2 registers space.
The address 10005000 is for PIO, it was accesed through syscfg_pctl_a
regmap. The other one, 0x1000b000, is for EINT.

So,I think using address @10005000 is correct.

Joe.C

Re: [PATCH v2 05/10] arm: dts: mt7623: update pio, usb and crypto nodes

[Yingjoe Chen]

On Tue, 2017-09-26 at 10:02 +0800, Ryder Lee wrote:
> This patch updates pio, usb and crypto nodes to make them be consistent
> with the binding documents.
> 
> Signed-off-by: Ryder Lee 
> ---
>  arch/arm/boot/dts/mt7623.dtsi | 52 
> ++-
>  1 file changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/mt7623.dtsi b/arch/arm/boot/dts/mt7623.dtsi
> index 381843e..9ec3767 100644
> --- a/arch/arm/boot/dts/mt7623.dtsi
> +++ b/arch/arm/boot/dts/mt7623.dtsi
> @@ -226,21 +226,6 @@
>   #reset-cells = <1>;
>   };
>  
> - pio: pinctrl@10005000 {
> - compatible = "mediatek,mt7623-pinctrl",
> -  "mediatek,mt2701-pinctrl";
> - reg = <0 0x1000b000 0 0x1000>;
> - mediatek,pctl-regmap = <_pctl_a>;
> - pins-are-numbered;
> - gpio-controller;
> - #gpio-cells = <2>;
> - interrupt-controller;
> - interrupt-parent = <>;
> - #interrupt-cells = <2>;
> - interrupts = ,
> -  ;
> - };
> -
>   syscfg_pctl_a: syscfg@10005000 {
>   compatible = "mediatek,mt7623-pctl-a-syscfg", "syscon";
>   reg = <0 0x10005000 0 0x1000>;


Hi Ryder,

pio node is special, it have 2 registers space.
The address 10005000 is for PIO, it was accesed through syscfg_pctl_a
regmap. The other one, 0x1000b000, is for EINT.

So,I think using address @10005000 is correct.

Joe.C

Re: [PATCH 0/5] Add clk and scpsys support for MT6755

[Yingjoe Chen]

On Wed, 2017-08-16 at 11:59 +0800, Mars Cheng wrote:
> Hi Rob, Stephen, Matthias
> 
> gentle ping.
> 
> Thanks.
> 
> On Tue, 2017-08-08 at 16:13 +0800, Mars Cheng wrote:
> > Mars Cheng (3):
> >   clk: mediatek: add mt6755 clock ID
> >   clk: mediatek: add clk support for MT6755
> >   soc: mediatek: add MT6755 scpsys support

Hi Mars,

Sean reworked the scpsys driver to remove code duplication, see:
http://lists.infradead.org/pipermail/linux-mediatek/2017-August/009687.html
http://lists.infradead.org/pipermail/linux-mediatek/2017-August/009820.html

I think current driver will have conflict and can't merge as-is, you'll
need to rebase to Matthias' for-next branch.

Joe.C

Re: [PATCH 0/5] Add clk and scpsys support for MT6755

[Yingjoe Chen]

On Wed, 2017-08-16 at 11:59 +0800, Mars Cheng wrote:
> Hi Rob, Stephen, Matthias
> 
> gentle ping.
> 
> Thanks.
> 
> On Tue, 2017-08-08 at 16:13 +0800, Mars Cheng wrote:
> > Mars Cheng (3):
> >   clk: mediatek: add mt6755 clock ID
> >   clk: mediatek: add clk support for MT6755
> >   soc: mediatek: add MT6755 scpsys support

Hi Mars,

Sean reworked the scpsys driver to remove code duplication, see:
http://lists.infradead.org/pipermail/linux-mediatek/2017-August/009687.html
http://lists.infradead.org/pipermail/linux-mediatek/2017-August/009820.html

I think current driver will have conflict and can't merge as-is, you'll
need to rebase to Matthias' for-next branch.

Joe.C

Re: [RESEND PATCH 2/2] i2c: mediatek: Add i2c compatible for MediaTek MT7622

[Yingjoe Chen]

On Thu, 2017-08-10 at 15:22 +0800, Jun Gao wrote:
> On Thu, 2017-08-10 at 10:27 +0800, Jun Gao wrote:
> > From: Jun Gao <jun@mediatek.com>
> > 
> > Add i2c compatible for MT7622. Compare to MT8173 i2c controller,
> > MT7622 limits message numbers to 255, and does not support 4GB
> > DMA mode.
> > 
> 
> These two resend patches only modify commit message.
> Thanks!
> 
> Jun

Looks good to me, so for new version:
Reviewed-by: Yingjoe Chen <yingjoe.c...@mediatek.com>

Joe.C


> 
> > Signed-off-by: Jun Gao <jun@mediatek.com>
> > ---
> >  drivers/i2c/busses/i2c-mt65xx.c | 18 ++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-mt65xx.c 
> > b/drivers/i2c/busses/i2c-mt65xx.c
> > index 9bedf0b..2c7f847 100644
> > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > @@ -172,6 +172,14 @@ struct mtk_i2c {
> > .max_comb_2nd_msg_len = 31,
> >  };
> >  
> > +static const struct i2c_adapter_quirks mt7622_i2c_quirks = {
> > +   .max_num_msgs = 255,
> > +   .max_write_len = 65535,
> > +   .max_read_len = 65535,
> > +   .max_comb_1st_msg_len = 65535,
> > +   .max_comb_2nd_msg_len = 65535,
> > +};
> > +
> >  static const struct mtk_i2c_compatible mt6577_compat = {
> > .quirks = _i2c_quirks,
> > .pmic_i2c = 0,
> > @@ -190,6 +198,15 @@ struct mtk_i2c {
> > .support_33bits = 0,
> >  };
> >  
> > +static const struct mtk_i2c_compatible mt7622_compat = {
> > +   .quirks = _i2c_quirks,
> > +   .pmic_i2c = 0,
> > +   .dcm = 1,
> > +   .auto_restart = 1,
> > +   .aux_len_reg = 1,
> > +   .support_33bits = 0,
> > +};
> > +
> >  static const struct mtk_i2c_compatible mt8173_compat = {
> > .pmic_i2c = 0,
> > .dcm = 1,
> > @@ -201,6 +218,7 @@ struct mtk_i2c {
> >  static const struct of_device_id mtk_i2c_of_match[] = {
> > { .compatible = "mediatek,mt6577-i2c", .data = _compat },
> > { .compatible = "mediatek,mt6589-i2c", .data = _compat },
> > +   { .compatible = "mediatek,mt7622-i2c", .data = _compat },
> > { .compatible = "mediatek,mt8173-i2c", .data = _compat },
> > {}
> >  };
> 
> 
> 
> ___
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [RESEND PATCH 2/2] i2c: mediatek: Add i2c compatible for MediaTek MT7622

[Yingjoe Chen]

On Thu, 2017-08-10 at 15:22 +0800, Jun Gao wrote:
> On Thu, 2017-08-10 at 10:27 +0800, Jun Gao wrote:
> > From: Jun Gao 
> > 
> > Add i2c compatible for MT7622. Compare to MT8173 i2c controller,
> > MT7622 limits message numbers to 255, and does not support 4GB
> > DMA mode.
> > 
> 
> These two resend patches only modify commit message.
> Thanks!
> 
> Jun

Looks good to me, so for new version:
Reviewed-by: Yingjoe Chen 

Joe.C


> 
> > Signed-off-by: Jun Gao 
> > ---
> >  drivers/i2c/busses/i2c-mt65xx.c | 18 ++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-mt65xx.c 
> > b/drivers/i2c/busses/i2c-mt65xx.c
> > index 9bedf0b..2c7f847 100644
> > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > @@ -172,6 +172,14 @@ struct mtk_i2c {
> > .max_comb_2nd_msg_len = 31,
> >  };
> >  
> > +static const struct i2c_adapter_quirks mt7622_i2c_quirks = {
> > +   .max_num_msgs = 255,
> > +   .max_write_len = 65535,
> > +   .max_read_len = 65535,
> > +   .max_comb_1st_msg_len = 65535,
> > +   .max_comb_2nd_msg_len = 65535,
> > +};
> > +
> >  static const struct mtk_i2c_compatible mt6577_compat = {
> > .quirks = _i2c_quirks,
> > .pmic_i2c = 0,
> > @@ -190,6 +198,15 @@ struct mtk_i2c {
> > .support_33bits = 0,
> >  };
> >  
> > +static const struct mtk_i2c_compatible mt7622_compat = {
> > +   .quirks = _i2c_quirks,
> > +   .pmic_i2c = 0,
> > +   .dcm = 1,
> > +   .auto_restart = 1,
> > +   .aux_len_reg = 1,
> > +   .support_33bits = 0,
> > +};
> > +
> >  static const struct mtk_i2c_compatible mt8173_compat = {
> > .pmic_i2c = 0,
> > .dcm = 1,
> > @@ -201,6 +218,7 @@ struct mtk_i2c {
> >  static const struct of_device_id mtk_i2c_of_match[] = {
> > { .compatible = "mediatek,mt6577-i2c", .data = _compat },
> > { .compatible = "mediatek,mt6589-i2c", .data = _compat },
> > +   { .compatible = "mediatek,mt7622-i2c", .data = _compat },
> > { .compatible = "mediatek,mt8173-i2c", .data = _compat },
> > {}
> >  };
> 
> 
> 
> ___
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 2/2] i2c: mediatek: Add i2c compatible for MediaTek MT7622

[Yingjoe Chen]

On Wed, 2017-08-09 at 15:43 +0800, Jun Gao wrote:
> From: Jun Gao 
> 
> Add i2c compatible for MT7622. Compare to MT8173 i2c controller,
> MT7622 limit message size to 255, and not support 4GB DMA mode.


Jun,

Do you mean message numbers?


Joe.C

> 
> Signed-off-by: Jun Gao 
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index 9bedf0b..2c7f847 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -172,6 +172,14 @@ struct mtk_i2c {
>   .max_comb_2nd_msg_len = 31,
>  };
>  
> +static const struct i2c_adapter_quirks mt7622_i2c_quirks = {
> + .max_num_msgs = 255,
> + .max_write_len = 65535,
> + .max_read_len = 65535,
> + .max_comb_1st_msg_len = 65535,
> + .max_comb_2nd_msg_len = 65535,
> +};
> +
>  static const struct mtk_i2c_compatible mt6577_compat = {
>   .quirks = _i2c_quirks,
>   .pmic_i2c = 0,
> @@ -190,6 +198,15 @@ struct mtk_i2c {
>   .support_33bits = 0,
>  };
>  
> +static const struct mtk_i2c_compatible mt7622_compat = {
> + .quirks = _i2c_quirks,
> + .pmic_i2c = 0,
> + .dcm = 1,
> + .auto_restart = 1,
> + .aux_len_reg = 1,
> + .support_33bits = 0,
> +};
> +
>  static const struct mtk_i2c_compatible mt8173_compat = {
>   .pmic_i2c = 0,
>   .dcm = 1,
> @@ -201,6 +218,7 @@ struct mtk_i2c {
>  static const struct of_device_id mtk_i2c_of_match[] = {
>   { .compatible = "mediatek,mt6577-i2c", .data = _compat },
>   { .compatible = "mediatek,mt6589-i2c", .data = _compat },
> + { .compatible = "mediatek,mt7622-i2c", .data = _compat },
>   { .compatible = "mediatek,mt8173-i2c", .data = _compat },
>   {}
>  };

Re: [PATCH 2/2] i2c: mediatek: Add i2c compatible for MediaTek MT7622

[Yingjoe Chen]

On Wed, 2017-08-09 at 15:43 +0800, Jun Gao wrote:
> From: Jun Gao 
> 
> Add i2c compatible for MT7622. Compare to MT8173 i2c controller,
> MT7622 limit message size to 255, and not support 4GB DMA mode.


Jun,

Do you mean message numbers?


Joe.C

> 
> Signed-off-by: Jun Gao 
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index 9bedf0b..2c7f847 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -172,6 +172,14 @@ struct mtk_i2c {
>   .max_comb_2nd_msg_len = 31,
>  };
>  
> +static const struct i2c_adapter_quirks mt7622_i2c_quirks = {
> + .max_num_msgs = 255,
> + .max_write_len = 65535,
> + .max_read_len = 65535,
> + .max_comb_1st_msg_len = 65535,
> + .max_comb_2nd_msg_len = 65535,
> +};
> +
>  static const struct mtk_i2c_compatible mt6577_compat = {
>   .quirks = _i2c_quirks,
>   .pmic_i2c = 0,
> @@ -190,6 +198,15 @@ struct mtk_i2c {
>   .support_33bits = 0,
>  };
>  
> +static const struct mtk_i2c_compatible mt7622_compat = {
> + .quirks = _i2c_quirks,
> + .pmic_i2c = 0,
> + .dcm = 1,
> + .auto_restart = 1,
> + .aux_len_reg = 1,
> + .support_33bits = 0,
> +};
> +
>  static const struct mtk_i2c_compatible mt8173_compat = {
>   .pmic_i2c = 0,
>   .dcm = 1,
> @@ -201,6 +218,7 @@ struct mtk_i2c {
>  static const struct of_device_id mtk_i2c_of_match[] = {
>   { .compatible = "mediatek,mt6577-i2c", .data = _compat },
>   { .compatible = "mediatek,mt6589-i2c", .data = _compat },
> + { .compatible = "mediatek,mt7622-i2c", .data = _compat },
>   { .compatible = "mediatek,mt8173-i2c", .data = _compat },
>   {}
>  };

Re: [PATCH v2 3/9] regulator: mt6380: Add support for MT6380

[Yingjoe Chen]

On Tue, 2017-07-18 at 17:49 +0800, sean.w...@mediatek.com wrote:
> From: Chenglin Xu 
> 
> The MT6380 is a regulator found those boards with MediaTek MT7622 SoC
> It is connected as a slave to the SoC using MediaTek PMIC wrapper which
> is the common interface connecting with Mediatek made various PMICs.
> 
> Signed-off-by: Chenglin Xu 
> Signed-off-by: Sean Wang 
> ---
>  drivers/regulator/Kconfig  |   9 +
>  drivers/regulator/Makefile |   1 +
>  drivers/regulator/mt6380-regulator.c   | 359 
> +
>  include/linux/regulator/mt6380-regulator.h |  32 +++
>  4 files changed, 401 insertions(+)
>  create mode 100644 drivers/regulator/mt6380-regulator.c
>  create mode 100644 include/linux/regulator/mt6380-regulator.h
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 48db87d..c46ef9c 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -541,6 +541,15 @@ config REGULATOR_MT6323
> This driver supports the control of different power rails of device
> through regulator interface.
>  
> +config REGULATOR_MT6380
> + tristate "MediaTek MT6380 PMIC"
> + depends on MTK_PMIC_WRAP
> + help
> +   Say y here to select this option to enable the power regulator of
> +   MediaTek MT6380 PMIC.
> +   This driver supports the control of different power rails of device
> +   through regulator interface.
> +
>  config REGULATOR_MT6397
>   tristate "MediaTek MT6397 PMIC"
>   depends on MFD_MT6397
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index dc3503f..5148583 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
>  obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
>  obj-$(CONFIG_REGULATOR_MT6311) += mt6311-regulator.o
>  obj-$(CONFIG_REGULATOR_MT6323)   += mt6323-regulator.o
> +obj-$(CONFIG_REGULATOR_MT6380)   += mt6380-regulator.o
>  obj-$(CONFIG_REGULATOR_MT6397)   += mt6397-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
> diff --git a/drivers/regulator/mt6380-regulator.c 
> b/drivers/regulator/mt6380-regulator.c
> new file mode 100644
> index 000..5fca36f
> --- /dev/null
> +++ b/drivers/regulator/mt6380-regulator.c

<...>

> +static struct regulator_ops mt6380_volt_range_ops = {
> + .list_voltage = regulator_list_voltage_linear_range,
> + .map_voltage = regulator_map_voltage_linear_range,
> + .set_voltage_sel = regulator_set_voltage_sel_regmap,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> + .set_voltage_time_sel = regulator_set_voltage_time_sel,
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .set_mode = mt6380_regulator_set_mode,
> + .get_mode = mt6380_regulator_get_mode,
> +};
> +
> +static struct regulator_ops mt6380_volt_table_ops = {
> + .list_voltage = regulator_list_voltage_table,
> + .map_voltage = regulator_map_voltage_iterate,
> + .set_voltage_sel = regulator_set_voltage_sel_regmap,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> + .set_voltage_time_sel = regulator_set_voltage_time_sel,
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .set_mode = mt6380_regulator_set_mode,
> + .get_mode = mt6380_regulator_get_mode,
> +};
> +
> +static struct regulator_ops mt6380_volt_fixed_ops = {

this should be const.

Joe.C

Re: [PATCH v2 3/9] regulator: mt6380: Add support for MT6380

[Yingjoe Chen]

On Tue, 2017-07-18 at 17:49 +0800, sean.w...@mediatek.com wrote:
> From: Chenglin Xu 
> 
> The MT6380 is a regulator found those boards with MediaTek MT7622 SoC
> It is connected as a slave to the SoC using MediaTek PMIC wrapper which
> is the common interface connecting with Mediatek made various PMICs.
> 
> Signed-off-by: Chenglin Xu 
> Signed-off-by: Sean Wang 
> ---
>  drivers/regulator/Kconfig  |   9 +
>  drivers/regulator/Makefile |   1 +
>  drivers/regulator/mt6380-regulator.c   | 359 
> +
>  include/linux/regulator/mt6380-regulator.h |  32 +++
>  4 files changed, 401 insertions(+)
>  create mode 100644 drivers/regulator/mt6380-regulator.c
>  create mode 100644 include/linux/regulator/mt6380-regulator.h
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 48db87d..c46ef9c 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -541,6 +541,15 @@ config REGULATOR_MT6323
> This driver supports the control of different power rails of device
> through regulator interface.
>  
> +config REGULATOR_MT6380
> + tristate "MediaTek MT6380 PMIC"
> + depends on MTK_PMIC_WRAP
> + help
> +   Say y here to select this option to enable the power regulator of
> +   MediaTek MT6380 PMIC.
> +   This driver supports the control of different power rails of device
> +   through regulator interface.
> +
>  config REGULATOR_MT6397
>   tristate "MediaTek MT6397 PMIC"
>   depends on MFD_MT6397
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index dc3503f..5148583 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
>  obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
>  obj-$(CONFIG_REGULATOR_MT6311) += mt6311-regulator.o
>  obj-$(CONFIG_REGULATOR_MT6323)   += mt6323-regulator.o
> +obj-$(CONFIG_REGULATOR_MT6380)   += mt6380-regulator.o
>  obj-$(CONFIG_REGULATOR_MT6397)   += mt6397-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_RPM) += qcom_rpm-regulator.o
>  obj-$(CONFIG_REGULATOR_QCOM_SMD_RPM) += qcom_smd-regulator.o
> diff --git a/drivers/regulator/mt6380-regulator.c 
> b/drivers/regulator/mt6380-regulator.c
> new file mode 100644
> index 000..5fca36f
> --- /dev/null
> +++ b/drivers/regulator/mt6380-regulator.c

<...>

> +static struct regulator_ops mt6380_volt_range_ops = {
> + .list_voltage = regulator_list_voltage_linear_range,
> + .map_voltage = regulator_map_voltage_linear_range,
> + .set_voltage_sel = regulator_set_voltage_sel_regmap,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> + .set_voltage_time_sel = regulator_set_voltage_time_sel,
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .set_mode = mt6380_regulator_set_mode,
> + .get_mode = mt6380_regulator_get_mode,
> +};
> +
> +static struct regulator_ops mt6380_volt_table_ops = {
> + .list_voltage = regulator_list_voltage_table,
> + .map_voltage = regulator_map_voltage_iterate,
> + .set_voltage_sel = regulator_set_voltage_sel_regmap,
> + .get_voltage_sel = regulator_get_voltage_sel_regmap,
> + .set_voltage_time_sel = regulator_set_voltage_time_sel,
> + .enable = regulator_enable_regmap,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .set_mode = mt6380_regulator_set_mode,
> + .get_mode = mt6380_regulator_get_mode,
> +};
> +
> +static struct regulator_ops mt6380_volt_fixed_ops = {

this should be const.

Joe.C

Re: [PATCH 3/3] pinctrl: add mt2712 pinctrl driver

[Yingjoe Chen]

On Wed, 2017-08-02 at 14:03 +0800, Zhiyong Tao wrote:
> On Tue, 2017-08-01 at 17:14 +0800, Yingjoe Chen wrote:
> > 
> > Hi Zhiyong,
> > 
> > 
> > 
> > On Mon, 2017-07-31 at 16:22 +0800, Zhiyong Tao wrote:
> > <...>
> > > 3)Add "spec_dir_set" and "spec_dir_get" in "mtk_pinctrl_devdata".
> > > 4)Change "spec_dir_set" and add "spec_dir_get" in "pinctrl-mt2701.c"
> > >   and "pinctrl-mtk-common.c".
> > 
> > I think these deserve another patch.
> > Please also explain why we need this.
> 
> ==> ok, I will separate it in another patch in the next version.
> Because we should control another gpio base register for gpio16 and 17
> in mt2712 E1. It is special for the direction control in gpio16 and
> gpio17.
> > 
> > 
> > > 5)Change "port_mask" from "7" to "6" for EINT.
> > 
> > I'm assuming this is a bug fix for mt2701?
> > If yes, this should be a separate patch.
> 
> ==> yes, it is a bug fix for mt2701. When I use EINT bothe edge triggle,
> offset can't get the offset address which offset address is 1/3/5/7.
> I will separate it in another patch in the next version.
> > 
> > > 6)Remove generic pull config condition in "mtk_pconf_set_pull_select".
> > > 7)Change "arg" to "MTK_PUPD_SET_R1R0_00" of "mtk_pconf_set_pull_select".
> > 
> > Why we need to change arg?
> 
> ==> to parse the "bias-disable" property in dts for special pins.
> 
> > 
> > 
> > > 
> > > Signed-off-by: Zhiyong Tao <zhiyong@mediatek.com>
> > > ---
> > >  drivers/pinctrl/mediatek/Kconfig  |8 +
> > >  drivers/pinctrl/mediatek/Makefile |1 +
> > >  drivers/pinctrl/mediatek/pinctrl-mt2701.c |   21 +-
> > >  drivers/pinctrl/mediatek/pinctrl-mt2712.c |  670 +
> > >  drivers/pinctrl/mediatek/pinctrl-mtk-common.c |   16 +-
> > >  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |   44 +-
> > >  drivers/pinctrl/mediatek/pinctrl-mtk-mt2712.h | 1858 
> > > +
> > >  7 files changed, 2586 insertions(+), 32 deletions(-)
> > >  create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt2712.c
> > >  create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-mt2712.h
> > > 
> > 
> > <...>
> > 
> > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mt2701.c 
> > > b/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> > > index f86f3b3..4a43f5c 100644
> > > --- a/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> > > +++ b/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> > > @@ -503,10 +503,26 @@ static void mt2701_spec_pinmux_set(struct regmap 
> > > *reg, unsigned int pin,
> > >   regmap_update_bits(reg, mt2701_spec_pinmux[i].offset, mask, value);
> > >  }
> > >  
> > > -static void mt2701_spec_dir_set(unsigned int *reg_addr, unsigned int pin)
> > > +static int mt2701_spec_dir_set(struct mtk_pinctrl *pctl,
> > > + unsigned int *reg_addr,
> > > + unsigned int pin,
> > > + bool input)
> > >  {
> > >   if (pin > 175)
> > >   *reg_addr += 0x10;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mt2701_spec_dir_get(struct mtk_pinctrl *pctl,
> > > + unsigned int *reg_addr,
> > > + unsigned int pin,
> > > + bool input)
> > 
> > incorrect prototype?
> > 
> > > +{
> > > + if (pin > 175)
> > > + *reg_addr += 0x10;
> > > +
> > > + return 0;
> > >  }
> > >  
> > >  static const struct mtk_pinctrl_devdata mt2701_pinctrl_data = {
> > > @@ -520,6 +536,7 @@ static void mt2701_spec_dir_set(unsigned int 
> > > *reg_addr, unsigned int pin)
> > >   .spec_ies_smt_set = mt2701_ies_smt_set,
> > >   .spec_pinmux_set = mt2701_spec_pinmux_set,
> > >   .spec_dir_set = mt2701_spec_dir_set,
> > > + .spec_dir_get = mt2701_spec_dir_get,
> > >   .dir_offset = 0x,
> > >   .pullen_offset = 0x0150,
> > >   .pullsel_offset = 0x0280,
> > > @@ -551,7 +568,7 @@ static void mt2701_spec_dir_set(unsigned int 
> > > *reg_addr, unsigned int pin)
> > >   .dbnc_ctrl = 0x500,
> > >   .dbnc_set  = 0x600,
>

Re: [PATCH 3/3] pinctrl: add mt2712 pinctrl driver

[Yingjoe Chen]

On Wed, 2017-08-02 at 14:03 +0800, Zhiyong Tao wrote:
> On Tue, 2017-08-01 at 17:14 +0800, Yingjoe Chen wrote:
> > 
> > Hi Zhiyong,
> > 
> > 
> > 
> > On Mon, 2017-07-31 at 16:22 +0800, Zhiyong Tao wrote:
> > <...>
> > > 3)Add "spec_dir_set" and "spec_dir_get" in "mtk_pinctrl_devdata".
> > > 4)Change "spec_dir_set" and add "spec_dir_get" in "pinctrl-mt2701.c"
> > >   and "pinctrl-mtk-common.c".
> > 
> > I think these deserve another patch.
> > Please also explain why we need this.
> 
> ==> ok, I will separate it in another patch in the next version.
> Because we should control another gpio base register for gpio16 and 17
> in mt2712 E1. It is special for the direction control in gpio16 and
> gpio17.
> > 
> > 
> > > 5)Change "port_mask" from "7" to "6" for EINT.
> > 
> > I'm assuming this is a bug fix for mt2701?
> > If yes, this should be a separate patch.
> 
> ==> yes, it is a bug fix for mt2701. When I use EINT bothe edge triggle,
> offset can't get the offset address which offset address is 1/3/5/7.
> I will separate it in another patch in the next version.
> > 
> > > 6)Remove generic pull config condition in "mtk_pconf_set_pull_select".
> > > 7)Change "arg" to "MTK_PUPD_SET_R1R0_00" of "mtk_pconf_set_pull_select".
> > 
> > Why we need to change arg?
> 
> ==> to parse the "bias-disable" property in dts for special pins.
> 
> > 
> > 
> > > 
> > > Signed-off-by: Zhiyong Tao 
> > > ---
> > >  drivers/pinctrl/mediatek/Kconfig  |8 +
> > >  drivers/pinctrl/mediatek/Makefile |1 +
> > >  drivers/pinctrl/mediatek/pinctrl-mt2701.c |   21 +-
> > >  drivers/pinctrl/mediatek/pinctrl-mt2712.c |  670 +
> > >  drivers/pinctrl/mediatek/pinctrl-mtk-common.c |   16 +-
> > >  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |   44 +-
> > >  drivers/pinctrl/mediatek/pinctrl-mtk-mt2712.h | 1858 
> > > +
> > >  7 files changed, 2586 insertions(+), 32 deletions(-)
> > >  create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt2712.c
> > >  create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-mt2712.h
> > > 
> > 
> > <...>
> > 
> > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mt2701.c 
> > > b/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> > > index f86f3b3..4a43f5c 100644
> > > --- a/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> > > +++ b/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> > > @@ -503,10 +503,26 @@ static void mt2701_spec_pinmux_set(struct regmap 
> > > *reg, unsigned int pin,
> > >   regmap_update_bits(reg, mt2701_spec_pinmux[i].offset, mask, value);
> > >  }
> > >  
> > > -static void mt2701_spec_dir_set(unsigned int *reg_addr, unsigned int pin)
> > > +static int mt2701_spec_dir_set(struct mtk_pinctrl *pctl,
> > > + unsigned int *reg_addr,
> > > + unsigned int pin,
> > > + bool input)
> > >  {
> > >   if (pin > 175)
> > >   *reg_addr += 0x10;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int mt2701_spec_dir_get(struct mtk_pinctrl *pctl,
> > > + unsigned int *reg_addr,
> > > + unsigned int pin,
> > > + bool input)
> > 
> > incorrect prototype?
> > 
> > > +{
> > > + if (pin > 175)
> > > + *reg_addr += 0x10;
> > > +
> > > + return 0;
> > >  }
> > >  
> > >  static const struct mtk_pinctrl_devdata mt2701_pinctrl_data = {
> > > @@ -520,6 +536,7 @@ static void mt2701_spec_dir_set(unsigned int 
> > > *reg_addr, unsigned int pin)
> > >   .spec_ies_smt_set = mt2701_ies_smt_set,
> > >   .spec_pinmux_set = mt2701_spec_pinmux_set,
> > >   .spec_dir_set = mt2701_spec_dir_set,
> > > + .spec_dir_get = mt2701_spec_dir_get,
> > >   .dir_offset = 0x,
> > >   .pullen_offset = 0x0150,
> > >   .pullsel_offset = 0x0280,
> > > @@ -551,7 +568,7 @@ static void mt2701_spec_dir_set(unsigned int 
> > > *reg_addr, unsigned int pin)
> > >   .dbnc_ctrl = 0x500,
> > >   .dbnc_set  = 0x600,
> > >   

Re: [PATCH v5 03/10] arm: dts: mt7623: add mt6323.dtsi file

[Yingjoe Chen]

On Mon, 2017-07-31 at 15:36 +0800, sean.w...@mediatek.com wrote:
<...>
> diff --git a/arch/arm/boot/dts/mt7623-evb.dts 
> b/arch/arm/boot/dts/mt7623-evb.dts
> index b60b41c..0686ad7 100644
> --- a/arch/arm/boot/dts/mt7623-evb.dts
> +++ b/arch/arm/boot/dts/mt7623-evb.dts
> @@ -14,6 +14,7 @@
>  
>  /dts-v1/;
>  #include "mt7623.dtsi"
> +#include "mt6323.dtsi"
>  
>  / {
>   model = "MediaTek MT7623 evaluation board";
> @@ -23,6 +24,24 @@
>   stdout-path = 
>   };
>  
> + cpus {
> + cpu0 {
> + proc-supply = <_vproc_reg>;
> + };
> +
> + cpu1 {
> + proc-supply = <_vproc_reg>;
> + };
> +
> + cpu2 {
> + proc-supply = <_vproc_reg>;
> + };
> +
> + cpu3 {
> + proc-supply = <_vproc_reg>;
> + };
> + };
> +
>   memory {
>   reg = <0 0x8000 0 0x4000>;
>   };
> @@ -31,3 +50,13 @@
>   {
>   status = "okay";
>  };
> +
> + {
> + vmmc-supply = <_vemc3v3_reg>;
> + vqmmc-supply = <_vio18_reg>;
> +};
> +
> + {
> + vmmc-supply = <_vmch_reg>;
> + vqmmc-supply = <_vmc_reg>;
> +};

Please keep nodes sorted.

Joe.C

Re: [PATCH v5 03/10] arm: dts: mt7623: add mt6323.dtsi file

[Yingjoe Chen]

On Mon, 2017-07-31 at 15:36 +0800, sean.w...@mediatek.com wrote:
<...>
> diff --git a/arch/arm/boot/dts/mt7623-evb.dts 
> b/arch/arm/boot/dts/mt7623-evb.dts
> index b60b41c..0686ad7 100644
> --- a/arch/arm/boot/dts/mt7623-evb.dts
> +++ b/arch/arm/boot/dts/mt7623-evb.dts
> @@ -14,6 +14,7 @@
>  
>  /dts-v1/;
>  #include "mt7623.dtsi"
> +#include "mt6323.dtsi"
>  
>  / {
>   model = "MediaTek MT7623 evaluation board";
> @@ -23,6 +24,24 @@
>   stdout-path = 
>   };
>  
> + cpus {
> + cpu0 {
> + proc-supply = <_vproc_reg>;
> + };
> +
> + cpu1 {
> + proc-supply = <_vproc_reg>;
> + };
> +
> + cpu2 {
> + proc-supply = <_vproc_reg>;
> + };
> +
> + cpu3 {
> + proc-supply = <_vproc_reg>;
> + };
> + };
> +
>   memory {
>   reg = <0 0x8000 0 0x4000>;
>   };
> @@ -31,3 +50,13 @@
>   {
>   status = "okay";
>  };
> +
> + {
> + vmmc-supply = <_vemc3v3_reg>;
> + vqmmc-supply = <_vio18_reg>;
> +};
> +
> + {
> + vmmc-supply = <_vmch_reg>;
> + vqmmc-supply = <_vmc_reg>;
> +};

Please keep nodes sorted.

Joe.C

Re: [PATCH 3/3] pinctrl: add mt2712 pinctrl driver

[Yingjoe Chen]

Hi Zhiyong,



On Mon, 2017-07-31 at 16:22 +0800, Zhiyong Tao wrote:
<...>
> 3)Add "spec_dir_set" and "spec_dir_get" in "mtk_pinctrl_devdata".
> 4)Change "spec_dir_set" and add "spec_dir_get" in "pinctrl-mt2701.c"
>   and "pinctrl-mtk-common.c".

I think these deserve another patch.
Please also explain why we need this.


> 5)Change "port_mask" from "7" to "6" for EINT.

I'm assuming this is a bug fix for mt2701?
If yes, this should be a separate patch.

> 6)Remove generic pull config condition in "mtk_pconf_set_pull_select".
> 7)Change "arg" to "MTK_PUPD_SET_R1R0_00" of "mtk_pconf_set_pull_select".

Why we need to change arg?


> 
> Signed-off-by: Zhiyong Tao 
> ---
>  drivers/pinctrl/mediatek/Kconfig  |8 +
>  drivers/pinctrl/mediatek/Makefile |1 +
>  drivers/pinctrl/mediatek/pinctrl-mt2701.c |   21 +-
>  drivers/pinctrl/mediatek/pinctrl-mt2712.c |  670 +
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c |   16 +-
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |   44 +-
>  drivers/pinctrl/mediatek/pinctrl-mtk-mt2712.h | 1858 
> +
>  7 files changed, 2586 insertions(+), 32 deletions(-)
>  create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt2712.c
>  create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-mt2712.h
> 

<...>

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt2701.c 
> b/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> index f86f3b3..4a43f5c 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> @@ -503,10 +503,26 @@ static void mt2701_spec_pinmux_set(struct regmap *reg, 
> unsigned int pin,
>   regmap_update_bits(reg, mt2701_spec_pinmux[i].offset, mask, value);
>  }
>  
> -static void mt2701_spec_dir_set(unsigned int *reg_addr, unsigned int pin)
> +static int mt2701_spec_dir_set(struct mtk_pinctrl *pctl,
> + unsigned int *reg_addr,
> + unsigned int pin,
> + bool input)
>  {
>   if (pin > 175)
>   *reg_addr += 0x10;
> +
> + return 0;
> +}
> +
> +static int mt2701_spec_dir_get(struct mtk_pinctrl *pctl,
> + unsigned int *reg_addr,
> + unsigned int pin,
> + bool input)

incorrect prototype?

> +{
> + if (pin > 175)
> + *reg_addr += 0x10;
> +
> + return 0;
>  }
>  
>  static const struct mtk_pinctrl_devdata mt2701_pinctrl_data = {
> @@ -520,6 +536,7 @@ static void mt2701_spec_dir_set(unsigned int *reg_addr, 
> unsigned int pin)
>   .spec_ies_smt_set = mt2701_ies_smt_set,
>   .spec_pinmux_set = mt2701_spec_pinmux_set,
>   .spec_dir_set = mt2701_spec_dir_set,
> + .spec_dir_get = mt2701_spec_dir_get,
>   .dir_offset = 0x,
>   .pullen_offset = 0x0150,
>   .pullsel_offset = 0x0280,
> @@ -551,7 +568,7 @@ static void mt2701_spec_dir_set(unsigned int *reg_addr, 
> unsigned int pin)
>   .dbnc_ctrl = 0x500,
>   .dbnc_set  = 0x600,
>   .dbnc_clr  = 0x700,
> - .port_mask = 6,
> + .port_mask = 7,
>   .ports = 6,
>   },
>   .ap_num = 169,
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt2712.c 
> b/drivers/pinctrl/mediatek/pinctrl-mt2712.c
> new file mode 100644
> index 000..c933b75
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt2712.c

<...>

> +
> +static int mt2712_spec_dir_set(struct mtk_pinctrl *pctl,
> + unsigned int *reg_addr,
> + unsigned int pin,
> + bool input)
> +{
> + u32 reg_val;
> +
> + if (pin == 16) {
> + regmap_read(pctl->regmap2, 0x940, _val);
> + reg_val |= BIT(15);
> + if (input == true)
> + reg_val &= ~BIT(14);
> + else
> + reg_val |= BIT(14);
> + regmap_write(pctl->regmap2, 0x940, reg_val);
> + }
> +
> + if (pin == 17) {
> + regmap_read(pctl->regmap2, 0x940, _val);
> + reg_val |= BIT(7);
> + if (input == true)
> + reg_val &= ~BIT(6);
> + else
> + reg_val |= BIT(6);
> + regmap_write(pctl->regmap2, 0x940, reg_val);
> + }
> +
> + return 0;
> +}

Does this means pin 16, 17 is in different/special reg/bit location?
I didn't see spec_dir_get in your patch, does this means they are in
standard location or you just forgot it?

The original idea of spec_dir_set is to get the register offset for the
pin, so both set_direction and get_direction are using the same
extension function. Instead of adding a new spec_dir_get, can we just
extend the function to also include bit location?



<...>

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
> b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index 

Re: [PATCH 3/3] pinctrl: add mt2712 pinctrl driver

[Yingjoe Chen]

Hi Zhiyong,



On Mon, 2017-07-31 at 16:22 +0800, Zhiyong Tao wrote:
<...>
> 3)Add "spec_dir_set" and "spec_dir_get" in "mtk_pinctrl_devdata".
> 4)Change "spec_dir_set" and add "spec_dir_get" in "pinctrl-mt2701.c"
>   and "pinctrl-mtk-common.c".

I think these deserve another patch.
Please also explain why we need this.


> 5)Change "port_mask" from "7" to "6" for EINT.

I'm assuming this is a bug fix for mt2701?
If yes, this should be a separate patch.

> 6)Remove generic pull config condition in "mtk_pconf_set_pull_select".
> 7)Change "arg" to "MTK_PUPD_SET_R1R0_00" of "mtk_pconf_set_pull_select".

Why we need to change arg?


> 
> Signed-off-by: Zhiyong Tao 
> ---
>  drivers/pinctrl/mediatek/Kconfig  |8 +
>  drivers/pinctrl/mediatek/Makefile |1 +
>  drivers/pinctrl/mediatek/pinctrl-mt2701.c |   21 +-
>  drivers/pinctrl/mediatek/pinctrl-mt2712.c |  670 +
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.c |   16 +-
>  drivers/pinctrl/mediatek/pinctrl-mtk-common.h |   44 +-
>  drivers/pinctrl/mediatek/pinctrl-mtk-mt2712.h | 1858 
> +
>  7 files changed, 2586 insertions(+), 32 deletions(-)
>  create mode 100644 drivers/pinctrl/mediatek/pinctrl-mt2712.c
>  create mode 100644 drivers/pinctrl/mediatek/pinctrl-mtk-mt2712.h
> 

<...>

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt2701.c 
> b/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> index f86f3b3..4a43f5c 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt2701.c
> @@ -503,10 +503,26 @@ static void mt2701_spec_pinmux_set(struct regmap *reg, 
> unsigned int pin,
>   regmap_update_bits(reg, mt2701_spec_pinmux[i].offset, mask, value);
>  }
>  
> -static void mt2701_spec_dir_set(unsigned int *reg_addr, unsigned int pin)
> +static int mt2701_spec_dir_set(struct mtk_pinctrl *pctl,
> + unsigned int *reg_addr,
> + unsigned int pin,
> + bool input)
>  {
>   if (pin > 175)
>   *reg_addr += 0x10;
> +
> + return 0;
> +}
> +
> +static int mt2701_spec_dir_get(struct mtk_pinctrl *pctl,
> + unsigned int *reg_addr,
> + unsigned int pin,
> + bool input)

incorrect prototype?

> +{
> + if (pin > 175)
> + *reg_addr += 0x10;
> +
> + return 0;
>  }
>  
>  static const struct mtk_pinctrl_devdata mt2701_pinctrl_data = {
> @@ -520,6 +536,7 @@ static void mt2701_spec_dir_set(unsigned int *reg_addr, 
> unsigned int pin)
>   .spec_ies_smt_set = mt2701_ies_smt_set,
>   .spec_pinmux_set = mt2701_spec_pinmux_set,
>   .spec_dir_set = mt2701_spec_dir_set,
> + .spec_dir_get = mt2701_spec_dir_get,
>   .dir_offset = 0x,
>   .pullen_offset = 0x0150,
>   .pullsel_offset = 0x0280,
> @@ -551,7 +568,7 @@ static void mt2701_spec_dir_set(unsigned int *reg_addr, 
> unsigned int pin)
>   .dbnc_ctrl = 0x500,
>   .dbnc_set  = 0x600,
>   .dbnc_clr  = 0x700,
> - .port_mask = 6,
> + .port_mask = 7,
>   .ports = 6,
>   },
>   .ap_num = 169,
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mt2712.c 
> b/drivers/pinctrl/mediatek/pinctrl-mt2712.c
> new file mode 100644
> index 000..c933b75
> --- /dev/null
> +++ b/drivers/pinctrl/mediatek/pinctrl-mt2712.c

<...>

> +
> +static int mt2712_spec_dir_set(struct mtk_pinctrl *pctl,
> + unsigned int *reg_addr,
> + unsigned int pin,
> + bool input)
> +{
> + u32 reg_val;
> +
> + if (pin == 16) {
> + regmap_read(pctl->regmap2, 0x940, _val);
> + reg_val |= BIT(15);
> + if (input == true)
> + reg_val &= ~BIT(14);
> + else
> + reg_val |= BIT(14);
> + regmap_write(pctl->regmap2, 0x940, reg_val);
> + }
> +
> + if (pin == 17) {
> + regmap_read(pctl->regmap2, 0x940, _val);
> + reg_val |= BIT(7);
> + if (input == true)
> + reg_val &= ~BIT(6);
> + else
> + reg_val |= BIT(6);
> + regmap_write(pctl->regmap2, 0x940, reg_val);
> + }
> +
> + return 0;
> +}

Does this means pin 16, 17 is in different/special reg/bit location?
I didn't see spec_dir_get in your patch, does this means they are in
standard location or you just forgot it?

The original idea of spec_dir_set is to get the register offset for the
pin, so both set_direction and get_direction are using the same
extension function. Instead of adding a new spec_dir_get, can we just
extend the function to also include bit location?



<...>

> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c 
> b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
> index 3cf384f..aeec87e 100644
> 

Re: [PATCH v5 2/2] arm64: dts: Add Mediatek SoC MT2712 and evaluation board dts and Makefile

[Yingjoe Chen]

On Fri, 2017-07-28 at 19:37 +0800, YT Shen wrote:
> diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi 
> b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> new file mode 100644
> index 000..1e135af
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi

<...>

> + timer {
> + compatible = "arm,armv8-timer";
> + interrupt-parent = <>;
> + interrupts =  +   (GIC_CPU_MASK_RAW(0x13) | IRQ_TYPE_LEVEL_LOW)>,
> +   +   (GIC_CPU_MASK_RAW(0x13) | IRQ_TYPE_LEVEL_LOW)>,
> +   +   (GIC_CPU_MASK_RAW(0x13) | IRQ_TYPE_LEVEL_LOW)>,
> +   +   (GIC_CPU_MASK_RAW(0x13) | IRQ_TYPE_LEVEL_LOW)>;
> + };
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + compatible = "simple-bus";
> + ranges;

Matthias,

I notice this have soc node.
Do we need to get rid of it?

Joe.C

Re: [PATCH v5 2/2] arm64: dts: Add Mediatek SoC MT2712 and evaluation board dts and Makefile

[Yingjoe Chen]

On Fri, 2017-07-28 at 19:37 +0800, YT Shen wrote:
> diff --git a/arch/arm64/boot/dts/mediatek/mt2712e.dtsi 
> b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi
> new file mode 100644
> index 000..1e135af
> --- /dev/null
> +++ b/arch/arm64/boot/dts/mediatek/mt2712e.dtsi

<...>

> + timer {
> + compatible = "arm,armv8-timer";
> + interrupt-parent = <>;
> + interrupts =  +   (GIC_CPU_MASK_RAW(0x13) | IRQ_TYPE_LEVEL_LOW)>,
> +   +   (GIC_CPU_MASK_RAW(0x13) | IRQ_TYPE_LEVEL_LOW)>,
> +   +   (GIC_CPU_MASK_RAW(0x13) | IRQ_TYPE_LEVEL_LOW)>,
> +   +   (GIC_CPU_MASK_RAW(0x13) | IRQ_TYPE_LEVEL_LOW)>;
> + };
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> + compatible = "simple-bus";
> + ranges;

Matthias,

I notice this have soc node.
Do we need to get rid of it?

Joe.C

Re: [PATCH 0/3] PINCTRL: Mediatek pinctrl driver for mt2712

[Yingjoe Chen]

On Mon, 2017-07-31 at 16:22 +0800, Zhiyong Tao wrote:
> This series includes three patches:
> 1.Add mt2712 compatible node in binding document.
> 2.Add mt2712 pinctrl device node.
> 3.Add mt2712 pinctrl driver.
> 
> Zhiyong Tao (3):
>   dt-bindings: pinctrl: mt2712: add binding document
>   arm64: dts: mt2712: add pintcrl device node.
>   pinctrl: add mt2712 pinctrl driver
> 
>  .../devicetree/bindings/pinctrl/pinctrl-mt65xx.txt |1 +
>  arch/arm64/boot/dts/mediatek/mt2712-pinfunc.h  | 1014 +++
>  arch/arm64/boot/dts/mediatek/mt2712e.dtsi  |   18 +

mt2712e.dtsi doesn't exists in v4.13-rc1.
What's the base for this series?

Joe.C

Re: [PATCH 0/3] PINCTRL: Mediatek pinctrl driver for mt2712

[Yingjoe Chen]

On Mon, 2017-07-31 at 16:22 +0800, Zhiyong Tao wrote:
> This series includes three patches:
> 1.Add mt2712 compatible node in binding document.
> 2.Add mt2712 pinctrl device node.
> 3.Add mt2712 pinctrl driver.
> 
> Zhiyong Tao (3):
>   dt-bindings: pinctrl: mt2712: add binding document
>   arm64: dts: mt2712: add pintcrl device node.
>   pinctrl: add mt2712 pinctrl driver
> 
>  .../devicetree/bindings/pinctrl/pinctrl-mt65xx.txt |1 +
>  arch/arm64/boot/dts/mediatek/mt2712-pinfunc.h  | 1014 +++
>  arch/arm64/boot/dts/mediatek/mt2712e.dtsi  |   18 +

mt2712e.dtsi doesn't exists in v4.13-rc1.
What's the base for this series?

Joe.C

[PATCH] checkpatch: testing more config for Kconfig help text

[Yingjoe Chen]

Current help text check only check a config option if it is followed
by another config.
Adding check for help text if the next entry is menuconfig, choice/
endchoice, comment, menu/endmenu, if/endif, source or end of file.

Change-Id: I9f0b29cebb93df2cadaf77d1027208fd01efd976
Signed-off-by: Yingjoe Chen <yingjoe.c...@mediatek.com>
---
Hi,

This was sent in Jun 2016[1].
I think we should check every configs for help text.

Please let me know if there's anything need to change.
Thanks

Joe.C

[1] https://lkml.org/lkml/2016/6/24/360

---
 scripts/checkpatch.pl |8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2287a0b..f778702 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2778,6 +2778,12 @@ sub process {
next if ($f =~ /^-/);
last if (!$file && $f =~ /^\@\@/);
 
+   if ($f !~ /^[+\- ]/) {
+   # End of file
+   $is_end = 1;
+   last;
+   }
+
if ($lines[$ln - 1] =~ 
/^\+\s*(?:bool|tristate)\s*\"/) {
$is_start = 1;
} elsif ($lines[$ln - 1] =~ 
/^\+\s*(?:---)?help(?:---)?$/) {
@@ -2788,7 +2794,7 @@ sub process {
$f =~ s/#.*//;
$f =~ s/^\s+//;
next if ($f =~ /^$/);
-   if ($f =~ /^\s*config\s/) {
+   if ($f =~ 
/^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu\s*$|if\s|endif\s*$|source\s)/)
 {
$is_end = 1;
last;
}
-- 
1.7.9.5

[PATCH] checkpatch: testing more config for Kconfig help text

[Yingjoe Chen]

Current help text check only check a config option if it is followed
by another config.
Adding check for help text if the next entry is menuconfig, choice/
endchoice, comment, menu/endmenu, if/endif, source or end of file.

Change-Id: I9f0b29cebb93df2cadaf77d1027208fd01efd976
Signed-off-by: Yingjoe Chen 
---
Hi,

This was sent in Jun 2016[1].
I think we should check every configs for help text.

Please let me know if there's anything need to change.
Thanks

Joe.C

[1] https://lkml.org/lkml/2016/6/24/360

---
 scripts/checkpatch.pl |8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2287a0b..f778702 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2778,6 +2778,12 @@ sub process {
next if ($f =~ /^-/);
last if (!$file && $f =~ /^\@\@/);
 
+   if ($f !~ /^[+\- ]/) {
+   # End of file
+   $is_end = 1;
+   last;
+   }
+
if ($lines[$ln - 1] =~ 
/^\+\s*(?:bool|tristate)\s*\"/) {
$is_start = 1;
} elsif ($lines[$ln - 1] =~ 
/^\+\s*(?:---)?help(?:---)?$/) {
@@ -2788,7 +2794,7 @@ sub process {
$f =~ s/#.*//;
$f =~ s/^\s+//;
next if ($f =~ /^$/);
-   if ($f =~ /^\s*config\s/) {
+   if ($f =~ 
/^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu\s*$|if\s|endif\s*$|source\s)/)
 {
$is_end = 1;
last;
}
-- 
1.7.9.5

Re: [PATCH v2 9/9] soc: mediatek: pwrap: fixup warnings from coding style

[Yingjoe Chen]

[Resend due to bad mail format, sorry about the noise if you got two]


On Tue, 2017-07-18 at 17:49 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> fixup those warnings such as lines over 80 words and parenthesis
> alignment which would be complained by checkpatch.pl.
> 
> Signed-off-by: Sean Wang 
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 20 +---
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c 
> b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 142f40a..5f1036f 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -1057,7 +1057,8 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>   /* wait for cipher data ready@PMIC */
>   ret = pwrap_wait_for_state(wrp, pwrap_is_pmic_cipher_ready);
>   if (ret) {
> - dev_err(wrp->dev, "timeout waiting for cipher data 
> ready@PMIC\n");
> + dev_err(wrp->dev,
> + "timeout waiting for cipher data ready@PMIC\n");

Sean,

I think the old code is easier to read. When the line is only slightly
over the 80 chars limit, some maintainers will even ask to keep it in
one line to make it easier to read.

So if it is OK with Matthias, I think we should just keep it as is.

Joe.C

Re: [PATCH v2 9/9] soc: mediatek: pwrap: fixup warnings from coding style

[Yingjoe Chen]

[Resend due to bad mail format, sorry about the noise if you got two]


On Tue, 2017-07-18 at 17:49 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> fixup those warnings such as lines over 80 words and parenthesis
> alignment which would be complained by checkpatch.pl.
> 
> Signed-off-by: Sean Wang 
> ---
>  drivers/soc/mediatek/mtk-pmic-wrap.c | 20 +---
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c 
> b/drivers/soc/mediatek/mtk-pmic-wrap.c
> index 142f40a..5f1036f 100644
> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c
> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c
> @@ -1057,7 +1057,8 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp)
>   /* wait for cipher data ready@PMIC */
>   ret = pwrap_wait_for_state(wrp, pwrap_is_pmic_cipher_ready);
>   if (ret) {
> - dev_err(wrp->dev, "timeout waiting for cipher data 
> ready@PMIC\n");
> + dev_err(wrp->dev,
> + "timeout waiting for cipher data ready@PMIC\n");

Sean,

I think the old code is easier to read. When the line is only slightly
over the 80 chars limit, some maintainers will even ask to keep it in
one line to make it easier to read.

So if it is OK with Matthias, I think we should just keep it as is.

Joe.C

Re: [PATCH v2 1/9] dt-bindings: arm: mediatek: add MT7622 string to the PMIC wrapper doc

[Yingjoe Chen]

On Tue, 2017-07-18 at 17:49 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> Signed-off-by: Chenglin Xu 
> Signed-off-by: Sean Wang 
> Acked-by: Rob Herring 
> ---
>  Documentation/devicetree/bindings/soc/mediatek/pwrap.txt | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt 
> b/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> index 107700d..2d6d4a9 100644
> --- a/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> +++ b/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> @@ -35,6 +35,10 @@ Required properties in pwrap device node.
>"wrap": Main module clock
>  - clocks: Must contain an entry for each entry in clock-names.
>  
> +Required properties in pwrap device node
> +- compatible:
> + "mediatek,mt7622-pwrap" for MT7622 SoCs
> +

Sean,

Why mt7622 need to be separated from others?


Joe.C

Re: [PATCH v2 1/9] dt-bindings: arm: mediatek: add MT7622 string to the PMIC wrapper doc

[Yingjoe Chen]

On Tue, 2017-07-18 at 17:49 +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> Signed-off-by: Chenglin Xu 
> Signed-off-by: Sean Wang 
> Acked-by: Rob Herring 
> ---
>  Documentation/devicetree/bindings/soc/mediatek/pwrap.txt | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt 
> b/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> index 107700d..2d6d4a9 100644
> --- a/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> +++ b/Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> @@ -35,6 +35,10 @@ Required properties in pwrap device node.
>"wrap": Main module clock
>  - clocks: Must contain an entry for each entry in clock-names.
>  
> +Required properties in pwrap device node
> +- compatible:
> + "mediatek,mt7622-pwrap" for MT7622 SoCs
> +

Sean,

Why mt7622 need to be separated from others?


Joe.C

Re: [PATCH v2 3/4] arm: dts: mt2701: Add display bls related nodes for MT2701

[Yingjoe Chen]

On Thu, 2017-06-22 at 16:43 +0800, Erin Lo wrote:
> From: Weiqing Kong 
> 
> This patch adds board related config for backlight
> 
> Signed-off-by: Weiqing Kong 
> Signed-off-by: Erin Lo 
> ---
>  arch/arm/boot/dts/mt2701-evb.dts | 29 +
>  1 file changed, 29 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/mt2701-evb.dts 
> b/arch/arm/boot/dts/mt2701-evb.dts
> index a483798..e49c2b7 100644
> --- a/arch/arm/boot/dts/mt2701-evb.dts
> +++ b/arch/arm/boot/dts/mt2701-evb.dts

<...>

> +
> + {
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + pwm_bls_gpio: pwm_bls_gpio {
> + pins_cmd_dat {
> + pinmux = ;
> + };
> + };
>  };
>  
>   {


To help merge, I think this should be sorted according to name.

Joe.C

Re: [PATCH v2 3/4] arm: dts: mt2701: Add display bls related nodes for MT2701

[Yingjoe Chen]

On Thu, 2017-06-22 at 16:43 +0800, Erin Lo wrote:
> From: Weiqing Kong 
> 
> This patch adds board related config for backlight
> 
> Signed-off-by: Weiqing Kong 
> Signed-off-by: Erin Lo 
> ---
>  arch/arm/boot/dts/mt2701-evb.dts | 29 +
>  1 file changed, 29 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/mt2701-evb.dts 
> b/arch/arm/boot/dts/mt2701-evb.dts
> index a483798..e49c2b7 100644
> --- a/arch/arm/boot/dts/mt2701-evb.dts
> +++ b/arch/arm/boot/dts/mt2701-evb.dts

<...>

> +
> + {
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + pwm_bls_gpio: pwm_bls_gpio {
> + pins_cmd_dat {
> + pinmux = ;
> + };
> + };
>  };
>  
>   {


To help merge, I think this should be sorted according to name.

Joe.C

Re: [PATCH 4/4] dts: arm64: enable mmc3 for supporting sdio feature

[Yingjoe Chen]

On Tue, 2016-11-08 at 14:09 +0800, Yong Mao wrote:
> From: yong mao 
> 
> Add description of mmc3 for supporting sdio feature
> 
> Signed-off-by: Yong Mao 
> Signed-off-by: Chaotian Jing 
> ---
>  arch/arm64/boot/dts/mediatek/mt8173-evb.dts |   82 
> +++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts 
> b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> index 2a7f731..4dbd299 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> @@ -43,6 +43,14 @@
>   enable-active-high;
>   };
>  
> + sdio_fixed_3v3: fixedregulator@0 {

This should be regulator@1 instead of fixedregulator.

> + compatible = "regulator-fixed";
> + regulator-name = "3V3";
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> + gpio = < 85 GPIO_ACTIVE_HIGH>;
> + };
> +
>   connector {
>   compatible = "hdmi-connector";
>   label = "hdmi";
> @@ -139,6 +147,25 @@
>   vqmmc-supply = <_vmc_reg>;
>  };
>  
> + {
> + status = "okay";
> + pinctrl-names = "default", "state_uhs";
> + pinctrl-0 = <_pins_default>;
> + pinctrl-1 = <_pins_uhs>;
> + bus-width = <4>;
> + max-frequency = <2>;
> + cap-sd-highspeed;
> + sd-uhs-sdr50;
> + sd-uhs-sdr104;
> + sdr104-clk-delay = <5>;
> + keep-power-in-suspend;
> + enable-sdio-wakeup;
> + cap-sdio-irq;
> + vmmc-supply = <_fixed_3v3>;
> + vqmmc-supply = <_vgp3_reg>;
> + non-removable;
> +};
> +
>   {
>   disp_pwm0_pins: disp_pwm0_pins {
>   pins1 {
> @@ -197,6 +224,36 @@
>   };
>   };
>  
> + mmc3_pins_default: mmc3default {

Please keep nodes in  sorted, move this one after mmc1_pins_uhs.

> + pins_dat {
> + pinmux = ,
> +  ,
> +  ,
> +  ;
> + input-enable;
> + drive-strength = ;
> + bias-pull-up = ;
> + };
> +
> + pins_cmd {
> + pinmux = ;
> + input-enable;
> + drive-strength = ;
> + bias-pull-up = ;
> + };
> +
> + pins_clk {
> + pinmux = ;
> + bias-pull-down;
> + drive-strength = ;
> + };
> +
> + pins_pdn {
> + pinmux = ;
> + output-low;
> + };

This one is used in regulator, not really an mmc pin.
Also, you don't need to add node for gpio usage, request_gpio will set
mode for you.
So please remove pins_pdn node.


> + };
> +
>   mmc0_pins_uhs: mmc0 {
>   pins_cmd_dat {
>   pinmux = ,
> @@ -243,6 +300,31 @@
>   bias-pull-down = ;
>   };
>   };
> +
> + mmc3_pins_uhs: mmc3 {


Please keep nodes in  sorted, move this one after mmc1_pins_uhs.


Joe.C

> + pins_dat {
> + pinmux = ,
> +  ,
> +  ,
> +  ;
> + input-enable;
> + drive-strength = ;
> + bias-pull-up = ;
> + };
> +
> + pins_cmd {
> + pinmux = ;
> + input-enable;
> + drive-strength = ;
> + bias-pull-up = ;
> + };
> +
> + pins_clk {
> + pinmux = ;
> + drive-strength = ;
> + bias-pull-down = ;
> + };
> + };
>  };
>  
>   {

Re: [PATCH 4/4] dts: arm64: enable mmc3 for supporting sdio feature

[Yingjoe Chen]

On Tue, 2016-11-08 at 14:09 +0800, Yong Mao wrote:
> From: yong mao 
> 
> Add description of mmc3 for supporting sdio feature
> 
> Signed-off-by: Yong Mao 
> Signed-off-by: Chaotian Jing 
> ---
>  arch/arm64/boot/dts/mediatek/mt8173-evb.dts |   82 
> +++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts 
> b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> index 2a7f731..4dbd299 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
> @@ -43,6 +43,14 @@
>   enable-active-high;
>   };
>  
> + sdio_fixed_3v3: fixedregulator@0 {

This should be regulator@1 instead of fixedregulator.

> + compatible = "regulator-fixed";
> + regulator-name = "3V3";
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> + gpio = < 85 GPIO_ACTIVE_HIGH>;
> + };
> +
>   connector {
>   compatible = "hdmi-connector";
>   label = "hdmi";
> @@ -139,6 +147,25 @@
>   vqmmc-supply = <_vmc_reg>;
>  };
>  
> + {
> + status = "okay";
> + pinctrl-names = "default", "state_uhs";
> + pinctrl-0 = <_pins_default>;
> + pinctrl-1 = <_pins_uhs>;
> + bus-width = <4>;
> + max-frequency = <2>;
> + cap-sd-highspeed;
> + sd-uhs-sdr50;
> + sd-uhs-sdr104;
> + sdr104-clk-delay = <5>;
> + keep-power-in-suspend;
> + enable-sdio-wakeup;
> + cap-sdio-irq;
> + vmmc-supply = <_fixed_3v3>;
> + vqmmc-supply = <_vgp3_reg>;
> + non-removable;
> +};
> +
>   {
>   disp_pwm0_pins: disp_pwm0_pins {
>   pins1 {
> @@ -197,6 +224,36 @@
>   };
>   };
>  
> + mmc3_pins_default: mmc3default {

Please keep nodes in  sorted, move this one after mmc1_pins_uhs.

> + pins_dat {
> + pinmux = ,
> +  ,
> +  ,
> +  ;
> + input-enable;
> + drive-strength = ;
> + bias-pull-up = ;
> + };
> +
> + pins_cmd {
> + pinmux = ;
> + input-enable;
> + drive-strength = ;
> + bias-pull-up = ;
> + };
> +
> + pins_clk {
> + pinmux = ;
> + bias-pull-down;
> + drive-strength = ;
> + };
> +
> + pins_pdn {
> + pinmux = ;
> + output-low;
> + };

This one is used in regulator, not really an mmc pin.
Also, you don't need to add node for gpio usage, request_gpio will set
mode for you.
So please remove pins_pdn node.


> + };
> +
>   mmc0_pins_uhs: mmc0 {
>   pins_cmd_dat {
>   pinmux = ,
> @@ -243,6 +300,31 @@
>   bias-pull-down = ;
>   };
>   };
> +
> + mmc3_pins_uhs: mmc3 {


Please keep nodes in  sorted, move this one after mmc1_pins_uhs.


Joe.C

> + pins_dat {
> + pinmux = ,
> +  ,
> +  ,
> +  ;
> + input-enable;
> + drive-strength = ;
> + bias-pull-up = ;
> + };
> +
> + pins_cmd {
> + pinmux = ;
> + input-enable;
> + drive-strength = ;
> + bias-pull-up = ;
> + };
> +
> + pins_clk {
> + pinmux = ;
> + drive-strength = ;
> + bias-pull-down = ;
> + };
> + };
>  };
>  
>   {

Re: [PATCH] vcodec: mediatek: add Maintainers entry for Mediatek MT8173 vcodec drivers

[Yingjoe Chen]

On Tue, 2016-09-06 at 14:35 +0800, Tiffany Lin wrote:
> Add Tiffany Lin and Andrew-CT Chen as maintainers for
> Mediatek MT8173 vcodec drivers
> 
> Signed-off-by: Tiffany Lin 
> Signed-off-by: Andrew-CT Chen 
> ---
>  MAINTAINERS |9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0a16a82..ed830c7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7590,6 +7590,15 @@ F: include/uapi/linux/meye.h
>  F:   include/uapi/linux/ivtv*
>  F:   include/uapi/linux/uvcvideo.h
>  
> +MT8173 MEDIA DRIVER

We might upstream mediate driver for other SoC based on this driver.
I think we can just write "MEDIATEK MEDIA DRIVER" here.

Joe.C


> +M:   Tiffany Lin 
> +M:   Andrew-CT Chen 
> +S:   Supported
> +F:   drivers/media/platform/mtk-vcodec/
> +F:   drivers/media/platform/mtk-vpu/
> +F:   Documentation/devicetree/bindings/media/mediatek-vcodec.txt
> +F:   Documentation/devicetree/bindings/media/mediatek-vpu.txt
> +
>  MEDIATEK ETHERNET DRIVER
>  M:   Felix Fietkau 
>  M:   John Crispin 

Re: [PATCH] vcodec: mediatek: add Maintainers entry for Mediatek MT8173 vcodec drivers

[Yingjoe Chen]

On Tue, 2016-09-06 at 14:35 +0800, Tiffany Lin wrote:
> Add Tiffany Lin and Andrew-CT Chen as maintainers for
> Mediatek MT8173 vcodec drivers
> 
> Signed-off-by: Tiffany Lin 
> Signed-off-by: Andrew-CT Chen 
> ---
>  MAINTAINERS |9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0a16a82..ed830c7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7590,6 +7590,15 @@ F: include/uapi/linux/meye.h
>  F:   include/uapi/linux/ivtv*
>  F:   include/uapi/linux/uvcvideo.h
>  
> +MT8173 MEDIA DRIVER

We might upstream mediate driver for other SoC based on this driver.
I think we can just write "MEDIATEK MEDIA DRIVER" here.

Joe.C


> +M:   Tiffany Lin 
> +M:   Andrew-CT Chen 
> +S:   Supported
> +F:   drivers/media/platform/mtk-vcodec/
> +F:   drivers/media/platform/mtk-vpu/
> +F:   Documentation/devicetree/bindings/media/mediatek-vcodec.txt
> +F:   Documentation/devicetree/bindings/media/mediatek-vpu.txt
> +
>  MEDIATEK ETHERNET DRIVER
>  M:   Felix Fietkau 
>  M:   John Crispin 

Re: [PATCH v7 1/4] soc: mediatek: Refine scpsys to support multiple platform

[Yingjoe Chen]

On Mon, 2016-07-11 at 15:10 +0200, Matthias Brugger wrote:
> 
> On 11/07/16 10:56, James Liao wrote:
> 
> [...]
> 
> > @@ -467,28 +386,54 @@ static int scpsys_probe(struct platform_device 
> > *pdev)
> > if (PTR_ERR(scpd->supply) == -ENODEV)
> > scpd->supply = NULL;
> > else
> > -   return PTR_ERR(scpd->supply);
> > +   return ERR_CAST(scpd->supply);
> > }
> > }
> >
> > -   pd_data->num_domains = NUM_DOMAINS;
> > +   pd_data->num_domains = num;
> >
> > -   for (i = 0; i < NUM_DOMAINS; i++) {
> > +   init_clks(pdev, clk);
> > +
> > +   for (i = 0; i < num; i++) {
> > struct scp_domain *scpd = >domains[i];
> > struct generic_pm_domain *genpd = >genpd;
> > const struct scp_domain_data *data = 
> > _domain_data[i];
> >
> > +   for (j = 0; j < MAX_CLKS && data->clk_id[j]; j++) {
> > +   struct clk *c = clk[data->clk_id[j]];
> > +
> > +   if (IS_ERR(c)) {
> > +   dev_err(>dev, "%s: clk 
> > unavailable\n",
> > +   data->name);
> > +   return ERR_CAST(c);
> > +   }
> > +
> > +   scpd->clk[j] = c;
> 
>  Put this in the else branch. Apart from that is there any reason you
> >>>
> >>> Do you mean to change like this?
> >>>
> >>>   if (IS_ERR(c)) {
> >>>   ...
> >>>   return ERR_CAST(c);
> >>>   } else {
> >>>   scpd->clk[j] = c;
> >>>   }
> >>>
> >>> checkpatch.pl will warn for above code due to it returns in 'if' branch.
> >>>
> >>
> >> I tried that on top of next-20160706 and it checkpatch didn't throw any
> >> warning. Which kernel version are based on?
> >
> > I don't remember which version of checkpatch warn on this pattern. This
> > patch series develop across several kernel versions.
> 
> We as the kernel community develop against master or linux-next. We only 
> care about older kernel version in the sense that we intent hard not to 
> break any userspace/kernel or firmware/kernel interfaces. Apart from 
> that it's up to every individual to backport patches from mainline 
> kernel to his respective version. But that's nothing the community as a 
> hole can take care of.
> 
> >
> > So do you prefer to put "scpd->clk[j] = c;" into 'else' branch?
> >
> 
> Yes please :)


Hi,

I just got next-20160711 and change this chunk to:

+   for (j = 0; j < MAX_CLKS && data->clk_id[j]; j++) {
+   struct clk *c = clk[data->clk_id[j]];
+
+   if (IS_ERR(c)) {
+   dev_err(>dev, "%s: clk unavailable\n",
+   data->name);
+   return ERR_CAST(c);
+   } else {
+   scpd->clk[j] = c;
+   }
+   }
+


and checkpatch give me this warning:

WARNING: else is not generally useful after a break or return
#313: FILE: drivers/soc/mediatek/mtk-scpsys.c:409:
+   return ERR_CAST(c);
+   } else {

Joe.C

Re: [PATCH v7 1/4] soc: mediatek: Refine scpsys to support multiple platform

[Yingjoe Chen]

On Mon, 2016-07-11 at 15:10 +0200, Matthias Brugger wrote:
> 
> On 11/07/16 10:56, James Liao wrote:
> 
> [...]
> 
> > @@ -467,28 +386,54 @@ static int scpsys_probe(struct platform_device 
> > *pdev)
> > if (PTR_ERR(scpd->supply) == -ENODEV)
> > scpd->supply = NULL;
> > else
> > -   return PTR_ERR(scpd->supply);
> > +   return ERR_CAST(scpd->supply);
> > }
> > }
> >
> > -   pd_data->num_domains = NUM_DOMAINS;
> > +   pd_data->num_domains = num;
> >
> > -   for (i = 0; i < NUM_DOMAINS; i++) {
> > +   init_clks(pdev, clk);
> > +
> > +   for (i = 0; i < num; i++) {
> > struct scp_domain *scpd = >domains[i];
> > struct generic_pm_domain *genpd = >genpd;
> > const struct scp_domain_data *data = 
> > _domain_data[i];
> >
> > +   for (j = 0; j < MAX_CLKS && data->clk_id[j]; j++) {
> > +   struct clk *c = clk[data->clk_id[j]];
> > +
> > +   if (IS_ERR(c)) {
> > +   dev_err(>dev, "%s: clk 
> > unavailable\n",
> > +   data->name);
> > +   return ERR_CAST(c);
> > +   }
> > +
> > +   scpd->clk[j] = c;
> 
>  Put this in the else branch. Apart from that is there any reason you
> >>>
> >>> Do you mean to change like this?
> >>>
> >>>   if (IS_ERR(c)) {
> >>>   ...
> >>>   return ERR_CAST(c);
> >>>   } else {
> >>>   scpd->clk[j] = c;
> >>>   }
> >>>
> >>> checkpatch.pl will warn for above code due to it returns in 'if' branch.
> >>>
> >>
> >> I tried that on top of next-20160706 and it checkpatch didn't throw any
> >> warning. Which kernel version are based on?
> >
> > I don't remember which version of checkpatch warn on this pattern. This
> > patch series develop across several kernel versions.
> 
> We as the kernel community develop against master or linux-next. We only 
> care about older kernel version in the sense that we intent hard not to 
> break any userspace/kernel or firmware/kernel interfaces. Apart from 
> that it's up to every individual to backport patches from mainline 
> kernel to his respective version. But that's nothing the community as a 
> hole can take care of.
> 
> >
> > So do you prefer to put "scpd->clk[j] = c;" into 'else' branch?
> >
> 
> Yes please :)


Hi,

I just got next-20160711 and change this chunk to:

+   for (j = 0; j < MAX_CLKS && data->clk_id[j]; j++) {
+   struct clk *c = clk[data->clk_id[j]];
+
+   if (IS_ERR(c)) {
+   dev_err(>dev, "%s: clk unavailable\n",
+   data->name);
+   return ERR_CAST(c);
+   } else {
+   scpd->clk[j] = c;
+   }
+   }
+


and checkpatch give me this warning:

WARNING: else is not generally useful after a break or return
#313: FILE: drivers/soc/mediatek/mtk-scpsys.c:409:
+   return ERR_CAST(c);
+   } else {

Joe.C

[PATCH v4 1/2] checkpatch: testing more config for Kconfig help text

[Yingjoe Chen]

Current help text check only check a config option if it is followed
by another config.
Adding check for help text if the next entry is menuconfig, choice/
endchoice, comment, menu/endmenu, if/endif, source or end of file.

Signed-off-by: Yingjoe Chen <yingjoe.c...@mediatek.com>
---
checkpatch Kconfig checking stuff again.

Change in this round:
In 'default n' check, don't warn if user comment on why the
'default n' is neccessary.

I also change patch order. The first one extend help message check
to check for all available config entries. If you think 'default n'
check is not that useful, please consider just merge this one.

Let me know what you think. Thanks.

Change in v3:
- Rebase to v4.7-rc1

Change in v2:
- Change according to Joe Perches' suggestion
---
 scripts/checkpatch.pl | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6750595..19b270b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2646,6 +2646,12 @@ sub process {
next if ($f =~ /^-/);
last if (!$file && $f =~ /^\@\@/);
 
+   if ($f !~ /^[+\- ]/) {
+   # End of file
+   $is_end = 1;
+   last;
+   }
+
if ($lines[$ln - 1] =~ 
/^\+\s*(?:bool|tristate)\s*\"/) {
$is_start = 1;
} elsif ($lines[$ln - 1] =~ 
/^\+\s*(?:---)?help(?:---)?$/) {
@@ -2656,7 +2662,7 @@ sub process {
$f =~ s/#.*//;
$f =~ s/^\s+//;
next if ($f =~ /^$/);
-   if ($f =~ /^\s*config\s/) {
+   if ($f =~ 
/^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu\s*$|if\s|endif\s*$|source\s)/)
 {
$is_end = 1;
last;
}
-- 
1.9.1

[PATCH v4 1/2] checkpatch: testing more config for Kconfig help text

[Yingjoe Chen]

Current help text check only check a config option if it is followed
by another config.
Adding check for help text if the next entry is menuconfig, choice/
endchoice, comment, menu/endmenu, if/endif, source or end of file.

Signed-off-by: Yingjoe Chen 
---
checkpatch Kconfig checking stuff again.

Change in this round:
In 'default n' check, don't warn if user comment on why the
'default n' is neccessary.

I also change patch order. The first one extend help message check
to check for all available config entries. If you think 'default n'
check is not that useful, please consider just merge this one.

Let me know what you think. Thanks.

Change in v3:
- Rebase to v4.7-rc1

Change in v2:
- Change according to Joe Perches' suggestion
---
 scripts/checkpatch.pl | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6750595..19b270b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2646,6 +2646,12 @@ sub process {
next if ($f =~ /^-/);
last if (!$file && $f =~ /^\@\@/);
 
+   if ($f !~ /^[+\- ]/) {
+   # End of file
+   $is_end = 1;
+   last;
+   }
+
if ($lines[$ln - 1] =~ 
/^\+\s*(?:bool|tristate)\s*\"/) {
$is_start = 1;
} elsif ($lines[$ln - 1] =~ 
/^\+\s*(?:---)?help(?:---)?$/) {
@@ -2656,7 +2662,7 @@ sub process {
$f =~ s/#.*//;
$f =~ s/^\s+//;
next if ($f =~ /^$/);
-   if ($f =~ /^\s*config\s/) {
+   if ($f =~ 
/^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu\s*$|if\s|endif\s*$|source\s)/)
 {
$is_end = 1;
last;
}
-- 
1.9.1

[PATCH v4 2/2] checkpatch: add Kconfig 'default n' test

[Yingjoe Chen]

If a Kconfig config option doesn't specify 'default', the default
will be n. Adding 'default n' is unnecessary.

Add a test to warn about this.

Signed-off-by: Yingjoe Chen <yingjoe.c...@mediatek.com>
---
 scripts/checkpatch.pl | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 19b270b..b3f1fda 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2689,6 +2689,13 @@ sub process {
 "Use of boolean is deprecated, please use bool 
instead.\n" . $herecurr);
}
 
+# discourage the use of default n
+   if ($realfile =~ /Kconfig/ &&
+   $line =~ /^\+\s*default\s*n$/i) {
+   WARN("CONFIG_DEFAULT_N",
+"Use of default n is unnecessary, default is n 
when omitted.\n" . $herecurr);
+   }
+
if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
my $flag = $1;
-- 
1.9.1

[PATCH v4 2/2] checkpatch: add Kconfig 'default n' test

[Yingjoe Chen]

If a Kconfig config option doesn't specify 'default', the default
will be n. Adding 'default n' is unnecessary.

Add a test to warn about this.

Signed-off-by: Yingjoe Chen 
---
 scripts/checkpatch.pl | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 19b270b..b3f1fda 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2689,6 +2689,13 @@ sub process {
 "Use of boolean is deprecated, please use bool 
instead.\n" . $herecurr);
}
 
+# discourage the use of default n
+   if ($realfile =~ /Kconfig/ &&
+   $line =~ /^\+\s*default\s*n$/i) {
+   WARN("CONFIG_DEFAULT_N",
+"Use of default n is unnecessary, default is n 
when omitted.\n" . $herecurr);
+   }
+
if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
my $flag = $1;
-- 
1.9.1

Re: [alsa-devel] [PATCH v4 9/9] ASoC: mediatek: Add mt2701-cs42448 driver and config option.

[Yingjoe Chen]

On Mon, 2016-06-13 at 15:26 +0800, Garlic Tseng wrote:
> Add machine driver and config option for MT2701.
> 
> Signed-off-by: Garlic Tseng 
> ---
>  sound/soc/mediatek/Kconfig |  21 ++
>  sound/soc/mediatek/Makefile|   1 +
>  sound/soc/mediatek/mt2701/Makefile |  19 ++
>  sound/soc/mediatek/mt2701/mt2701-cs42448.c | 422 
> +
>  4 files changed, 463 insertions(+)
>  create mode 100644 sound/soc/mediatek/mt2701/Makefile
>  create mode 100644 sound/soc/mediatek/mt2701/mt2701-cs42448.c
> 
> diff --git a/sound/soc/mediatek/Kconfig b/sound/soc/mediatek/Kconfig
> index ff1a419..0ecb785 100644
> --- a/sound/soc/mediatek/Kconfig
> +++ b/sound/soc/mediatek/Kconfig
> @@ -7,6 +7,27 @@ config SND_SOC_MEDIATEK
> Select Y if you have such device.
> If unsure select "N".
>  
> +config SND_SOC_MT2701
> + tristate "ASoC support for Mediatek MT2701 chip"
> + depends on ARCH_MEDIATEK
> + select SND_SOC_MEDIATEK
> + help
> +   This adds ASoC driver for Mediatek MT2701 boards
> +   that can be used with other codecs.
> +   Select Y if you have such device.
> +   If unsure select "N".
> +
> +config SND_SOC_MT2701_CS42448
> + tristate "SND_SOC_MT2701_CS42448"

Please provide proper config prompt here.

Joe.C

Re: [alsa-devel] [PATCH v4 9/9] ASoC: mediatek: Add mt2701-cs42448 driver and config option.

[Yingjoe Chen]

On Mon, 2016-06-13 at 15:26 +0800, Garlic Tseng wrote:
> Add machine driver and config option for MT2701.
> 
> Signed-off-by: Garlic Tseng 
> ---
>  sound/soc/mediatek/Kconfig |  21 ++
>  sound/soc/mediatek/Makefile|   1 +
>  sound/soc/mediatek/mt2701/Makefile |  19 ++
>  sound/soc/mediatek/mt2701/mt2701-cs42448.c | 422 
> +
>  4 files changed, 463 insertions(+)
>  create mode 100644 sound/soc/mediatek/mt2701/Makefile
>  create mode 100644 sound/soc/mediatek/mt2701/mt2701-cs42448.c
> 
> diff --git a/sound/soc/mediatek/Kconfig b/sound/soc/mediatek/Kconfig
> index ff1a419..0ecb785 100644
> --- a/sound/soc/mediatek/Kconfig
> +++ b/sound/soc/mediatek/Kconfig
> @@ -7,6 +7,27 @@ config SND_SOC_MEDIATEK
> Select Y if you have such device.
> If unsure select "N".
>  
> +config SND_SOC_MT2701
> + tristate "ASoC support for Mediatek MT2701 chip"
> + depends on ARCH_MEDIATEK
> + select SND_SOC_MEDIATEK
> + help
> +   This adds ASoC driver for Mediatek MT2701 boards
> +   that can be used with other codecs.
> +   Select Y if you have such device.
> +   If unsure select "N".
> +
> +config SND_SOC_MT2701_CS42448
> + tristate "SND_SOC_MT2701_CS42448"

Please provide proper config prompt here.

Joe.C

Re: [alsa-devel] [PATCH v4 3/9] ASoC: mediatek: let mt8173 use mediatek common structure

[Yingjoe Chen]

On Mon, 2016-06-13 at 15:26 +0800, Garlic Tseng wrote:
> Modify mt8173 driver implementation to use common structure.
> 
> Signed-off-by: Garlic Tseng 
> ---
>  sound/soc/mediatek/Kconfig|  11 +
>  sound/soc/mediatek/Makefile   |   3 +-
>  sound/soc/mediatek/common/Makefile|  17 +
>  sound/soc/mediatek/mt8173/mt8173-afe-common.h |  42 +-
>  sound/soc/mediatek/mt8173/mt8173-afe-pcm.c| 679 
> +++---
>  5 files changed, 327 insertions(+), 425 deletions(-)
>  create mode 100644 sound/soc/mediatek/common/Makefile
> 
> diff --git a/sound/soc/mediatek/Kconfig b/sound/soc/mediatek/Kconfig
> index ae9f664..ff1a419 100644
> --- a/sound/soc/mediatek/Kconfig
> +++ b/sound/soc/mediatek/Kconfig
> @@ -1,6 +1,16 @@
> +config SND_SOC_MEDIATEK
> + tristate "SND_SOC_MEDIATEK"
> + depends on ARCH_MEDIATEK
> + help
> +   This adds ASoC driver for Mediatek boards
> +   that can be used with other codecs.
> +   Select Y if you have such device.
> +   If unsure select "N".
> +
>  config SND_SOC_MT8173
>   tristate "ASoC support for Mediatek MT8173 chip"
>   depends on ARCH_MEDIATEK
> + select SND_SOC_MEDIATEK
>   help
> This adds ASoC platform driver support for Mediatek MT8173 chip
> that can be used with other codecs.

It seems both SND_SOC_MT8173 and SND_SOC_MT2701 select SND_SOC_MEDIATEK,
and enabling only SND_SOC_MEDIATEK itself is not really useful. Do we
need to make SND_SOC_MEDIATEK user configurable option?

If it is, we should at least have a proper config prompt instead of just
"SND_SOC_MEDIATEK".


> @@ -49,3 +59,4 @@ config SND_SOC_MT8173_RT5650_RT5676
> with the RT5650 and RT5676 codecs.
> Select Y if you have such device.
> If unsure select "N".
> +

nit: This change is not necessary.

Joe.C

Re: [alsa-devel] [PATCH v4 3/9] ASoC: mediatek: let mt8173 use mediatek common structure

[Yingjoe Chen]

On Mon, 2016-06-13 at 15:26 +0800, Garlic Tseng wrote:
> Modify mt8173 driver implementation to use common structure.
> 
> Signed-off-by: Garlic Tseng 
> ---
>  sound/soc/mediatek/Kconfig|  11 +
>  sound/soc/mediatek/Makefile   |   3 +-
>  sound/soc/mediatek/common/Makefile|  17 +
>  sound/soc/mediatek/mt8173/mt8173-afe-common.h |  42 +-
>  sound/soc/mediatek/mt8173/mt8173-afe-pcm.c| 679 
> +++---
>  5 files changed, 327 insertions(+), 425 deletions(-)
>  create mode 100644 sound/soc/mediatek/common/Makefile
> 
> diff --git a/sound/soc/mediatek/Kconfig b/sound/soc/mediatek/Kconfig
> index ae9f664..ff1a419 100644
> --- a/sound/soc/mediatek/Kconfig
> +++ b/sound/soc/mediatek/Kconfig
> @@ -1,6 +1,16 @@
> +config SND_SOC_MEDIATEK
> + tristate "SND_SOC_MEDIATEK"
> + depends on ARCH_MEDIATEK
> + help
> +   This adds ASoC driver for Mediatek boards
> +   that can be used with other codecs.
> +   Select Y if you have such device.
> +   If unsure select "N".
> +
>  config SND_SOC_MT8173
>   tristate "ASoC support for Mediatek MT8173 chip"
>   depends on ARCH_MEDIATEK
> + select SND_SOC_MEDIATEK
>   help
> This adds ASoC platform driver support for Mediatek MT8173 chip
> that can be used with other codecs.

It seems both SND_SOC_MT8173 and SND_SOC_MT2701 select SND_SOC_MEDIATEK,
and enabling only SND_SOC_MEDIATEK itself is not really useful. Do we
need to make SND_SOC_MEDIATEK user configurable option?

If it is, we should at least have a proper config prompt instead of just
"SND_SOC_MEDIATEK".


> @@ -49,3 +59,4 @@ config SND_SOC_MT8173_RT5650_RT5676
> with the RT5650 and RT5676 codecs.
> Select Y if you have such device.
> If unsure select "N".
> +

nit: This change is not necessary.

Joe.C

Re: [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test

[Yingjoe Chen]

Hi,

Thanks for the review.


On Mon, 2016-06-06 at 20:10 +0100, Andy Whitcroft wrote:
> On Mon, Jun 06, 2016 at 09:43:15AM -0700, Joe Perches wrote:
> > On Sat, 2016-06-04 at 13:10 +0800, Yingjoe Chen wrote:
> > > If a Kconfig config option doesn't specify 'default', the default
> > > will be n. Adding 'default n' is unnecessary.
> > > Add a test to warn about this.
> > 
> > Is it obvious that a Kconfig has "default n" ?
> > This seems to work, but is this useful?


While sending patch for upstream, I saw maintainers request it to be
removed. So I think it might worth adding check to it.
Some examples from google:

http://lists.infradead.org/pipermail/linux-arm-kernel/2012-September/120733.html
https://lkml.org/lkml/2013/3/16/153
https://lkml.org/lkml/2016/5/23/657


> 
> > > + if ($realfile =~ /Kconfig/ &&
> > > + $line =~ /^\+\s*default\s*n\s*(#.*|$)/i) {
> 
> I wonder particually when the submitter has supplied a comment, presumably
> to tell us why it defaults to 'n'.  I feel more accepting of rejecting
> uncommented ones than those with.


How about change this to /^\+\s*default\s*n$/i ?

Joe.C

Re: [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test

[Yingjoe Chen]

Hi,

Thanks for the review.


On Mon, 2016-06-06 at 20:10 +0100, Andy Whitcroft wrote:
> On Mon, Jun 06, 2016 at 09:43:15AM -0700, Joe Perches wrote:
> > On Sat, 2016-06-04 at 13:10 +0800, Yingjoe Chen wrote:
> > > If a Kconfig config option doesn't specify 'default', the default
> > > will be n. Adding 'default n' is unnecessary.
> > > Add a test to warn about this.
> > 
> > Is it obvious that a Kconfig has "default n" ?
> > This seems to work, but is this useful?


While sending patch for upstream, I saw maintainers request it to be
removed. So I think it might worth adding check to it.
Some examples from google:

http://lists.infradead.org/pipermail/linux-arm-kernel/2012-September/120733.html
https://lkml.org/lkml/2013/3/16/153
https://lkml.org/lkml/2016/5/23/657


> 
> > > + if ($realfile =~ /Kconfig/ &&
> > > + $line =~ /^\+\s*default\s*n\s*(#.*|$)/i) {
> 
> I wonder particually when the submitter has supplied a comment, presumably
> to tell us why it defaults to 'n'.  I feel more accepting of rejecting
> uncommented ones than those with.


How about change this to /^\+\s*default\s*n$/i ?

Joe.C

[PATCH v3 2/2] checkpatch: testing more config for Kconfig help text

[Yingjoe Chen]

Current help text check only check a config option if it is followed
by another config.
Adding check for help text if the next entry is menuconfig, choice/
endchoice, comment, menu/endmenu, if/endif, source or end of file.

Signed-off-by: Yingjoe Chen <yingjoe.c...@mediatek.com>
---
 scripts/checkpatch.pl | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f5ce804..8e17593 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2646,6 +2646,12 @@ sub process {
next if ($f =~ /^-/);
last if (!$file && $f =~ /^\@\@/);
 
+   if ($f !~ /^[+\- ]/) {
+   # End of file
+   $is_end = 1;
+   last;
+   }
+
if ($lines[$ln - 1] =~ 
/^\+\s*(?:bool|tristate)\s*\"/) {
$is_start = 1;
} elsif ($lines[$ln - 1] =~ 
/^\+\s*(?:---)?help(?:---)?$/) {
@@ -2656,7 +2662,7 @@ sub process {
$f =~ s/#.*//;
$f =~ s/^\s+//;
next if ($f =~ /^$/);
-   if ($f =~ /^\s*config\s/) {
+   if ($f =~ 
/^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu\s*$|if\s|endif\s*$|source\s)/)
 {
$is_end = 1;
last;
}
-- 
1.9.1

[PATCH v3 1/2] checkpatch: add Kconfig 'default n' test

[Yingjoe Chen]

If a Kconfig config option doesn't specify 'default', the default
will be n. Adding 'default n' is unnecessary.

Add a test to warn about this.

Signed-off-by: Yingjoe Chen <yingjoe.c...@mediatek.com>
---
For this series, rebase to v4.7-rc1 and dropped 'relax Kconfig help text
line number threshold' patch.

Let me know what you think. Thanks.


Change in v3:
- Rebase to v4.7-rc1

Change in v2:
- Change according to Joe Perches' suggesti

 scripts/checkpatch.pl | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6750595..f5ce804 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2683,6 +2683,13 @@ sub process {
 "Use of boolean is deprecated, please use bool 
instead.\n" . $herecurr);
}
 
+# discourage the use of default n
+   if ($realfile =~ /Kconfig/ &&
+   $line =~ /^\+\s*default\s*n\s*(#.*|$)/i) {
+   WARN("CONFIG_DEFAULT_N",
+"Use of default n is unnecessary, default is n 
when omitted.\n" . $herecurr);
+   }
+
if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
my $flag = $1;
-- 
1.9.1

[PATCH v3 2/2] checkpatch: testing more config for Kconfig help text

[Yingjoe Chen]

Current help text check only check a config option if it is followed
by another config.
Adding check for help text if the next entry is menuconfig, choice/
endchoice, comment, menu/endmenu, if/endif, source or end of file.

Signed-off-by: Yingjoe Chen 
---
 scripts/checkpatch.pl | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f5ce804..8e17593 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2646,6 +2646,12 @@ sub process {
next if ($f =~ /^-/);
last if (!$file && $f =~ /^\@\@/);
 
+   if ($f !~ /^[+\- ]/) {
+   # End of file
+   $is_end = 1;
+   last;
+   }
+
if ($lines[$ln - 1] =~ 
/^\+\s*(?:bool|tristate)\s*\"/) {
$is_start = 1;
} elsif ($lines[$ln - 1] =~ 
/^\+\s*(?:---)?help(?:---)?$/) {
@@ -2656,7 +2662,7 @@ sub process {
$f =~ s/#.*//;
$f =~ s/^\s+//;
next if ($f =~ /^$/);
-   if ($f =~ /^\s*config\s/) {
+   if ($f =~ 
/^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu\s*$|if\s|endif\s*$|source\s)/)
 {
$is_end = 1;
last;
}
-- 
1.9.1

[PATCH v3 1/2] checkpatch: add Kconfig 'default n' test

[Yingjoe Chen]

If a Kconfig config option doesn't specify 'default', the default
will be n. Adding 'default n' is unnecessary.

Add a test to warn about this.

Signed-off-by: Yingjoe Chen 
---
For this series, rebase to v4.7-rc1 and dropped 'relax Kconfig help text
line number threshold' patch.

Let me know what you think. Thanks.


Change in v3:
- Rebase to v4.7-rc1

Change in v2:
- Change according to Joe Perches' suggesti

 scripts/checkpatch.pl | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6750595..f5ce804 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2683,6 +2683,13 @@ sub process {
 "Use of boolean is deprecated, please use bool 
instead.\n" . $herecurr);
}
 
+# discourage the use of default n
+   if ($realfile =~ /Kconfig/ &&
+   $line =~ /^\+\s*default\s*n\s*(#.*|$)/i) {
+   WARN("CONFIG_DEFAULT_N",
+"Use of default n is unnecessary, default is n 
when omitted.\n" . $herecurr);
+   }
+
if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
my $flag = $1;
-- 
1.9.1

Re: [RFC 1/3] dt-bindings: drm/mediatek: Add display binding for Mediatek SoC MT2701

[Yingjoe Chen]

On Thu, 2016-05-12 at 19:49 +0800, yt.s...@mediatek.com wrote:
> From: YT Shen 
> 
> Add device tree binding documentation for the display subsystem in
> Mediatek SoC MT2701
> 
> Signed-off-by: YT Shen 
> ---
>  .../bindings/display/mediatek/mediatek,disp.txt|1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git 
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt 
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index db6e77e..53b3cc3 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -40,6 +40,7 @@ Required properties (all function blocks):
>   "mediatek,-dpi"- DPI controller, see mediatek,dpi.txt
>   "mediatek,-disp-mutex" - display mutex
>   "mediatek,-disp-od"- overdrive
> + "mediatek,-disp-bls"   - backlight
>  - reg: Physical base address and length of the function block register space
>  - interrupts: The interrupt signal from the function block (required, except 
> for
>merge and split function blocks).


Hi,

This one is in conflict with "Support Mediatek Soc MT2701 disp pwm"
series Weiqing sent last week, which add disp-bls support to display pwm
driver.

Joe.C

Re: [RFC 1/3] dt-bindings: drm/mediatek: Add display binding for Mediatek SoC MT2701

[Yingjoe Chen]

On Thu, 2016-05-12 at 19:49 +0800, yt.s...@mediatek.com wrote:
> From: YT Shen 
> 
> Add device tree binding documentation for the display subsystem in
> Mediatek SoC MT2701
> 
> Signed-off-by: YT Shen 
> ---
>  .../bindings/display/mediatek/mediatek,disp.txt|1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git 
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt 
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index db6e77e..53b3cc3 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -40,6 +40,7 @@ Required properties (all function blocks):
>   "mediatek,-dpi"- DPI controller, see mediatek,dpi.txt
>   "mediatek,-disp-mutex" - display mutex
>   "mediatek,-disp-od"- overdrive
> + "mediatek,-disp-bls"   - backlight
>  - reg: Physical base address and length of the function block register space
>  - interrupts: The interrupt signal from the function block (required, except 
> for
>merge and split function blocks).


Hi,

This one is in conflict with "Support Mediatek Soc MT2701 disp pwm"
series Weiqing sent last week, which add disp-bls support to display pwm
driver.

Joe.C

Re: [PATCH v2 1/3] checkpatch: add Kconfig 'default n' test

[Yingjoe Chen]

On Fri, 2016-04-22 at 22:32 +0800, Yingjoe Chen wrote:
> If a Kconfig config option doesn't specify 'default', the default
> will be n. Adding 'default n' is unnecessary.
> 
> Add a test to warn about this.
> 
> Signed-off-by: Yingjoe Chen <yingjoe.c...@mediatek.com>
> ---
> Change in v2:
> - Change according to Joe Perches' suggestion
> 
>  scripts/checkpatch.pl | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d574d13..3cb7c2d 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2600,6 +2600,13 @@ sub process {
>"Use of boolean is deprecated, please use bool 
> instead.\n" . $herecurr);
>   }
>  
> +# discourage the use of default n
> + if ($realfile =~ /Kconfig/ &&
> + $line =~ /^\+\s*default\s*n\s*(#.*|$)/i) {
> + WARN("CONFIG_DEFAULT_N",
> +  "Use of default n is unnecessary, default is n 
> when omitted.\n" . $herecurr);
> + }
> +
>   if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
>   ($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
>   my $flag = $1;

Hi,

Any suggestion for this new round?

Joe.C

Re: [PATCH v2 1/3] checkpatch: add Kconfig 'default n' test

[Yingjoe Chen]

On Fri, 2016-04-22 at 22:32 +0800, Yingjoe Chen wrote:
> If a Kconfig config option doesn't specify 'default', the default
> will be n. Adding 'default n' is unnecessary.
> 
> Add a test to warn about this.
> 
> Signed-off-by: Yingjoe Chen 
> ---
> Change in v2:
> - Change according to Joe Perches' suggestion
> 
>  scripts/checkpatch.pl | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d574d13..3cb7c2d 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2600,6 +2600,13 @@ sub process {
>"Use of boolean is deprecated, please use bool 
> instead.\n" . $herecurr);
>   }
>  
> +# discourage the use of default n
> + if ($realfile =~ /Kconfig/ &&
> + $line =~ /^\+\s*default\s*n\s*(#.*|$)/i) {
> + WARN("CONFIG_DEFAULT_N",
> +  "Use of default n is unnecessary, default is n 
> when omitted.\n" . $herecurr);
> + }
> +
>   if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
>   ($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
>   my $flag = $1;

Hi,

Any suggestion for this new round?

Joe.C

[PATCH v2 3/3] checkpatch: relax Kconfig help text line number threshold

[Yingjoe Chen]

Current threshold is too strict and many upstream patch doesn't pass
this test. Relax it.

Signed-off-by: Yingjoe Chen <yingjoe.c...@mediatek.com>

---
In v4.6-rc1, 171 new config options was added, and 87 of those options
have < 4 lines and 24 options have only 1 line. After this change,
checkpatch only raise warning when help text only contain 1 line.

Some options try to workaround this check by adding 2 lines
template like 'If you have this device...' which doesn't add value.

---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 403ebbc..4bfb23b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -47,7 +47,7 @@ my $configuration_file = ".checkpatch.conf";
 my $max_line_length = 80;
 my $ignore_perl_version = 0;
 my $minimum_perl_version = 5.10.0;
-my $min_conf_desc_length = 4;
+my $min_conf_desc_length = 2;
 my $spelling_file = "$D/spelling.txt";
 my $codespell = 0;
 my $codespellfile = "/usr/share/codespell/dictionary.txt";
-- 
1.9.1

[PATCH v2 3/3] checkpatch: relax Kconfig help text line number threshold

[Yingjoe Chen]

Current threshold is too strict and many upstream patch doesn't pass
this test. Relax it.

Signed-off-by: Yingjoe Chen 

---
In v4.6-rc1, 171 new config options was added, and 87 of those options
have < 4 lines and 24 options have only 1 line. After this change,
checkpatch only raise warning when help text only contain 1 line.

Some options try to workaround this check by adding 2 lines
template like 'If you have this device...' which doesn't add value.

---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 403ebbc..4bfb23b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -47,7 +47,7 @@ my $configuration_file = ".checkpatch.conf";
 my $max_line_length = 80;
 my $ignore_perl_version = 0;
 my $minimum_perl_version = 5.10.0;
-my $min_conf_desc_length = 4;
+my $min_conf_desc_length = 2;
 my $spelling_file = "$D/spelling.txt";
 my $codespell = 0;
 my $codespellfile = "/usr/share/codespell/dictionary.txt";
-- 
1.9.1

[PATCH v2 2/3] checkpatch: testing more config for Kconfig help text

[Yingjoe Chen]

Current help text check only check a config option if it is followed
by another config.
Adding check for help text if the next entry is menuconfig, choice/
endchoice, comment, menu/endmenu, if/endif, source or end of file.

Signed-off-by: Yingjoe Chen <yingjoe.c...@mediatek.com>
---
Change in v2:
- Change according to Joe Perches' suggestion

 scripts/checkpatch.pl | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3cb7c2d..403ebbc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2563,6 +2563,12 @@ sub process {
next if ($f =~ /^-/);
last if (!$file && $f =~ /^\@\@/);
 
+   if ($f !~ /^[+\- ]/) {
+   # End of file
+   $is_end = 1;
+   last;
+   }
+
if ($lines[$ln - 1] =~ 
/^\+\s*(?:bool|tristate)\s*\"/) {
$is_start = 1;
} elsif ($lines[$ln - 1] =~ 
/^\+\s*(?:---)?help(?:---)?$/) {
@@ -2573,7 +2579,7 @@ sub process {
$f =~ s/#.*//;
$f =~ s/^\s+//;
next if ($f =~ /^$/);
-   if ($f =~ /^\s*config\s/) {
+   if ($f =~ 
/^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu\s*$|if\s|endif\s*$|source\s)/)
 {
$is_end = 1;
last;
}
-- 
1.9.1

[PATCH v2 2/3] checkpatch: testing more config for Kconfig help text

[Yingjoe Chen]

Current help text check only check a config option if it is followed
by another config.
Adding check for help text if the next entry is menuconfig, choice/
endchoice, comment, menu/endmenu, if/endif, source or end of file.

Signed-off-by: Yingjoe Chen 
---
Change in v2:
- Change according to Joe Perches' suggestion

 scripts/checkpatch.pl | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3cb7c2d..403ebbc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2563,6 +2563,12 @@ sub process {
next if ($f =~ /^-/);
last if (!$file && $f =~ /^\@\@/);
 
+   if ($f !~ /^[+\- ]/) {
+   # End of file
+   $is_end = 1;
+   last;
+   }
+
if ($lines[$ln - 1] =~ 
/^\+\s*(?:bool|tristate)\s*\"/) {
$is_start = 1;
} elsif ($lines[$ln - 1] =~ 
/^\+\s*(?:---)?help(?:---)?$/) {
@@ -2573,7 +2579,7 @@ sub process {
$f =~ s/#.*//;
$f =~ s/^\s+//;
next if ($f =~ /^$/);
-   if ($f =~ /^\s*config\s/) {
+   if ($f =~ 
/^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu\s*$|if\s|endif\s*$|source\s)/)
 {
$is_end = 1;
last;
}
-- 
1.9.1

[PATCH v2 1/3] checkpatch: add Kconfig 'default n' test

[Yingjoe Chen]

If a Kconfig config option doesn't specify 'default', the default
will be n. Adding 'default n' is unnecessary.

Add a test to warn about this.

Signed-off-by: Yingjoe Chen <yingjoe.c...@mediatek.com>
---
Change in v2:
- Change according to Joe Perches' suggestion

 scripts/checkpatch.pl | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d574d13..3cb7c2d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2600,6 +2600,13 @@ sub process {
 "Use of boolean is deprecated, please use bool 
instead.\n" . $herecurr);
}
 
+# discourage the use of default n
+   if ($realfile =~ /Kconfig/ &&
+   $line =~ /^\+\s*default\s*n\s*(#.*|$)/i) {
+   WARN("CONFIG_DEFAULT_N",
+"Use of default n is unnecessary, default is n 
when omitted.\n" . $herecurr);
+   }
+
if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
my $flag = $1;
-- 
1.9.1