| Message ID | 20201013130503.2412-1-dongchun.zhu@mediatek.com |
|---|---|
| Headers | show |
| Series | media: i2c: Add support for OV02A10 sensor | expand |
On Tue, Oct 13, 2020 at 09:05:03PM +0800, Dongchun Zhu wrote: > Add a V4L2 sub-device driver for OmniVision OV02A10 image sensor. ... > +#define OV02A10_ID_MASK 0xffff GENMASK() (And include bits.h for that) ... > +static int __ov02a10_start_stream(struct ov02a10 *ov02a10) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > + const struct ov02a10_reg_list *reg_list; > + int ret; > + > + /* Apply default values of current mode */ > + reg_list = &ov02a10->cur_mode->reg_list; > + ret = ov02a10_write_array(ov02a10, reg_list); > + if (ret) > + return ret; > + > + /* Apply customized values from user */ > + ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler); > + if (ret) > + return ret; > + > + /* Set orientation to 180 degree */ > + if (ov02a10->upside_down) { > + ret = i2c_smbus_write_byte_data(client, REG_MIRROR_FLIP_CONTROL, > + REG_MIRROR_FLIP_ENABLE); > + if (ret) { Shouldn't you use 'ret < 0' here as well? > + dev_err(&client->dev, "failed to set orientation\n"); > + return ret; > + } > + ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE, > + REG_ENABLE); > + if (ret < 0) > + return ret; > + } > + > + /* Set MIPI TX speed according to DT property */ > + if (ov02a10->mipi_clock_voltage != OV02A10_MIPI_TX_SPEED_DEFAULT) { > + ret = i2c_smbus_write_byte_data(client, TX_SPEED_AREA_SEL, > + ov02a10->mipi_clock_voltage); > + if (ret < 0) > + return ret; > + } > + > + /* Set stream on register */ > + return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE, > + SC_CTRL_MODE_STREAMING); > +} ... > +/* Was your intention to declare it as a kernel doc? > + * ov02a10_set_exposure - Function called when setting exposure time > + * @priv: Pointer to device structure > + * @val: Variable for exposure time, in the unit of micro-second > + * > + * Set exposure time based on input value. > + * > + * Return: 0 on success > + */ > +static int ov02a10_set_exposure(struct ov02a10 *ov02a10, int val) ... > +static int ov02a10_check_hwcfg(struct device *dev, struct ov02a10 *ov02a10) > +{ > + struct fwnode_handle *ep; > + struct fwnode_handle *fwnode = dev_fwnode(dev); > + struct v4l2_fwnode_endpoint bus_cfg = { > + .bus_type = V4L2_MBUS_CSI2_DPHY, > + }; > + unsigned int i, j; > + int ret; > + if (!fwnode) > + return -EINVAL; Basically you can avoid this check, but it's up to you. > + ep = fwnode_graph_get_next_endpoint(fwnode, NULL); > + if (!ep) > + return -ENXIO; > + > + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); > + fwnode_handle_put(ep); > + if (ret) > + return ret; > + > + for (i = 0; i < ARRAY_SIZE(link_freq_menu_items); i++) { > + for (j = 0; j < bus_cfg.nr_of_link_frequencies; j++) { > + if (link_freq_menu_items[i] == > + bus_cfg.link_frequencies[j]) { > + ov02a10->freq_index = i; > + break; > + } > + } > + > + if (j == bus_cfg.nr_of_link_frequencies) { > + dev_err(dev, "no link frequency %lld supported\n", > + link_freq_menu_items[i]); > + ret = -EINVAL; > + break; > + } > + } > + > + v4l2_fwnode_endpoint_free(&bus_cfg); > + > + return ret; > +} ... > + fwnode_property_read_u32(dev_fwnode(dev), "rotation", &rotation); Same Q as per previous reviews. Why device property API can't be used here? And everywhere else when you have fwnode_property_read_*(dev_fwnode(dev), ...) calls.
Hi Andy, On Fri, 2020-10-23 at 17:31 +0300, Andy Shevchenko wrote: > On Tue, Oct 13, 2020 at 09:05:03PM +0800, Dongchun Zhu wrote: > > Add a V4L2 sub-device driver for OmniVision OV02A10 image sensor. > > ... > > > +#define OV02A10_ID_MASK 0xffff > > GENMASK() > > (And include bits.h for that) > It seems could build pass without including bits.h, as DW9768 once used. > ... > > > +static int __ov02a10_start_stream(struct ov02a10 *ov02a10) > > +{ > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev); > > + const struct ov02a10_reg_list *reg_list; > > + int ret; > > + > > + /* Apply default values of current mode */ > > + reg_list = &ov02a10->cur_mode->reg_list; > > + ret = ov02a10_write_array(ov02a10, reg_list); > > + if (ret) > > + return ret; > > + > > + /* Apply customized values from user */ > > + ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler); > > + if (ret) > > + return ret; > > + > > + /* Set orientation to 180 degree */ > > + if (ov02a10->upside_down) { > > + ret = i2c_smbus_write_byte_data(client, REG_MIRROR_FLIP_CONTROL, > > + REG_MIRROR_FLIP_ENABLE); > > + if (ret) { > > Shouldn't you use 'ret < 0' here as well? > Fixed in next release. > > + dev_err(&client->dev, "failed to set orientation\n"); > > + return ret; > > + } > > + ret = i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE, > > + REG_ENABLE); > > + if (ret < 0) > > + return ret; > > + } > > + > > + /* Set MIPI TX speed according to DT property */ > > + if (ov02a10->mipi_clock_voltage != OV02A10_MIPI_TX_SPEED_DEFAULT) { > > + ret = i2c_smbus_write_byte_data(client, TX_SPEED_AREA_SEL, > > + ov02a10->mipi_clock_voltage); > > + if (ret < 0) > > + return ret; > > + } > > + > > + /* Set stream on register */ > > + return i2c_smbus_write_byte_data(client, REG_SC_CTRL_MODE, > > + SC_CTRL_MODE_STREAMING); > > +} > > ... > > > +/* > > Was your intention to declare it as a kernel doc? > Removed in next release. > > + * ov02a10_set_exposure - Function called when setting exposure time > > + * @priv: Pointer to device structure > > + * @val: Variable for exposure time, in the unit of micro-second > > + * > > + * Set exposure time based on input value. > > + * > > + * Return: 0 on success > > + */ > > +static int ov02a10_set_exposure(struct ov02a10 *ov02a10, int val) > > ... > > > +static int ov02a10_check_hwcfg(struct device *dev, struct ov02a10 *ov02a10) > > +{ > > + struct fwnode_handle *ep; > > + struct fwnode_handle *fwnode = dev_fwnode(dev); > > + struct v4l2_fwnode_endpoint bus_cfg = { > > + .bus_type = V4L2_MBUS_CSI2_DPHY, > > + }; > > + unsigned int i, j; > > + int ret; > > > + if (!fwnode) > > + return -EINVAL; > > Basically you can avoid this check, but it's up to you. > I'd like to keep it. Thank you. > > + ep = fwnode_graph_get_next_endpoint(fwnode, NULL); > > + if (!ep) > > + return -ENXIO; > > + > > + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); > > + fwnode_handle_put(ep); > > + if (ret) > > + return ret; > > + > > + for (i = 0; i < ARRAY_SIZE(link_freq_menu_items); i++) { > > + for (j = 0; j < bus_cfg.nr_of_link_frequencies; j++) { > > + if (link_freq_menu_items[i] == > > + bus_cfg.link_frequencies[j]) { > > + ov02a10->freq_index = i; > > + break; > > + } > > + } > > + > > + if (j == bus_cfg.nr_of_link_frequencies) { > > + dev_err(dev, "no link frequency %lld supported\n", > > + link_freq_menu_items[i]); > > + ret = -EINVAL; > > + break; > > + } > > + } > > + > > + v4l2_fwnode_endpoint_free(&bus_cfg); > > + > > + return ret; > > +} > > ... > > > + fwnode_property_read_u32(dev_fwnode(dev), "rotation", &rotation); > > Same Q as per previous reviews. Why device property API can't be used here? > > And everywhere else when you have > fwnode_property_read_*(dev_fwnode(dev), ...) > calls. > Thanks for the reminder. 'fwnode_property_read_u32' and 'fwnode_property_read_u32_array' would be replaced in next release if local test passes.
On Thu, Nov 19, 2020 at 09:06:41PM +0800, Dongchun Zhu wrote: > On Fri, 2020-10-23 at 17:31 +0300, Andy Shevchenko wrote: > > On Tue, Oct 13, 2020 at 09:05:03PM +0800, Dongchun Zhu wrote: > > > Add a V4L2 sub-device driver for OmniVision OV02A10 image sensor. ... > > > +#define OV02A10_ID_MASK 0xffff > > > > GENMASK() > > > > (And include bits.h for that) > > > > It seems could build pass without including bits.h, as DW9768 once used. The rule of thumb is to include all headers you have direct users for. Exceptions when you have indirect inclusion that guarantees to provide others (like bitmap.h implies bits.h, etc).