Re: [PATCH 1/2] platform/x86: x86-android-tablets: Add support for more then 1 gpio_key

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 5/8/23 09:38, Ilpo Järvinen wrote:
> On Fri, 5 May 2023, Hans de Goede wrote:
> 
>> Modify the gpio_keys support in x86_android_tablet_init() for
>> tablets which have more then 1 key/button which needs to be handled
>> by the gpio_keys driver.
>>
>> This requires copying over the struct gpio_keys_button from
>> the x86_gpio_button struct array to a new gpio_keys_button struct array,
>> as an added benefit this allows marking the per model x86_gpio_button
>> arrays __initconst so that they all can be freed after module init().
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  .../platform/x86/x86-android-tablets/asus.c   |  4 ++-
>>  .../platform/x86/x86-android-tablets/core.c   | 33 ++++++++++++-------
>>  .../platform/x86/x86-android-tablets/lenovo.c |  6 ++--
>>  .../platform/x86/x86-android-tablets/other.c  |  6 ++--
>>  .../x86-android-tablets/x86-android-tablets.h |  3 +-
>>  5 files changed, 35 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/platform/x86/x86-android-tablets/asus.c b/drivers/platform/x86/x86-android-tablets/asus.c
>> index 2aca91678219..f9c4083be86d 100644
>> --- a/drivers/platform/x86/x86-android-tablets/asus.c
>> +++ b/drivers/platform/x86/x86-android-tablets/asus.c
>> @@ -24,7 +24,7 @@ static struct gpiod_lookup_table int3496_gpo2_pin22_gpios = {
>>  	},
>>  };
>>  
>> -static struct x86_gpio_button asus_me176c_tf103c_lid = {
>> +static const struct x86_gpio_button asus_me176c_tf103c_lid __initconst = {
>>  	.button = {
>>  		.code = SW_LID,
>>  		.active_low = true,
>> @@ -175,6 +175,7 @@ const struct x86_dev_info asus_me176c_info __initconst = {
>>  	.serdev_info = asus_me176c_serdevs,
>>  	.serdev_count = ARRAY_SIZE(asus_me176c_serdevs),
>>  	.gpio_button = &asus_me176c_tf103c_lid,
>> +	.gpio_button_count = 1,
>>  	.gpiod_lookup_tables = asus_me176c_gpios,
>>  	.bat_swnode = &generic_lipo_hv_4v35_battery_node,
>>  	.modules = bq24190_modules,
>> @@ -317,6 +318,7 @@ const struct x86_dev_info asus_tf103c_info __initconst = {
>>  	.pdev_info = int3496_pdevs,
>>  	.pdev_count = 1,
>>  	.gpio_button = &asus_me176c_tf103c_lid,
>> +	.gpio_button_count = 1,
>>  	.gpiod_lookup_tables = asus_tf103c_gpios,
>>  	.bat_swnode = &asus_tf103c_battery_node,
>>  	.modules = bq24190_modules,
>> diff --git a/drivers/platform/x86/x86-android-tablets/core.c b/drivers/platform/x86/x86-android-tablets/core.c
>> index 245167674aa2..0c7142f4eccc 100644
>> --- a/drivers/platform/x86/x86-android-tablets/core.c
>> +++ b/drivers/platform/x86/x86-android-tablets/core.c
>> @@ -124,6 +124,7 @@ static int serdev_count;
>>  static struct i2c_client **i2c_clients;
>>  static struct platform_device **pdevs;
>>  static struct serdev_device **serdevs;
>> +static struct gpio_keys_button *buttons;
>>  static struct gpiod_lookup_table * const *gpiod_lookup_tables;
>>  static const struct software_node *bat_swnode;
>>  static void (*exit_handler)(void);
>> @@ -238,6 +239,7 @@ static void x86_android_tablet_cleanup(void)
>>  		platform_device_unregister(pdevs[i]);
>>  
>>  	kfree(pdevs);
>> +	kfree(buttons);
>>  
>>  	for (i = 0; i < i2c_client_count; i++)
>>  		i2c_unregister_device(i2c_clients[i]);
>> @@ -353,22 +355,31 @@ static __init int x86_android_tablet_init(void)
>>  		}
>>  	}
>>  
>> -	if (dev_info->gpio_button) {
>> -		struct gpio_keys_platform_data pdata = {
>> -			.buttons = &dev_info->gpio_button->button,
>> -			.nbuttons = 1,
>> -		};
>> +	if (dev_info->gpio_button_count) {
>> +		struct gpio_keys_platform_data pdata = { };
>>  		struct gpio_desc *gpiod;
>>  
>> -		/* Get GPIO for the gpio-button */
>> -		ret = x86_android_tablet_get_gpiod(dev_info->gpio_button->chip,
>> -						   dev_info->gpio_button->pin, &gpiod);
>> -		if (ret < 0) {
>> +		buttons = kcalloc(dev_info->gpio_button_count, sizeof(*buttons), GFP_KERNEL);
>> +		if (!buttons) {
>>  			x86_android_tablet_cleanup();
>> -			return ret;
>> +			return -ENOMEM;
>> +		}
>> +
>> +		for (i = 0; i < dev_info->gpio_button_count; i++) {
>> +			buttons[i] = dev_info->gpio_button[i].button;
>> +			/* Get GPIO for the gpio-button */
>> +			ret = x86_android_tablet_get_gpiod(dev_info->gpio_button[i].chip,
>> +							   dev_info->gpio_button[i].pin, &gpiod);
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thank you.

> With the note that the comment seems to just spell out what the code does 
> so it feels useless.

That is a valid note. I've dropped the comment while merging this.

I've added this series to my review-hans (soon to be for-next) branch now.

Regards,

Hans