Hi Dan,
On 6/16/23 14:34, Dan Scally wrote:
> Hi Hans
>
> On 09/06/2023 21:42, Hans de Goede wrote:
>> Currently the only 2 sensor_config-s both specify "avdd" as supply-id.
>>
>> The INT3472 device is going to be the only supplier of a regulator for
>> the sensor device.
>>
>> So there is no chance of collisions with other regulator suppliers
>> and it is undesirable to need to manually add new entries to
>> int3472_sensor_configs[] for each new sensor module which uses
>> a GPIO regulator.
>>
>> Instead just always use "avdd" as supply-id when registering
>> the GPIO regulator.
>>
>> If necessary for specific sensor drivers then other supply-ids can
>> be added as aliases in the future, adding aliases will be safe
>> since INT3472 will be the only regulator supplier for the sensor.
>>
>> Cc: Hao Yao <hao.yao@xxxxxxxxx>
>> Cc: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> .../x86/intel/int3472/clk_and_regulator.c | 38 ++++++++++------
>> drivers/platform/x86/intel/int3472/common.h | 7 +--
>> drivers/platform/x86/intel/int3472/discrete.c | 45 +++----------------
>> 3 files changed, 31 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> index b3a55c618151..30686091300d 100644
>> --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c
>> @@ -232,32 +232,42 @@ void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472)
>> gpiod_put(int3472->clock.ena_gpio);
>> }
>> +/*
>> + * The INT3472 device is going to be the only supplier of a regulator for
>> + * the sensor device. But unlike the clk framework the regulator framework
>> + * does not allow matching by consumer-device-name only.
>> + *
>> + * Ideally all sensor drivers would use "avdd" as supply-id. But for drivers
>> + * where this cannot be changed because another supply-id is already used in
>> + * e.g. DeviceTree files an alias for the other supply-id can be added here.
>> + *
>> + * Do not forget to update GPIO_REGULATOR_SUPPLY_MAP_COUNT when changing this.
>> + */
>> +static const char * const skl_int3472_regulator_map_supplies[] = {
>> + "avdd",
>> +};
>> +
>> int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
>> struct acpi_resource_gpio *agpio)
>> {
>> - const struct int3472_sensor_config *sensor_config;
>> char *path = agpio->resource_source.string_ptr;
>> - struct regulator_consumer_supply supply_map;
>> struct regulator_init_data init_data = { };
>> struct regulator_config cfg = { };
>> - int ret;
>> + int i, ret;
>> - sensor_config = int3472->sensor_config;
>> - if (IS_ERR(sensor_config)) {
>> - dev_err(int3472->dev, "No sensor module config\n");
>> - return PTR_ERR(sensor_config);
>> + if (ARRAY_SIZE(skl_int3472_regulator_map_supplies) != GPIO_REGULATOR_SUPPLY_MAP_COUNT) {
>> + dev_err(int3472->dev, "Internal error ARRAY_SIZE(skl_int3472_regulator_map_supplies) != GPIO_REGULATOR_SUPPLY_MAP_COUNT\n");
>> + return -EINVAL;
>
>
> It would be nice to be able to prevent this mismatch somehow so it's never a problem; can we use static_assert() perhaps? Or at least less of a problem, as I gather that gets compiled to a no-op sometimes.
Using static_assert() in the header file indeed seems to be better. I'll prep a v2 with this (and drop the v1 from my review-hans branch).
>> }
>> - if (!sensor_config->supply_map.supply) {
>> - dev_err(int3472->dev, "No supply name defined\n");
>> - return -ENODEV;
>> + for (i = 0; i < ARRAY_SIZE(skl_int3472_regulator_map_supplies); i++) {
>> + int3472->regulator.supply_map[i].supply = skl_int3472_regulator_map_supplies[i];
>> + int3472->regulator.supply_map[i].dev_name = int3472->sensor_name;
>> }
>> init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
>> - init_data.num_consumer_supplies = 1;
>> - supply_map = sensor_config->supply_map;
>> - supply_map.dev_name = int3472->sensor_name;
>> - init_data.consumer_supplies = &supply_map;
>> + init_data.consumer_supplies = int3472->regulator.supply_map;
>> + init_data.num_consumer_supplies = GPIO_REGULATOR_SUPPLY_MAP_COUNT;
>> snprintf(int3472->regulator.regulator_name,
>> sizeof(int3472->regulator.regulator_name), "%s-regulator",
>> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h
>> index 735567f374a6..225b067c854d 100644
>> --- a/drivers/platform/x86/intel/int3472/common.h
>> +++ b/drivers/platform/x86/intel/int3472/common.h
>> @@ -28,6 +28,7 @@
>> #define GPIO_REGULATOR_NAME_LENGTH 21
>> #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9
>> +#define GPIO_REGULATOR_SUPPLY_MAP_COUNT 1
>> #define INT3472_LED_MAX_NAME_LEN 32
>> @@ -69,11 +70,6 @@ struct int3472_cldb {
>> u8 reserved2[17];
>> };
>> -struct int3472_sensor_config {
>> - const char *sensor_module_name;
>> - struct regulator_consumer_supply supply_map;
>> -};
>> -
>> struct int3472_discrete_device {
>> struct acpi_device *adev;
>> struct device *dev;
>> @@ -83,6 +79,7 @@ struct int3472_discrete_device {
>> const struct int3472_sensor_config *sensor_config;
>> struct int3472_gpio_regulator {
>> + struct regulator_consumer_supply supply_map[GPIO_REGULATOR_SUPPLY_MAP_COUNT];
>> char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
>> char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
>> struct gpio_desc *gpio;
>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
>> index 2ab3c7466986..3b410428cec2 100644
>> --- a/drivers/platform/x86/intel/int3472/discrete.c
>> +++ b/drivers/platform/x86/intel/int3472/discrete.c
>> @@ -34,48 +34,17 @@ static const guid_t cio2_sensor_module_guid =
>> GUID_INIT(0x822ace8f, 0x2814, 0x4174,
>> 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
>> -/*
>> - * Here follows platform specific mapping information that we can pass to
>> - * the functions mapping resources to the sensors. Where the sensors have
>> - * a power enable pin defined in DSDT we need to provide a supply name so
>> - * the sensor drivers can find the regulator. The device name will be derived
>> - * from the sensor's ACPI device within the code.
>> - */
>> -static const struct int3472_sensor_config int3472_sensor_configs[] = {
>> - /* Lenovo Miix 510-12ISK - OV5648, Rear */
>> - { "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL) },
>> - /* Surface Go 1&2 - OV5693, Front */
>> - { "YHCU", REGULATOR_SUPPLY("avdd", NULL) },
>> -};
>> -
>> -static const struct int3472_sensor_config *
>> -skl_int3472_get_sensor_module_config(struct int3472_discrete_device *int3472)
>> +static void skl_int3472_log_sensor_module_name(struct int3472_discrete_device *int3472)
>
>
> I don't really think this is worth logging if we're removing the matching based on it - we can get it from the DSDT anyway if we need to.
The DSDTs on these devices often read this from some memory location
rather then specifying it directly in the DSDT, so the only way to
get this often is to actually call the DSM.
Note this is logged with a dev_dbg, so normally users won't see this,
but I think this may be handy for debugging sometimes.
Regards,
Hans
