| Message ID | 1354021990-16978-4-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Lee, On Tue, Nov 27, 2012 at 01:13:10PM +0000, Lee Jones wrote: > Now we can register the BU21013_ts touch screen when booting with > Device Tree enabled. Here we parse all the necessary components > previously expected to be passed from platform data. I applied these 3 patches, but for DT we also need to specify compatible ID and set up of_match_table pointer. Please send me a follow-up patches doing that and also describing DT bindings for BU21013. Thanks.
Hi Dmitry, > On Tue, Nov 27, 2012 at 01:13:10PM +0000, Lee Jones wrote: > > Now we can register the BU21013_ts touch screen when booting with > > Device Tree enabled. Here we parse all the necessary components > > previously expected to be passed from platform data. > > I applied these 3 patches, but for DT we also need to specify compatible > ID and set up of_match_table pointer. Why do you need a compatible string? > Please send me a follow-up patches > doing that and also describing DT bindings for BU21013. I'll send the documentation as a seperate reply to your email. Kind regards, Lee
On Wed, Nov 28, 2012 at 08:57:31AM +0000, Lee Jones wrote: > > I applied these 3 patches, but for DT we also need to specify compatible > > ID and set up of_match_table pointer. > Why do you need a compatible string? The I2C subsystem guesses at a compatible string by default but it's much better to explicitly set one as conflicts do arise from time to time (eg, Wolfson parts are called WMxxxx but the WM prefix is also used by at least WonderMedia). -- 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 Wed, 28 Nov 2012, Mark Brown wrote: > On Wed, Nov 28, 2012 at 08:57:31AM +0000, Lee Jones wrote: > > > > I applied these 3 patches, but for DT we also need to specify compatible > > > ID and set up of_match_table pointer. > > > Why do you need a compatible string? > > The I2C subsystem guesses at a compatible string by default but it's > much better to explicitly set one as conflicts do arise from time to > time (eg, Wolfson parts are called WMxxxx but the WM prefix is also used > by at least WonderMedia). It uses the exact device name, rather than guessing. I don't think you're allowed to have duplicate device names in the system, or there would be clashes at registration time. The system is the same for platform data and DT alike. In platform data we have: > struct i2c_board_info mop500_i2c0_devices_stuib[] = { > { > .type = "bu21013_tp", > .addr = 0x5C, > } > }; Where (i2c_client)client->name is populated by 'type', which should then match 'driver.name' exactly. And in DT we have: bu21013_tp@0x5c { compatible = "rhom,bu21013_tp"; reg = <0x5c>; }; Where (i2c_client)client->name is populated by of_modalias_node(), which uses the compatible string in the DTS(I) file and takes off the '<vendor>,' which should then match 'driver.name' exactly. Hence, there should be no need to have a compatible string in any i2c driver registration information.
On Thu, Nov 29, 2012 at 10:08:08AM +0000, Lee Jones wrote: > On Wed, 28 Nov 2012, Mark Brown wrote: > > The I2C subsystem guesses at a compatible string by default but it's > > much better to explicitly set one as conflicts do arise from time to > > time (eg, Wolfson parts are called WMxxxx but the WM prefix is also used > > by at least WonderMedia). > It uses the exact device name, rather than guessing. I don't think > you're allowed to have duplicate device names in the system, or there > would be clashes at registration time. > The system is the same for platform data and DT alike. Right, which is why this mostly works, but it's still better to provide an actual compatible string which we can be 100% certain will avoid conflicts. This is very low cost when one is already defining DT bindings. > Hence, there should be no need to have a compatible string in any i2c > driver registration information. We're getting away with it at present (and frankly nobody's going to build in two different drivers for a chip of the same name for practical systems anyway).
On Thu, 29 Nov 2012, Mark Brown wrote: > On Thu, Nov 29, 2012 at 10:08:08AM +0000, Lee Jones wrote: > > On Wed, 28 Nov 2012, Mark Brown wrote: > > > > The I2C subsystem guesses at a compatible string by default but it's > > > much better to explicitly set one as conflicts do arise from time to > > > time (eg, Wolfson parts are called WMxxxx but the WM prefix is also used > > > by at least WonderMedia). > > > It uses the exact device name, rather than guessing. I don't think > > you're allowed to have duplicate device names in the system, or there > > would be clashes at registration time. > > > The system is the same for platform data and DT alike. > > Right, which is why this mostly works, but it's still better to provide > an actual compatible string which we can be 100% certain will avoid > conflicts. This is very low cost when one is already defining DT > bindings. I know that it's an easy thing to do, but I'm more worried about what would happen if the an I2C device is registered using the current way (stripping the compatible string and using it as a client look-up) and we also provide a compatible entry in the driver itself. Do you know if there is any danger of duplicate registration or suchlike? > > Hence, there should be no need to have a compatible string in any i2c > > driver registration information. > > We're getting away with it at present (and frankly nobody's going to > build in two different drivers for a chip of the same name for practical > systems anyway). Right. In the same way as we're getting away with it when we register a platform_device using the register/add routines.
On Thu, Nov 29, 2012 at 12:00:00PM +0000, Lee Jones wrote: > On Thu, 29 Nov 2012, Mark Brown wrote: > > Right, which is why this mostly works, but it's still better to provide > > an actual compatible string which we can be 100% certain will avoid > > conflicts. This is very low cost when one is already defining DT > > bindings. > I know that it's an easy thing to do, but I'm more worried about > what would happen if the an I2C device is registered using the > current way (stripping the compatible string and using it as a > client look-up) and we also provide a compatible entry in the > driver itself. Do you know if there is any danger of duplicate > registration or suchlike? That's totally fine, we try first with compatible properties and only look for the fallback if there aren't any. > > > Hence, there should be no need to have a compatible string in any i2c > > > driver registration information. > > We're getting away with it at present (and frankly nobody's going to > > build in two different drivers for a chip of the same name for practical > > systems anyway). > Right. In the same way as we're getting away with it when we > register a platform_device using the register/add routines. Those are all completely Linux-defined of course which helps as well.
diff --git a/drivers/input/touchscreen/bu21013_ts.c b/drivers/input/touchscreen/bu21013_ts.c index 8982bea..dbbd1f8 100644 --- a/drivers/input/touchscreen/bu21013_ts.c +++ b/drivers/input/touchscreen/bu21013_ts.c @@ -15,6 +15,8 @@ #include <linux/regulator/consumer.h> #include <linux/module.h> #include <linux/gpio.h> +#include <linux/of.h> +#include <linux/of_gpio.h> #define PEN_DOWN_INTR 0 #define MAX_FINGERS 2 @@ -454,13 +456,30 @@ static void bu21013_cs_disable(struct bu21013_ts_data *bu21013_data) * This function used to initializes the i2c-client touchscreen * driver and returns integer. */ +static void __devinit bu21013_of_probe(struct device_node *np, + struct bu21013_platform_device *pdata) +{ + pdata->y_flip = pdata->x_flip = false; + + pdata->x_flip = of_property_read_bool(np, "rohm,flip-x"); + pdata->y_flip = of_property_read_bool(np, "rohm,flip-y"); + + of_property_read_u32(np, "rohm,touch-max-x", &pdata->touch_x_max); + of_property_read_u32(np, "rohm,touch-max-y", &pdata->touch_y_max); + + pdata->touch_pin = of_get_named_gpio(np, "touch-gpio", 0); + pdata->cs_pin = of_get_named_gpio(np, "reset-gpio", 0); + + pdata->ext_clk = false; +} + static int __devinit bu21013_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct bu21013_ts_data *bu21013_data; struct input_dev *in_dev; - const struct bu21013_platform_device *pdata = - client->dev.platform_data; + struct device_node *np = client->dev.of_node; + struct bu21013_platform_device *pdata = client->dev.platform_data; int error; if (!i2c_check_functionality(client->adapter, @@ -470,8 +489,17 @@ static int __devinit bu21013_probe(struct i2c_client *client, } if (!pdata) { - dev_err(&client->dev, "platform data not defined\n"); - return -EINVAL; + if (np) { + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), + GFP_KERNEL); + if (!pdata) + return -ENOMEM; + + bu21013_of_probe(np, pdata); + } else { + dev_err(&client->dev, "no device tree or platform data\n"); + return -EINVAL; + } } if (!gpio_is_valid(pdata->touch_pin)) {