| Message ID | 1509331789-1949-1-git-send-email-hl@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+ devicetree list Hi, On Mon, Oct 30, 2017 at 10:49:49AM +0800, Lin Huang wrote: > some i2c hid devices have reset gpio, need to control > it in the driver. > > Change-Id: I87bca954bffc7eb7b35711406f522cb3d0fc2ded You should be removing these Gerrit ID lines from patches before sending them to these mailing lists. > Signed-off-by: Lin Huang <hl@rock-chips.com> > --- > drivers/hid/i2c-hid/i2c-hid.c | 63 +++++++++++++++++++++++++++-------- > include/linux/platform_data/i2c-hid.h | 4 +++ > 2 files changed, 53 insertions(+), 14 deletions(-) > > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c > index 9145c21..a0e3a8e 100644 > --- a/drivers/hid/i2c-hid/i2c-hid.c > +++ b/drivers/hid/i2c-hid/i2c-hid.c > @@ -38,6 +38,7 @@ > #include <linux/mutex.h> > #include <linux/acpi.h> > #include <linux/of.h> > +#include <linux/gpio/consumer.h> > #include <linux/regulator/consumer.h> > > #include <linux/platform_data/i2c-hid.h> > @@ -793,6 +794,34 @@ struct hid_ll_driver i2c_hid_ll_driver = { > }; > EXPORT_SYMBOL_GPL(i2c_hid_ll_driver); > > +static int i2c_hid_hw_power_on(struct i2c_hid *ihid) > +{ > + int ret; > + > + ret = regulator_enable(ihid->pdata.supply); > + if (ret < 0) > + return ret; > + > + if (ihid->pdata.post_power_delay_ms) > + msleep(ihid->pdata.post_power_delay_ms); > + > + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1); > + usleep_range(ihid->pdata.assert_reset_us, > + ihid->pdata.assert_reset_us); What's the point of using the same value on both ends of the range? Does it make sense to give some padding to this? e.g., from X to X + 10% ? (Note that reasonable values for this are on the order of a few milliseconds.) Also, the above msleep() has a 'if non-zero' condition on it...but that's not actually necessary now that I look again, since msleep() and usleep_range() both handle zero times OK... But it feels a little inconsistent. We should probably change one or the other sometime. > + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0); > + usleep_range(ihid->pdata.deassert_reset_us, > + ihid->pdata.deassert_reset_us); > + > + return ret; > +} > + > +static int i2c_hid_hw_power_off(struct i2c_hid *ihid) > +{ > + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1); > + > + return regulator_disable(ihid->pdata.supply); > +} > + > static int i2c_hid_init_irq(struct i2c_client *client) > { > struct i2c_hid *ihid = i2c_get_clientdata(client); > @@ -934,6 +963,12 @@ static int i2c_hid_of_probe(struct i2c_client *client, > if (!ret) > pdata->post_power_delay_ms = val; > > + ret = of_property_read_u32(dev->of_node, "assert-reset-us", > + &pdata->assert_reset_us); > + > + ret = of_property_read_u32(dev->of_node, "deassert-reset-us", > + &pdata->deassert_reset_us); You need to document these. Brian > + > return 0; > } > > @@ -1002,14 +1037,16 @@ static int i2c_hid_probe(struct i2c_client *client, > goto err; > } > > - ret = regulator_enable(ihid->pdata.supply); > + ihid->pdata.reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(ihid->pdata.reset_gpio)) > + goto err; > + > + ret = i2c_hid_hw_power_on(ihid); > if (ret < 0) { > - dev_err(&client->dev, "Failed to enable regulator: %d\n", > - ret); > + dev_err(&client->dev, "Failed to power on: %d\n", ret); > goto err; > } > - if (ihid->pdata.post_power_delay_ms) > - msleep(ihid->pdata.post_power_delay_ms); > > i2c_set_clientdata(client, ihid); > > @@ -1086,7 +1123,7 @@ static int i2c_hid_probe(struct i2c_client *client, > pm_runtime_disable(&client->dev); > > err_regulator: > - regulator_disable(ihid->pdata.supply); > + i2c_hid_hw_power_off(ihid); > > err: > i2c_hid_free_buffers(ihid); > @@ -1112,8 +1149,7 @@ static int i2c_hid_remove(struct i2c_client *client) > if (ihid->bufsize) > i2c_hid_free_buffers(ihid); > > - regulator_disable(ihid->pdata.supply); > - > + i2c_hid_hw_power_off(ihid); > kfree(ihid); > > return 0; > @@ -1165,9 +1201,10 @@ static int i2c_hid_suspend(struct device *dev) > hid_warn(hid, "Failed to enable irq wake: %d\n", > wake_status); > } else { > - ret = regulator_disable(ihid->pdata.supply); > + ret = i2c_hid_hw_power_off(ihid); > if (ret < 0) > - hid_warn(hid, "Failed to disable supply: %d\n", ret); > + hid_warn(hid, "Failed to disable hw power: %d\n", > + ret); > } > > return 0; > @@ -1182,11 +1219,9 @@ static int i2c_hid_resume(struct device *dev) > int wake_status; > > if (!device_may_wakeup(&client->dev)) { > - ret = regulator_enable(ihid->pdata.supply); > + ret = i2c_hid_hw_power_on(ihid); > if (ret < 0) > - hid_warn(hid, "Failed to enable supply: %d\n", ret); > - if (ihid->pdata.post_power_delay_ms) > - msleep(ihid->pdata.post_power_delay_ms); > + hid_warn(hid, "Failed to enable hw power: %d\n", ret); > } else if (ihid->irq_wake_enabled) { > wake_status = disable_irq_wake(client->irq); > if (!wake_status) > diff --git a/include/linux/platform_data/i2c-hid.h b/include/linux/platform_data/i2c-hid.h > index 1fb0882..3afc781 100644 > --- a/include/linux/platform_data/i2c-hid.h > +++ b/include/linux/platform_data/i2c-hid.h > @@ -14,6 +14,7 @@ > > #include <linux/types.h> > > +struct gpio_desc; > struct regulator; > > /** > @@ -37,6 +38,9 @@ struct i2c_hid_platform_data { > u16 hid_descriptor_address; > struct regulator *supply; > int post_power_delay_ms; > + struct gpio_desc *reset_gpio; > + int assert_reset_us; > + int deassert_reset_us; > }; > > #endif /* __LINUX_I2C_HID_H */ > -- > 2.7.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 30, 2017 at 11:57 AM, Brian Norris <briannorris@chromium.org> wrote: > + devicetree list > > Hi, > > On Mon, Oct 30, 2017 at 10:49:49AM +0800, Lin Huang wrote: >> some i2c hid devices have reset gpio, need to control >> it in the driver. >> >> Change-Id: I87bca954bffc7eb7b35711406f522cb3d0fc2ded > > You should be removing these Gerrit ID lines from patches before sending > them to these mailing lists. > >> Signed-off-by: Lin Huang <hl@rock-chips.com> >> --- >> drivers/hid/i2c-hid/i2c-hid.c | 63 +++++++++++++++++++++++++++-------- >> include/linux/platform_data/i2c-hid.h | 4 +++ >> 2 files changed, 53 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c >> index 9145c21..a0e3a8e 100644 >> --- a/drivers/hid/i2c-hid/i2c-hid.c >> +++ b/drivers/hid/i2c-hid/i2c-hid.c >> @@ -38,6 +38,7 @@ >> #include <linux/mutex.h> >> #include <linux/acpi.h> >> #include <linux/of.h> >> +#include <linux/gpio/consumer.h> >> #include <linux/regulator/consumer.h> >> >> #include <linux/platform_data/i2c-hid.h> >> @@ -793,6 +794,34 @@ struct hid_ll_driver i2c_hid_ll_driver = { >> }; >> EXPORT_SYMBOL_GPL(i2c_hid_ll_driver); >> >> +static int i2c_hid_hw_power_on(struct i2c_hid *ihid) >> +{ >> + int ret; >> + >> + ret = regulator_enable(ihid->pdata.supply); >> + if (ret < 0) >> + return ret; >> + >> + if (ihid->pdata.post_power_delay_ms) >> + msleep(ihid->pdata.post_power_delay_ms); >> + >> + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1); >> + usleep_range(ihid->pdata.assert_reset_us, >> + ihid->pdata.assert_reset_us); You already ensure that reset is asserted before power on, so why does the post_power_delay_ms not work for you? IIRC, there's already a property for it. > > What's the point of using the same value on both ends of the range? Does > it make sense to give some padding to this? e.g., from X to X + 10% ? > (Note that reasonable values for this are on the order of a few > milliseconds.) > > Also, the above msleep() has a 'if non-zero' condition on it...but > that's not actually necessary now that I look again, since msleep() and > usleep_range() both handle zero times OK... But it feels a little > inconsistent. We should probably change one or the other sometime. > >> + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0); >> + usleep_range(ihid->pdata.deassert_reset_us, >> + ihid->pdata.deassert_reset_us); >> + >> + return ret; >> +} >> + >> +static int i2c_hid_hw_power_off(struct i2c_hid *ihid) >> +{ >> + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1); >> + >> + return regulator_disable(ihid->pdata.supply); >> +} >> + >> static int i2c_hid_init_irq(struct i2c_client *client) >> { >> struct i2c_hid *ihid = i2c_get_clientdata(client); >> @@ -934,6 +963,12 @@ static int i2c_hid_of_probe(struct i2c_client *client, >> if (!ret) >> pdata->post_power_delay_ms = val; >> >> + ret = of_property_read_u32(dev->of_node, "assert-reset-us", >> + &pdata->assert_reset_us); >> + >> + ret = of_property_read_u32(dev->of_node, "deassert-reset-us", >> + &pdata->deassert_reset_us); > > You need to document these. I'm not inclined to accept these. Sure, it's only 2 properties. No big deal on their own. However, what about the next device that needs some other delay. Or has another control signal with timing constraints. Or has a clock with specific enable timing. The list goes on... If we wanted to handle all cases of power sequencing in DT, then we'd write something flexible to handle any case. But we don't because DT is not a scripting language. This is why we have compatibles for each specific chip. How many devices need this? Do you have cases needing different times? Why can't this time just be fixed in the driver? Is this really timing critical or have really long delays? Rob -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 30, 2017 at 03:15:19PM -0500, Rob Herring wrote: > On Mon, Oct 30, 2017 at 11:57 AM, Brian Norris <briannorris@chromium.org> wrote: > > On Mon, Oct 30, 2017 at 10:49:49AM +0800, Lin Huang wrote: > >> --- a/drivers/hid/i2c-hid/i2c-hid.c > >> +++ b/drivers/hid/i2c-hid/i2c-hid.c > >> @@ -793,6 +794,34 @@ struct hid_ll_driver i2c_hid_ll_driver = { > >> }; > >> EXPORT_SYMBOL_GPL(i2c_hid_ll_driver); > >> > >> +static int i2c_hid_hw_power_on(struct i2c_hid *ihid) > >> +{ > >> + int ret; > >> + > >> + ret = regulator_enable(ihid->pdata.supply); > >> + if (ret < 0) > >> + return ret; > >> + > >> + if (ihid->pdata.post_power_delay_ms) > >> + msleep(ihid->pdata.post_power_delay_ms); > >> + > >> + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1); > >> + usleep_range(ihid->pdata.assert_reset_us, > >> + ihid->pdata.assert_reset_us); > > You already ensure that reset is asserted before power on, so why does > the post_power_delay_ms not work for you? IIRC, there's already a > property for it. That would be more like the equivalent of the "deassert" delay used a few lines below. The "assert" delay would be a different property, unless we want to guess at a reasonable value, or else key off something like 'compatible'. But otherwise, sure, we could reuse it. We'd then want to modify the binding documentation to be less specific to the "vdd-supply" though. > > What's the point of using the same value on both ends of the range? Does > > it make sense to give some padding to this? e.g., from X to X + 10% ? > > (Note that reasonable values for this are on the order of a few > > milliseconds.) > > > > Also, the above msleep() has a 'if non-zero' condition on it...but > > that's not actually necessary now that I look again, since msleep() and > > usleep_range() both handle zero times OK... But it feels a little > > inconsistent. We should probably change one or the other sometime. > > > >> + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0); > >> + usleep_range(ihid->pdata.deassert_reset_us, > >> + ihid->pdata.deassert_reset_us); > >> + > >> + return ret; > >> +} > >> + > >> +static int i2c_hid_hw_power_off(struct i2c_hid *ihid) > >> +{ > >> + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1); > >> + > >> + return regulator_disable(ihid->pdata.supply); > >> +} > >> + > >> static int i2c_hid_init_irq(struct i2c_client *client) > >> { > >> struct i2c_hid *ihid = i2c_get_clientdata(client); > >> @@ -934,6 +963,12 @@ static int i2c_hid_of_probe(struct i2c_client *client, > >> if (!ret) > >> pdata->post_power_delay_ms = val; > >> > >> + ret = of_property_read_u32(dev->of_node, "assert-reset-us", > >> + &pdata->assert_reset_us); > >> + > >> + ret = of_property_read_u32(dev->of_node, "deassert-reset-us", > >> + &pdata->deassert_reset_us); > > > > You need to document these. > > I'm not inclined to accept these. Sure, it's only 2 properties. No big > deal on their own. However, what about the next device that needs some > other delay. Or has another control signal with timing constraints. Or > has a clock with specific enable timing. The list goes on... If we > wanted to handle all cases of power sequencing in DT, then we'd write > something flexible to handle any case. But we don't because DT is not > a scripting language. This is why we have compatibles for each > specific chip. I feel like we've had this discussion. I think the essence of the disagreement was that the maintainer of this driver did not want to support a long list of 'compatible' properties for stuff that's otherwise all hidden by the ACPI model -- the DT binding doc even refers to a Windows/ACPI spec. (A secondary concern was that the I2C subsystem still doesn't support multiple compatible strings with modules correctly, IIRC.) I don't really know how to bridge that gap. DT-based platforms are inherently not ACPI, and all the device "power on" logic just has to be done in the kernel. Unfortunately, some ways of slicing this require more burden on driver maintainers... > How many devices need this? Do you have cases needing different times? > Why can't this time just be fixed in the driver? Is this really timing > critical or have really long delays? I don't know how many devices need something like this, but this is a pretty common model. I don't see why the next device with a reset pin wouldn't be able to reuse the same timing knobs. And FWIW, this is still the same "wacom,w9013", just different board design. We could theoretically key off the compatible, and list the datasheet numbers in the driver; but see my comment above. Not sure what you mean about "timing critical"; in this case the values are 1ms and 30ms respectively, and they are used at power-on and (potentially) system resume. I'm not sure there would be much harm if these timings were significantly longer than the datasheet recommends, but at some point, excessive delay would cause boot or suspend/resume inefficiencies. (Asynchronous probe and suspend/resume can often mitigate these inefficiencies.) Brian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Oct 30 2017 or thereabouts, Brian Norris wrote: > On Mon, Oct 30, 2017 at 03:15:19PM -0500, Rob Herring wrote: > > On Mon, Oct 30, 2017 at 11:57 AM, Brian Norris <briannorris@chromium.org> wrote: > > > On Mon, Oct 30, 2017 at 10:49:49AM +0800, Lin Huang wrote: > > >> --- a/drivers/hid/i2c-hid/i2c-hid.c > > >> +++ b/drivers/hid/i2c-hid/i2c-hid.c > > > >> @@ -793,6 +794,34 @@ struct hid_ll_driver i2c_hid_ll_driver = { > > >> }; > > >> EXPORT_SYMBOL_GPL(i2c_hid_ll_driver); > > >> > > >> +static int i2c_hid_hw_power_on(struct i2c_hid *ihid) > > >> +{ > > >> + int ret; > > >> + > > >> + ret = regulator_enable(ihid->pdata.supply); > > >> + if (ret < 0) > > >> + return ret; > > >> + > > >> + if (ihid->pdata.post_power_delay_ms) > > >> + msleep(ihid->pdata.post_power_delay_ms); > > >> + > > >> + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1); > > >> + usleep_range(ihid->pdata.assert_reset_us, > > >> + ihid->pdata.assert_reset_us); > > > > You already ensure that reset is asserted before power on, so why does > > the post_power_delay_ms not work for you? IIRC, there's already a > > property for it. > > That would be more like the equivalent of the "deassert" delay used a > few lines below. The "assert" delay would be a different property, > unless we want to guess at a reasonable value, or else key off something > like 'compatible'. > > But otherwise, sure, we could reuse it. We'd then want to modify the > binding documentation to be less specific to the "vdd-supply" though. > > > > What's the point of using the same value on both ends of the range? Does > > > it make sense to give some padding to this? e.g., from X to X + 10% ? > > > (Note that reasonable values for this are on the order of a few > > > milliseconds.) > > > > > > Also, the above msleep() has a 'if non-zero' condition on it...but > > > that's not actually necessary now that I look again, since msleep() and > > > usleep_range() both handle zero times OK... But it feels a little > > > inconsistent. We should probably change one or the other sometime. > > > > > >> + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0); > > >> + usleep_range(ihid->pdata.deassert_reset_us, > > >> + ihid->pdata.deassert_reset_us); > > >> + > > >> + return ret; > > >> +} > > >> + > > >> +static int i2c_hid_hw_power_off(struct i2c_hid *ihid) > > >> +{ > > >> + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1); > > >> + > > >> + return regulator_disable(ihid->pdata.supply); > > >> +} > > >> + > > >> static int i2c_hid_init_irq(struct i2c_client *client) > > >> { > > >> struct i2c_hid *ihid = i2c_get_clientdata(client); > > >> @@ -934,6 +963,12 @@ static int i2c_hid_of_probe(struct i2c_client *client, > > >> if (!ret) > > >> pdata->post_power_delay_ms = val; > > >> > > >> + ret = of_property_read_u32(dev->of_node, "assert-reset-us", > > >> + &pdata->assert_reset_us); > > >> + > > >> + ret = of_property_read_u32(dev->of_node, "deassert-reset-us", > > >> + &pdata->deassert_reset_us); > > > > > > You need to document these. > > > > I'm not inclined to accept these. Sure, it's only 2 properties. No big > > deal on their own. However, what about the next device that needs some > > other delay. Or has another control signal with timing constraints. Or > > has a clock with specific enable timing. The list goes on... If we > > wanted to handle all cases of power sequencing in DT, then we'd write > > something flexible to handle any case. But we don't because DT is not > > a scripting language. This is why we have compatibles for each > > specific chip. > > I feel like we've had this discussion. I think the essence of the > disagreement was that the maintainer of this driver did not want to > support a long list of 'compatible' properties for stuff that's > otherwise all hidden by the ACPI model -- the DT binding doc even refers > to a Windows/ACPI spec. (A secondary concern was that the I2C subsystem > still doesn't support multiple compatible strings with modules > correctly, IIRC.) > > I don't really know how to bridge that gap. DT-based platforms are > inherently not ACPI, and all the device "power on" logic just has to be > done in the kernel. Unfortunately, some ways of slicing this require > more burden on driver maintainers... I asked one other question to Rob in the new thread, but now that I re-read this, I wonder: As you said, HID over I2C is a protocol designed only for ACPI (because Windows). It is generic enough but relies on ACPI to do its usual crap like setting the reset lines and timing issues. The initial OF implementation was done "because we can and it works". Given that the OF firmware is a different world, how about we strip out the OF part of i2c-hid, and make its own module (or compilation unit). This way, the i2c-hid OF version would communicate with i2c-hid (ACPI) through the platform data, and we could handle how many compatible devices you want, given that it'll be given it's own maintainer and separate file. Cheers, Benjamin > > > How many devices need this? Do you have cases needing different times? > > Why can't this time just be fixed in the driver? Is this really timing > > critical or have really long delays? > > I don't know how many devices need something like this, but this is a > pretty common model. I don't see why the next device with a reset pin > wouldn't be able to reuse the same timing knobs. > > And FWIW, this is still the same "wacom,w9013", just different board > design. We could theoretically key off the compatible, and list the > datasheet numbers in the driver; but see my comment above. > > Not sure what you mean about "timing critical"; in this case the values > are 1ms and 30ms respectively, and they are used at power-on and > (potentially) system resume. I'm not sure there would be much harm if > these timings were significantly longer than the datasheet recommends, > but at some point, excessive delay would cause boot or suspend/resume > inefficiencies. (Asynchronous probe and suspend/resume can often > mitigate these inefficiencies.) > > Brian -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c index 9145c21..a0e3a8e 100644 --- a/drivers/hid/i2c-hid/i2c-hid.c +++ b/drivers/hid/i2c-hid/i2c-hid.c @@ -38,6 +38,7 @@ #include <linux/mutex.h> #include <linux/acpi.h> #include <linux/of.h> +#include <linux/gpio/consumer.h> #include <linux/regulator/consumer.h> #include <linux/platform_data/i2c-hid.h> @@ -793,6 +794,34 @@ struct hid_ll_driver i2c_hid_ll_driver = { }; EXPORT_SYMBOL_GPL(i2c_hid_ll_driver); +static int i2c_hid_hw_power_on(struct i2c_hid *ihid) +{ + int ret; + + ret = regulator_enable(ihid->pdata.supply); + if (ret < 0) + return ret; + + if (ihid->pdata.post_power_delay_ms) + msleep(ihid->pdata.post_power_delay_ms); + + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1); + usleep_range(ihid->pdata.assert_reset_us, + ihid->pdata.assert_reset_us); + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 0); + usleep_range(ihid->pdata.deassert_reset_us, + ihid->pdata.deassert_reset_us); + + return ret; +} + +static int i2c_hid_hw_power_off(struct i2c_hid *ihid) +{ + gpiod_set_value_cansleep(ihid->pdata.reset_gpio, 1); + + return regulator_disable(ihid->pdata.supply); +} + static int i2c_hid_init_irq(struct i2c_client *client) { struct i2c_hid *ihid = i2c_get_clientdata(client); @@ -934,6 +963,12 @@ static int i2c_hid_of_probe(struct i2c_client *client, if (!ret) pdata->post_power_delay_ms = val; + ret = of_property_read_u32(dev->of_node, "assert-reset-us", + &pdata->assert_reset_us); + + ret = of_property_read_u32(dev->of_node, "deassert-reset-us", + &pdata->deassert_reset_us); + return 0; } @@ -1002,14 +1037,16 @@ static int i2c_hid_probe(struct i2c_client *client, goto err; } - ret = regulator_enable(ihid->pdata.supply); + ihid->pdata.reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", + GPIOD_OUT_HIGH); + if (IS_ERR(ihid->pdata.reset_gpio)) + goto err; + + ret = i2c_hid_hw_power_on(ihid); if (ret < 0) { - dev_err(&client->dev, "Failed to enable regulator: %d\n", - ret); + dev_err(&client->dev, "Failed to power on: %d\n", ret); goto err; } - if (ihid->pdata.post_power_delay_ms) - msleep(ihid->pdata.post_power_delay_ms); i2c_set_clientdata(client, ihid); @@ -1086,7 +1123,7 @@ static int i2c_hid_probe(struct i2c_client *client, pm_runtime_disable(&client->dev); err_regulator: - regulator_disable(ihid->pdata.supply); + i2c_hid_hw_power_off(ihid); err: i2c_hid_free_buffers(ihid); @@ -1112,8 +1149,7 @@ static int i2c_hid_remove(struct i2c_client *client) if (ihid->bufsize) i2c_hid_free_buffers(ihid); - regulator_disable(ihid->pdata.supply); - + i2c_hid_hw_power_off(ihid); kfree(ihid); return 0; @@ -1165,9 +1201,10 @@ static int i2c_hid_suspend(struct device *dev) hid_warn(hid, "Failed to enable irq wake: %d\n", wake_status); } else { - ret = regulator_disable(ihid->pdata.supply); + ret = i2c_hid_hw_power_off(ihid); if (ret < 0) - hid_warn(hid, "Failed to disable supply: %d\n", ret); + hid_warn(hid, "Failed to disable hw power: %d\n", + ret); } return 0; @@ -1182,11 +1219,9 @@ static int i2c_hid_resume(struct device *dev) int wake_status; if (!device_may_wakeup(&client->dev)) { - ret = regulator_enable(ihid->pdata.supply); + ret = i2c_hid_hw_power_on(ihid); if (ret < 0) - hid_warn(hid, "Failed to enable supply: %d\n", ret); - if (ihid->pdata.post_power_delay_ms) - msleep(ihid->pdata.post_power_delay_ms); + hid_warn(hid, "Failed to enable hw power: %d\n", ret); } else if (ihid->irq_wake_enabled) { wake_status = disable_irq_wake(client->irq); if (!wake_status) diff --git a/include/linux/platform_data/i2c-hid.h b/include/linux/platform_data/i2c-hid.h index 1fb0882..3afc781 100644 --- a/include/linux/platform_data/i2c-hid.h +++ b/include/linux/platform_data/i2c-hid.h @@ -14,6 +14,7 @@ #include <linux/types.h> +struct gpio_desc; struct regulator; /** @@ -37,6 +38,9 @@ struct i2c_hid_platform_data { u16 hid_descriptor_address; struct regulator *supply; int post_power_delay_ms; + struct gpio_desc *reset_gpio; + int assert_reset_us; + int deassert_reset_us; }; #endif /* __LINUX_I2C_HID_H */
some i2c hid devices have reset gpio, need to control it in the driver. Change-Id: I87bca954bffc7eb7b35711406f522cb3d0fc2ded Signed-off-by: Lin Huang <hl@rock-chips.com> --- drivers/hid/i2c-hid/i2c-hid.c | 63 +++++++++++++++++++++++++++-------- include/linux/platform_data/i2c-hid.h | 4 +++ 2 files changed, 53 insertions(+), 14 deletions(-)