Re: [RFC 11/12] media: m5mols: Adding dt support to m5mols driver
Hi Sylwester,
Thanks for the review.
Actually I agree with all of your review comments.
This was just a temporary test patch, used to test exynos5-mdev.
I thought for some one to test exynos5-mdev series patches, i need to
provide one working m5mols dt driver.
Good to hear you already have one patch for this driver.
If you can able to post the dt patch for this driver, I will use that.
Regards,
Shaik Ameer Basha
On Sat, Mar 23, 2013 at 5:26 PM, Sylwester Nawrocki
sylvester.nawro...@gmail.com wrote:
Hi Shaik,
On 03/06/2013 12:53 PM, Shaik Ameer Basha wrote:
This patch adds the dt support to m5mols driver.
Signed-off-by: Shaik Ameer Bashashaik.am...@samsung.com
---
drivers/media/i2c/m5mols/m5mols_core.c | 54
+++-
1 file changed, 53 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/m5mols/m5mols_core.c
b/drivers/media/i2c/m5mols/m5mols_core.c
index d4e7567..21c66ef 100644
--- a/drivers/media/i2c/m5mols/m5mols_core.c
+++ b/drivers/media/i2c/m5mols/m5mols_core.c
@@ -19,6 +19,8 @@
#includelinux/interrupt.h
#includelinux/delay.h
#includelinux/gpio.h
+#includelinux/of_gpio.h
+#includelinux/pinctrl/consumer.h
What would you need pinctrl for ? In most cases this driver just needs one
GPIO
(sensor RESET), which is normally passed in gpios property.
#includelinux/regulator/consumer.h
#includelinux/videodev2.h
#includelinux/module.h
@@ -926,13 +928,38 @@ static irqreturn_t m5mols_irq_handler(int irq, void
*data)
return IRQ_HANDLED;
}
+static const struct of_device_id m5mols_match[];
+
static int m5mols_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
- const struct m5mols_platform_data *pdata =
client-dev.platform_data;
+ struct m5mols_platform_data *pdata;
struct m5mols_info *info;
+ const struct of_device_id *of_id;
struct v4l2_subdev *sd;
int ret;
+ struct pinctrl *pctrl;
+ int eint_gpio = 0;
+
+ if (client-dev.of_node) {
+ of_id = of_match_node(m5mols_match, client-dev.of_node);
+ if (of_id)
+ pdata = (struct m5mols_platform_data
*)of_id-data;
+ client-dev.platform_data = pdata;
Oh, no. Probably best thing to do would be to get rid of struct
m5mols_platform_data pointer from struct m5mols_info and just add gpio_reset
field there. That's what we have currently in the driver's platform data
structure.
struct m5mols_platform_data {
int gpio_reset;
u8 reset_polarity;
int (*set_power)(struct device *dev, int on);
};
gpio_reset and reset_polarity are already handled in the driver, and for
this we just need a single entry in 'gpios' property.
set_power callback can't be supported. Luckily there seems to be no board
that needs it any more. So we just drop it. One solution for more complex
power sequence could be the Runtime Interpreted Power Sequences. Once it
is available we might be able to describe what was previously in a board
file in set_power callback in the device tree.
+ } else {
+ pdata = client-dev.platform_data;
+ }
+
+ if (!pdata)
+ return -EINVAL;
+
+ pctrl = devm_pinctrl_get_select_default(client-dev);
Two issues here:
1. m5mols DT node doesn't include pinctrl property, does it ?
2. default pinctrl state is now being handled in the driver core.
Hence this pinctrl set up could well be removed.
+ if (client-dev.of_node) {
+ eint_gpio = of_get_named_gpio(client-dev.of_node,
gpios, 0);
+ client-irq = gpio_to_irq(eint_gpio);
+ pdata-gpio_reset = of_get_named_gpio(client-dev.of_node,
+ gpios,
1);
Err, now when pinctrl and generic GPIO DT bindings are supported on Exynos5
this should not be needed at all. request_irq() should work when you add
relevant properties in this device DT node. Please see exynos4210-trats.dts,
mms114-touchscreen node for an example.
mms114-touchscreen@48 {
...
interrupt-parent = gpx0;
interrupts = 4 2;
...
};
You specify GPIO bank in the 'interrupt-parent' property, and the GPIO
index within the bank in first cell of 'interrupts' property. The second
cell are interrupt flags as defined in /Documentation/devicetree/bindings/
interrupt-controller/interrupts.txt. I'm not sure how this interacts with
the interrupt flags passed to request_irq ATM.
It's the pinctrl driver's task to configure the GPIO pinmux into EINT
function, when the above properties are present in device DT node.
+ }
if (pdata == NULL) {
dev_err(client-dev, No platform data\n);
@@ -1040,9 +1067,34 @@ static const struct i2c_device_id m5mols_id[] = {
};
Re: [RFC 11/12] media: m5mols: Adding dt support to m5mols driver
Hi Shaik,
On 03/06/2013 12:53 PM, Shaik Ameer Basha wrote:
This patch adds the dt support to m5mols driver.
Signed-off-by: Shaik Ameer Bashashaik.am...@samsung.com
---
drivers/media/i2c/m5mols/m5mols_core.c | 54 +++-
1 file changed, 53 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/m5mols/m5mols_core.c
b/drivers/media/i2c/m5mols/m5mols_core.c
index d4e7567..21c66ef 100644
--- a/drivers/media/i2c/m5mols/m5mols_core.c
+++ b/drivers/media/i2c/m5mols/m5mols_core.c
@@ -19,6 +19,8 @@
#includelinux/interrupt.h
#includelinux/delay.h
#includelinux/gpio.h
+#includelinux/of_gpio.h
+#includelinux/pinctrl/consumer.h
What would you need pinctrl for ? In most cases this driver just needs
one GPIO
(sensor RESET), which is normally passed in gpios property.
#includelinux/regulator/consumer.h
#includelinux/videodev2.h
#includelinux/module.h
@@ -926,13 +928,38 @@ static irqreturn_t m5mols_irq_handler(int irq, void *data)
return IRQ_HANDLED;
}
+static const struct of_device_id m5mols_match[];
+
static int m5mols_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
- const struct m5mols_platform_data *pdata = client-dev.platform_data;
+ struct m5mols_platform_data *pdata;
struct m5mols_info *info;
+ const struct of_device_id *of_id;
struct v4l2_subdev *sd;
int ret;
+ struct pinctrl *pctrl;
+ int eint_gpio = 0;
+
+ if (client-dev.of_node) {
+ of_id = of_match_node(m5mols_match, client-dev.of_node);
+ if (of_id)
+ pdata = (struct m5mols_platform_data *)of_id-data;
+ client-dev.platform_data = pdata;
Oh, no. Probably best thing to do would be to get rid of struct
m5mols_platform_data pointer from struct m5mols_info and just add gpio_reset
field there. That's what we have currently in the driver's platform data
structure.
struct m5mols_platform_data {
int gpio_reset;
u8 reset_polarity;
int (*set_power)(struct device *dev, int on);
};
gpio_reset and reset_polarity are already handled in the driver, and for
this we just need a single entry in 'gpios' property.
set_power callback can't be supported. Luckily there seems to be no board
that needs it any more. So we just drop it. One solution for more complex
power sequence could be the Runtime Interpreted Power Sequences. Once it
is available we might be able to describe what was previously in a board
file in set_power callback in the device tree.
+ } else {
+ pdata = client-dev.platform_data;
+ }
+
+ if (!pdata)
+ return -EINVAL;
+
+ pctrl = devm_pinctrl_get_select_default(client-dev);
Two issues here:
1. m5mols DT node doesn't include pinctrl property, does it ?
2. default pinctrl state is now being handled in the driver core.
Hence this pinctrl set up could well be removed.
+ if (client-dev.of_node) {
+ eint_gpio = of_get_named_gpio(client-dev.of_node, gpios, 0);
+ client-irq = gpio_to_irq(eint_gpio);
+ pdata-gpio_reset = of_get_named_gpio(client-dev.of_node,
+ gpios, 1);
Err, now when pinctrl and generic GPIO DT bindings are supported on Exynos5
this should not be needed at all. request_irq() should work when you add
relevant properties in this device DT node. Please see exynos4210-trats.dts,
mms114-touchscreen node for an example.
mms114-touchscreen@48 {
...
interrupt-parent = gpx0;
interrupts = 4 2;
...
};
You specify GPIO bank in the 'interrupt-parent' property, and the GPIO
index within the bank in first cell of 'interrupts' property. The second
cell are interrupt flags as defined in /Documentation/devicetree/bindings/
interrupt-controller/interrupts.txt. I'm not sure how this interacts with
the interrupt flags passed to request_irq ATM.
It's the pinctrl driver's task to configure the GPIO pinmux into EINT
function, when the above properties are present in device DT node.
+ }
if (pdata == NULL) {
dev_err(client-dev, No platform data\n);
@@ -1040,9 +1067,34 @@ static const struct i2c_device_id m5mols_id[] = {
};
MODULE_DEVICE_TABLE(i2c, m5mols_id);
+static int m5mols_set_power(struct device *dev, int on)
+{
+ struct m5mols_platform_data *pdata =
+ (struct m5mols_platform_data *)dev-platform_data;
+ gpio_set_value(pdata-gpio_reset, !on);
+ gpio_set_value(pdata-gpio_reset, !!on);
+ return 0;
+}
+
+static struct m5mols_platform_data m5mols_drvdata = {
+ .gpio_reset = 0,
+ .reset_polarity = 0,
+ .set_power = m5mols_set_power,
+};
Huh, you've got extra bonus points for creativity! :)
Do you have fixed voltage regulators, permanently turned on
