Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <da606e4b-9fe9-ca47-2c9b-1b32ae2ce498@linaro.org>
Date:   Mon, 1 May 2023 19:40:35 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.1
Subject: Re: [PATCH 1/2] gnss: ubx: customize serial device open to set U-Blox
 Zed-F9P baud
Content-Language: en-US
To:     alison@she-devel.com, johan@kernel.org
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        achaiken@aurora.tech
References: <20230501170124.1218603-1-alison@she-devel.com>
 <20230501170124.1218603-2-alison@she-devel.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230501170124.1218603-2-alison@she-devel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 01/05/2023 19:01, alison@she-devel.com wrote:
> From: Alison Chaiken <achaiken@aurora.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@aurora.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