Re: [PATCH v4 2/2] media: i2c: Add GC08A3 image sensor driver

quoted

Hi Sakari,

Thanks for your feedback.

On Wed, 2024-02-21 at 07:43 +0000, sakari.ailus@linux.intel.com wrote:
 	 
External email : Please do not click links or open attachments until
you have verified the sender or the content.
 Hi Zhi,

On Wed, Feb 21, 2024 at 02:37:27AM +0000, Zhi Mao (毛智) wrote:
Hi Laurent & sakari,

Thanks for your feedback.

On Tue, 2024-02-20 at 07:25 +0000, sakari.ailus@linux.intel.com
wrote:
  
External email : Please do not click links or open attachments
until
you have verified the sender or the content.
 Hi Zhi,

On Tue, Feb 20, 2024 at 05:45:54AM +0000, Zhi Mao (毛智) wrote:
Hi Laurent,

Thanks for you reply.
I'd like to ask for advice about how to contrl "reset-pin",
please
check the below comments.

On Tue, 2024-02-20 at 05:01 +0200, Laurent Pinchart wrote:
  
External email : Please do not click links or open
attachments
until
you have verified the sender or the content.
 Hi Zhi,

On Tue, Feb 20, 2024 at 02:12:26AM +0000, Zhi Mao (毛智) wrote:
On Tue, 2024-02-06 at 20:45 +0200, Laurent Pinchart wrote:
On Sun, Feb 04, 2024 at 02:15:38PM +0800, Zhi Mao wrote:
Add a V4L2 sub-device driver for Galaxycore GC08A3
image
sensor.
Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/media/i2c/Kconfig  |   10 +
 drivers/media/i2c/Makefile |    1 +
 drivers/media/i2c/gc08a3.c | 1448
++++++++++++++++++++++++++++++++++++
 3 files changed, 1459 insertions(+)
 create mode 100644 drivers/media/i2c/gc08a3.c
[snip]

diff --git a/drivers/media/i2c/gc08a3.c
b/drivers/media/i2c/gc08a3.c
new file mode 100644
index 000000000000..3fc7fffb815c
--- /dev/null
+++ b/drivers/media/i2c/gc08a3.c
@@ -0,0 +1,1448 @@
[snip]

+static int gc08a3_power_on(struct device *dev)
+{
+struct i2c_client *client = to_i2c_client(dev);
+struct v4l2_subdev *sd = i2c_get_clientdata(client);
+struct gc08a3 *gc08a3 = to_gc08a3(sd);
+int ret;
+
+ret =
regulator_bulk_enable(ARRAY_SIZE(gc08a3_supply_name),
+    gc08a3->supplies);
+if (ret < 0) {
+dev_err(gc08a3->dev, "failed to enable regulators:
%d\n",
ret);
+return ret;
+}
+
+ret = clk_prepare_enable(gc08a3->xclk);
+if (ret < 0) {
+regulator_bulk_disable(ARRAY_SIZE(gc08a3_supply_name),
+       gc08a3->supplies);
+dev_err(gc08a3->dev, "clk prepare enable failed\n");
+return ret;
+}
+
+usleep_range(GC08A3_MIN_SLEEP_US,
GC08A3_MAX_SLEEP_US);
+
+gpiod_set_value_cansleep(gc08a3->reset_gpio, 1);
Are you asserting reset when powering on ? That sounds
wrong,
you
should
de-assert reset here (and acquire the reset gpio in
probe()
with
GPIOD_OUT_HIGH). Drivers should use logical levels for
GPIOs,
setting a
GPIO named "reset" to 1 should assert the reset signal,
even
if
the
physical signal is active low. You may have the wrong
polarity in
the
device tree.
According to the sensor power sequence sepc, "reset" pin
should
be
pull
from low to high after "dovdd/dvdd/avdd" power on, so I
follow
this
power sequece to pull "reset" pin high in software flow.
From a hardware point of view that's right, but the Linux
kernel
handles
logical level of GPIOs. If a GPIO is named "reset", it is
expected
that
calling

gpiod_set_value_cansleep(gc08a3->reset_gpio, 1);

will "assert" the reset signal, setting it to a logical
"reset =
true"
level. This maps to the hardware 0V output level, as the
signal
is
active-low. To achieve this, define the reset GPIO as active
low
in
DT,
and the GPIO framework will invert the signal for you. You
should
then
call

gpiod_set_value_cansleep(gc08a3->reset_gpio, 1);

in the driver when you want to assert reset (set it to 0V),
and
gpiod_set_value_cansleep(gc08a3->reset_gpio, 0);

when you want to deassert it (set it to 3.3V, or whatever the
I/O
voltage for the signal is).

This way all driver use logical states, and the inversion is
handled
in
DT.
Sensor power sequence as below:
                      ------------------
                     | | |
                     | | | 
dvdd/avdd/dovdd  --------
                               ---------
                              |
                              |
reset-pin        -------------

In order to match this power sequece, "reset-pin" contrl flow
is
below:
1. config the "reset-pin" is "active-high" in DTS:
    reset-gpios = <&pio 19 GPIO_ACTIVE_HIGH>;

2. image sensor driver probe function:
gc08a3->reset_gpio = devm_gpiod_get(dev, "reset",
GPIOD_OUT_LOW);  //init "reset-pin" is low

3. image sensor driver power_on function:
gpiod_set_value_cansleep(gc08a3->reset_gpio, 1); //pull "reset-
pin"
high 

so, the expect state of "reset-pin" is from low to high.
If I am wrong, please correct me.

From Documentation/driver-api/gpio/consumer.rst:

As a consumer should not have to care about the physical line
level, all of the gpiod_set_value_xxx() or
gpiod_set_array_value_xxx() functions operate with the *logical*
value. With this they take the active low property into account.
This means that they check whether the GPIO is configured to be
active low, and if so, they manipulate the passed value before
the
physical line level is driven.

I.e. when you want to enable reset, you set the value to 1 in the
driver. I
think you're now setting the value to 0 in that case. The
opposite
for
disabling it of course.
After checking "Documentation/driver-api/gpio/consumer.rst":
To summarize::
  Function (example)                 line
property          physical
line
...
  gpiod_set_value(desc, 1);          default (active high)  high
  gpiod_set_value(desc, 0);          active low             high
...


From my understanding, it seems that "reset-pin" is using the
following(active_high) case in current code: 
"gpiod_set_value(desc, 1);          default (active high)  high" 

Do you mean, we should use the "active_low" case:
"gpiod_set_value(desc, 0);          active low             high"
Code should be changed as below:
1. config the "reset-pin" is "active-low" in DTS:
   - reset-gpios = <&pio 19 GPIO_ACTIVE_LOW>
2. image sensor driver power_on function:
   - gpiod_set_value_cansleep(gc08a3->reset_gpio, 0); //pull high 
Is that so?
Correct.
fixed in patch:v6

-- 
Regards,

Sakari Ailus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help