Hi Laurent,
2013/4/7 Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>:
> Hi Bastian,
>
> Thanks for the patch.
Thanks for the review.
> On Saturday 06 April 2013 15:18:45 Bastian Hecht wrote:
>> We add the possiblity to hand over a GPIO number for the reset pin.
>> This way we can remove existing board code that takes care of it and
>> group this information properly in the platform data or in the device
>> tree confguration.
>>
>> The implementation is analogous to the cy8ctmg110 driver, thanks.
>>
>> Signed-off-by: Bastian Hecht <hechtb+renesas@xxxxxxxxx>
>> ---
>> .../bindings/input/touchscreen/sitronix-st1232.txt | 24 ++++++++++
>> drivers/input/touchscreen/st1232.c | 47 +++++++++++++++--
>> include/linux/input/st1232_pdata.h | 13 ++++++
>> 3 files changed, 81 insertions(+), 3 deletions(-)
>> create mode 100644
>> Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
>> create mode 100644 include/linux/input/st1232_pdata.h
>>
>> diff --git
>> a/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
>> b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
>> new file mode 100644
>> index 0000000..1936969
>> --- /dev/null
>> +++
>> b/Documentation/devicetree/bindings/input/touchscreen/sitronix-st1232.txt
>> @@ -0,0 +1,24 @@
>> +* Sitronix st1232 touchscreen controller
>> +
>> +Required properties:
>> +- compatible: must be "sitronix,st1232"
>> +- reg: I2C address of the chip
>> +- interrupts: interrupt to which the chip is connected
>> +
>> +Optional properties:
>> +- sitronix,reset-pin: GPIO number of the reset pin
>
> GPIO bindings use phandles, please see
> Documentation/devicetree/bindings/gpio/gpio.txt.
Ok done. I haven't tested that part yet as the pfc is not set up via
the DT. I saw in topic/pinmux-dt that you added OF support for the
pfc, but I couldn't apply the patch without conflicts. Do you think
this part is going to be merged to next any time soon?
>> +Example:
>> +
>> + i2c@00000000 {
>> + /* ... */
>> +
>> + touchscreen@55 {
>> + compatible = "sitronix,st1232";
>> + reg = <0x55>;
>> + interrupts = <2 0>;
>> + sitronix,reset-pin = <166>;
>> + };
>> +
>> + /* ... */
>> + };
>> diff --git a/drivers/input/touchscreen/st1232.c
>> b/drivers/input/touchscreen/st1232.c index d9d05e2..6aa5038 100644
>> --- a/drivers/input/touchscreen/st1232.c
>> +++ b/drivers/input/touchscreen/st1232.c
>
> [snip]
>
>> @@ -48,6 +50,7 @@ struct st1232_ts_data {
>> struct input_dev *input_dev;
>> struct st1232_ts_finger finger[MAX_FINGERS];
>> struct dev_pm_qos_request low_latency_req;
>> + int reset_pin;
>
> Nitpicking, I would call it reset_gpio.
Consistency is a nice thing, so thanks! Changed.
>> };
>>
>> static int st1232_ts_read_data(struct st1232_ts_data *ts)
>> @@ -139,10 +142,17 @@ end:
>> return IRQ_HANDLED;
>> }
>>
>> +static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron)
>> +{
>> + if (ts->reset_pin)
>> + gpio_direction_output(ts->reset_pin, poweron);
>> +}
>> +
>> static int st1232_ts_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> struct st1232_ts_data *ts;
>> + struct st1232_pdata *pdata = client->dev.platform_data;
>> struct input_dev *input_dev;
>> int error;
>>
>> @@ -167,6 +177,25 @@ static int st1232_ts_probe(struct i2c_client *client,
>> ts->client = client;
>> ts->input_dev = input_dev;
>>
>> + if (client->dev.of_node)
>> + of_property_read_u32(client->dev.of_node,
>> + "sitronix,reset-pin", &ts->reset_pin);
>> + else if (pdata)
>> + ts->reset_pin = pdata->reset_pin;
>> +
>> +
>> + if (ts->reset_pin) {
>> + error = gpio_request(ts->reset_pin, NULL);
>
> You should use devm_gpio_request(), it will simplify the error path and the
> remove function.
Ok.
>> + if (error) {
>> + dev_err(&client->dev,
>> + "Unable to request GPIO pin %d.\n",
>> + ts->reset_pin);
>> + goto err_free_mem;
>> + }
>> + }
>> +
>> + st1232_ts_power(ts, true);
>> +
>> input_dev->name = "st1232-touchscreen";
>> input_dev->id.bustype = BUS_I2C;
>> input_dev->dev.parent = &client->dev;
>
> [snip]
>
>> diff --git a/include/linux/input/st1232_pdata.h
>> b/include/linux/input/st1232_pdata.h new file mode 100644
>> index 0000000..ef99354
>> --- /dev/null
>> +++ b/include/linux/input/st1232_pdata.h
>> @@ -0,0 +1,13 @@
>> +#ifndef _LINUX_ST1232_PDATA_H
>> +#define _LINUX_ST1232_PDATA_H
>> +
>> +/*
>> + * Optional platform data
>> + *
>> + * Use this if you want the driver to drive the reset pin.
>> + */
>> +struct st1232_pdata {
>> + int reset_pin;
>
> I would call this reset_gpio as well.
Done.
>> +};
>> +
>> +#endif
> --
Thanks,
Bastian
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
