Hi Dave,
On Mon, Jan 20, 2025 at 12:28:04PM +0000, Dave Stevenson wrote:
> Hi Sakari
>
> On Mon, 20 Jan 2025 at 10:59, Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> >
> > Hi Dave,
> >
> > On Fri, Jan 10, 2020 at 11:09:15PM +0300, Andrey Konovalov wrote:
> > > +/* Power/clock management functions */
> > > +static int imx219_power_on(struct device *dev)
> > > +{
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > + struct imx219 *imx219 = to_imx219(sd);
> > > + int ret;
> > > +
> > > + ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES,
> > > + imx219->supplies);
> > > + if (ret) {
> > > + dev_err(&client->dev, "%s: failed to enable regulators\n",
> > > + __func__);
> > > + return ret;
> > > + }
> > > +
> > > + ret = clk_prepare_enable(imx219->xclk);
> > > + if (ret) {
> > > + dev_err(&client->dev, "%s: failed to enable clock\n",
> > > + __func__);
> > > + goto reg_off;
> > > + }
> > > +
> > > + gpiod_set_value_cansleep(imx219->reset_gpio, 1);
> > > + msleep(IMX219_XCLR_DELAY_MS);
> > > +
> > > + return 0;
> > > +reg_off:
> > > + regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
> > > + return ret;
> > > +}
> > > +
> > > +static int imx219_power_off(struct device *dev)
> > > +{
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > + struct imx219 *imx219 = to_imx219(sd);
> > > +
> > > + gpiod_set_value_cansleep(imx219->reset_gpio, 0);
> >
> > The polarity of the reset GPIO appears to be wrong above. Given it works
> > somewhere (arch/arm64/boot/dts/ti/k3-am62x-sk-csi2-imx219.dtso), the
> > existing DTS files have it wrong, too. The bindings still appear to
> > document it correctly.
>
> Why do you say it is wrong?
> I don't recall why this got called reset-gpio - the signal on the
> sensor is XCLR, and that is documented in the binding.
> The datasheet says low is standby and high is active, which matches
> what the driver does.
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.
In other words, if the driver calls gpiod_set_value_cansleep(, 0), it
enables reset signal on the device, bringing the device to hard reset. This
is the opposite of what the driver does in its resume callback.
So in order to function, the DT must have wrong polarity as well.
Similarly, if someone writes a correct DT for a board connecting the reset
GPIO, the imx219 driver simply won't work.
Looking at the arch/arm64/boot/dts/ti/k3-am62x-sk-csi2-imx219.dtso overlay,
it is for the RPi IMX219 module that does not connect the reset GPIO at
all. In other words, there doesn't seem to be a upstream user of the reset
GPIO in IMX219.
>
> > Laurent confirmed xcrl isn't controllable in the RPi imx219 camera module.
> >
> > How about fixing this? Currently correctly written DTBs including imx219
> > won't work.
>
> Seeing as the DTB is ABI, the only improvement I can see is to rename
> "imx219->reset_gpio" to "imx219->xclr_gpio".
> What else would you be proposing?
If there's a concern this could break existing DTs, the driver could try
inverting the polarity of the GPIO if the sensor isn't immediately
accessible.
I wonder what others think.
Cc Laurent.
>
> Dave
>
> > I noticed this while fixing the power sequences in this and a few other
> > drivers.
> >
> > > + regulator_bulk_disable(IMX219_NUM_SUPPLIES, imx219->supplies);
> > > + clk_disable_unprepare(imx219->xclk);
> > > +
> > > + return 0;
> > > +}
> >
> > ...
> >
> > > +static int imx219_probe(struct i2c_client *client,
> > > + const struct i2c_device_id *id)
> > > +{
> > > + struct device *dev = &client->dev;
> > > + struct fwnode_handle *endpoint;
> > > + struct imx219 *imx219;
> > > + int ret;
> > > +
> > > + imx219 = devm_kzalloc(&client->dev, sizeof(*imx219), GFP_KERNEL);
> > > + if (!imx219)
> > > + return -ENOMEM;
> > > +
> > > + imx219->dev = dev;
> > > +
> > > + v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops);
> > > +
> > > + /* Get CSI2 bus config */
> > > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> > > + NULL);
> > > + if (!endpoint) {
> > > + dev_err(dev, "endpoint node not found\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = v4l2_fwnode_endpoint_parse(endpoint, &imx219->ep);
> > > + fwnode_handle_put(endpoint);
> > > + if (ret) {
> > > + dev_err(dev, "could not parse endpoint\n");
> > > + return ret;
> > > + }
> > > +
> > > + /* Check the number of MIPI CSI2 data lanes */
> > > + if (imx219->ep.bus_type != V4L2_MBUS_CSI2_DPHY ||
> > > + imx219->ep.bus.mipi_csi2.num_data_lanes != 2) {
> > > + dev_err(dev, "only 2 data lanes are currently supported\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + /* Get system clock (xclk) */
> > > + imx219->xclk = devm_clk_get(dev, NULL);
> > > + if (IS_ERR(imx219->xclk)) {
> > > + dev_err(dev, "failed to get xclk\n");
> > > + return PTR_ERR(imx219->xclk);
> > > + }
> > > +
> > > + imx219->xclk_freq = clk_get_rate(imx219->xclk);
> > > + if (imx219->xclk_freq != IMX219_XCLK_FREQ) {
> > > + dev_err(dev, "xclk frequency not supported: %d Hz\n",
> > > + imx219->xclk_freq);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = imx219_get_regulators(imx219);
> > > + if (ret) {
> > > + dev_err(dev, "failed to get regulators\n");
> > > + return ret;
> > > + }
> > > +
> > > + /* Request optional enable pin */
> > > + imx219->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > > + GPIOD_OUT_HIGH);
> > > +
> > > + /*
> > > + * The sensor must be powered for imx219_identify_module()
> > > + * to be able to read the CHIP_ID register
> > > + */
> > > + ret = imx219_power_on(dev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = imx219_identify_module(imx219);
> > > + if (ret)
> > > + goto error_power_off;
> > > +
> > > + /* Set default mode to max resolution */
> > > + imx219->mode = &supported_modes[0];
> > > +
> > > + ret = imx219_init_controls(imx219);
> > > + if (ret)
> > > + goto error_power_off;
> > > +
> > > + /* Initialize subdev */
> > > + imx219->sd.internal_ops = &imx219_internal_ops;
> > > + imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > + imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > > +
> > > + /* Initialize source pad */
> > > + imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > +
> > > + ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> > > + if (ret) {
> > > + dev_err(dev, "failed to init entity pads: %d\n", ret);
> > > + goto error_handler_free;
> > > + }
> > > +
> > > + ret = v4l2_async_register_subdev_sensor_common(&imx219->sd);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> > > + goto error_media_entity;
> > > + }
> > > +
> > > + /* Enable runtime PM and turn off the device */
> > > + pm_runtime_set_active(dev);
> > > + pm_runtime_enable(dev);
> > > + pm_runtime_idle(dev);
> > > +
> > > + return 0;
> > > +
> > > +error_media_entity:
> > > + media_entity_cleanup(&imx219->sd.entity);
> > > +
> > > +error_handler_free:
> > > + imx219_free_controls(imx219);
> > > +
> > > +error_power_off:
> > > + imx219_power_off(dev);
> > > +
> > > + return ret;
> > > +}
> >
> > --
> > Kind regards,
> >
> > Sakari Ailus
--
Kind regards,
Sakari Ailus
