| Message ID | 20250507184737.154747-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | platform/x86: int3472: Allow re-using sensor GPIO mapping in atomisp | expand |
On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote: > This patch series does some small refactoring of the int3472 code to allow > re-using the sensor GPIO mapping code in the atomisp driver and then the > final patch in the series moves the atomisp driver over. > > About merging this, maybe the int3472 patches can be merged in time for > the 6.16 merge window and then the atomisp patch can be merged after > 6.16-rc1 is released, otherwise an immutable pdx86 branch with the first > 5 patches will be necessary. Overall I'm pretty much liking this series, but one comment against the last patch (see there) and one question here. Can you isolate GPIO mapping code in a separate file, please? This will help to generalise this code outside of two mentioned drivers (I might need it in the future for something else, not related to cameras at all).
On Thu, May 8, 2025 at 11:36 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > This patch series does some small refactoring of the int3472 code to allow > > re-using the sensor GPIO mapping code in the atomisp driver and then the > > final patch in the series moves the atomisp driver over. > > > > About merging this, maybe the int3472 patches can be merged in time for > > the 6.16 merge window and then the atomisp patch can be merged after > > 6.16-rc1 is released, otherwise an immutable pdx86 branch with the first > > 5 patches will be necessary. > > Overall I'm pretty much liking this series, but one comment against > the last patch (see there) and one question here. Can you isolate GPIO > mapping code in a separate file, please? This will help to generalise > this code outside of two mentioned drivers (I might need it in the > future for something else, not related to cameras at all). Ah, and formal Reviewed-by: Andy Shevchenko <andy@kernel.org> for patches 1-5.
On Wed, May 07, 2025 at 08:47:31PM +0200, Hans de Goede wrote: > Hi All, > > This patch series does some small refactoring of the int3472 code to allow > re-using the sensor GPIO mapping code in the atomisp driver and then the > final patch in the series moves the atomisp driver over. > > About merging this, maybe the int3472 patches can be merged in time for > the 6.16 merge window and then the atomisp patch can be merged after > 6.16-rc1 is released, otherwise an immutable pdx86 branch with the first > 5 patches will be necessary. Thanks, Hans! For the first five: Reviwed-by: Sakari Ailus <sakari.ailus@linux.intel.com> Feel free to add to the last patch (i haven't looked at the details, but the approach seems good): Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Hi, On 8-May-25 10:37 AM, Andy Shevchenko wrote: > On Thu, May 8, 2025 at 11:36 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >>> This patch series does some small refactoring of the int3472 code to allow >>> re-using the sensor GPIO mapping code in the atomisp driver and then the >>> final patch in the series moves the atomisp driver over. >>> >>> About merging this, maybe the int3472 patches can be merged in time for >>> the 6.16 merge window and then the atomisp patch can be merged after >>> 6.16-rc1 is released, otherwise an immutable pdx86 branch with the first >>> 5 patches will be necessary. >> >> Overall I'm pretty much liking this series, but one comment against >> the last patch (see there) and one question here. Can you isolate GPIO >> mapping code in a separate file, please? This will help to generalise >> this code outside of two mentioned drivers (I might need it in the >> future for something else, not related to cameras at all). > > Ah, and formal > Reviewed-by: Andy Shevchenko <andy@kernel.org> > for patches 1-5. Andy, Sakari, Thank you both for your reviews of patches 1-5. Ilpo, since patches 1-5 have 2 Reviewed-by-s now, can we maybe still get those in before the 6.16 merge window ? That would make merging patch 6/6 a bit easier, avoiding the need to have an immutable branch for the int3472 bits patch 6/6 depends on. Regards, Hans
Hi Andy, On 8-May-25 10:36 AM, Andy Shevchenko wrote: > On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> This patch series does some small refactoring of the int3472 code to allow >> re-using the sensor GPIO mapping code in the atomisp driver and then the >> final patch in the series moves the atomisp driver over. >> >> About merging this, maybe the int3472 patches can be merged in time for >> the 6.16 merge window and then the atomisp patch can be merged after >> 6.16-rc1 is released, otherwise an immutable pdx86 branch with the first >> 5 patches will be necessary. > > Overall I'm pretty much liking this series, but one comment against > the last patch (see there) and one question here. Can you isolate GPIO > mapping code in a separate file, please? This will help to generalise > this code outside of two mentioned drivers (I might need it in the > future for something else, not related to cameras at all). If you want to re-use this elsewhere then splitting it out further sounds like a good plan. But which bits do you need? Do you actually need the full code calling the special DSM and then adding either GPIO-lookups, or gpio controlled regulators / clks / LEDs ? Because atm the int3472/discrete.c + other c files linked into the .ko does all of that and for the atomisp2 case we actually want all of that (although for now GPIO -> clk and LED is unused there). Anyway I think it would be best for you (Andy) to come up with a proposal / RFC patch series to split out what you need. I'm certainly open to that and happy to review such a series. Regards, Hans
On Thu, May 8, 2025 at 5:00 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 8-May-25 10:36 AM, Andy Shevchenko wrote: > > On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote: ... > > Can you isolate GPIO > > mapping code in a separate file, please? This will help to generalise > > this code outside of two mentioned drivers (I might need it in the > > future for something else, not related to cameras at all). > > If you want to re-use this elsewhere then splitting it out > further sounds like a good plan. > > But which bits do you need? Do you actually need the full code calling > the special DSM and then adding either GPIO-lookups, or gpio controlled > regulators / clks / LEDs ? > > Because atm the int3472/discrete.c + other c files linked into the .ko > does all of that and for the atomisp2 case we actually want all of > that (although for now GPIO -> clk and LED is unused there). I basically want to have remap\ing quirk part to be isolated. > Anyway I think it would be best for you (Andy) to come up with > a proposal / RFC patch series to split out what you need. I'm certainly > open to that and happy to review such a series. Fine, but can you do this in the discrete.c internally, like an embedded C file so it doesn't require headers to be used, but being an interface-ready solution?
On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote: > This patch series does some small refactoring of the int3472 code to allow > re-using the sensor GPIO mapping code in the atomisp driver and then the > final patch in the series moves the atomisp driver over. I just realised that the AtomISP variant is very likely a predecessor of INT3472, and hence a lot of code has to be shared between two. Or even having INT3472 being enumerated as a pure platform driver for the case of AtomISP.
Hi, On 8-May-25 4:55 PM, Andy Shevchenko wrote: > On Wed, May 7, 2025 at 9:52 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> This patch series does some small refactoring of the int3472 code to allow >> re-using the sensor GPIO mapping code in the atomisp driver and then the >> final patch in the series moves the atomisp driver over. > > I just realised that the AtomISP variant is very likely a predecessor > of INT3472, and hence a lot of code has to be shared between two. Yes at least the sensor-module identification _DSM and the GPIO-type/map _DSM have the same GUID. So the INT3472 device is clearly derived from the atomisp case, The weird thing is that the atomisp got a bunch of things more correct from an ACPI modelling pov. The _DSM and the GPIO resources are on the sensor ACPI-dev rather then a separate dev and regulators on the PMIC are simply handled through ACPI power-resources rather then having the OS have to figure all the PMIC stuff out like on IPU3 and IPU6 models with a separate camera PMIC. Regards, Hans
Hi All, This patch series does some small refactoring of the int3472 code to allow re-using the sensor GPIO mapping code in the atomisp driver and then the final patch in the series moves the atomisp driver over. About merging this, maybe the int3472 patches can be merged in time for the 6.16 merge window and then the atomisp patch can be merged after 6.16-rc1 is released, otherwise an immutable pdx86 branch with the first 5 patches will be necessary. Regards, Hans Hans de Goede (6): platform/x86: int3472: Move common.h to public includes, symbols to INTEL_INT3472 platform/x86: int3472: Stop using devm_gpiod_get() platform/x86: int3472: Export int3472_discrete_parse_crs() platform/x86: int3472: Remove unused sensor_config struct member platform/x86: int3472: For mt9m114 sensors map powerdown to powerenable media: atomisp: Switch to int3472 driver sensor GPIO mapping code MAINTAINERS | 1 + .../x86/intel/int3472/clk_and_regulator.c | 9 +- drivers/platform/x86/intel/int3472/common.c | 9 +- drivers/platform/x86/intel/int3472/discrete.c | 28 +- .../x86/intel/int3472/discrete_quirks.c | 3 +- drivers/platform/x86/intel/int3472/led.c | 3 +- drivers/platform/x86/intel/int3472/tps68470.c | 3 +- .../staging/media/atomisp/pci/atomisp_csi2.h | 17 -- .../media/atomisp/pci/atomisp_csi2_bridge.c | 239 ++---------------- .../staging/media/atomisp/pci/atomisp_v4l2.c | 1 + .../linux/platform_data/x86/int3472.h | 16 +- 11 files changed, 76 insertions(+), 253 deletions(-) rename drivers/platform/x86/intel/int3472/common.h => include/linux/platform_data/x86/int3472.h (92%)