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

[Dmitry Torokhov]

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.

> 
> 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?

> +
> +	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.

> +}
> +#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.

> +
> +	} 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.

> +
>  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.

>  
>  #endif
> -- 
> 1.8.3.2
> 

Thanks.

-- 
Dmitry
--
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/