Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Wed, 1 Nov 2023 17:31:05 +0300
From:   Dmitry Rokosov <ddrokosov@salutedevices.com>
To:     <andy.shevchenko@gmail.com>
CC:     <kernel@sberdevices.ru>, <rockosov@gmail.com>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-leds@vger.kernel.org>, <lee@kernel.org>, <pavel@ucw.cz>,
        <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <conor+dt@kernel.org>, <andy.shevchenko@gmail.com>
Subject: Re: [PATCH v3 02/11] leds: aw200xx: support HWEN hardware control
Message-ID: <20231101143105.lh2ruhv27m5v5evb@CAB-WSD-L081021>
References: <20231101142445.8753-1-ddrokosov@salutedevices.com>
 <20231101142445.8753-3-ddrokosov@salutedevices.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20231101142445.8753-3-ddrokosov@salutedevices.com>
User-Agent: NeoMutt/20220415
Precedence: bulk

Hello Andy,

I would like to share my thoughts on the gpio-regulator. I have tried
using it and found it to be applicable in this context without any
issues. However, when it comes to the gpio regulator, it is necessary to
directly set the voltage value on the pin from the driver. While the
datasheet provides a description for the HIGH voltage value, it does not
precisely describe the LOW voltage value. To gain more insights, I have
examined other drivers, such as the mmc with vqmmc-supply. In the MMC
driver, the gpio-regulator is used to directly set the voltage values.
However, in our driver, this functionality is not required. Instead,
other led drivers utilize the gpiod subsystem to handle enable-gpios
pins.

On Wed, Nov 01, 2023 at 05:24:36PM +0300, Dmitry Rokosov wrote:
> HWEN is hardware control, which is used for enable/disable aw200xx chip.
> It's high active, internally pulled down to GND.
> 
> After HWEN pin set high the chip begins to load the OTP information,
> which takes 200us to complete. About 200us wait time is needed for
> internal oscillator startup and display SRAM initialization. After
> display SRAM initialization, the registers in page 1 to page 5 can be
> configured via i2c interface.
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  drivers/leds/leds-aw200xx.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/leds/leds-aw200xx.c b/drivers/leds/leds-aw200xx.c
> index 842a22087b16..7762b3a132ac 100644
> --- a/drivers/leds/leds-aw200xx.c
> +++ b/drivers/leds/leds-aw200xx.c
> @@ -10,6 +10,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
>  #include <linux/container_of.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/leds.h>
>  #include <linux/mod_devicetable.h>
> @@ -116,6 +117,7 @@ struct aw200xx {
>  	struct mutex mutex;
>  	u32 num_leds;
>  	u32 display_rows;
> +	struct gpio_desc *hwen;
>  	struct aw200xx_led leds[] __counted_by(num_leds);
>  };
>  
> @@ -358,6 +360,25 @@ static int aw200xx_chip_check(const struct aw200xx *const chip)
>  	return 0;
>  }
>  
> +static void aw200xx_enable(const struct aw200xx *const chip)
> +{
> +	gpiod_set_value_cansleep(chip->hwen, 1);
> +
> +	/*
> +	 * After HWEN pin set high the chip begins to load the OTP information,
> +	 * which takes 200us to complete. About 200us wait time is needed for
> +	 * internal oscillator startup and display SRAM initialization. After
> +	 * display SRAM initialization, the registers in page1 to page5 can be
> +	 * configured via i2c interface.
> +	 */
> +	fsleep(400);
> +}
> +
> +static void aw200xx_disable(const struct aw200xx *const chip)
> +{
> +	return gpiod_set_value_cansleep(chip->hwen, 0);
> +}
> +
>  static int aw200xx_probe_fw(struct device *dev, struct aw200xx *chip)
>  {
>  	struct fwnode_handle *child;
> @@ -517,6 +538,14 @@ static int aw200xx_probe(struct i2c_client *client)
>  	if (IS_ERR(chip->regmap))
>  		return PTR_ERR(chip->regmap);
>  
> +	chip->hwen = devm_gpiod_get_optional(&client->dev, "enable",
> +					     GPIOD_OUT_HIGH);
> +	if (IS_ERR(chip->hwen))
> +		return dev_err_probe(&client->dev, PTR_ERR(chip->hwen),
> +				     "Cannot get enable gpio");
> +
> +	aw200xx_enable(chip);
> +
>  	ret = aw200xx_chip_check(chip);
>  	if (ret)
>  		return ret;
> @@ -537,6 +566,9 @@ static int aw200xx_probe(struct i2c_client *client)
>  	ret = aw200xx_chip_init(chip);
>  
>  out_unlock:
> +	if (ret)
> +		aw200xx_disable(chip);
> +
>  	mutex_unlock(&chip->mutex);
>  	return ret;
>  }
> @@ -546,6 +578,7 @@ static void aw200xx_remove(struct i2c_client *client)
>  	struct aw200xx *chip = i2c_get_clientdata(client);
>  
>  	aw200xx_chip_reset(chip);
> +	aw200xx_disable(chip);
>  	mutex_destroy(&chip->mutex);
>  }
>  
> -- 
> 2.36.0
> 

-- 
Thank you,
Dmitry