All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] gnss: get serial speed from subdrivers
@ 2019-05-16 16:37 ` Loys Ollivier
  0 siblings, 0 replies; 6+ messages in thread
From: Loys Ollivier @ 2019-05-16 16:37 UTC (permalink / raw)
  To: Johan Hovold, Matthias Brugger
  Cc: Loys Ollivier, Colin Ian King, Corentin Labbe, linux-arm-kernel,
	linux-mediatek, linux-kernel, baylibre-upstreaming

The default serial speed was hardcoded in the code.
Rename current-speed to default-speed.
Add a function parameter that lets the subdrivers specify their
default speed.
If not specified fallback to the device-tree default-speed.

Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
---
Hello,

This patch moves the currently hardcoded, default serial speed
to the subdrivers.
If the default speed is not specified by the subdriver then it is read
from the device tree.

Changes since v1[0]
- Use u32 data types instead of uint

[0]: https://lore.kernel.org/lkml/1557322788-10403-1-git-send-email-lollivier@baylibre.com/

Cheers,
Loys

 drivers/gnss/mtk.c    |  7 ++++++-
 drivers/gnss/serial.c | 21 +++++++++++++--------
 drivers/gnss/serial.h |  3 ++-
 drivers/gnss/ubx.c    |  3 ++-
 4 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/gnss/mtk.c b/drivers/gnss/mtk.c
index d1fc55560daf..1d35bcb52072 100644
--- a/drivers/gnss/mtk.c
+++ b/drivers/gnss/mtk.c
@@ -16,6 +16,10 @@
 
 #include "serial.h"
 
+static uint serial_speed = 9600; /* Serial speed (baud rate) */
+module_param(serial_speed, uint, 0644);
+MODULE_PARM_DESC(serial_speed, "Serial baud rate (bit/s), (default = 9600)");
+
 struct mtk_data {
 	struct regulator *vbackup;
 	struct regulator *vcc;
@@ -69,7 +73,8 @@ static int mtk_probe(struct serdev_device *serdev)
 	struct mtk_data *data;
 	int ret;
 
-	gserial = gnss_serial_allocate(serdev, sizeof(*data));
+	gserial = gnss_serial_allocate(serdev, sizeof(*data),
+				       (u32)serial_speed);
 	if (IS_ERR(gserial)) {
 		ret = PTR_ERR(gserial);
 		return ret;
diff --git a/drivers/gnss/serial.c b/drivers/gnss/serial.c
index def64b36d994..3be799702291 100644
--- a/drivers/gnss/serial.c
+++ b/drivers/gnss/serial.c
@@ -103,17 +103,13 @@ static int gnss_serial_set_power(struct gnss_serial *gserial,
 	return gserial->ops->set_power(gserial, state);
 }
 
-/*
- * FIXME: need to provide subdriver defaults or separate dt parsing from
- * allocation.
- */
 static int gnss_serial_parse_dt(struct serdev_device *serdev)
 {
 	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
 	struct device_node *node = serdev->dev.of_node;
-	u32 speed = 4800;
+	u32 speed;
 
-	of_property_read_u32(node, "current-speed", &speed);
+	of_property_read_u32(node, "default-speed", &speed);
 
 	gserial->speed = speed;
 
@@ -121,7 +117,8 @@ static int gnss_serial_parse_dt(struct serdev_device *serdev)
 }
 
 struct gnss_serial *gnss_serial_allocate(struct serdev_device *serdev,
-						size_t data_size)
+					 size_t data_size,
+					 u32 serial_speed)
 {
 	struct gnss_serial *gserial;
 	struct gnss_device *gdev;
@@ -146,10 +143,18 @@ struct gnss_serial *gnss_serial_allocate(struct serdev_device *serdev,
 	serdev_device_set_drvdata(serdev, gserial);
 	serdev_device_set_client_ops(serdev, &gnss_serial_serdev_ops);
 
-	ret = gnss_serial_parse_dt(serdev);
+	/* Serial speed provided by subdriver takes precedence over dt*/
+	if (!serial_speed)
+		ret = gnss_serial_parse_dt(serdev);
+	else
+		gserial->speed = serial_speed;
+
 	if (ret)
 		goto err_put_device;
 
+	if (!gserial->speed)
+		return -EINVAL;
+
 	return gserial;
 
 err_put_device:
diff --git a/drivers/gnss/serial.h b/drivers/gnss/serial.h
index 980ffdc86c2a..17df61e399e6 100644
--- a/drivers/gnss/serial.h
+++ b/drivers/gnss/serial.h
@@ -33,7 +33,8 @@ struct gnss_serial_ops {
 extern const struct dev_pm_ops gnss_serial_pm_ops;
 
 struct gnss_serial *gnss_serial_allocate(struct serdev_device *gserial,
-						size_t data_size);
+					 size_t data_size,
+					 u32 serial_speed);
 void gnss_serial_free(struct gnss_serial *gserial);
 
 int gnss_serial_register(struct gnss_serial *gserial);
diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
index 7b05bc40532e..52ae6e4987e0 100644
--- a/drivers/gnss/ubx.c
+++ b/drivers/gnss/ubx.c
@@ -68,8 +68,9 @@ static int ubx_probe(struct serdev_device *serdev)
 	struct gnss_serial *gserial;
 	struct ubx_data *data;
 	int ret;
+	u32 speed = 4800;
 
-	gserial = gnss_serial_allocate(serdev, sizeof(*data));
+	gserial = gnss_serial_allocate(serdev, sizeof(*data), speed);
 	if (IS_ERR(gserial)) {
 		ret = PTR_ERR(gserial);
 		return ret;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v2] gnss: get serial speed from subdrivers
@ 2019-05-16 16:37 ` Loys Ollivier
  0 siblings, 0 replies; 6+ messages in thread
From: Loys Ollivier @ 2019-05-16 16:37 UTC (permalink / raw)
  To: Johan Hovold, Matthias Brugger
  Cc: baylibre-upstreaming, Loys Ollivier, linux-kernel,
	Corentin Labbe, linux-mediatek, Colin Ian King, linux-arm-kernel

The default serial speed was hardcoded in the code.
Rename current-speed to default-speed.
Add a function parameter that lets the subdrivers specify their
default speed.
If not specified fallback to the device-tree default-speed.

Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
---
Hello,

This patch moves the currently hardcoded, default serial speed
to the subdrivers.
If the default speed is not specified by the subdriver then it is read
from the device tree.

Changes since v1[0]
- Use u32 data types instead of uint

[0]: https://lore.kernel.org/lkml/1557322788-10403-1-git-send-email-lollivier@baylibre.com/

Cheers,
Loys

 drivers/gnss/mtk.c    |  7 ++++++-
 drivers/gnss/serial.c | 21 +++++++++++++--------
 drivers/gnss/serial.h |  3 ++-
 drivers/gnss/ubx.c    |  3 ++-
 4 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/gnss/mtk.c b/drivers/gnss/mtk.c
index d1fc55560daf..1d35bcb52072 100644
--- a/drivers/gnss/mtk.c
+++ b/drivers/gnss/mtk.c
@@ -16,6 +16,10 @@
 
 #include "serial.h"
 
+static uint serial_speed = 9600; /* Serial speed (baud rate) */
+module_param(serial_speed, uint, 0644);
+MODULE_PARM_DESC(serial_speed, "Serial baud rate (bit/s), (default = 9600)");
+
 struct mtk_data {
 	struct regulator *vbackup;
 	struct regulator *vcc;
@@ -69,7 +73,8 @@ static int mtk_probe(struct serdev_device *serdev)
 	struct mtk_data *data;
 	int ret;
 
-	gserial = gnss_serial_allocate(serdev, sizeof(*data));
+	gserial = gnss_serial_allocate(serdev, sizeof(*data),
+				       (u32)serial_speed);
 	if (IS_ERR(gserial)) {
 		ret = PTR_ERR(gserial);
 		return ret;
diff --git a/drivers/gnss/serial.c b/drivers/gnss/serial.c
index def64b36d994..3be799702291 100644
--- a/drivers/gnss/serial.c
+++ b/drivers/gnss/serial.c
@@ -103,17 +103,13 @@ static int gnss_serial_set_power(struct gnss_serial *gserial,
 	return gserial->ops->set_power(gserial, state);
 }
 
-/*
- * FIXME: need to provide subdriver defaults or separate dt parsing from
- * allocation.
- */
 static int gnss_serial_parse_dt(struct serdev_device *serdev)
 {
 	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
 	struct device_node *node = serdev->dev.of_node;
-	u32 speed = 4800;
+	u32 speed;
 
-	of_property_read_u32(node, "current-speed", &speed);
+	of_property_read_u32(node, "default-speed", &speed);
 
 	gserial->speed = speed;
 
@@ -121,7 +117,8 @@ static int gnss_serial_parse_dt(struct serdev_device *serdev)
 }
 
 struct gnss_serial *gnss_serial_allocate(struct serdev_device *serdev,
-						size_t data_size)
+					 size_t data_size,
+					 u32 serial_speed)
 {
 	struct gnss_serial *gserial;
 	struct gnss_device *gdev;
@@ -146,10 +143,18 @@ struct gnss_serial *gnss_serial_allocate(struct serdev_device *serdev,
 	serdev_device_set_drvdata(serdev, gserial);
 	serdev_device_set_client_ops(serdev, &gnss_serial_serdev_ops);
 
-	ret = gnss_serial_parse_dt(serdev);
+	/* Serial speed provided by subdriver takes precedence over dt*/
+	if (!serial_speed)
+		ret = gnss_serial_parse_dt(serdev);
+	else
+		gserial->speed = serial_speed;
+
 	if (ret)
 		goto err_put_device;
 
+	if (!gserial->speed)
+		return -EINVAL;
+
 	return gserial;
 
 err_put_device:
diff --git a/drivers/gnss/serial.h b/drivers/gnss/serial.h
index 980ffdc86c2a..17df61e399e6 100644
--- a/drivers/gnss/serial.h
+++ b/drivers/gnss/serial.h
@@ -33,7 +33,8 @@ struct gnss_serial_ops {
 extern const struct dev_pm_ops gnss_serial_pm_ops;
 
 struct gnss_serial *gnss_serial_allocate(struct serdev_device *gserial,
-						size_t data_size);
+					 size_t data_size,
+					 u32 serial_speed);
 void gnss_serial_free(struct gnss_serial *gserial);
 
 int gnss_serial_register(struct gnss_serial *gserial);
diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
index 7b05bc40532e..52ae6e4987e0 100644
--- a/drivers/gnss/ubx.c
+++ b/drivers/gnss/ubx.c
@@ -68,8 +68,9 @@ static int ubx_probe(struct serdev_device *serdev)
 	struct gnss_serial *gserial;
 	struct ubx_data *data;
 	int ret;
+	u32 speed = 4800;
 
-	gserial = gnss_serial_allocate(serdev, sizeof(*data));
+	gserial = gnss_serial_allocate(serdev, sizeof(*data), speed);
 	if (IS_ERR(gserial)) {
 		ret = PTR_ERR(gserial);
 		return ret;
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] gnss: get serial speed from subdrivers
  2019-05-16 16:37 ` Loys Ollivier
@ 2019-05-16 18:02   ` Robin Murphy
  -1 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2019-05-16 18:02 UTC (permalink / raw)
  To: Loys Ollivier, Johan Hovold, Matthias Brugger
  Cc: baylibre-upstreaming, linux-kernel, Corentin Labbe,
	linux-mediatek, Colin Ian King, linux-arm-kernel

On 16/05/2019 17:37, Loys Ollivier wrote:
> The default serial speed was hardcoded in the code.
> Rename current-speed to default-speed.
> Add a function parameter that lets the subdrivers specify their
> default speed.
> If not specified fallback to the device-tree default-speed.
> 
> Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
> ---
> Hello,
> 
> This patch moves the currently hardcoded, default serial speed
> to the subdrivers.
> If the default speed is not specified by the subdriver then it is read
> from the device tree.
> 
> Changes since v1[0]
> - Use u32 data types instead of uint
> 
> [0]: https://lore.kernel.org/lkml/1557322788-10403-1-git-send-email-lollivier@baylibre.com/
> 
> Cheers,
> Loys
> 
>   drivers/gnss/mtk.c    |  7 ++++++-
>   drivers/gnss/serial.c | 21 +++++++++++++--------
>   drivers/gnss/serial.h |  3 ++-
>   drivers/gnss/ubx.c    |  3 ++-
>   4 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gnss/mtk.c b/drivers/gnss/mtk.c
> index d1fc55560daf..1d35bcb52072 100644
> --- a/drivers/gnss/mtk.c
> +++ b/drivers/gnss/mtk.c
> @@ -16,6 +16,10 @@
>   
>   #include "serial.h"
>   
> +static uint serial_speed = 9600; /* Serial speed (baud rate) */
> +module_param(serial_speed, uint, 0644);
> +MODULE_PARM_DESC(serial_speed, "Serial baud rate (bit/s), (default = 9600)");
> +
>   struct mtk_data {
>   	struct regulator *vbackup;
>   	struct regulator *vcc;
> @@ -69,7 +73,8 @@ static int mtk_probe(struct serdev_device *serdev)
>   	struct mtk_data *data;
>   	int ret;
>   
> -	gserial = gnss_serial_allocate(serdev, sizeof(*data));
> +	gserial = gnss_serial_allocate(serdev, sizeof(*data),
> +				       (u32)serial_speed);
>   	if (IS_ERR(gserial)) {
>   		ret = PTR_ERR(gserial);
>   		return ret;
> diff --git a/drivers/gnss/serial.c b/drivers/gnss/serial.c
> index def64b36d994..3be799702291 100644
> --- a/drivers/gnss/serial.c
> +++ b/drivers/gnss/serial.c
> @@ -103,17 +103,13 @@ static int gnss_serial_set_power(struct gnss_serial *gserial,
>   	return gserial->ops->set_power(gserial, state);
>   }
>   
> -/*
> - * FIXME: need to provide subdriver defaults or separate dt parsing from
> - * allocation.
> - */
>   static int gnss_serial_parse_dt(struct serdev_device *serdev)
>   {
>   	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
>   	struct device_node *node = serdev->dev.of_node;
> -	u32 speed = 4800;
> +	u32 speed;
>   
> -	of_property_read_u32(node, "current-speed", &speed);
> +	of_property_read_u32(node, "default-speed", &speed);
>   
>   	gserial->speed = speed;

Hmm, maybe it's a bit too convoluted for the compiler to warn about, but 
if a "default-speed" property is not present, gserial->speed will now be 
assigned an uninitialised value. I don't know what the intent is neither 
the driver nor the DT provides a value, but you'll either need to bring 
back the fallback initialisation above or propagate errors from 
of_property_read_u32().

Robin.

>   
> @@ -121,7 +117,8 @@ static int gnss_serial_parse_dt(struct serdev_device *serdev)
>   }
>   
>   struct gnss_serial *gnss_serial_allocate(struct serdev_device *serdev,
> -						size_t data_size)
> +					 size_t data_size,
> +					 u32 serial_speed)
>   {
>   	struct gnss_serial *gserial;
>   	struct gnss_device *gdev;
> @@ -146,10 +143,18 @@ struct gnss_serial *gnss_serial_allocate(struct serdev_device *serdev,
>   	serdev_device_set_drvdata(serdev, gserial);
>   	serdev_device_set_client_ops(serdev, &gnss_serial_serdev_ops);
>   
> -	ret = gnss_serial_parse_dt(serdev);
> +	/* Serial speed provided by subdriver takes precedence over dt*/
> +	if (!serial_speed)
> +		ret = gnss_serial_parse_dt(serdev);
> +	else
> +		gserial->speed = serial_speed;
> +
>   	if (ret)
>   		goto err_put_device;
>   
> +	if (!gserial->speed)
> +		return -EINVAL;
> +
>   	return gserial;
>   
>   err_put_device:
> diff --git a/drivers/gnss/serial.h b/drivers/gnss/serial.h
> index 980ffdc86c2a..17df61e399e6 100644
> --- a/drivers/gnss/serial.h
> +++ b/drivers/gnss/serial.h
> @@ -33,7 +33,8 @@ struct gnss_serial_ops {
>   extern const struct dev_pm_ops gnss_serial_pm_ops;
>   
>   struct gnss_serial *gnss_serial_allocate(struct serdev_device *gserial,
> -						size_t data_size);
> +					 size_t data_size,
> +					 u32 serial_speed);
>   void gnss_serial_free(struct gnss_serial *gserial);
>   
>   int gnss_serial_register(struct gnss_serial *gserial);
> diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
> index 7b05bc40532e..52ae6e4987e0 100644
> --- a/drivers/gnss/ubx.c
> +++ b/drivers/gnss/ubx.c
> @@ -68,8 +68,9 @@ static int ubx_probe(struct serdev_device *serdev)
>   	struct gnss_serial *gserial;
>   	struct ubx_data *data;
>   	int ret;
> +	u32 speed = 4800;
>   
> -	gserial = gnss_serial_allocate(serdev, sizeof(*data));
> +	gserial = gnss_serial_allocate(serdev, sizeof(*data), speed);
>   	if (IS_ERR(gserial)) {
>   		ret = PTR_ERR(gserial);
>   		return ret;
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] gnss: get serial speed from subdrivers
@ 2019-05-16 18:02   ` Robin Murphy
  0 siblings, 0 replies; 6+ messages in thread
From: Robin Murphy @ 2019-05-16 18:02 UTC (permalink / raw)
  To: Loys Ollivier, Johan Hovold, Matthias Brugger
  Cc: baylibre-upstreaming, linux-kernel, linux-mediatek,
	Colin Ian King, Corentin Labbe, linux-arm-kernel

On 16/05/2019 17:37, Loys Ollivier wrote:
> The default serial speed was hardcoded in the code.
> Rename current-speed to default-speed.
> Add a function parameter that lets the subdrivers specify their
> default speed.
> If not specified fallback to the device-tree default-speed.
> 
> Signed-off-by: Loys Ollivier <lollivier@baylibre.com>
> ---
> Hello,
> 
> This patch moves the currently hardcoded, default serial speed
> to the subdrivers.
> If the default speed is not specified by the subdriver then it is read
> from the device tree.
> 
> Changes since v1[0]
> - Use u32 data types instead of uint
> 
> [0]: https://lore.kernel.org/lkml/1557322788-10403-1-git-send-email-lollivier@baylibre.com/
> 
> Cheers,
> Loys
> 
>   drivers/gnss/mtk.c    |  7 ++++++-
>   drivers/gnss/serial.c | 21 +++++++++++++--------
>   drivers/gnss/serial.h |  3 ++-
>   drivers/gnss/ubx.c    |  3 ++-
>   4 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gnss/mtk.c b/drivers/gnss/mtk.c
> index d1fc55560daf..1d35bcb52072 100644
> --- a/drivers/gnss/mtk.c
> +++ b/drivers/gnss/mtk.c
> @@ -16,6 +16,10 @@
>   
>   #include "serial.h"
>   
> +static uint serial_speed = 9600; /* Serial speed (baud rate) */
> +module_param(serial_speed, uint, 0644);
> +MODULE_PARM_DESC(serial_speed, "Serial baud rate (bit/s), (default = 9600)");
> +
>   struct mtk_data {
>   	struct regulator *vbackup;
>   	struct regulator *vcc;
> @@ -69,7 +73,8 @@ static int mtk_probe(struct serdev_device *serdev)
>   	struct mtk_data *data;
>   	int ret;
>   
> -	gserial = gnss_serial_allocate(serdev, sizeof(*data));
> +	gserial = gnss_serial_allocate(serdev, sizeof(*data),
> +				       (u32)serial_speed);
>   	if (IS_ERR(gserial)) {
>   		ret = PTR_ERR(gserial);
>   		return ret;
> diff --git a/drivers/gnss/serial.c b/drivers/gnss/serial.c
> index def64b36d994..3be799702291 100644
> --- a/drivers/gnss/serial.c
> +++ b/drivers/gnss/serial.c
> @@ -103,17 +103,13 @@ static int gnss_serial_set_power(struct gnss_serial *gserial,
>   	return gserial->ops->set_power(gserial, state);
>   }
>   
> -/*
> - * FIXME: need to provide subdriver defaults or separate dt parsing from
> - * allocation.
> - */
>   static int gnss_serial_parse_dt(struct serdev_device *serdev)
>   {
>   	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
>   	struct device_node *node = serdev->dev.of_node;
> -	u32 speed = 4800;
> +	u32 speed;
>   
> -	of_property_read_u32(node, "current-speed", &speed);
> +	of_property_read_u32(node, "default-speed", &speed);
>   
>   	gserial->speed = speed;

Hmm, maybe it's a bit too convoluted for the compiler to warn about, but 
if a "default-speed" property is not present, gserial->speed will now be 
assigned an uninitialised value. I don't know what the intent is neither 
the driver nor the DT provides a value, but you'll either need to bring 
back the fallback initialisation above or propagate errors from 
of_property_read_u32().

Robin.

>   
> @@ -121,7 +117,8 @@ static int gnss_serial_parse_dt(struct serdev_device *serdev)
>   }
>   
>   struct gnss_serial *gnss_serial_allocate(struct serdev_device *serdev,
> -						size_t data_size)
> +					 size_t data_size,
> +					 u32 serial_speed)
>   {
>   	struct gnss_serial *gserial;
>   	struct gnss_device *gdev;
> @@ -146,10 +143,18 @@ struct gnss_serial *gnss_serial_allocate(struct serdev_device *serdev,
>   	serdev_device_set_drvdata(serdev, gserial);
>   	serdev_device_set_client_ops(serdev, &gnss_serial_serdev_ops);
>   
> -	ret = gnss_serial_parse_dt(serdev);
> +	/* Serial speed provided by subdriver takes precedence over dt*/
> +	if (!serial_speed)
> +		ret = gnss_serial_parse_dt(serdev);
> +	else
> +		gserial->speed = serial_speed;
> +
>   	if (ret)
>   		goto err_put_device;
>   
> +	if (!gserial->speed)
> +		return -EINVAL;
> +
>   	return gserial;
>   
>   err_put_device:
> diff --git a/drivers/gnss/serial.h b/drivers/gnss/serial.h
> index 980ffdc86c2a..17df61e399e6 100644
> --- a/drivers/gnss/serial.h
> +++ b/drivers/gnss/serial.h
> @@ -33,7 +33,8 @@ struct gnss_serial_ops {
>   extern const struct dev_pm_ops gnss_serial_pm_ops;
>   
>   struct gnss_serial *gnss_serial_allocate(struct serdev_device *gserial,
> -						size_t data_size);
> +					 size_t data_size,
> +					 u32 serial_speed);
>   void gnss_serial_free(struct gnss_serial *gserial);
>   
>   int gnss_serial_register(struct gnss_serial *gserial);
> diff --git a/drivers/gnss/ubx.c b/drivers/gnss/ubx.c
> index 7b05bc40532e..52ae6e4987e0 100644
> --- a/drivers/gnss/ubx.c
> +++ b/drivers/gnss/ubx.c
> @@ -68,8 +68,9 @@ static int ubx_probe(struct serdev_device *serdev)
>   	struct gnss_serial *gserial;
>   	struct ubx_data *data;
>   	int ret;
> +	u32 speed = 4800;
>   
> -	gserial = gnss_serial_allocate(serdev, sizeof(*data));
> +	gserial = gnss_serial_allocate(serdev, sizeof(*data), speed);
>   	if (IS_ERR(gserial)) {
>   		ret = PTR_ERR(gserial);
>   		return ret;
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] gnss: get serial speed from subdrivers
  2019-05-16 18:02   ` Robin Murphy
@ 2019-05-17  8:45     ` Loys Ollivier
  -1 siblings, 0 replies; 6+ messages in thread
From: Loys Ollivier @ 2019-05-17  8:45 UTC (permalink / raw)
  To: Robin Murphy, Johan Hovold, Matthias Brugger
  Cc: baylibre-upstreaming, linux-kernel, Corentin Labbe,
	linux-mediatek, Colin Ian King, linux-arm-kernel

On Thu 16 May 2019 at 19:02, Robin Murphy <robin.murphy@arm.com> wrote:

>> -/*
>> - * FIXME: need to provide subdriver defaults or separate dt parsing from
>> - * allocation.
>> - */
>>   static int gnss_serial_parse_dt(struct serdev_device *serdev)
>>   {
>>   	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
>>   	struct device_node *node = serdev->dev.of_node;
>> -	u32 speed = 4800;
>> +	u32 speed;
>>   
>> -	of_property_read_u32(node, "current-speed", &speed);
>> +	of_property_read_u32(node, "default-speed", &speed);
>>   
>>   	gserial->speed = speed;
>
> Hmm, maybe it's a bit too convoluted for the compiler to warn about, but 
> if a "default-speed" property is not present, gserial->speed will now be 
> assigned an uninitialised value. I don't know what the intent is neither 
> the driver nor the DT provides a value, but you'll either need to bring 
> back the fallback initialisation above or propagate errors from 
> of_property_read_u32().
>
> Robin.
>

Robin,
Good point, thank you for your review. I'll think about it and see about
a fallback scenario.
I would be in favour of propagating the error because default values are
very dependent on the hardware in use.

Best,

Loys

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] gnss: get serial speed from subdrivers
@ 2019-05-17  8:45     ` Loys Ollivier
  0 siblings, 0 replies; 6+ messages in thread
From: Loys Ollivier @ 2019-05-17  8:45 UTC (permalink / raw)
  To: Robin Murphy, Johan Hovold, Matthias Brugger
  Cc: baylibre-upstreaming, linux-kernel, linux-mediatek,
	Colin Ian King, Corentin Labbe, linux-arm-kernel

On Thu 16 May 2019 at 19:02, Robin Murphy <robin.murphy@arm.com> wrote:

>> -/*
>> - * FIXME: need to provide subdriver defaults or separate dt parsing from
>> - * allocation.
>> - */
>>   static int gnss_serial_parse_dt(struct serdev_device *serdev)
>>   {
>>   	struct gnss_serial *gserial = serdev_device_get_drvdata(serdev);
>>   	struct device_node *node = serdev->dev.of_node;
>> -	u32 speed = 4800;
>> +	u32 speed;
>>   
>> -	of_property_read_u32(node, "current-speed", &speed);
>> +	of_property_read_u32(node, "default-speed", &speed);
>>   
>>   	gserial->speed = speed;
>
> Hmm, maybe it's a bit too convoluted for the compiler to warn about, but 
> if a "default-speed" property is not present, gserial->speed will now be 
> assigned an uninitialised value. I don't know what the intent is neither 
> the driver nor the DT provides a value, but you'll either need to bring 
> back the fallback initialisation above or propagate errors from 
> of_property_read_u32().
>
> Robin.
>

Robin,
Good point, thank you for your review. I'll think about it and see about
a fallback scenario.
I would be in favour of propagating the error because default values are
very dependent on the hardware in use.

Best,

Loys

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-17  8:46 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-16 16:37 [PATCH v2] gnss: get serial speed from subdrivers Loys Ollivier
2019-05-16 16:37 ` Loys Ollivier
2019-05-16 18:02 ` Robin Murphy
2019-05-16 18:02   ` Robin Murphy
2019-05-17  8:45   ` Loys Ollivier
2019-05-17  8:45     ` Loys Ollivier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.