diff mbox series

[v2,8/9] Input: iqs5xx - make reset GPIO optional

Message ID 20210313191236.4366-9-jeff@labundy.com (mailing list archive)
State New, archived
Headers show
Series Input: iqs5xx - more enhancements and optimizations | expand

Commit Message

Jeff LaBundy March 13, 2021, 7:12 p.m. UTC 
The device's hardware reset pin is only required if the platform
must be able to update the device's firmware.

As such, demote the reset GPIO to optional in support of devices
that ship with pre-programmed firmware and don't route the reset
pin back to the SoC.

In that case, the 'fw_file' attribute is hidden because there is
no way to open the bootloader. The logic is extended to the case
in which the device does not advertise bootloader support in the
first place.

Last but not least, remove the hardware reset performed at probe
because there is no reason to reset the device manually. A power
on reset function already ensures a clean reset at start-up.

Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v2:
 - None

 drivers/input/touchscreen/iqs5xx.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

--
2.17.1

Comments

Dmitry Torokhov March 22, 2021, 4:06 a.m. UTC | #1 
Hi Jeff,

On Sat, Mar 13, 2021 at 01:12:35PM -0600, Jeff LaBundy wrote:
> @@ -989,7 +993,11 @@ static int iqs5xx_probe(struct i2c_client *client,
>  		return error;
>  	}
> 
> -	error = devm_device_add_group(&client->dev, &iqs5xx_attr_group);
> +	if (iqs5xx->reset_gpio &&
> +	    iqs5xx->dev_id_info.bl_status != IQS5XX_BL_STATUS_NONE)
> +		attr_group = &iqs5xx_attr_group_bl;

I would prefer if we defined is_visible for the attribute instead of
conditional registration.

> +
> +	error = devm_device_add_group(&client->dev, attr_group);
>  	if (error) {
>  		dev_err(&client->dev, "Failed to add attributes: %d\n", error);
>  		return error;
> --
> 2.17.1
> 

Thanks.
Jeff LaBundy March 22, 2021, 3:42 p.m. UTC | #2 
Hi Dmitry,

On Sun, Mar 21, 2021 at 09:06:55PM -0700, Dmitry Torokhov wrote:
> Hi Jeff,
> 
> On Sat, Mar 13, 2021 at 01:12:35PM -0600, Jeff LaBundy wrote:
> > @@ -989,7 +993,11 @@ static int iqs5xx_probe(struct i2c_client *client,
> >  		return error;
> >  	}
> > 
> > -	error = devm_device_add_group(&client->dev, &iqs5xx_attr_group);
> > +	if (iqs5xx->reset_gpio &&
> > +	    iqs5xx->dev_id_info.bl_status != IQS5XX_BL_STATUS_NONE)
> > +		attr_group = &iqs5xx_attr_group_bl;
> 
> I would prefer if we defined is_visible for the attribute instead of
> conditional registration.

Thank you for pointing me to that option; it's much cleaner and I'll
send a v3 with an is_visible callback.

> 
> > +
> > +	error = devm_device_add_group(&client->dev, attr_group);
> >  	if (error) {
> >  		dev_err(&client->dev, "Failed to add attributes: %d\n", error);
> >  		return error;
> > --
> > 2.17.1
> > 
> 
> Thanks.
> 
> -- 
> Dmitry

Kind regards,
Jeff LaBundy
diff mbox series

Patch

diff --git a/drivers/input/touchscreen/iqs5xx.c b/drivers/input/touchscreen/iqs5xx.c
index 180d2618d8c6..6e53b455bfb9 100644
--- a/drivers/input/touchscreen/iqs5xx.c
+++ b/drivers/input/touchscreen/iqs5xx.c
@@ -807,9 +807,6 @@  static int iqs5xx_fw_file_write(struct i2c_client *client, const char *fw_file)
 	int error, error_init = 0;
 	u8 *pmap;

-	if (iqs5xx->dev_id_info.bl_status == IQS5XX_BL_STATUS_NONE)
-		return -EPERM;
-
 	pmap = kzalloc(IQS5XX_PMAP_LEN, GFP_KERNEL);
 	if (!pmap)
 		return -ENOMEM;
@@ -929,12 +926,21 @@  static ssize_t fw_info_show(struct device *dev,
 static DEVICE_ATTR_WO(fw_file);
 static DEVICE_ATTR_RO(fw_info);

-static struct attribute *iqs5xx_attrs[] = {
+static struct attribute *iqs5xx_attrs_bl[] = {
 	&dev_attr_fw_file.attr,
 	&dev_attr_fw_info.attr,
 	NULL,
 };

+static struct attribute *iqs5xx_attrs[] = {
+	&dev_attr_fw_info.attr,
+	NULL,
+};
+
+static const struct attribute_group iqs5xx_attr_group_bl = {
+	.attrs = iqs5xx_attrs_bl,
+};
+
 static const struct attribute_group iqs5xx_attr_group = {
 	.attrs = iqs5xx_attrs,
 };
@@ -954,6 +960,7 @@  static SIMPLE_DEV_PM_OPS(iqs5xx_pm, iqs5xx_suspend, iqs5xx_resume);
 static int iqs5xx_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
+	const struct attribute_group *attr_group = &iqs5xx_attr_group;
 	struct iqs5xx_private *iqs5xx;
 	int error;

@@ -964,8 +971,8 @@  static int iqs5xx_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, iqs5xx);
 	iqs5xx->client = client;

-	iqs5xx->reset_gpio = devm_gpiod_get(&client->dev,
-					    "reset", GPIOD_OUT_LOW);
+	iqs5xx->reset_gpio = devm_gpiod_get_optional(&client->dev,
+						     "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(iqs5xx->reset_gpio)) {
 		error = PTR_ERR(iqs5xx->reset_gpio);
 		dev_err(&client->dev, "Failed to request GPIO: %d\n", error);
@@ -974,9 +981,6 @@  static int iqs5xx_probe(struct i2c_client *client,

 	mutex_init(&iqs5xx->lock);

-	iqs5xx_reset(client);
-	usleep_range(10000, 10100);
-
 	error = iqs5xx_dev_init(client);
 	if (error)
 		return error;
@@ -989,7 +993,11 @@  static int iqs5xx_probe(struct i2c_client *client,
 		return error;
 	}

-	error = devm_device_add_group(&client->dev, &iqs5xx_attr_group);
+	if (iqs5xx->reset_gpio &&
+	    iqs5xx->dev_id_info.bl_status != IQS5XX_BL_STATUS_NONE)
+		attr_group = &iqs5xx_attr_group_bl;
+
+	error = devm_device_add_group(&client->dev, attr_group);
 	if (error) {
 		dev_err(&client->dev, "Failed to add attributes: %d\n", error);
 		return error;