Received-SPF: pass (google.com: domain of linux-kernel+bounces-55854-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: "Tamseel Shams" <m.shams@samsung.com>
To: "'Krzysztof Kozlowski'" <krzysztof.kozlowski@linaro.org>,
	<alim.akhtar@samsung.com>, <krzysztof.kozlowski+dt@linaro.org>,
	<gregkh@linuxfoundation.org>, <jirislaby@kernel.org>
Cc: <linux-arm-kernel@lists.infradead.org>,
	<linux-samsung-soc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-serial@vger.kernel.org>
In-Reply-To: <0a2604e7-8b30-4cf9-9099-ba3681bc5538@linaro.org>
Subject: RE: [PATCH v2] serial: samsung: honor fifosize from dts at first
Date: Tue, 6 Feb 2024 17:03:56 +0530
Message-ID: <006b01da58f0$601316b0$20394410$@samsung.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Language: en-in
Thread-Index: AQGLvBEqWUIU8btMzlwH+Ehg5kjUAwE8LAIjAhrCDkaxf69ZIA==
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 105P
DLP-Filter: Pass
References: <CGME20240205082441epcas5p1e904f4f95852de3fd8663742ed5131df@epcas5p1.samsung.com>
	<20240205082434.36531-1-m.shams@samsung.com>
	<0a2604e7-8b30-4cf9-9099-ba3681bc5538@linaro.org>

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@linaro.org]
> Sent: 06 February 2024 15:42
> To: Tamseel Shams <m.shams@samsung.com>; alim.akhtar@samsung.com;
> krzysztof.kozlowski+dt@linaro.org; gregkh@linuxfoundation.org;
> jirislaby@kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; linux-samsung-
> soc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> serial@vger.kernel.org
> Subject: Re: [PATCH v2] serial: samsung: honor fifosize from dts at first
> 
> On 05/02/2024 09:24, Tamseel Shams wrote:
> > Currently for platforms which passes UART fifosize from DT gets
> > override by local driver structure "s3c24xx_serial_drv_data", which is
> > not intended. Change the code to honor fifosize from device tree at
> > first.
> >
> > Signed-off-by: Tamseel Shams <m.shams@samsung.com>
> > ---
> > Change Log:
> > v1 -> v2:
> > Acknowledged Krzysztof's comments
> > Initialized "ret" variable
> >
> >  drivers/tty/serial/samsung_tty.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c
> > b/drivers/tty/serial/samsung_tty.c
> > index 71d17d804fda..e5dc2c32b1bd 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -1952,7 +1952,7 @@ static int s3c24xx_serial_probe(struct
> platform_device *pdev)
> >  	struct device_node *np = pdev->dev.of_node;
> >  	struct s3c24xx_uart_port *ourport;
> >  	int index = probe_index;
> > -	int ret, prop = 0;
> > +	int ret = 1, prop = 0;
> 
> I am sorry, but return of probe function cannot be positive.
> 

Thanks for the review.

I am reusing the "ret" variable to check whether fifosize property
is present in DT or not. It will be overwritten at later stage in probe
function before returning. Also, "ret" variable is used to return from
probe function only in case of failure i.e. "ret" is negative.
Currently, probe function always returns 0 in case of success.

For better readability, I am thinking of introducing a new variable for
checking the return value from function "of_property_read_u32" while
getting "samsung,uart-fifosize" property.

What are your thoughts on that?

> >
> >  	if (np) {
> >  		ret = of_alias_get_id(np, "serial"); @@ -1990,8 +1990,7 @@
> static
> > int s3c24xx_serial_probe(struct platform_device *pdev)
> >  	}
> >
> >  	if (np) {
> > -		of_property_read_u32(np,
> > -			"samsung,uart-fifosize", &ourport->port.fifosize);
> > +		ret = of_property_read_u32(np, "samsung,uart-fifosize",
> > +&ourport->port.fifosize);
> >
> >  		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> >  			switch (prop) {
> > @@ -2009,10 +2008,13 @@ static int s3c24xx_serial_probe(struct
> platform_device *pdev)
> >  		}
> >  	}
> >
> > -	if (ourport->drv_data->fifosize[index])
> > -		ourport->port.fifosize = ourport->drv_data->fifosize[index];
> > -	else if (ourport->info->fifosize)
> > -		ourport->port.fifosize = ourport->info->fifosize;
> > +	if (ret) {
> 
> Are you sure that you are checking correct ret?

Explanation same as above. 


Thanks & Regards,
Tamseel Shams