[PATCH v3 0/3]

[PATCH v3 0/3]

[Kefeng Wang]

Make dw8250_set_termios() as the default set_termios callback for 8250 dw uart, correct me
if I am wrong.

Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that it is not 16500
compatible. Meanwhile, set dw8250_serial_out32 to keep consistent between serial_out
and serial_in in ACPI.

Change since v2:
- Add a new patch to use new var dev in probe
- Use built-in device properties to set device parameters for existing device probed by acpi,
  suggested by Andy Shevchenko 


Change since v1:
- Use acpi_match_device() instead of acpi_dev_found(), limit the check to the device
  being probed and not a global search for whole DSDT (pointed by graeme.gregory@linaro.org)

Kefeng Wang (3):
  serial: 8250_dw: make dw8250_set_termios as default set_termios
    callback
  serial: 8250_dw: Use new dev variable in probe
  serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc

 drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 30 deletions(-)

-- 
1.7.12.4

[PATCH v3 1/3] serial: 8250_dw: make dw8250_set_termios as default set_termios callback

[Kefeng Wang]

We can safely use dw8250_set_termios() as the default set_termios
callback instead of serial8250_do_set_termios(), so do it.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 drivers/tty/serial/8250/8250_dw.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index e199696..65f3da7 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -301,7 +301,6 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 		p->iotype = UPIO_MEM32;
 		p->regshift = 2;
 		p->serial_in = dw8250_serial_in32;
-		p->set_termios = dw8250_set_termios;
 		/* So far none of there implement the Busy Functionality */
 		data->uart_16550_compatible = true;
 	}
@@ -309,7 +308,6 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 	/* Platforms with iDMA */
 	if (platform_get_resource_byname(to_platform_device(p->dev),
 					 IORESOURCE_MEM, "lpss_priv")) {
-		p->set_termios = dw8250_set_termios;
 		data->dma.rx_param = p->dev->parent;
 		data->dma.tx_param = p->dev->parent;
 		data->dma.fn = dw8250_idma_filter;
@@ -386,6 +384,7 @@ static int dw8250_probe(struct platform_device *pdev)
 	p->iotype	= UPIO_MEM;
 	p->serial_in	= dw8250_serial_in;
 	p->serial_out	= dw8250_serial_out;
+	p->set_termios	= dw8250_set_termios;
 
 	p->membase = devm_ioremap(&pdev->dev, regs->start, resource_size(regs));
 	if (!p->membase)
-- 
1.7.12.4

Re: [PATCH v3 1/3] serial: 8250_dw: make dw8250_set_termios as default set_termios callback

[Andy Shevchenko]

On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
> We can safely use dw8250_set_termios() as the default set_termios
> callback instead of serial8250_do_set_termios(), so do it.

So, current set_termios() relies on clock to be defined or be an error
pointer. I'm not sure that it handles NULL properly.

So, the question is does set_termios() handle all possible cases? If no,
do we guarantee that the case it doesn't handle never happened?

Answer to this question and amend either the patch or improve commit
message.

> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c
> b/drivers/tty/serial/8250/8250_dw.c
> index e199696..65f3da7 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -301,7 +301,6 @@ static void dw8250_quirks(struct uart_port *p,
> struct dw8250_data *data)
>  		p->iotype = UPIO_MEM32;
>  		p->regshift = 2;
>  		p->serial_in = dw8250_serial_in32;
> -		p->set_termios = dw8250_set_termios;
>  		/* So far none of there implement the Busy
> Functionality */
>  		data->uart_16550_compatible = true;
>  	}
> @@ -309,7 +308,6 @@ static void dw8250_quirks(struct uart_port *p,
> struct dw8250_data *data)
>  	/* Platforms with iDMA */
>  	if (platform_get_resource_byname(to_platform_device(p->dev),
>  					 IORESOURCE_MEM,
> "lpss_priv")) {
> -		p->set_termios = dw8250_set_termios;
>  		data->dma.rx_param = p->dev->parent;
>  		data->dma.tx_param = p->dev->parent;
>  		data->dma.fn = dw8250_idma_filter;
> @@ -386,6 +384,7 @@ static int dw8250_probe(struct platform_device
> *pdev)
>  	p->iotype	= UPIO_MEM;
>  	p->serial_in	= dw8250_serial_in;
>  	p->serial_out	= dw8250_serial_out;
> +	p->set_termios	= dw8250_set_termios;
>  
>  	p->membase = devm_ioremap(&pdev->dev, regs->start,
> resource_size(regs));
>  	if (!p->membase)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 1/3] serial: 8250_dw: make dw8250_set_termios as default set_termios callback

[Kefeng Wang]

On 2016/8/22 19:08, Andy Shevchenko wrote:
> On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
>> We can safely use dw8250_set_termios() as the default set_termios
>> callback instead of serial8250_do_set_termios(), so do it.
> 
> So, current set_termios() relies on clock to be defined or be an error
> pointer. I'm not sure that it handles NULL properly.
> 
> So, the question is does set_termios() handle all possible cases? If no,
> do we guarantee that the case it doesn't handle never happened?
> 
> Answer to this question and amend either the patch or improve commit
> message.
> 

If clk do be NUL, uartclk will be set wrong value, zero, will judge this case
in dw8250_set_termios, then make it as default callback.


>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>  drivers/tty/serial/8250/8250_dw.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c
>> b/drivers/tty/serial/8250/8250_dw.c
>> index e199696..65f3da7 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -301,7 +301,6 @@ static void dw8250_quirks(struct uart_port *p,
>> struct dw8250_data *data)
>>  		p->iotype = UPIO_MEM32;
>>  		p->regshift = 2;
>>  		p->serial_in = dw8250_serial_in32;
>> -		p->set_termios = dw8250_set_termios;
>>  		/* So far none of there implement the Busy
>> Functionality */
>>  		data->uart_16550_compatible = true;
>>  	}
>> @@ -309,7 +308,6 @@ static void dw8250_quirks(struct uart_port *p,
>> struct dw8250_data *data)
>>  	/* Platforms with iDMA */
>>  	if (platform_get_resource_byname(to_platform_device(p->dev),
>>  					 IORESOURCE_MEM,
>> "lpss_priv")) {
>> -		p->set_termios = dw8250_set_termios;
>>  		data->dma.rx_param = p->dev->parent;
>>  		data->dma.tx_param = p->dev->parent;
>>  		data->dma.fn = dw8250_idma_filter;
>> @@ -386,6 +384,7 @@ static int dw8250_probe(struct platform_device
>> *pdev)
>>  	p->iotype	= UPIO_MEM;
>>  	p->serial_in	= dw8250_serial_in;
>>  	p->serial_out	= dw8250_serial_out;
>> +	p->set_termios	= dw8250_set_termios;
>>  
>>  	p->membase = devm_ioremap(&pdev->dev, regs->start,
>> resource_size(regs));
>>  	if (!p->membase)
> 

[PATCH v3 2/3] serial: 8250_dw: Use new dev variable in probe

[Kefeng Wang]

Use new dev variable instead of pdev->dev and p->dev in probe function.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 drivers/tty/serial/8250/8250_dw.c | 45 ++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 65f3da7..d6934310 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -358,18 +358,19 @@ static int dw8250_probe(struct platform_device *pdev)
 	struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	int irq = platform_get_irq(pdev, 0);
 	struct uart_port *p = &uart.port;
+	struct device *dev = &pdev->dev;
 	struct dw8250_data *data;
 	int err;
 	u32 val;
 
 	if (!regs) {
-		dev_err(&pdev->dev, "no registers defined\n");
+		dev_err(dev, "no registers defined\n");
 		return -EINVAL;
 	}
 
 	if (irq < 0) {
 		if (irq != -EPROBE_DEFER)
-			dev_err(&pdev->dev, "cannot get irq\n");
+			dev_err(dev, "cannot get irq\n");
 		return irq;
 	}
 
@@ -380,17 +381,17 @@ static int dw8250_probe(struct platform_device *pdev)
 	p->pm		= dw8250_do_pm;
 	p->type		= PORT_8250;
 	p->flags	= UPF_SHARE_IRQ | UPF_FIXED_PORT;
-	p->dev		= &pdev->dev;
+	p->dev		= dev;
 	p->iotype	= UPIO_MEM;
 	p->serial_in	= dw8250_serial_in;
 	p->serial_out	= dw8250_serial_out;
 	p->set_termios	= dw8250_set_termios;
 
-	p->membase = devm_ioremap(&pdev->dev, regs->start, resource_size(regs));
+	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
 	if (!p->membase)
 		return -ENOMEM;
 
-	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -398,57 +399,57 @@ static int dw8250_probe(struct platform_device *pdev)
 	data->usr_reg = DW_UART_USR;
 	p->private_data = data;
 
-	data->uart_16550_compatible = device_property_read_bool(p->dev,
+	data->uart_16550_compatible = device_property_read_bool(dev,
 						"snps,uart-16550-compatible");
 
-	err = device_property_read_u32(p->dev, "reg-shift", &val);
+	err = device_property_read_u32(dev, "reg-shift", &val);
 	if (!err)
 		p->regshift = val;
 
-	err = device_property_read_u32(p->dev, "reg-io-width", &val);
+	err = device_property_read_u32(dev, "reg-io-width", &val);
 	if (!err && val == 4) {
 		p->iotype = UPIO_MEM32;
 		p->serial_in = dw8250_serial_in32;
 		p->serial_out = dw8250_serial_out32;
 	}
 
-	if (device_property_read_bool(p->dev, "dcd-override")) {
+	if (device_property_read_bool(dev, "dcd-override")) {
 		/* Always report DCD as active */
 		data->msr_mask_on |= UART_MSR_DCD;
 		data->msr_mask_off |= UART_MSR_DDCD;
 	}
 
-	if (device_property_read_bool(p->dev, "dsr-override")) {
+	if (device_property_read_bool(dev, "dsr-override")) {
 		/* Always report DSR as active */
 		data->msr_mask_on |= UART_MSR_DSR;
 		data->msr_mask_off |= UART_MSR_DDSR;
 	}
 
-	if (device_property_read_bool(p->dev, "cts-override")) {
+	if (device_property_read_bool(dev, "cts-override")) {
 		/* Always report CTS as active */
 		data->msr_mask_on |= UART_MSR_CTS;
 		data->msr_mask_off |= UART_MSR_DCTS;
 	}
 
-	if (device_property_read_bool(p->dev, "ri-override")) {
+	if (device_property_read_bool(dev, "ri-override")) {
 		/* Always report Ring indicator as inactive */
 		data->msr_mask_off |= UART_MSR_RI;
 		data->msr_mask_off |= UART_MSR_TERI;
 	}
 
 	/* Always ask for fixed clock rate from a property. */
-	device_property_read_u32(p->dev, "clock-frequency", &p->uartclk);
+	device_property_read_u32(dev, "clock-frequency", &p->uartclk);
 
 	/* If there is separate baudclk, get the rate from it. */
-	data->clk = devm_clk_get(&pdev->dev, "baudclk");
+	data->clk = devm_clk_get(dev, "baudclk");
 	if (IS_ERR(data->clk) && PTR_ERR(data->clk) != -EPROBE_DEFER)
-		data->clk = devm_clk_get(&pdev->dev, NULL);
+		data->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 	if (!IS_ERR_OR_NULL(data->clk)) {
 		err = clk_prepare_enable(data->clk);
 		if (err)
-			dev_warn(&pdev->dev, "could not enable optional baudclk: %d\n",
+			dev_warn(dev, "could not enable optional baudclk: %d\n",
 				 err);
 		else
 			p->uartclk = clk_get_rate(data->clk);
@@ -456,11 +457,11 @@ static int dw8250_probe(struct platform_device *pdev)
 
 	/* If no clock rate is defined, fail. */
 	if (!p->uartclk) {
-		dev_err(&pdev->dev, "clock rate not defined\n");
+		dev_err(dev, "clock rate not defined\n");
 		return -EINVAL;
 	}
 
-	data->pclk = devm_clk_get(&pdev->dev, "apb_pclk");
+	data->pclk = devm_clk_get(dev, "apb_pclk");
 	if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER) {
 		err = -EPROBE_DEFER;
 		goto err_clk;
@@ -468,12 +469,12 @@ static int dw8250_probe(struct platform_device *pdev)
 	if (!IS_ERR(data->pclk)) {
 		err = clk_prepare_enable(data->pclk);
 		if (err) {
-			dev_err(&pdev->dev, "could not enable apb_pclk\n");
+			dev_err(dev, "could not enable apb_pclk\n");
 			goto err_clk;
 		}
 	}
 
-	data->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
+	data->rst = devm_reset_control_get_optional(dev, NULL);
 	if (IS_ERR(data->rst) && PTR_ERR(data->rst) == -EPROBE_DEFER) {
 		err = -EPROBE_DEFER;
 		goto err_pclk;
@@ -505,8 +506,8 @@ static int dw8250_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
-	pm_runtime_set_active(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
 
 	return 0;
 
-- 
1.7.12.4

Re: [PATCH v3 2/3] serial: 8250_dw: Use new dev variable in probe

[Andy Shevchenko]

On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
> Use new dev variable instead of pdev->dev and p->dev in probe
> function.

I'm not sure we need this one. What is wrong with &pdev->dev?

> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 45 ++++++++++++++++++++--------
> -----------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c
> b/drivers/tty/serial/8250/8250_dw.c
> index 65f3da7..d6934310 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -358,18 +358,19 @@ static int dw8250_probe(struct platform_device
> *pdev)
>  	struct resource *regs = platform_get_resource(pdev,
> IORESOURCE_MEM, 0);
>  	int irq = platform_get_irq(pdev, 0);
>  	struct uart_port *p = &uart.port;
> +	struct device *dev = &pdev->dev;
>  	struct dw8250_data *data;
>  	int err;
>  	u32 val;
>  
>  	if (!regs) {
> -		dev_err(&pdev->dev, "no registers defined\n");
> +		dev_err(dev, "no registers defined\n");
>  		return -EINVAL;
>  	}
>  
>  	if (irq < 0) {
>  		if (irq != -EPROBE_DEFER)
> -			dev_err(&pdev->dev, "cannot get irq\n");
> +			dev_err(dev, "cannot get irq\n");
>  		return irq;
>  	}
>  
> @@ -380,17 +381,17 @@ static int dw8250_probe(struct platform_device
> *pdev)
>  	p->pm		= dw8250_do_pm;
>  	p->type		= PORT_8250;
>  	p->flags	= UPF_SHARE_IRQ | UPF_FIXED_PORT;
> -	p->dev		= &pdev->dev;
> +	p->dev		= dev;
>  	p->iotype	= UPIO_MEM;
>  	p->serial_in	= dw8250_serial_in;
>  	p->serial_out	= dw8250_serial_out;
>  	p->set_termios	= dw8250_set_termios;
>  
> -	p->membase = devm_ioremap(&pdev->dev, regs->start,
> resource_size(regs));
> +	p->membase = devm_ioremap(dev, regs->start,
> resource_size(regs));
>  	if (!p->membase)
>  		return -ENOMEM;
>  
> -	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> @@ -398,57 +399,57 @@ static int dw8250_probe(struct platform_device
> *pdev)
>  	data->usr_reg = DW_UART_USR;
>  	p->private_data = data;
>  
> -	data->uart_16550_compatible = device_property_read_bool(p-
> >dev,
> +	data->uart_16550_compatible = device_property_read_bool(dev,
>  						"snps,uart-16550-
> compatible");
>  
> -	err = device_property_read_u32(p->dev, "reg-shift", &val);
> +	err = device_property_read_u32(dev, "reg-shift", &val);
>  	if (!err)
>  		p->regshift = val;
>  
> -	err = device_property_read_u32(p->dev, "reg-io-width", &val);
> +	err = device_property_read_u32(dev, "reg-io-width", &val);
>  	if (!err && val == 4) {
>  		p->iotype = UPIO_MEM32;
>  		p->serial_in = dw8250_serial_in32;
>  		p->serial_out = dw8250_serial_out32;
>  	}
>  
> -	if (device_property_read_bool(p->dev, "dcd-override")) {
> +	if (device_property_read_bool(dev, "dcd-override")) {
>  		/* Always report DCD as active */
>  		data->msr_mask_on |= UART_MSR_DCD;
>  		data->msr_mask_off |= UART_MSR_DDCD;
>  	}
>  
> -	if (device_property_read_bool(p->dev, "dsr-override")) {
> +	if (device_property_read_bool(dev, "dsr-override")) {
>  		/* Always report DSR as active */
>  		data->msr_mask_on |= UART_MSR_DSR;
>  		data->msr_mask_off |= UART_MSR_DDSR;
>  	}
>  
> -	if (device_property_read_bool(p->dev, "cts-override")) {
> +	if (device_property_read_bool(dev, "cts-override")) {
>  		/* Always report CTS as active */
>  		data->msr_mask_on |= UART_MSR_CTS;
>  		data->msr_mask_off |= UART_MSR_DCTS;
>  	}
>  
> -	if (device_property_read_bool(p->dev, "ri-override")) {
> +	if (device_property_read_bool(dev, "ri-override")) {
>  		/* Always report Ring indicator as inactive */
>  		data->msr_mask_off |= UART_MSR_RI;
>  		data->msr_mask_off |= UART_MSR_TERI;
>  	}
>  
>  	/* Always ask for fixed clock rate from a property. */
> -	device_property_read_u32(p->dev, "clock-frequency", &p-
> >uartclk);
> +	device_property_read_u32(dev, "clock-frequency", &p-
> >uartclk);
>  
>  	/* If there is separate baudclk, get the rate from it. */
> -	data->clk = devm_clk_get(&pdev->dev, "baudclk");
> +	data->clk = devm_clk_get(dev, "baudclk");
>  	if (IS_ERR(data->clk) && PTR_ERR(data->clk) != -EPROBE_DEFER)
> -		data->clk = devm_clk_get(&pdev->dev, NULL);
> +		data->clk = devm_clk_get(dev, NULL);
>  	if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER)
>  		return -EPROBE_DEFER;
>  	if (!IS_ERR_OR_NULL(data->clk)) {
>  		err = clk_prepare_enable(data->clk);
>  		if (err)
> -			dev_warn(&pdev->dev, "could not enable
> optional baudclk: %d\n",
> +			dev_warn(dev, "could not enable optional
> baudclk: %d\n",
>  				 err);
>  		else
>  			p->uartclk = clk_get_rate(data->clk);
> @@ -456,11 +457,11 @@ static int dw8250_probe(struct platform_device
> *pdev)
>  
>  	/* If no clock rate is defined, fail. */
>  	if (!p->uartclk) {
> -		dev_err(&pdev->dev, "clock rate not defined\n");
> +		dev_err(dev, "clock rate not defined\n");
>  		return -EINVAL;
>  	}
>  
> -	data->pclk = devm_clk_get(&pdev->dev, "apb_pclk");
> +	data->pclk = devm_clk_get(dev, "apb_pclk");
>  	if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER)
> {
>  		err = -EPROBE_DEFER;
>  		goto err_clk;
> @@ -468,12 +469,12 @@ static int dw8250_probe(struct platform_device
> *pdev)
>  	if (!IS_ERR(data->pclk)) {
>  		err = clk_prepare_enable(data->pclk);
>  		if (err) {
> -			dev_err(&pdev->dev, "could not enable
> apb_pclk\n");
> +			dev_err(dev, "could not enable apb_pclk\n");
>  			goto err_clk;
>  		}
>  	}
>  
> -	data->rst = devm_reset_control_get_optional(&pdev->dev,
> NULL);
> +	data->rst = devm_reset_control_get_optional(dev, NULL);
>  	if (IS_ERR(data->rst) && PTR_ERR(data->rst) == -EPROBE_DEFER)
> {
>  		err = -EPROBE_DEFER;
>  		goto err_pclk;
> @@ -505,8 +506,8 @@ static int dw8250_probe(struct platform_device
> *pdev)
>  
>  	platform_set_drvdata(pdev, data);
>  
> -	pm_runtime_set_active(&pdev->dev);
> -	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
>  
>  	return 0;
>  

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 2/3] serial: 8250_dw: Use new dev variable in probe

[Kefeng Wang]

On 2016/8/22 19:13, Andy Shevchenko wrote:
> On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
>> Use new dev variable instead of pdev->dev and p->dev in probe
>> function.
> 
> I'm not sure we need this one. What is wrong with &pdev->dev?

Yes, no error in current code. there are &pdev->dev(A) and p->dev(B),
some codes use A, others use B, so I want to use an unified one.

If someone don't like this, I will drop it, :)


> 
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>  drivers/tty/serial/8250/8250_dw.c | 45 ++++++++++++++++++++--------
>> -----------
>>  1 file changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_dw.c
>> b/drivers/tty/serial/8250/8250_dw.c
>> index 65f3da7..d6934310 100644
>> --- a/drivers/tty/serial/8250/8250_dw.c
>> +++ b/drivers/tty/serial/8250/8250_dw.c
>> @@ -358,18 +358,19 @@ static int dw8250_probe(struct platform_device
>> *pdev)
>>  	struct resource *regs = platform_get_resource(pdev,
>> IORESOURCE_MEM, 0);
>>  	int irq = platform_get_irq(pdev, 0);
>>  	struct uart_port *p = &uart.port;
>> +	struct device *dev = &pdev->dev;
>>  	struct dw8250_data *data;
>>  	int err;
>>  	u32 val;
>>  
>>  	if (!regs) {
>> -		dev_err(&pdev->dev, "no registers defined\n");
>> +		dev_err(dev, "no registers defined\n");
>>  		return -EINVAL;
>>  	}
>>  
>>  	if (irq < 0) {
>>  		if (irq != -EPROBE_DEFER)
>> -			dev_err(&pdev->dev, "cannot get irq\n");
>> +			dev_err(dev, "cannot get irq\n");
>>  		return irq;
>>  	}
>>  
>> @@ -380,17 +381,17 @@ static int dw8250_probe(struct platform_device
>> *pdev)
>>  	p->pm		= dw8250_do_pm;
>>  	p->type		= PORT_8250;
>>  	p->flags	= UPF_SHARE_IRQ | UPF_FIXED_PORT;
>> -	p->dev		= &pdev->dev;
>> +	p->dev		= dev;
>>  	p->iotype	= UPIO_MEM;
>>  	p->serial_in	= dw8250_serial_in;
>>  	p->serial_out	= dw8250_serial_out;
>>  	p->set_termios	= dw8250_set_termios;
>>  
>> -	p->membase = devm_ioremap(&pdev->dev, regs->start,
>> resource_size(regs));
>> +	p->membase = devm_ioremap(dev, regs->start,
>> resource_size(regs));
>>  	if (!p->membase)
>>  		return -ENOMEM;
>>  
>> -	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>  	if (!data)
>>  		return -ENOMEM;
>>  
>> @@ -398,57 +399,57 @@ static int dw8250_probe(struct platform_device
>> *pdev)
>>  	data->usr_reg = DW_UART_USR;
>>  	p->private_data = data;
>>  
>> -	data->uart_16550_compatible = device_property_read_bool(p-
>>> dev,
>> +	data->uart_16550_compatible = device_property_read_bool(dev,
>>  						"snps,uart-16550-
>> compatible");
>>  
>> -	err = device_property_read_u32(p->dev, "reg-shift", &val);
>> +	err = device_property_read_u32(dev, "reg-shift", &val);
>>  	if (!err)
>>  		p->regshift = val;
>>  
>> -	err = device_property_read_u32(p->dev, "reg-io-width", &val);
>> +	err = device_property_read_u32(dev, "reg-io-width", &val);
>>  	if (!err && val == 4) {
>>  		p->iotype = UPIO_MEM32;
>>  		p->serial_in = dw8250_serial_in32;
>>  		p->serial_out = dw8250_serial_out32;
>>  	}
>>  
>> -	if (device_property_read_bool(p->dev, "dcd-override")) {
>> +	if (device_property_read_bool(dev, "dcd-override")) {
>>  		/* Always report DCD as active */
>>  		data->msr_mask_on |= UART_MSR_DCD;
>>  		data->msr_mask_off |= UART_MSR_DDCD;
>>  	}
>>  
>> -	if (device_property_read_bool(p->dev, "dsr-override")) {
>> +	if (device_property_read_bool(dev, "dsr-override")) {
>>  		/* Always report DSR as active */
>>  		data->msr_mask_on |= UART_MSR_DSR;
>>  		data->msr_mask_off |= UART_MSR_DDSR;
>>  	}
>>  
>> -	if (device_property_read_bool(p->dev, "cts-override")) {
>> +	if (device_property_read_bool(dev, "cts-override")) {
>>  		/* Always report CTS as active */
>>  		data->msr_mask_on |= UART_MSR_CTS;
>>  		data->msr_mask_off |= UART_MSR_DCTS;
>>  	}
>>  
>> -	if (device_property_read_bool(p->dev, "ri-override")) {
>> +	if (device_property_read_bool(dev, "ri-override")) {
>>  		/* Always report Ring indicator as inactive */
>>  		data->msr_mask_off |= UART_MSR_RI;
>>  		data->msr_mask_off |= UART_MSR_TERI;
>>  	}
>>  
>>  	/* Always ask for fixed clock rate from a property. */
>> -	device_property_read_u32(p->dev, "clock-frequency", &p-
>>> uartclk);
>> +	device_property_read_u32(dev, "clock-frequency", &p-
>>> uartclk);
>>  
>>  	/* If there is separate baudclk, get the rate from it. */
>> -	data->clk = devm_clk_get(&pdev->dev, "baudclk");
>> +	data->clk = devm_clk_get(dev, "baudclk");
>>  	if (IS_ERR(data->clk) && PTR_ERR(data->clk) != -EPROBE_DEFER)
>> -		data->clk = devm_clk_get(&pdev->dev, NULL);
>> +		data->clk = devm_clk_get(dev, NULL);
>>  	if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER)
>>  		return -EPROBE_DEFER;
>>  	if (!IS_ERR_OR_NULL(data->clk)) {
>>  		err = clk_prepare_enable(data->clk);
>>  		if (err)
>> -			dev_warn(&pdev->dev, "could not enable
>> optional baudclk: %d\n",
>> +			dev_warn(dev, "could not enable optional
>> baudclk: %d\n",
>>  				 err);
>>  		else
>>  			p->uartclk = clk_get_rate(data->clk);
>> @@ -456,11 +457,11 @@ static int dw8250_probe(struct platform_device
>> *pdev)
>>  
>>  	/* If no clock rate is defined, fail. */
>>  	if (!p->uartclk) {
>> -		dev_err(&pdev->dev, "clock rate not defined\n");
>> +		dev_err(dev, "clock rate not defined\n");
>>  		return -EINVAL;
>>  	}
>>  
>> -	data->pclk = devm_clk_get(&pdev->dev, "apb_pclk");
>> +	data->pclk = devm_clk_get(dev, "apb_pclk");
>>  	if (IS_ERR(data->clk) && PTR_ERR(data->clk) == -EPROBE_DEFER)
>> {
>>  		err = -EPROBE_DEFER;
>>  		goto err_clk;
>> @@ -468,12 +469,12 @@ static int dw8250_probe(struct platform_device
>> *pdev)
>>  	if (!IS_ERR(data->pclk)) {
>>  		err = clk_prepare_enable(data->pclk);
>>  		if (err) {
>> -			dev_err(&pdev->dev, "could not enable
>> apb_pclk\n");
>> +			dev_err(dev, "could not enable apb_pclk\n");
>>  			goto err_clk;
>>  		}
>>  	}
>>  
>> -	data->rst = devm_reset_control_get_optional(&pdev->dev,
>> NULL);
>> +	data->rst = devm_reset_control_get_optional(dev, NULL);
>>  	if (IS_ERR(data->rst) && PTR_ERR(data->rst) == -EPROBE_DEFER)
>> {
>>  		err = -EPROBE_DEFER;
>>  		goto err_pclk;
>> @@ -505,8 +506,8 @@ static int dw8250_probe(struct platform_device
>> *pdev)
>>  
>>  	platform_set_drvdata(pdev, data);
>>  
>> -	pm_runtime_set_active(&pdev->dev);
>> -	pm_runtime_enable(&pdev->dev);
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
>>  
>>  	return 0;
>>  
> 

Re: [PATCH v3 2/3] serial: 8250_dw: Use new dev variable in probe

[Andy Shevchenko]

On Wed, 2016-08-24 at 16:20 +0800, Kefeng Wang wrote:
> 
> On 2016/8/22 19:13, Andy Shevchenko wrote:
> > 
> > On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
> > > 
> > > Use new dev variable instead of pdev->dev and p->dev in probe
> > > function.
> > 
> > I'm not sure we need this one. What is wrong with &pdev->dev?
> 
> Yes, no error in current code. there are &pdev->dev(A) and p->dev(B),
> some codes use A, others use B, so I want to use an unified one.
> 
> If someone don't like this, I will drop it, :)

I want to say that this change doesn't belong to the series in this
case. You may submit it separately later on.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 2/3] serial: 8250_dw: Use new dev variable in probe

[Kefeng Wang]

On 2016/8/24 18:00, Andy Shevchenko wrote:
> On Wed, 2016-08-24 at 16:20 +0800, Kefeng Wang wrote:
>>
>> On 2016/8/22 19:13, Andy Shevchenko wrote:
>>>
>>> On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
>>>>
>>>> Use new dev variable instead of pdev->dev and p->dev in probe
>>>> function.
>>>
>>> I'm not sure we need this one. What is wrong with &pdev->dev?
>>
>> Yes, no error in current code. there are &pdev->dev(A) and p->dev(B),
>> some codes use A, others use B, so I want to use an unified one.
>>
>> If someone don't like this, I will drop it, :)
> 
> I want to say that this change doesn't belong to the series in this
> case. You may submit it separately later on.
> 

Will do, thanks.

[PATCH v3 3/3] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc

[Kefeng Wang]

Use built-in device properties to set device parameters for the
existing device probed by acpi.

Add ACPI identifier for UART on Hisilicon Hip05 soc, be careful
that it is not 16550 compatibal, so we need set "reg-io-width"
and "reg-shift" by _DSD method in DSDT.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 drivers/tty/serial/8250/8250_dw.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index d6934310..5ab9cfe 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -297,12 +297,6 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 			p->serial_in = dw8250_serial_in32be;
 			p->serial_out = dw8250_serial_out32be;
 		}
-	} else if (has_acpi_companion(p->dev)) {
-		p->iotype = UPIO_MEM32;
-		p->regshift = 2;
-		p->serial_in = dw8250_serial_in32;
-		/* So far none of there implement the Busy Functionality */
-		data->uart_16550_compatible = true;
 	}
 
 	/* Platforms with iDMA */
@@ -352,6 +346,19 @@ static void dw8250_setup_port(struct uart_port *p)
 		up->capabilities |= UART_CAP_AFE;
 }
 
+static struct property_entry dw8250_properties[] = {
+	PROPERTY_ENTRY_U32("reg-io-width", 4),
+	PROPERTY_ENTRY_U32("reg-shift", 2),
+	PROPERTY_ENTRY_BOOL("snps,uart-16550-compatible"),
+	{ },
+};
+
+/* non 16550 compatible id list*/
+static const struct acpi_device_id non_16550_ids[] = {
+	{ "HISI0031", 0 },
+	{ },
+};
+
 static int dw8250_probe(struct platform_device *pdev)
 {
 	struct uart_8250_port uart = {};
@@ -395,6 +402,9 @@ static int dw8250_probe(struct platform_device *pdev)
 	if (!data)
 		return -ENOMEM;
 
+	if (has_acpi_companion(dev) && !acpi_match_device(non_16550_ids, dev))
+		platform_device_add_properties(pdev, dw8250_properties);
+
 	data->dma.fn = dw8250_fallback_dma_filter;
 	data->usr_reg = DW_UART_USR;
 	p->private_data = data;
@@ -619,6 +629,7 @@ static const struct acpi_device_id dw8250_acpi_match[] = {
 	{ "APMC0D08", 0},
 	{ "AMD0020", 0 },
 	{ "AMDI0020", 0 },
+	{ "HISI0031", 0 },
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, dw8250_acpi_match);
-- 
1.7.12.4

Re: [PATCH v3 3/3] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc

[Kefeng Wang]

+ kernel malilist

Hi Andriy and all, any comments, thanks.

On 2016/7/15 19:01, Kefeng Wang wrote:
> Use built-in device properties to set device parameters for the
> existing device probed by acpi.
> 
> Add ACPI identifier for UART on Hisilicon Hip05 soc, be careful
> that it is not 16550 compatibal, so we need set "reg-io-width"
> and "reg-shift" by _DSD method in DSDT.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index d6934310..5ab9cfe 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -297,12 +297,6 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>  			p->serial_in = dw8250_serial_in32be;
>  			p->serial_out = dw8250_serial_out32be;
>  		}
> -	} else if (has_acpi_companion(p->dev)) {
> -		p->iotype = UPIO_MEM32;
> -		p->regshift = 2;
> -		p->serial_in = dw8250_serial_in32;
> -		/* So far none of there implement the Busy Functionality */
> -		data->uart_16550_compatible = true;
>  	}
>  
>  	/* Platforms with iDMA */
> @@ -352,6 +346,19 @@ static void dw8250_setup_port(struct uart_port *p)
>  		up->capabilities |= UART_CAP_AFE;
>  }
>  
> +static struct property_entry dw8250_properties[] = {
> +	PROPERTY_ENTRY_U32("reg-io-width", 4),
> +	PROPERTY_ENTRY_U32("reg-shift", 2),
> +	PROPERTY_ENTRY_BOOL("snps,uart-16550-compatible"),
> +	{ },
> +};
> +
> +/* non 16550 compatible id list*/
> +static const struct acpi_device_id non_16550_ids[] = {
> +	{ "HISI0031", 0 },
> +	{ },
> +};
> +
>  static int dw8250_probe(struct platform_device *pdev)
>  {
>  	struct uart_8250_port uart = {};
> @@ -395,6 +402,9 @@ static int dw8250_probe(struct platform_device *pdev)
>  	if (!data)
>  		return -ENOMEM;
>  
> +	if (has_acpi_companion(dev) && !acpi_match_device(non_16550_ids, dev))
> +		platform_device_add_properties(pdev, dw8250_properties);
> +
>  	data->dma.fn = dw8250_fallback_dma_filter;
>  	data->usr_reg = DW_UART_USR;
>  	p->private_data = data;
> @@ -619,6 +629,7 @@ static const struct acpi_device_id dw8250_acpi_match[] = {
>  	{ "APMC0D08", 0},
>  	{ "AMD0020", 0 },
>  	{ "AMDI0020", 0 },
> +	{ "HISI0031", 0 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(acpi, dw8250_acpi_match);
> 

Re: [PATCH v3 3/3] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc

[Andy Shevchenko]

On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
> Use built-in device properties to set device parameters for the
> existing device probed by acpi.

acpi -> ACPI

> 
> Add ACPI identifier for UART on Hisilicon Hip05 soc, be careful

soc -> SoC

> that it is not 16550 compatibal, so we need set "reg-io-width"

compatibal -> compatible

> and "reg-shift" by _DSD method in DSDT.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c
> b/drivers/tty/serial/8250/8250_dw.c
> index d6934310..5ab9cfe 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -297,12 +297,6 @@ static void dw8250_quirks(struct uart_port *p,
> struct dw8250_data *data)
>  			p->serial_in = dw8250_serial_in32be;
>  			p->serial_out = dw8250_serial_out32be;
>  		}
> -	} else if (has_acpi_companion(p->dev)) {
> -		p->iotype = UPIO_MEM32;
> -		p->regshift = 2;
> -		p->serial_in = dw8250_serial_in32;
> -		/* So far none of there implement the Busy
> Functionality */
> -		data->uart_16550_compatible = true;
>  	}
>  
>  	/* Platforms with iDMA */
> @@ -352,6 +346,19 @@ static void dw8250_setup_port(struct uart_port
> *p)
>  		up->capabilities |= UART_CAP_AFE;
>  }
>  
> +static struct property_entry dw8250_properties[] = {
> +	PROPERTY_ENTRY_U32("reg-io-width", 4),
> +	PROPERTY_ENTRY_U32("reg-shift", 2),
> +	PROPERTY_ENTRY_BOOL("snps,uart-16550-compatible"),
> +	{ },
> +};
> +
> +/* non 16550 compatible id list*/

non 16550 -> Non-16550
Space before */

> +static const struct acpi_device_id non_16550_ids[] = {
> +	{ "HISI0031", 0 },
> +	{ },
> +};
> +
>  static int dw8250_probe(struct platform_device *pdev)
>  {
>  	struct uart_8250_port uart = {};
> @@ -395,6 +402,9 @@ static int dw8250_probe(struct platform_device
> *pdev)
>  	if (!data)
>  		return -ENOMEM;
>  
> +	if (has_acpi_companion(dev) &&
> !acpi_match_device(non_16550_ids, dev))


> +		platform_device_add_properties(pdev,
> dw8250_properties);

What if we probe device which has already properties assigned?

Heikki, are you good with such trick?


> +
>  	data->dma.fn = dw8250_fallback_dma_filter;
>  	data->usr_reg = DW_UART_USR;
>  	p->private_data = data;
> @@ -619,6 +629,7 @@ static const struct acpi_device_id
> dw8250_acpi_match[] = {
>  	{ "APMC0D08", 0},
>  	{ "AMD0020", 0 },
>  	{ "AMDI0020", 0 },
> +	{ "HISI0031", 0 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(acpi, dw8250_acpi_match);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 3/3] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc

[Heikki Krogerus]

Hi,

On Mon, Aug 22, 2016 at 02:24:14PM +0300, Andy Shevchenko wrote:
> On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
> > Use built-in device properties to set device parameters for the
> > existing device probed by acpi.
> 
> acpi -> ACPI
> 
> > 
> > Add ACPI identifier for UART on Hisilicon Hip05 soc, be careful
> 
> soc -> SoC
> 
> > that it is not 16550 compatibal, so we need set "reg-io-width"
> 
> compatibal -> compatible
> 
> > and "reg-shift" by _DSD method in DSDT.
> > 
> > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> > ---
> >  drivers/tty/serial/8250/8250_dw.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/tty/serial/8250/8250_dw.c
> > b/drivers/tty/serial/8250/8250_dw.c
> > index d6934310..5ab9cfe 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -297,12 +297,6 @@ static void dw8250_quirks(struct uart_port *p,
> > struct dw8250_data *data)
> >  			p->serial_in = dw8250_serial_in32be;
> >  			p->serial_out = dw8250_serial_out32be;
> >  		}
> > -	} else if (has_acpi_companion(p->dev)) {
> > -		p->iotype = UPIO_MEM32;
> > -		p->regshift = 2;
> > -		p->serial_in = dw8250_serial_in32;
> > -		/* So far none of there implement the Busy
> > Functionality */
> > -		data->uart_16550_compatible = true;
> >  	}
> >  
> >  	/* Platforms with iDMA */
> > @@ -352,6 +346,19 @@ static void dw8250_setup_port(struct uart_port
> > *p)
> >  		up->capabilities |= UART_CAP_AFE;
> >  }
> >  
> > +static struct property_entry dw8250_properties[] = {
> > +	PROPERTY_ENTRY_U32("reg-io-width", 4),
> > +	PROPERTY_ENTRY_U32("reg-shift", 2),
> > +	PROPERTY_ENTRY_BOOL("snps,uart-16550-compatible"),
> > +	{ },
> > +};
> > +
> > +/* non 16550 compatible id list*/
> 
> non 16550 -> Non-16550
> Space before */
> 
> > +static const struct acpi_device_id non_16550_ids[] = {
> > +	{ "HISI0031", 0 },
> > +	{ },
> > +};
> > +
> >  static int dw8250_probe(struct platform_device *pdev)
> >  {
> >  	struct uart_8250_port uart = {};
> > @@ -395,6 +402,9 @@ static int dw8250_probe(struct platform_device
> > *pdev)
> >  	if (!data)
> >  		return -ENOMEM;
> >  
> > +	if (has_acpi_companion(dev) &&
> > !acpi_match_device(non_16550_ids, dev))
> 
> 
> > +		platform_device_add_properties(pdev,
> > dw8250_properties);
> 
> What if we probe device which has already properties assigned?
> 
> Heikki, are you good with such trick?

No this is not the way to do this. As we talked, we need to add the
properties in the mfd drivers when they exist and not in the driver
itself. Only if there is no mfd driver that creates the actual
platform device for dw8250 and when also the ACPI tables don't provide
the properties, we identify the board in dw8250_quirks and handle it
separately. Right now there is only one such board.

I'll prepare the patches for that right now. This series can then be
made on top of those.


Thanks,

-- 
heikki

Re: [PATCH v3 3/3] serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc

[Kefeng Wang]

On 2016/8/22 22:05, Heikki Krogerus wrote:
> Hi,
> 
> On Mon, Aug 22, 2016 at 02:24:14PM +0300, Andy Shevchenko wrote:
>> On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
>>> Use built-in device properties to set device parameters for the
>>> existing device probed by acpi.
>>
>> acpi -> ACPI
>>
>>>
>>> Add ACPI identifier for UART on Hisilicon Hip05 soc, be careful
>>
>> soc -> SoC
>>
>>> that it is not 16550 compatibal, so we need set "reg-io-width"
>>
>> compatibal -> compatible

Will fix.

>>
>>> and "reg-shift" by _DSD method in DSDT.
>>>
>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>> ---
>>>  drivers/tty/serial/8250/8250_dw.c | 23 +++++++++++++++++------
>>>  1 file changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_dw.c
>>> b/drivers/tty/serial/8250/8250_dw.c
>>> index d6934310..5ab9cfe 100644
>>> --- a/drivers/tty/serial/8250/8250_dw.c
>>> +++ b/drivers/tty/serial/8250/8250_dw.c
>>> @@ -297,12 +297,6 @@ static void dw8250_quirks(struct uart_port *p,
>>> struct dw8250_data *data)
>>>  			p->serial_in = dw8250_serial_in32be;
>>>  			p->serial_out = dw8250_serial_out32be;
>>>  		}
>>> -	} else if (has_acpi_companion(p->dev)) {
>>> -		p->iotype = UPIO_MEM32;
>>> -		p->regshift = 2;
>>> -		p->serial_in = dw8250_serial_in32;
>>> -		/* So far none of there implement the Busy
>>> Functionality */
>>> -		data->uart_16550_compatible = true;
>>>  	}
>>>  
>>>  	/* Platforms with iDMA */
>>> @@ -352,6 +346,19 @@ static void dw8250_setup_port(struct uart_port
>>> *p)
>>>  		up->capabilities |= UART_CAP_AFE;
>>>  }
>>>  
>>> +static struct property_entry dw8250_properties[] = {
>>> +	PROPERTY_ENTRY_U32("reg-io-width", 4),
>>> +	PROPERTY_ENTRY_U32("reg-shift", 2),
>>> +	PROPERTY_ENTRY_BOOL("snps,uart-16550-compatible"),
>>> +	{ },
>>> +};
>>> +
>>> +/* non 16550 compatible id list*/
>>
>> non 16550 -> Non-16550
>> Space before */
>>
>>> +static const struct acpi_device_id non_16550_ids[] = {
>>> +	{ "HISI0031", 0 },
>>> +	{ },
>>> +};
>>> +
>>>  static int dw8250_probe(struct platform_device *pdev)
>>>  {
>>>  	struct uart_8250_port uart = {};
>>> @@ -395,6 +402,9 @@ static int dw8250_probe(struct platform_device
>>> *pdev)
>>>  	if (!data)
>>>  		return -ENOMEM;
>>>  
>>> +	if (has_acpi_companion(dev) &&
>>> !acpi_match_device(non_16550_ids, dev))
>>
>>
>>> +		platform_device_add_properties(pdev,
>>> dw8250_properties);
>>
>> What if we probe device which has already properties assigned?
>>
>> Heikki, are you good with such trick?
> 
> No this is not the way to do this. As we talked, we need to add the
> properties in the mfd drivers when they exist and not in the driver
> itself. Only if there is no mfd driver that creates the actual
> platform device for dw8250 and when also the ACPI tables don't provide
> the properties, we identify the board in dw8250_quirks and handle it
> separately. Right now there is only one such board.
> 
> I'll prepare the patches for that right now. This series can then be
> made on top of those.
> 

OK, will base on your patchset.

Thanks Andy and Heikki for your comments.

Kefeng


> 
> Thanks,
> 

Re: [PATCH v3 0/3]

[Andy Shevchenko]

On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
> Make dw8250_set_termios() as the default set_termios callback for 8250
> dw uart, correct me
> if I am wrong.
> 
> Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that
> it is not 16500
> compatible. Meanwhile, set dw8250_serial_out32 to keep consistent
> between serial_out
> and serial_in in ACPI.

I will review it perhaps late this week.
Though have a quick question, did you test it? I hope you have done.

> 
> Change since v2:
> - Add a new patch to use new var dev in probe
> - Use built-in device properties to set device parameters for existing
> device probed by acpi,
>   suggested by Andy Shevchenko 
> 
> 
> Change since v1:
> - Use acpi_match_device() instead of acpi_dev_found(), limit the check
> to the device
>   being probed and not a global search for whole DSDT (pointed by grae
> me.gregory@linaro.org)
> 
> Kefeng Wang (3):
>   serial: 8250_dw: make dw8250_set_termios as default set_termios
>     callback
>   serial: 8250_dw: Use new dev variable in probe
>   serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc
> 
>  drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++------
> -----------
>  1 file changed, 41 insertions(+), 30 deletions(-)
> 

-- 

Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 0/3]

[Kefeng Wang]

On 2016/8/10 0:01, Andy Shevchenko wrote:
> On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
>> Make dw8250_set_termios() as the default set_termios callback for 8250
>> dw uart, correct me
>> if I am wrong.
>>
>> Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that
>> it is not 16500
>> compatible. Meanwhile, set dw8250_serial_out32 to keep consistent
>> between serial_out
>> and serial_in in ACPI.
> 
> I will review it perhaps late this week.

Thanks.

> Though have a quick question, did you test it? I hope you have done.

Yes, I have already tested the patchset on D02 board.

Kefeng

> 
>>
>> Change since v2:
>> - Add a new patch to use new var dev in probe
>> - Use built-in device properties to set device parameters for existing
>> device probed by acpi,
>>   suggested by Andy Shevchenko 
>>
>>
>> Change since v1:
>> - Use acpi_match_device() instead of acpi_dev_found(), limit the check
>> to the device
>>   being probed and not a global search for whole DSDT (pointed by grae
>> me.gregory@linaro.org)
>>
>> Kefeng Wang (3):
>>   serial: 8250_dw: make dw8250_set_termios as default set_termios
>>     callback
>>   serial: 8250_dw: Use new dev variable in probe
>>   serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc
>>
>>  drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++------
>> -----------
>>  1 file changed, 41 insertions(+), 30 deletions(-)
>>
> 

Re: [PATCH v3 0/3]

[Kefeng Wang]

On 2016/8/10 13:36, Kefeng Wang wrote:
> 
> 
> On 2016/8/10 0:01, Andy Shevchenko wrote:
>> On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
>>> Make dw8250_set_termios() as the default set_termios callback for 8250
>>> dw uart, correct me
>>> if I am wrong.
>>>
>>> Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that
>>> it is not 16500
>>> compatible. Meanwhile, set dw8250_serial_out32 to keep consistent
>>> between serial_out
>>> and serial_in in ACPI.
>>
>> I will review it perhaps late this week.
> 

Hi Andy, kindly ping...


> Thanks.
> 
>> Though have a quick question, did you test it? I hope you have done.
> 
> Yes, I have already tested the patchset on D02 board.
> 
> Kefeng
> 
>>
>>>
>>> Change since v2:
>>> - Add a new patch to use new var dev in probe
>>> - Use built-in device properties to set device parameters for existing
>>> device probed by acpi,
>>>   suggested by Andy Shevchenko 
>>>
>>>
>>> Change since v1:
>>> - Use acpi_match_device() instead of acpi_dev_found(), limit the check
>>> to the device
>>>   being probed and not a global search for whole DSDT (pointed by grae
>>> me.gregory@linaro.org)
>>>
>>> Kefeng Wang (3):
>>>   serial: 8250_dw: make dw8250_set_termios as default set_termios
>>>     callback
>>>   serial: 8250_dw: Use new dev variable in probe
>>>   serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc
>>>
>>>  drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++------
>>> -----------
>>>  1 file changed, 41 insertions(+), 30 deletions(-)
>>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH v3 0/3]

[Andy Shevchenko]

On Fri, 2016-08-19 at 14:26 +0800, Kefeng Wang wrote:
> 
> Make dw8250_set_termios() as the default set_termios callback
> > > > for 8250
> > > > dw uart, correct me
> > > > if I am wrong.
> > > > 
> > > > Then add ACPI support for uart on Hisilicon Hip05 soc, be
> > > > careful that
> > > > it is not 16500
> > > > compatible. Meanwhile, set dw8250_serial_out32 to keep
> > > > consistent
> > > > between serial_out
> > > > and serial_in in ACPI.
> > > 
> > > I will review it perhaps late this week.
> > 
> 
> Hi Andy, kindly ping...

Sorry for being a bit late, but here we are.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy