Hi Sakari, On 20-Jan-25 3:21 PM, Sakari Ailus wrote: > Hi Hans, > > On Mon, Jan 20, 2025 at 02:39:39PM +0100, Hans de Goede wrote: >> Hi Sakari, >> >> On 20-Jan-25 11:17 AM, Sakari Ailus wrote: >>> The DT bindings for ov7251 specify "enable" GPIO (xshutdown in >>> documentation) but the int3472 indiscriminately provides this as a "reset" >>> GPIO to sensor drivers. Take this into account by assigning it as "enable" >>> with active high polarity for INT347E devices, i.e. ov7251. >>> >>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> >> >> Sorry but no this won't fly. If we go this route the amount of >> quirk code in the int3472 driver is going to get out of control. >> >> Since you are matching this mapping on the sensor-type, this >> should be handled in sensor specific code, IOW in the sensor driver. >> >> Also see my reply on the linux-media list here: >> >> https://lore.kernel.org/linux-media/0a0b765e-b342-4433-9c6c-07da78f0f63b@xxxxxxxxxx/ >> >> Sorry but I have to hard nack this. There has to be some line >> where the pdx86 glue code stops bending over backwards and >> where some of the burden of supporting more then just devicetree >> moves to the sensor drivers. >> >> *especially* since this mapping is going to be different per sensor-driver, >> with there being *no consistency at all* in the GPIO/pin names used in >> the sensor drivers just looking at drivers/media/i2c/ov*.c I see all of: > > I beg you to reconsider as I have to disagree with your assessment, for the > following reasons: > > The "reset" naming used by the int3472 driver comes from the int3472 driver > / Windows specific ACPI amendments, not from the ACPI standard nor sensor > datasheets. Yet looking at the datasheet it is a more correct name then "enable". > Also in a proper ACPI implementation we wouldn't be dealing > with such GPIOs at all, instead this would simply work through ACPI power > resources. GPIOs being handles as ACPI power-resources is not something which is typically done. This was actually done correct for the atomisp devices, clks and regulators are power-resources there, but the GPIOs are listed as plain ACPI GPIO resources under the sensor ACPI-fwnode. And ACPI GPIO resources do not have a name/label only an index. So drivers which need GPIOs and want to work on ACPI platforms need an index -> label map, see for example: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen/goodix.c#n811 note how this is handled in the driver, and is not expected to be handled by some platform specific code. > There generally isn't a single datasheet name used for such signals across > sensor vendors (or even sensors from a single vendor). Thus the assumption > made in the int3472 driver isn't correct, even if DT bindings were using > the vendor-provided GPIO signal name from datasheets as-is. > > We've done quite a bit of work to make the system firmware (including > design differences or outright bugs) invisible to the drivers elsewhere, I > don't see why we should make an exception here. To add to that, mapping the > GPIO name / function to what the driver expects is trivially done in the > int3472 driver, as this patch shows. IMHO it is not so trivial done, you are assuming a 1:1 mapping for all laptop/tablet models this is not necessarily true. E.g. on atomisp tablets there typically is a standard camera connector with both reset and powerdown signals and the atomisp _DSM equivalent of the INT3472 GPIO map _DSM typically contains both signals. But on sensors with only 1 reset pin it is unclear which of the 2 is actually used. One model camera module with sensor X may connect the sensor's single xshutdown pin to the powerdown signal on the standard camera connector, while another camera module may use the reset signal on the standard connector. There are 2 ways to handle this: 1. Make the driver deal with the fact that there may be 2 GPIOs, treating both as optional and driving both low/high at the same time since only 1 will actually be used. This is somewhat ugly on the driver side, but then fixes things for all tablets/laptops out there using this sensor model. 2. Make this the platform glue's problem and require the platform code to have per laptop/tablet model quirks using DMI matching meaning that instead of things just working OOTB for models not added to the quirk table, we now need users to report an issue and then manually fix that for every model under the sun. Which is very much sub optimal. See e.g.: https://github.com/jwrdegoede/linux-sunxi/commit/e43be8f19b5913a2e4bd715e7eef88fd909a2d1d (which I have not posted upstream yet since I don't have the mt9m114 driver working on atomisp models yet) I foresee similar problems with the INT3472 stuff. On Andy's device we need to map reset to enable, but maybe next time it is powerdown ? Multiply these kinda per laptop/tablet model issues by the amount of different sensors which there are and this becomes a big mess of per-sensor + per-laptop-model quirks in the int3472 code, where as these things can typically be handled reasonable well inside the sensor driver. > The naming used in Devicetree bindings is an ABI, we cannot change that. Right and Andy's patch does not try to change, nor break the ABI, Andy's patch merely adds a fallback to look for a "reset" pin if there is no "enable" pin which is nice and simple and clean and really not much of a burden to carry inside the sensor driver. Drivers outside of drivers/media/i2c have much bigger kludges to deal with interfacing with ACPI tables. > For drivers it's an authoritative reference, even if the naming is not > aligned with hardware datasheets. DT binding authors are within their > rights in naming things differently form datasheets, too. I object drivers > having to assume GPIO naming imposed by the int3472 driver when it > conflicts with the naming (and functionality) used in DT bindings. Camera > sensors are a bit special as they require less trivial resources (GPIOs, > regulators and clocks) than most other devices while they are similarly > used in both DT and ACPI based systems. > >> >> "enable" >> "powerdown" >> "pwdn" >> "reset" >> "resetb" >> "Camera power" >> "Camera reset" >> >> being used. Given this total lack of consistent pin naming this really >> needs to be fixed in the sensor drivers. > > FWIW, the int3472 driver uses "power-enable" for a GPIO that powers on a > device. Do you expect drivers to support that as-is? Currently the int3472 > driver appears to be the only one using that string in the kernel. Finding that pin actually results in the INT3472 code registering an avdd regulator, not a GPIO lookup, in this case the "power-enable" label is something purely internal to the INT3472 code. This is all part of the INT3472 code already bending itself over backwards to make things "invisible to the drivers" but IMHO we need to draw the line somewhere. For me the main reason for saying no to this is that it is a per sensor thing and we already have a place to handle per sensor things and that is in the sensor driver. I've not seen any convincing arguments from you why this sensor specific handling does not belong in the sensor specific driver code. Regards, Hans >>> --- >>> since v1: >>> >>> - Fixed device name string. >>> >>> drivers/platform/x86/intel/int3472/discrete.c | 45 ++++++++++++++++--- >>> 1 file changed, 40 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >>> index d881b2cfcdfc..6404ef1eb4a7 100644 >>> --- a/drivers/platform/x86/intel/int3472/discrete.c >>> +++ b/drivers/platform/x86/intel/int3472/discrete.c >>> @@ -122,13 +122,47 @@ skl_int3472_gpiod_get_from_temp_lookup(struct int3472_discrete_device *int3472, >>> return desc; >>> } >>> >>> -static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity) >>> +/** >>> + * struct int3472_reset_gpio_map - Map "reset" GPIO to whatever is expected by >>> + * the sensor driver (as in DT bindings) >>> + * @devname: The name of the device without the instance number e.g. i2c-INT347E >>> + * @func: The function, e.g. "enable" >>> + * @polarity: GPIO_ACTIVE_{HIGH,LOW} >>> + */ >>> +static const struct int3472_reset_gpio_map { >>> + const char *devname; >>> + const char *func; >>> + unsigned int polarity; >>> +} int3472_reset_gpio_map[] = { >>> + { "i2c-INT347E", "enable", GPIO_ACTIVE_HIGH }, >>> +}; >>> + >>> +static void int3472_get_func_and_polarity(const char *sensor_name, u8 type, >>> + const char **func, u32 *polarity) >>> { >>> switch (type) { >>> - case INT3472_GPIO_TYPE_RESET: >>> - *func = "reset"; >>> - *polarity = GPIO_ACTIVE_LOW; >>> + case INT3472_GPIO_TYPE_RESET: { >>> + const struct int3472_reset_gpio_map *map = NULL; >>> + unsigned int i; >>> + >>> + for (i = 0; i < ARRAY_SIZE(int3472_reset_gpio_map); i++) { >>> + if (strncmp(sensor_name, int3472_reset_gpio_map[i].devname, >>> + strlen(int3472_reset_gpio_map[i].devname))) >>> + continue; >>> + >>> + map = &int3472_reset_gpio_map[i]; >>> + break; >>> + } >>> + >>> + if (map) { >>> + *func = map->func; >>> + *polarity = map->polarity; >>> + } else { >>> + *func = "reset"; >>> + *polarity = GPIO_ACTIVE_LOW; >>> + } >>> break; >>> + } >>> case INT3472_GPIO_TYPE_POWERDOWN: >>> *func = "powerdown"; >>> *polarity = GPIO_ACTIVE_LOW; >>> @@ -217,7 +251,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >>> >>> type = FIELD_GET(INT3472_GPIO_DSM_TYPE, obj->integer.value); >>> >>> - int3472_get_func_and_polarity(type, &func, &polarity); >>> + int3472_get_func_and_polarity(int3472->sensor_name, type, &func, >>> + &polarity); >>> >>> pin = FIELD_GET(INT3472_GPIO_DSM_PIN, obj->integer.value); >>> if (pin != agpio->pin_table[0]) >> >
