linux-kernel - Re: [PATCH v1] tty: serial: max310x: Add optional reset gpio

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <d9ac1295-e780-409d-b7de-b4c2db586e58@cesnet.cz>
Date:   Tue, 02 Jul 2019 16:44:20 +0200
From:   Jan Kundrát <jan.kundrat@...net.cz>
To:     Mylène Josserand <mylene.josserand@...tlin.com>
Cc:     <gregkh@...uxfoundation.org>, <robh+dt@...nel.org>,
        <mark.rutland@....com>, <linux-serial@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v1] tty: serial: max310x: Add optional reset gpio

On pátek 14. června 2019 16:11:12 CEST, Mylène Josserand wrote:
> --- a/Documentation/devicetree/bindings/serial/maxim,max310x.txt
> +++ b/Documentation/devicetree/bindings/serial/maxim,max310x.txt
> @@ -15,6 +15,7 @@ Required properties:
>    "osc" if an external clock source is used.
>  
>  Optional properties:
> +- reset-gpios: Gpio to use for reset.

"GPIO", not "Gpio", for consistency.

>  	if (spi->dev.of_node) {
> +		struct gpio_desc *reset_gpio;
>  		const struct of_device_id *of_id =
>  			of_match_device(max310x_dt_ids, &spi->dev);
>  		if (!of_id)
>  			return -ENODEV;
>  
>  		devtype = (struct max310x_devtype *)of_id->data;
> +		reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset",
> +						     GPIOD_OUT_HIGH);
> +		if (IS_ERR(reset_gpio))
> +			return PTR_ERR(reset_gpio);
> +		gpiod_set_value_cansleep(reset_gpio, 0);
>  	} else {

The RST signal is active-low on the chip, but the code initializes the 
output to GPIOD_OUT_HIGH. Are you perhaps relying on a DT binding setting 
an ACTIVE_LOW flag on the reset GPIO lane? This should be documented.

Assuming that this polarity inversion works, the code first asserts the 
reset, then it performs no explicit waiting, and then it clears the RST 
signal. I checked MAX14830's datasheet, and there's no minimal reset 
duration, so perhaps this is safe, but it looks a bit odd to me.

With kind regards,
Jan