diff mbox series

[v5,2/4] drivers/tty/serial/8250: refactor sirq and lpc address setting code

Message ID 20210408011637.5361-3-zev@bewilderbeest.net
State New
Headers show
Series serial: 8250_aspeed_vuart: generalized DT properties | expand

Commit Message

Zev Weiss April 8, 2021, 1:16 a.m. UTC 
This splits dedicated aspeed_vuart_set_{sirq,lpc_address}() functions
out of the sysfs store functions in preparation for adding DT
properties that will be poking the same registers.  While we're at it,
these functions now provide some basic bounds-checking on their
arguments.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/tty/serial/8250/8250_aspeed_vuart.c | 51 ++++++++++++++-------
 1 file changed, 35 insertions(+), 16 deletions(-)

Comments

Andrew Jeffery April 9, 2021, 5:06 a.m. UTC | #1 
On Thu, 8 Apr 2021, at 10:46, Zev Weiss wrote:
> This splits dedicated aspeed_vuart_set_{sirq,lpc_address}() functions
> out of the sysfs store functions in preparation for adding DT
> properties that will be poking the same registers.  While we're at it,
> these functions now provide some basic bounds-checking on their
> arguments.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  drivers/tty/serial/8250/8250_aspeed_vuart.c | 51 ++++++++++++++-------
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c 
> b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index c33e02cbde93..8433f8dbb186 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -72,22 +72,31 @@ static ssize_t lpc_address_show(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr);
>  }
>  
> +static int aspeed_vuart_set_lpc_address(struct aspeed_vuart *vuart, u32 addr)
> +{
> +	if (addr > U16_MAX)
> +		return -EINVAL;
> +
> +	writeb(addr >> 8, vuart->regs + ASPEED_VUART_ADDRH);
> +	writeb(addr >> 0, vuart->regs + ASPEED_VUART_ADDRL);
> +
> +	return 0;
> +}
> +
>  static ssize_t lpc_address_store(struct device *dev,
>  				 struct device_attribute *attr,
>  				 const char *buf, size_t count)
>  {
>  	struct aspeed_vuart *vuart = dev_get_drvdata(dev);
> -	unsigned long val;
> +	u32 val;
>  	int err;
>  
> -	err = kstrtoul(buf, 0, &val);
> +	err = kstrtou32(buf, 0, &val);
>  	if (err)
>  		return err;
>  
> -	writeb(val >> 8, vuart->regs + ASPEED_VUART_ADDRH);
> -	writeb(val >> 0, vuart->regs + ASPEED_VUART_ADDRL);
> -
> -	return count;
> +	err = aspeed_vuart_set_lpc_address(vuart, val);
> +	return err ? : count;
>  }
>  
>  static DEVICE_ATTR_RW(lpc_address);
> @@ -105,27 +114,37 @@ static ssize_t sirq_show(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg);
>  }
>  
> +static int aspeed_vuart_set_sirq(struct aspeed_vuart *vuart, u32 sirq)
> +{
> +	u8 reg;
> +
> +	if (sirq > (ASPEED_VUART_GCRB_HOST_SIRQ_MASK >> ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT))
> +		return -EINVAL;
> +
> +	sirq <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT;
> +	sirq &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK;

This might be less verbose if we reordered things a little:

```
sirq <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT;
if (sirq & ASPEED_VUART_GCRB_HOST_SIRQ_MASK)
	return -EINVAL;
sirq &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK;
```

But otherwise it looks okay, so

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

> +
> +	reg = readb(vuart->regs + ASPEED_VUART_GCRB);
> +	reg &= ~ASPEED_VUART_GCRB_HOST_SIRQ_MASK;
> +	reg |= sirq;
> +	writeb(reg, vuart->regs + ASPEED_VUART_GCRB);
> +
> +	return 0;
> +}
> +
>  static ssize_t sirq_store(struct device *dev, struct device_attribute 
> *attr,
>  			  const char *buf, size_t count)
>  {
>  	struct aspeed_vuart *vuart = dev_get_drvdata(dev);
>  	unsigned long val;
>  	int err;
> -	u8 reg;
>  
>  	err = kstrtoul(buf, 0, &val);
>  	if (err)
>  		return err;
>  
> -	val <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT;
> -	val &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK;
> -
> -	reg = readb(vuart->regs + ASPEED_VUART_GCRB);
> -	reg &= ~ASPEED_VUART_GCRB_HOST_SIRQ_MASK;
> -	reg |= val;
> -	writeb(reg, vuart->regs + ASPEED_VUART_GCRB);
> -
> -	return count;
> +	err = aspeed_vuart_set_sirq(vuart, val);
> +	return err ? : count;
>  }
>  
>  static DEVICE_ATTR_RW(sirq);
> -- 
> 2.31.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Zev Weiss April 9, 2021, 7:38 a.m. UTC | #2 
On Fri, Apr 09, 2021 at 02:24:08AM CDT, Andy Shevchenko wrote:
>On Thursday, April 8, 2021, Zev Weiss <zev@bewilderbeest.net> wrote:
>
>> This splits dedicated aspeed_vuart_set_{sirq,lpc_address}() functions
>> out of the sysfs store functions in preparation for adding DT
>> properties that will be poking the same registers.  While we're at it,
>> these functions now provide some basic bounds-checking on their
>> arguments.
>>
>>
>
>Please, use prefix “serial: 8250_aspeed_vuart:” instead of what you have in
>the subject line. I think I have told this already
>
>

Ah, sorry -- I fixed the cover letter after your first comment (which 
had definitely been under-tagged); for the patches themselves I was 
following the example of the last patch in that particular area 
(8d310c9107a2), though I guess that wasn't the right model to follow.  
I'll use the requested format in the future.
Andy Shevchenko April 9, 2021, 9:51 a.m. UTC | #3 
On Fri, Apr 9, 2021 at 10:38 AM Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Fri, Apr 09, 2021 at 02:24:08AM CDT, Andy Shevchenko wrote:
> >On Thursday, April 8, 2021, Zev Weiss <zev@bewilderbeest.net> wrote:
> >
> >> This splits dedicated aspeed_vuart_set_{sirq,lpc_address}() functions
> >> out of the sysfs store functions in preparation for adding DT
> >> properties that will be poking the same registers.  While we're at it,
> >> these functions now provide some basic bounds-checking on their
> >> arguments.
> >>
> >>
> >
> >Please, use prefix “serial: 8250_aspeed_vuart:” instead of what you have in
> >the subject line. I think I have told this already
> >
> >
>
> Ah, sorry -- I fixed the cover letter after your first comment (which
> had definitely been under-tagged); for the patches themselves I was
> following the example of the last patch in that particular area
> (8d310c9107a2), though I guess that wasn't the right model to follow.
> I'll use the requested format in the future.

Just random amount of most recent patches against 8250 driver:

e47eb5241a8f serial: 8250: Avoid new transfers when shutting down
e49950d3e737 serial: 8250_dma: use linear buffer for transmit
34255381fabd serial: 8250_port: Try to run DMA Rx on timeout condition
7d7dec450a66 8250_tegra: clean up tegra_uart_handle_break
c3ae3dc896fa serial: 8250_pci: Drop bogus __refdata annotation
d96f04d347e4 serial: 8250_omap: Avoid FIFO corruption caused by MDR1 access
6e4e636e0e3e serial: 8250-mtk: Fix reference leak in mtk8250_probe
a609c58086e3 tty: serial: 8250: 8250_port: Move prototypes to shared location
6f9918504129 serial: 8250: 8250_omap: Fix unused variable warning
d4548b14dd7e serial: 8250: 8250_omap: Fix possible array out of bounds access
912ab37c7987 serial: 8250_mtk: Fix uart_get_baud_rate warning
439c7183e5b9 serial: 8250: 8250_omap: Disable RX interrupt after DMA enable
32ed248042d1 tty: serial: 8250: serial_cs: Remove unused/unchecked
variable 'err'
85985a3dcd74 serial: 8250_dw: Fix clk-notifier/port suspend deadlock
c8dff3aa8241 serial: 8250: Skip uninitialized TTY port baud rate update
7718453e3696 serial: 8250: Discard RTS/DTS setting from clock update method
409cc4541ade serial: 8250_fsl: Fix TX interrupt handling condition
3c5a87be170a serial: 8250_pci: Add Realtek 816a and 816b
ea4de367e57d tty: serial: 8250_mtk: set regshift for mmio32
57cee0713118 serial: 8250_pci: Remove unused function get_pci_irq()
11361610b005 serial: 8250_fsl: Add ACPI support
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
index c33e02cbde93..8433f8dbb186 100644
--- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
+++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
@@ -72,22 +72,31 @@  static ssize_t lpc_address_show(struct device *dev,
 	return snprintf(buf, PAGE_SIZE - 1, "0x%x\n", addr);
 }
 
+static int aspeed_vuart_set_lpc_address(struct aspeed_vuart *vuart, u32 addr)
+{
+	if (addr > U16_MAX)
+		return -EINVAL;
+
+	writeb(addr >> 8, vuart->regs + ASPEED_VUART_ADDRH);
+	writeb(addr >> 0, vuart->regs + ASPEED_VUART_ADDRL);
+
+	return 0;
+}
+
 static ssize_t lpc_address_store(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)
 {
 	struct aspeed_vuart *vuart = dev_get_drvdata(dev);
-	unsigned long val;
+	u32 val;
 	int err;
 
-	err = kstrtoul(buf, 0, &val);
+	err = kstrtou32(buf, 0, &val);
 	if (err)
 		return err;
 
-	writeb(val >> 8, vuart->regs + ASPEED_VUART_ADDRH);
-	writeb(val >> 0, vuart->regs + ASPEED_VUART_ADDRL);
-
-	return count;
+	err = aspeed_vuart_set_lpc_address(vuart, val);
+	return err ? : count;
 }
 
 static DEVICE_ATTR_RW(lpc_address);
@@ -105,27 +114,37 @@  static ssize_t sirq_show(struct device *dev,
 	return snprintf(buf, PAGE_SIZE - 1, "%u\n", reg);
 }
 
+static int aspeed_vuart_set_sirq(struct aspeed_vuart *vuart, u32 sirq)
+{
+	u8 reg;
+
+	if (sirq > (ASPEED_VUART_GCRB_HOST_SIRQ_MASK >> ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT))
+		return -EINVAL;
+
+	sirq <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT;
+	sirq &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK;
+
+	reg = readb(vuart->regs + ASPEED_VUART_GCRB);
+	reg &= ~ASPEED_VUART_GCRB_HOST_SIRQ_MASK;
+	reg |= sirq;
+	writeb(reg, vuart->regs + ASPEED_VUART_GCRB);
+
+	return 0;
+}
+
 static ssize_t sirq_store(struct device *dev, struct device_attribute *attr,
 			  const char *buf, size_t count)
 {
 	struct aspeed_vuart *vuart = dev_get_drvdata(dev);
 	unsigned long val;
 	int err;
-	u8 reg;
 
 	err = kstrtoul(buf, 0, &val);
 	if (err)
 		return err;
 
-	val <<= ASPEED_VUART_GCRB_HOST_SIRQ_SHIFT;
-	val &= ASPEED_VUART_GCRB_HOST_SIRQ_MASK;
-
-	reg = readb(vuart->regs + ASPEED_VUART_GCRB);
-	reg &= ~ASPEED_VUART_GCRB_HOST_SIRQ_MASK;
-	reg |= val;
-	writeb(reg, vuart->regs + ASPEED_VUART_GCRB);
-
-	return count;
+	err = aspeed_vuart_set_sirq(vuart, val);
+	return err ? : count;
 }
 
 static DEVICE_ATTR_RW(sirq);