On 15/02/2024 21:44, Umang Jain wrote: > From: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx> > > Add a v4l2 subdevice driver for the Sony IMX283 image sensor. > > The IMX283 is a 20MP Diagonal 15.86 mm (Type 1) CMOS Image Sensor with > Square Pixel for Color Cameras. > ... > + > +static int imx283_power_off(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx283 *imx283 = to_imx283(sd); > + > + gpiod_set_value_cansleep(imx283->reset_gpio, 0); That's odd. It is reset GPIO, so 0 means deassert means "stop resetting device". You have wrong handling of the GPIO. > + regulator_bulk_disable(ARRAY_SIZE(imx283_supply_name), imx283->supplies); > + clk_disable_unprepare(imx283->xclk); > + > + return 0; > +} > + > +static int imx283_get_regulators(struct imx283 *imx283) > +{ > + unsigned int i; > + > + for (i = 0; i < ARRAY_SIZE(imx283_supply_name); i++) > + imx283->supplies[i].supply = imx283_supply_name[i]; > + > + return devm_regulator_bulk_get(imx283->dev, > + ARRAY_SIZE(imx283_supply_name), > + imx283->supplies); > +} ... > +static const struct of_device_id imx283_dt_ids[] = { > + { .compatible = "sony,imx283", }, > + { /* sentinel */ } > +}; This goes next to driver structure. You also miss module device table. > + > +static int imx283_parse_endpoint(struct imx283 *imx283) > +{ > + struct fwnode_handle *fwnode = dev_fwnode(imx283->dev); > + struct v4l2_fwnode_endpoint bus_cfg = { > + .bus_type = V4L2_MBUS_CSI2_DPHY > + }; > + struct fwnode_handle *ep; > + int ret; > + > + if (!fwnode) > + return -ENXIO; > + > + ep = fwnode_graph_get_next_endpoint(fwnode, NULL); > + if (!ep) { > + dev_err(imx283->dev, "Failed to get next endpoint\n"); > + return -ENXIO; > + } > + > + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); > + fwnode_handle_put(ep); > + if (ret) > + return ret; > + > + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) { > + dev_err(imx283->dev, > + "number of CSI2 data lanes %d is not supported\n", > + bus_cfg.bus.mipi_csi2.num_data_lanes); > + ret = -EINVAL; > + goto done_endpoint_free; > + } > + > + ret = v4l2_link_freq_to_bitmap(imx283->dev, bus_cfg.link_frequencies, > + bus_cfg.nr_of_link_frequencies, > + link_frequencies, ARRAY_SIZE(link_frequencies), > + &imx283->link_freq_bitmap); > + > +done_endpoint_free: > + v4l2_fwnode_endpoint_free(&bus_cfg); > + > + return ret; > +}; > + > +static int imx283_probe(struct i2c_client *client) > +{ > + struct imx283 *imx283; > + int ret; > + unsigned int i; > + unsigned int xclk_freq; > + > + imx283 = devm_kzalloc(&client->dev, sizeof(*imx283), GFP_KERNEL); > + if (!imx283) > + return -ENOMEM; > + > + imx283->dev = &client->dev; > + > + v4l2_i2c_subdev_init(&imx283->sd, client, &imx283_subdev_ops); > + > + imx283->cci = devm_cci_regmap_init_i2c(client, 16); > + if (IS_ERR(imx283->cci)) { > + ret = PTR_ERR(imx283->cci); > + dev_err(imx283->dev, "failed to initialize CCI: %d\n", ret); > + return ret; > + } > + > + /* Get system clock (xclk) */ > + imx283->xclk = devm_clk_get(imx283->dev, NULL); > + if (IS_ERR(imx283->xclk)) { > + dev_err(imx283->dev, "failed to get xclk\n"); Syntax is: return dev_err_probe() > + return PTR_ERR(imx283->xclk); > + } > + > + xclk_freq = clk_get_rate(imx283->xclk); > + for (i = 0; i < ARRAY_SIZE(imx283_frequencies); i++) { > + if (xclk_freq == imx283_frequencies[i].mhz) { > + imx283->freq = &imx283_frequencies[i]; > + break; > + } > + } > + if (!imx283->freq) { > + dev_err(imx283->dev, "xclk frequency unsupported: %d Hz\n", xclk_freq); > + return -EINVAL; > + } > + > + ret = imx283_get_regulators(imx283); > + if (ret) { > + dev_err(imx283->dev, "failed to get regulators\n"); > + return ret; return dev_err_probe() > + } > + > + ret = imx283_parse_endpoint(imx283); > + if (ret) { > + dev_err(imx283->dev, "failed to parse endpoint configuration\n"); > + return ret; > + } > + > + /* Request optional enable pin */ > + imx283->reset_gpio = devm_gpiod_get_optional(imx283->dev, "reset", > + GPIOD_OUT_HIGH); No, that's not allowed in your binding. Don't add undocumented properties. > + > + mutex_init(&imx283->mutex); > + > + /* > + * The sensor must be powered for imx283_identify_module() > + * to be able to read the CHIP_ID register > + */ > + ret = imx283_power_on(imx283->dev); > + if (ret) > + return ret; > + Best regards, Krzysztof