RE: [PATCH v2] serial: samsung: honor fifosize from dts at first

[Tamseel Shams]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@xxxxxxxxxx]
> Sent: 06 February 2024 15:42
> To: Tamseel Shams <m.shams@xxxxxxxxxxx>; alim.akhtar@xxxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> jirislaby@xxxxxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-samsung-
> soc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> serial@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxx>
> > ---
> > 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