Hi Saikari, On 21-Jan-25 9:08 AM, Sakari Ailus wrote: > Hi Hans, > > On Mon, Jan 20, 2025 at 04:08:45PM +0100, Hans de Goede wrote: >> 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". > > Possibly in this instance, but whether it is or not is still besides the > point: this may not be the name on the datasheet. > >> >>> 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. > > The power resources do contain GPIO handling when it is required for > implementing the device's power on / off sequences. FWIW, on Chromebooks > this "just works" without the IPU bridge and without . > >> >>> 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. > > In our case here there's really no difference DMI-wise, is there? In both > cases the GPIO handling is determined based on the device. Besides, my > patch is technically better in the sense that it is explicitly using an > existing firmware interface instead of adding driver support for a firmware > interface that doesn't exist (neither as a firmware interface definition > nor an implementation of one). > >> >> See e.g.: >> >> https://github.com/jwrdegoede/linux-sunxi/commit/e43be8f19b5913a2e4bd715e7eef88fd909a2d1d > > I guess we don't have DT bindings for a (virtual) device that would allow > controlling one or more actual GPIOs using a single virtual one? > >> >> (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. > > Again, I don't want to add DMI checks in client drivers if that can > be reasonably handled elsewhere. Ok, good that was my main concern. <snip> As discussed on IRC lets move forward with doing the mapping in the INT3472 as the patch from this thread does. But I do want to see the mapping code be a bit more generic, I'll reply to the original patch with a suggestion on how to do that. Regards, Hans
