Re: [PATCH 1/9] Input: pixcir_i2c_ts: Add device tree support

[Roger Quadros]

On 12/18/2013 07:39 PM, Dmitry Torokhov wrote:
> Hi Roger,
> 
> On Wed, Dec 18, 2013 at 02:51:12PM +0530, Roger Quadros wrote:
>> Provide device tree support and binding information.
>> Change platform data parameters from x/y_max to x/y_size..
> 
> I'd rather keep them as they were.

OK.

> 
>>
>> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
>> Acked-by: Mugunthan V N <mugunthanvnm@xxxxxx>
>> ---
>>  .../bindings/input/touchscreen/pixcir_i2c_ts.txt   | 26 ++++++++
>>  drivers/input/touchscreen/pixcir_i2c_ts.c          | 77 ++++++++++++++++++++--
>>  include/linux/input/pixcir_ts.h                    |  5 +-
>>  3 files changed, 101 insertions(+), 7 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>> new file mode 100644
>> index 0000000..c0b0b270
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/pixcir_i2c_ts.txt
>> @@ -0,0 +1,26 @@
>> +* Pixcir I2C touchscreen controllers
>> +
>> +Required properties:
>> +- compatible: must be "pixcir,pixcir_ts"
>> +- reg: I2C address of the chip
>> +- interrupts: interrupt to which the chip is connected
>> +- attb-gpio: GPIO connected to the ATTB line of the chip
>> +- x-size: horizontal resolution of touchscreen
>> +- y-size: vertical resolution of touchscreen
>> +
>> +Example:
>> +
>> +	i2c@00000000 {
>> +		/* ... */
>> +
>> +		pixcir_ts@5c {
>> +			compatible = "pixcir,pixcir_ts";
>> +			reg = <0x5c>;
>> +			interrupts = <2 0>;
>> +			attb-gpio = <&gpf 2 0 2>;
>> +			x-size = <800>;
>> +			y-size = <600>;
>> +		};
>> +
>> +		/* ... */
>> +	};
>> diff --git a/drivers/input/touchscreen/pixcir_i2c_ts.c b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> index 6cc6b36..3a447c9 100644
>> --- a/drivers/input/touchscreen/pixcir_i2c_ts.c
>> +++ b/drivers/input/touchscreen/pixcir_i2c_ts.c
>> @@ -24,6 +24,10 @@
>>  #include <linux/i2c.h>
>>  #include <linux/input.h>
>>  #include <linux/input/pixcir_ts.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_device.h>
>>  
>>  struct pixcir_i2c_ts_data {
>>  	struct i2c_client *client;
>> @@ -125,15 +129,65 @@ static int pixcir_i2c_ts_resume(struct device *dev)
>>  static SIMPLE_DEV_PM_OPS(pixcir_dev_pm_ops,
>>  			 pixcir_i2c_ts_suspend, pixcir_i2c_ts_resume);
>>  
>> +#if defined(CONFIG_OF)
> 
> #ifdef is preferred for simple conditions.
> 
>> +static const struct of_device_id pixcir_of_match[];
>> +
>> +static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>> +{
>> +	struct pixcir_ts_platform_data *pdata;
>> +	struct device_node *np = dev->of_node;
>> +	const struct of_device_id *match;
>> +
>> +	match = of_match_device(of_match_ptr(pixcir_of_match), dev);
>> +	if (!match)
>> +		return ERR_PTR(-EINVAL);
> 
> Why do you need an explicit match here?
> 

Right, it is not needed here for this patch but used later in patch 6
to get chip specific data. I'll fix this while re-arranging the patches.

>> +
>> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +	if (!pdata)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	pdata->gpio_attb = of_get_named_gpio(np, "attb-gpio", 0);
>> +	if (!gpio_is_valid(pdata->gpio_attb))
>> +		dev_err(dev, "Failed to get ATTB GPIO\n");
>> +
>> +	if (of_property_read_u32(np, "x-size", &pdata->x_size)) {
>> +		dev_err(dev, "Failed to get x-size property\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	if (of_property_read_u32(np, "y-size", &pdata->y_size)) {
>> +		dev_err(dev, "Failed to get y-size property\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	dev_dbg(dev, "%s: x %d, y %d, gpio %d\n", __func__,
>> +				pdata->x_size, pdata->y_size, pdata->gpio_attb);
>> +
>> +	return pdata;
>> +}
>> +#else
>> +static struct pixcir_ts_platform_data *pixcir_parse_dt(struct device *dev)
>> +{
>> +	return NULL;
> 
> This should be
> 
> 	return ERR_PTR(-EINVAL);
> 
> since you test it with IS_ERR() later.

OK.

> 
>> +}
>> +#endif
>> +
>>  static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>  					 const struct i2c_device_id *id)
>>  {
>>  	const struct pixcir_ts_platform_data *pdata = client->dev.platform_data;
>> +	struct device *dev = &client->dev;
>> +	struct device_node *np = dev->of_node;
>>  	struct pixcir_i2c_ts_data *tsdata;
>>  	struct input_dev *input;
>>  	int error;
>>  
>> -	if (!pdata) {
>> +	if (np) {
>> +		pdata = pixcir_parse_dt(dev);
>> +		if (IS_ERR(pdata))
>> +			return PTR_ERR(pdata);
> 
> We should be favoring kernel-provided platform data if it is pesent even
> if there is dt-data available. This way user can override firmware,m if
> needed.
> 
OK. But how can the user override kernel provided platform data? Can't device
tree overlays be used to patch DT data in the same fashion.

I've not tried either so I don't know which one is more user friendly.

>> +
>> +	} else if (!pdata) {
>>  		dev_err(&client->dev, "platform data not defined\n");
>>  		return -EINVAL;
>>  	}
>> @@ -157,10 +211,14 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
>>  	__set_bit(EV_KEY, input->evbit);
>>  	__set_bit(EV_ABS, input->evbit);
>>  	__set_bit(BTN_TOUCH, input->keybit);
>> -	input_set_abs_params(input, ABS_X, 0, pdata->x_max, 0, 0);
>> -	input_set_abs_params(input, ABS_Y, 0, pdata->y_max, 0, 0);
>> -	input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata->x_max, 0, 0);
>> -	input_set_abs_params(input, ABS_MT_POSITION_Y, 0, pdata->y_max, 0, 0);
>> +	input_set_abs_params(input, ABS_X,
>> +					0, pdata->x_size - 1, 0, 0);
>> +	input_set_abs_params(input, ABS_Y,
>> +					0, pdata->y_size - 1, 0, 0);
>> +	input_set_abs_params(input, ABS_MT_POSITION_X,
>> +					0, pdata->x_size - 1, 0, 0);
>> +	input_set_abs_params(input, ABS_MT_POSITION_Y,
>> +					0, pdata->y_size - 1, 0, 0);
>>  
>>  	input_set_drvdata(input, tsdata);
>>  
>> @@ -211,11 +269,20 @@ static const struct i2c_device_id pixcir_i2c_ts_id[] = {
>>  };
>>  MODULE_DEVICE_TABLE(i2c, pixcir_i2c_ts_id);
>>  
>> +#if defined(CONFIG_OF)
>> +static const struct of_device_id pixcir_of_match[] = {
>> +	{ .compatible = "pixcir,pixcir_ts", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, pixcir_of_match);
>> +#endif
> 
> #ifdef is preferred for simple conditions.
> 
OK.

>> +
>>  static struct i2c_driver pixcir_i2c_ts_driver = {
>>  	.driver = {
>>  		.owner	= THIS_MODULE,
>>  		.name	= "pixcir_ts",
>>  		.pm	= &pixcir_dev_pm_ops,
>> +		.of_match_table = of_match_ptr(pixcir_of_match),
>>  	},
>>  	.probe		= pixcir_i2c_ts_probe,
>>  	.remove		= pixcir_i2c_ts_remove,
>> diff --git a/include/linux/input/pixcir_ts.h b/include/linux/input/pixcir_ts.h
>> index 7163d91..b34ff7e 100644
>> --- a/include/linux/input/pixcir_ts.h
>> +++ b/include/linux/input/pixcir_ts.h
>> @@ -3,8 +3,9 @@
>>  
>>  struct pixcir_ts_platform_data {
>>  	int (*attb_read_val)(void);
>> -	int x_max;
>> -	int y_max;
>> +	unsigned int x_size;	/* X axis resolution */
>> +	unsigned int y_size;	/* Y axis resolution */
>> +	int gpio_attb;		/* GPIO connected to ATTB line */
>>  };
> 
> OK, it looks like the series were split awkwardly: you are defining data
> that is used by follow-up patches and its purpose is unclear when
> reviewing patch-by-patch. I'd recommend rearranging so that DT support
> patch is the very last in the series.
> 
Got it.

>>  
>>  #endif
>> -- 
>> 1.8.3.2

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/