On Fri, Oct 27, 2023 at 03:45:30PM +0300, Laurent Pinchart wrote:
> On Fri, Oct 27, 2023 at 12:30:37PM +0000, Sakari Ailus wrote:
> > On Tue, Oct 17, 2023 at 04:21:03PM +0300, Laurent Pinchart wrote:
> > > From: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
> > >
> > > The THP7312 is an external camera ISP from THine. Add a V4L2 subdev
> > > driver for it.
> > >
> > > Signed-off-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
> > > Co-developed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > ---
> > > Changes since v3:
> > >
> > > - Move thp7312_get_regulators() to probe section
> > > - Turn firmware update handlers static
> > > - Wire up power management in struct driver
> > > - Remove unnecessary double underscore function prefixes
> > > - Configure CSI-2 lanes at stream on time
> > > - Clean up naming of power management functions
> > >
> > > Changes since v2:
> > >
> > > - Make boot-mode property optional
> > > - Fix dev_err_probe() usage in DT parsing
> > > - Additional dev_err_probe() usage
> > > - Use %u instead of %d for unsigned values
> > > - Don't split lines unnecessarily
> > > - Fix error handling in firmware upload initialization
> > > - Use CCI helpers in firmware update code
> > > - Fix runtime PM usage count
> > > ---
> > > MAINTAINERS | 1 +
> > > drivers/media/i2c/Kconfig | 16 +
> > > drivers/media/i2c/Makefile | 1 +
> > > drivers/media/i2c/thp7312.c | 2339 +++++++++++++++++++++++++++++++++++
> > > 4 files changed, 2357 insertions(+)
> > > create mode 100644 drivers/media/i2c/thp7312.c
[snip]
> > > diff --git a/drivers/media/i2c/thp7312.c b/drivers/media/i2c/thp7312.c
> > > new file mode 100644
> > > index 000000000000..7d3de929079d
> > > --- /dev/null
> > > +++ b/drivers/media/i2c/thp7312.c
> > > @@ -0,0 +1,2339 @@
[snip]
> > > +static int thp7312_sensor_init(struct thp7312_sensor *sensor, unsigned int index)
> > > +{
> > > + struct thp7312_device *thp7312 = sensor->thp7312;
> > > + struct device *dev = thp7312->dev;
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + sensor->index = index;
> > > +
> > > + /*
> > > + * Register a device for the sensor, to support usage of the regulator
> > > + * API.
> > > + */
> > > + sensor->dev = kzalloc(sizeof(*sensor->dev), GFP_KERNEL);
> > > + if (!sensor->dev)
> > > + return -ENOMEM;
> > > +
> > > + sensor->dev->parent = dev;
> > > + sensor->dev->of_node = of_node_get(sensor->of_node);
> >
> > This device could well find its way to a non-OF system. Could you use the
> > fwnode property API instead?
>
> I'm pretty sure there will be problems if someone was using this driver
> on an ACPI-based system, so trying to pretend it's supported without
> being able to test it may not be the best use of development time. I'll
> try, but if I hit any issue, I'll keep using the OF-specific functions
> in the next version.
>
> > > + sensor->dev->release = &thp7312_sensor_dev_release;
> > > + dev_set_name(sensor->dev, "%s-%s.%u", dev_name(dev),
> > > + thp7312->sensor_info->name, index);
> > > +
> > > + ret = device_register(sensor->dev);
> > > + if (ret < 0) {
> > > + dev_err(dev, "Failed to register device for sensor %u\n", index);
> > > + put_device(sensor->dev);
> > > + sensor->dev = NULL;
> > > + return ret;
> > > + }
> > > +
> > > + /* Retrieve the power supplies for the sensor, if any. */
> > > + if (thp7312->sensor_info->supplies) {
> > > + const struct thp7312_sensor_supply *supplies =
> > > + thp7312->sensor_info->supplies;
> > > + unsigned int num_supplies;
> > > +
> > > + for (num_supplies = 0; supplies[num_supplies].name; ++num_supplies)
> > > + ;
> > > +
> > > + sensor->supplies = devm_kcalloc(dev, num_supplies,
> > > + sizeof(*sensor->supplies),
> > > + GFP_KERNEL);
> > > + if (!sensor->supplies) {
> > > + ret = -ENOMEM;
> > > + goto error;
> > > + }
> > > +
> > > + for (i = 0; i < num_supplies; ++i)
> > > + sensor->supplies[i].supply = supplies[i].name;
> > > +
> > > + ret = regulator_bulk_get(sensor->dev, num_supplies,
> > > + sensor->supplies);
> > > + if (ret < 0) {
> > > + dev_err(dev, "Failed to get supplies for sensor %u\n", index);
> > > + goto error;
> > > + }
> > > +
> > > + sensor->num_supplies = i;
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +error:
> > > + device_unregister(sensor->dev);
> > > + return ret;
> > > +}
> > > +
> > > +static int thp7312_init_sensors(struct thp7312_device *thp7312)
> > > +{
> > > + unsigned int i;
> > > + int ret;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(thp7312->sensors); i++) {
> > > + struct thp7312_sensor *sensor = &thp7312->sensors[i];
> > > +
> > > + if (!sensor->thp7312)
> > > + continue;
> > > +
> > > + ret = thp7312_sensor_init(sensor, i);
> > > + if (ret < 0)
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void thp7312_sensor_cleanup(struct thp7312_sensor *sensor)
> > > +{
> > > + if (sensor->num_supplies)
> > > + regulator_bulk_free(sensor->num_supplies, sensor->supplies);
> > > +
> > > + if (sensor->dev)
> > > + device_unregister(sensor->dev);
> > > + of_node_put(sensor->of_node);
> > > +}
> > > +
> > > +static void thp7312_remove_sensors(struct thp7312_device *thp7312)
> > > +{
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(thp7312->sensors); i++) {
> > > + struct thp7312_sensor *sensor = &thp7312->sensors[i];
> > > +
> > > + if (!sensor->thp7312)
> > > + continue;
> > > +
> > > + thp7312_sensor_cleanup(sensor);
> > > + }
> > > +}
> > > +
> > > +static int thp7312_sensor_parse_dt(struct thp7312_device *thp7312,
> > > + struct device_node *node)
> > > +{
> > > + struct device *dev = thp7312->dev;
> > > + struct thp7312_sensor *sensor;
> > > + u32 data_lanes_rx[4];
> > > + const char *model;
> > > + unsigned int i;
> > > + u32 reg;
> > > + int ret;
> > > +
> > > + if (!of_device_is_available(node))
> > > + return -ENODEV;
> > > +
> > > + /* Retrieve the sensor index from the reg property. */
> > > + ret = of_property_read_u32(node, "reg", ®);
> > > + if (ret < 0) {
> > > + dev_err(dev, "'reg' property missing in sensor node\n");
> >
> > Shouldn't you assume it's zero instead?
>
> The property is mandatory.
>
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (reg >= ARRAY_SIZE(thp7312->sensors)) {
> > > + dev_err(dev, "Out-of-bounds 'reg' value %u\n", reg);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + sensor = &thp7312->sensors[reg];
> > > + if (sensor->thp7312) {
> > > + dev_err(dev, "Duplicate entry for sensor %u\n", reg);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = of_property_read_string(node, "thine,model", &model);
> > > + if (ret < 0) {
> > > + dev_err(dev, "'thine,model' property missing in sensor node\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(thp7312_sensor_info); i++) {
> > > + const struct thp7312_sensor_info *info =
> > > + &thp7312_sensor_info[i];
> > > +
> > > + if (!strcmp(info->model, model)) {
> > > + thp7312->sensor_info = info;
> > > + break;
> > > + }
> > > + }
> > > +
> > > + if (!thp7312->sensor_info) {
> > > + dev_err(dev, "Unsupported sensor model %s\n", model);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = of_property_read_u32_array(node, "data-lanes",
> > > + data_lanes_rx, ARRAY_SIZE(data_lanes_rx));
> > > + if (ret < 0) {
> > > + dev_err(dev, "Failed to read property data-lanes: %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(sensor->data_lanes); i++)
> > > + sensor->data_lanes[i] = (u8)data_lanes_rx[i];
> >
> > I don't think you need the cast here.
> >
> > > +
> > > + sensor->thp7312 = thp7312;
> > > + sensor->of_node = of_node_get(node);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int thp7312_parse_dt(struct thp7312_device *thp7312)
> > > +{
> > > + struct device *dev = thp7312->dev;
> > > + struct fwnode_handle *endpoint;
> > > + struct device_node *sensors;
> > > + unsigned int num_sensors = 0;
> > > + struct device_node *node;
> > > + int ret;
> > > +
> > > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> > > + if (!endpoint)
> > > + return dev_err_probe(dev, -EINVAL, "Endpoint node not found\n");
> > > +
> > > + ret = v4l2_fwnode_endpoint_parse(endpoint, &thp7312->ep);
> >
> > You should assign the bus_type before parsing. It is deprecated to guess
> > it --- there's no universal guarantee it'll be successful.
>
> As only CSI-2 is supported for now, I'll do so.
>
> > > + fwnode_handle_put(endpoint);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret, "Could not parse endpoint\n");
> > > +
> > > + if (thp7312->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > + return dev_err_probe(dev, -EINVAL, "Unsupported bus type %d\n",
> > > + thp7312->ep.bus_type);
> > > +
> > > + /*
> > > + * The thine,boot-mode property is optional and default to
> > > + * THP7312_BOOT_MODE_SPI_MASTER (1).
> > > + */
> > > + thp7312->boot_mode = THP7312_BOOT_MODE_SPI_MASTER;
> > > + ret = of_property_read_u32(dev->of_node, "thine,boot-mode",
> > > + &thp7312->boot_mode);
> > > + if (ret && ret != -EINVAL)
> > > + return dev_err_probe(dev, ret, "Property '%s' is invalid\n",
> > > + "thine,boot-mode");
> > > +
> > > + if (thp7312->boot_mode != THP7312_BOOT_MODE_2WIRE_SLAVE &&
> > > + thp7312->boot_mode != THP7312_BOOT_MODE_SPI_MASTER)
> > > + return dev_err_probe(dev, -EINVAL, "Invalid '%s' value %u\n",
> > > + "thine,boot-mode", thp7312->boot_mode);
> > > +
> > > + /* Sensors */
> > > + sensors = of_get_child_by_name(dev->of_node, "sensors");
> > > + if (!sensors) {
> > > + dev_err(dev, "'sensors' child node not found\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + for_each_child_of_node(sensors, node) {
> > > + if (of_node_name_eq(node, "sensor")) {
I couldn't find a fwnode equivalent to this, so I'll keep using the OF
API in the next version. If someone ever wants to use this device on a
non-OF system, they will have to implement support for it on top.
> > > + if (!thp7312_sensor_parse_dt(thp7312, node))
> > > + num_sensors++;
> > > + }
> > > + }
> > > +
> > > + of_node_put(sensors);
> > > +
> > > + if (!num_sensors) {
> > > + dev_err(dev, "No sensor found\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
[snip]
--
Regards,
Laurent Pinchart
