linux-kernel - Re: [PATCH 1/2] gnss: ubx: customize serial device open to set U-Blox Zed-F9P baud

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <da606e4b-9fe9-ca47-2c9b-1b32ae2ce498@linaro.org>
Date:   Mon, 1 May 2023 19:40:35 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     alison@...-devel.com, johan@...nel.org
Cc:     robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        achaiken@...ora.tech
Subject: Re: [PATCH 1/2] gnss: ubx: customize serial device open to set U-Blox
 Zed-F9P baud

On 01/05/2023 19:01, alison@...-devel.com wrote:
> From: Alison Chaiken <achaiken@...ora.tech>
> 
> Add support for setting the baud rate of U-Blox Zed-F9P GNSS devices.
> Provide functions that support writing of arbitrary configuration
> messages to the device plus one that specifically configures the baud
> rate.  Override the default gnss_serial_open() with a new method that
> writes the configuration message to the GNSS if the devicetree declares
> it to be a Zed F9P and requests a non-default baud.  Add a boolean flag
> to the ubx_data private data of the GNSS driver in order to track
> whether the configuration message has already been written.  Set the Zed
> F9P to its default port speed if the devicetree does not specify a
> value.
> 
> Signed-off-by: Alison Chaiken <achaiken@...ora.tech>
> ---
>  drivers/gnss/ubx.c | 195 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 195 insertions(+)
> 

Thank you for your patch. There is something to discuss/improve.

> +/* Configure the Zed F9P baud rate via the UBX-CFG-VALSET message. */
> +static int set_zedf9p_baud(struct gnss_device *gdev,
> +					struct serdev_device *serdev, const speed_t speed)
> +{
> +	size_t count = 0U;
> +	int ret;
> +
> +	if (speed == ZED_F9P_DEFAULT_BAUD)
> +		return 0;
> +
> +	ret = prepare_zedf9p_config_msg(speed, &gdev->dev, ZED_F9P_BAUD_CONFIG_REGISTER);
> +	if (ret)
> +		return ret;
> +	/* Initially set the UART to the default speed to match the GNSS' power-on value. */
> +	serdev_device_set_baudrate(serdev, ZED_F9P_DEFAULT_BAUD);
> +	/* Now set the new baud rate. */
> +	count = gdev->ops->write_raw(gdev, ZED_F9P_CFG_VALSET_MSG, CFG_MSG_TOTAL_LEN);
> +	if (count != CFG_MSG_TOTAL_LEN)
> +		return count;
> +
> +	return 0;
> +}
> +
> +static int ubx_serial_open(struct gnss_device *gdev)
> +{
> +	struct gnss_serial *gserial = gnss_get_drvdata(gdev);
> +	struct serdev_device *serdev = gserial->serdev;
> +	struct ubx_data *data = gnss_serial_get_drvdata(gserial);
> +	struct device_node *np;
> +	int ret;
> +
> +	ret = serdev_device_open(serdev);
> +	if (ret)
> +		return ret;
> +
> +	serdev_device_set_flow_control(serdev, false);
> +
> +	np = serdev->dev.of_node;
> +	if ((of_device_is_compatible(np, "u-blox,zed-f9p")) && (!data->is_configured)) {

Use driver data/match data for such customizations. compatibles
sprinkled over the driver code do not scale, make code unreadable. They
also obfuscate a but compatibility - based on your of_device_id I would
claim devices are compatible and you can remove all the entries except one.

...

> @@ -133,6 +327,7 @@ static const struct of_device_id ubx_of_match[] = {
>  	{ .compatible = "u-blox,neo-6m" },
>  	{ .compatible = "u-blox,neo-8" },
>  	{ .compatible = "u-blox,neo-m8" },
> +	{ .compatible = "u-blox,zed-f9p" },

Looks compatible with previous, right?

>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, ubx_of_match);

Best regards,
Krzysztof