Re: [PATCH net-next v5 7/7] ptp: ocp: Add Time Card V9 I2C peripheral topology

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ptp: ocp: Add Time Card V9 I2C peripheral topology

This adds a new I2C profile describing the Time Card V9 board: a BME280
pressure sensor, a BNO055 IMU, and an IS32FL3207 LED controller at 0x37
with six RGB groups. A new helper, ptp_ocp_board_id_matches(), selects
the profile when the fixed-width board ID equals TIMECARD-V9 with 0x00 or
0xff padding.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 5ae50ad83bdac..5100278f164d3 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -643,6 +643,73 @@ static const struct ptp_ocp_i2c_profile ptp_ocp_r4006_profile = {
>  	.led_max_microamp = 8150,
>  };
>  
> +static const struct ptp_ocp_i2c_device ptp_ocp_v9_sensors[] = {
> +	{ "pressure@76", "bosch,bme280", "bme280", 1, 0x76 },
> +	{ "imu@29", "bosch,bno055", "bno055", 1, 0x29 },
> +};

[Severity: Low]
This isn't a bug, but the generated software node for pressure@76 does not
describe the supplies that the BME280 binding marks as required.

ptp_ocp_i2c_init_nodes() emits only two properties per sensor:

	if (sensor->compatible)
		topology->sensor_props[i][prop++] =
			PROPERTY_ENTRY_STRING("compatible",
					      sensor->compatible);
	topology->sensor_props[i][prop] =
		PROPERTY_ENTRY_U32("reg", sensor->address);

Documentation/devicetree/bindings/iio/pressure/bmp085.yaml lists
vddd-supply and vdda-supply in its required list, and bmp280_common_probe()
calls devm_regulator_bulk_get() for both.

To be clear, this does not break probing on the hosts this card plugs into:
devm_regulator_bulk_get() uses NORMAL_GET, and _regulator_get() substitutes
dummy_regulator_rdev whenever have_full_constraints() is true, which is the
case on ACPI systems because acpi_early_init() calls
regulator_has_full_constraints(). The visible effect is two dev_warn()
lines of the form "supply vddd not found, using dummy regulator", plus the
description mismatch against the binding. The preceding patch in this
series relies on the same behaviour for the R4006 invensense,icp10100 node.

Is relying on the dummy regulator here intentional, and would it be worth
saying so in the commit message?

[ ... ]

> @@ -2243,6 +2310,22 @@ ptp_ocp_i2c_supported(struct ptp_ocp *bp)
>  		bp->pdev->device == PCI_DEVICE_ID_CELESTICA_TIMECARD);
>  }
>  
> +static bool
> +ptp_ocp_board_id_matches(const u8 *board_id, const char *id)
> +{
> +	size_t len = strlen(id);
> +	unsigned int i;
> +
> +	if (len > OCP_BOARD_ID_LEN || memcmp(board_id, id, len))
> +		return false;
> +
> +	for (i = len; i < OCP_BOARD_ID_LEN; i++)
> +		if (board_id[i] != 0 && board_id[i] != 0xff)
> +			return false;
> +
> +	return true;
> +}

[Severity: Low]
This isn't a bug introduced by this patch, but the new helper documents
that 0xff padding really does appear in bp->board_id, which makes an
existing formatting inconsistency reachable.

ptp_ocp_devlink_info_get() formats the same field for userspace:

	snprintf(buf, sizeof(buf), "%.*s", OCP_BOARD_ID_LEN,
		 (const char *)bp->board_id);
	err = devlink_info_version_fixed_put(req, board_id_key, buf);

The "%.*s" conversion stops early only on a NUL byte, so for exactly the
boards targeted here, "TIMECARD-V9" followed by erased-flash 0xff bytes,
devlink publishes a DEVLINK_INFO_VERSION_GENERIC_BOARD_ID string with raw
0xff bytes appended. The write is bounded by OCP_BOARD_ID_LEN over a
u8[13] array, so this is presentational only, but the two consumers of the
field now disagree: one trims 0x00 and 0xff padding, the other forwards it
verbatim.

Would it make sense to trim the padding in a shared helper, or in the
devlink path, so both call sites agree on the encoding of board_id?