On 01/05/2023 19:01, alison@xxxxxxxxxxxxx wrote:
> From: Alison Chaiken <achaiken@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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
