| Message ID | 20230321153718.1355511-2-hpa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | leds: tps68470: LED driver for TPS68470 | expand |
Hi, On 3/21/23 16:37, Kate Hsuan wrote: > Add MFD cell for tps68470-led. > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > Signed-off-by: Kate Hsuan <hpa@redhat.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/platform/x86/intel/int3472/tps68470.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c > index 5b8d1a9620a5..82ef022f8916 100644 > --- a/drivers/platform/x86/intel/int3472/tps68470.c > +++ b/drivers/platform/x86/intel/int3472/tps68470.c > @@ -17,7 +17,7 @@ > #define DESIGNED_FOR_CHROMEOS 1 > #define DESIGNED_FOR_WINDOWS 2 > > -#define TPS68470_WIN_MFD_CELL_COUNT 3 > +#define TPS68470_WIN_MFD_CELL_COUNT 4 > > static const struct mfd_cell tps68470_cros[] = { > { .name = "tps68470-gpio" }, > @@ -193,7 +193,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client) > cells[1].name = "tps68470-regulator"; > cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; > cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data); > - cells[2].name = "tps68470-gpio"; > + cells[2].name = "tps68470-led"; > + cells[3].name = "tps68470-gpio"; > > for (i = 0; i < board_data->n_gpiod_lookups; i++) > gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
On Tue, 21 Mar 2023, Kate Hsuan wrote: > Add MFD cell for tps68470-led. > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > Signed-off-by: Kate Hsuan <hpa@redhat.com> > --- > drivers/platform/x86/intel/int3472/tps68470.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c > index 5b8d1a9620a5..82ef022f8916 100644 > --- a/drivers/platform/x86/intel/int3472/tps68470.c > +++ b/drivers/platform/x86/intel/int3472/tps68470.c > @@ -17,7 +17,7 @@ > #define DESIGNED_FOR_CHROMEOS 1 > #define DESIGNED_FOR_WINDOWS 2 > > -#define TPS68470_WIN_MFD_CELL_COUNT 3 > +#define TPS68470_WIN_MFD_CELL_COUNT 4 > > static const struct mfd_cell tps68470_cros[] = { > { .name = "tps68470-gpio" }, > @@ -193,7 +193,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client) > cells[1].name = "tps68470-regulator"; > cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; > cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data); > - cells[2].name = "tps68470-gpio"; > + cells[2].name = "tps68470-led"; > + cells[3].name = "tps68470-gpio"; The question is, why is the MFD API being used out side of drivers/mfd? > for (i = 0; i < board_data->n_gpiod_lookups; i++) > gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]); > -- > 2.39.2 > -- Lee Jones [李琼斯]
Hi, On 3/23/23 13:23, Lee Jones wrote: > On Tue, 21 Mar 2023, Kate Hsuan wrote: > >> Add MFD cell for tps68470-led. >> >> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> >> Signed-off-by: Kate Hsuan <hpa@redhat.com> >> --- >> drivers/platform/x86/intel/int3472/tps68470.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c >> index 5b8d1a9620a5..82ef022f8916 100644 >> --- a/drivers/platform/x86/intel/int3472/tps68470.c >> +++ b/drivers/platform/x86/intel/int3472/tps68470.c >> @@ -17,7 +17,7 @@ >> #define DESIGNED_FOR_CHROMEOS 1 >> #define DESIGNED_FOR_WINDOWS 2 >> >> -#define TPS68470_WIN_MFD_CELL_COUNT 3 >> +#define TPS68470_WIN_MFD_CELL_COUNT 4 >> >> static const struct mfd_cell tps68470_cros[] = { >> { .name = "tps68470-gpio" }, >> @@ -193,7 +193,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client) >> cells[1].name = "tps68470-regulator"; >> cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; >> cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data); >> - cells[2].name = "tps68470-gpio"; >> + cells[2].name = "tps68470-led"; >> + cells[3].name = "tps68470-gpio"; > > The question is, why is the MFD API being used out side of drivers/mfd? Because Intel made a big mess about how they describe camera sensors + the matching clks / regulators / GPIOs and the optional PMIC in ACPI. The drivers/platform/x86/intel/int3472/ code untangles this mess and in some cases it instantiates MFD cells (with a whole bunch of derived platform_data per cell) for a TPS68470 PMIC. And sometimes while binding to an INT3472 ACPI device-node it does not instantiate any MFD cells at all since the INT3472 ACPI device-node does not always describe such a PMIC. Oh and also depending on of the ACPI tables are targetting ChromeOS or Windows a different set of MFD cells needs to be instantiated. On ChromeOS most of the PMIC poking is done through ACPI through a ChomeOS specific custom ACPI OpRegion, so there there are only cells for GPIO and a driver providing the OpRegion are created. So lots of ugly x86 platform specific handling, ACPI parsing, etc. which is why this landed under drivers/platform/x86/ . IIRC you were even involved in the original merge since there once was a much simpler MFD driver under driver/mfd which only supported the ChromeOS setup. (but my memory may be deceiving me here). Regards, Hans > >> for (i = 0; i < board_data->n_gpiod_lookups; i++) >> gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]); >> -- >> 2.39.2 >> > > -- > Lee Jones [李琼斯] >
On Thu, 23 Mar 2023, Hans de Goede wrote: > Hi, > > On 3/23/23 13:23, Lee Jones wrote: > > On Tue, 21 Mar 2023, Kate Hsuan wrote: > > > >> Add MFD cell for tps68470-led. > >> > >> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > >> Signed-off-by: Kate Hsuan <hpa@redhat.com> > >> --- > >> drivers/platform/x86/intel/int3472/tps68470.c | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c > >> index 5b8d1a9620a5..82ef022f8916 100644 > >> --- a/drivers/platform/x86/intel/int3472/tps68470.c > >> +++ b/drivers/platform/x86/intel/int3472/tps68470.c > >> @@ -17,7 +17,7 @@ > >> #define DESIGNED_FOR_CHROMEOS 1 > >> #define DESIGNED_FOR_WINDOWS 2 > >> > >> -#define TPS68470_WIN_MFD_CELL_COUNT 3 > >> +#define TPS68470_WIN_MFD_CELL_COUNT 4 > >> > >> static const struct mfd_cell tps68470_cros[] = { > >> { .name = "tps68470-gpio" }, > >> @@ -193,7 +193,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client) > >> cells[1].name = "tps68470-regulator"; > >> cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; > >> cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data); > >> - cells[2].name = "tps68470-gpio"; > >> + cells[2].name = "tps68470-led"; > >> + cells[3].name = "tps68470-gpio"; > > > > The question is, why is the MFD API being used out side of drivers/mfd? > > Because Intel made a big mess about how they describe camera sensors + the matching clks / regulators / GPIOs and the optional PMIC in ACPI. > > The drivers/platform/x86/intel/int3472/ code untangles this mess and in some cases it instantiates MFD cells (with a whole bunch of derived platform_data per cell) for a TPS68470 PMIC. > > And sometimes while binding to an INT3472 ACPI device-node it does not instantiate any MFD cells at all since the INT3472 ACPI device-node does not always describe such a PMIC. > > Oh and also depending on of the ACPI tables are targetting ChromeOS or Windows a different set of MFD cells needs to be instantiated. On ChromeOS most of the PMIC poking is done through ACPI through a ChomeOS specific custom ACPI OpRegion, so there there are only cells for GPIO and a driver providing the OpRegion are created. > > So lots of ugly x86 platform specific handling, ACPI parsing, etc. which is why this landed under drivers/platform/x86/ . IIRC you were even involved in the original merge since there once was a much simpler MFD driver under driver/mfd which only supported the ChromeOS setup. > > (but my memory may be deceiving me here). Right, I guess we've both slept since then! My normal request is that MFD handling should be in drivers/mfd. Anything else can be farmed out to the various functional subsystems and drivers/platform. -- Lee Jones [李琼斯]
diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c index 5b8d1a9620a5..82ef022f8916 100644 --- a/drivers/platform/x86/intel/int3472/tps68470.c +++ b/drivers/platform/x86/intel/int3472/tps68470.c @@ -17,7 +17,7 @@ #define DESIGNED_FOR_CHROMEOS 1 #define DESIGNED_FOR_WINDOWS 2 -#define TPS68470_WIN_MFD_CELL_COUNT 3 +#define TPS68470_WIN_MFD_CELL_COUNT 4 static const struct mfd_cell tps68470_cros[] = { { .name = "tps68470-gpio" }, @@ -193,7 +193,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client) cells[1].name = "tps68470-regulator"; cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata; cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data); - cells[2].name = "tps68470-gpio"; + cells[2].name = "tps68470-led"; + cells[3].name = "tps68470-gpio"; for (i = 0; i < board_data->n_gpiod_lookups; i++) gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);