[PATCH v2 13/24] platform/x86: ideapad-laptop: rework is_visible() logic

[PATCH v2 13/24] platform/x86: ideapad-laptop: rework is_visible() logic

[Barnabás Pőcze]

Store the supported features in the driver private data, and modify the
is_visible() callback to use it, and create ideapad_check_features() to
populate it.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index c3234e0931a9..15d070b503dc 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -105,9 +105,14 @@ struct ideapad_private {
 	struct backlight_device *blightdev;
 	struct dentry *debug;
 	unsigned long cfg;
-	bool has_hw_rfkill_switch;
-	bool has_touchpad_switch;
 	const char *fnesc_guid;
+	struct {
+		bool hw_rfkill_switch     : 1,
+		     fan_mode             : 1,
+		     touchpad_ctrl_via_ec : 1,
+		     conservation_mode    : 1,
+		     fn_lock              : 1;
+	} features;
 };
 
 static bool no_bt_rfkill;
@@ -545,24 +550,18 @@ static umode_t ideapad_is_visible(struct kobject *kobj,
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct ideapad_private *priv = dev_get_drvdata(dev);
-	bool supported;
+	bool supported = true;
 
 	if (attr == &dev_attr_camera_power.attr)
 		supported = test_bit(CFG_CAP_CAM_BIT, &priv->cfg);
-	else if (attr == &dev_attr_fan_mode.attr) {
-		unsigned long value;
-		supported = !read_ec_data(priv->adev->handle, VPCCMD_R_FAN,
-					  &value);
-	} else if (attr == &dev_attr_conservation_mode.attr) {
-		supported = acpi_has_method(priv->adev->handle, "GBMD") &&
-			    acpi_has_method(priv->adev->handle, "SBMC");
-	} else if (attr == &dev_attr_fn_lock.attr) {
-		supported = acpi_has_method(priv->adev->handle, "HALS") &&
-			acpi_has_method(priv->adev->handle, "SALS");
-	} else if (attr == &dev_attr_touchpad.attr)
-		supported = priv->has_touchpad_switch;
-	else
-		supported = true;
+	else if (attr == &dev_attr_fan_mode.attr)
+		supported = priv->features.fan_mode;
+	else if (attr == &dev_attr_touchpad.attr)
+		supported = priv->features.touchpad_ctrl_via_ec;
+	else if (attr == &dev_attr_conservation_mode.attr)
+		supported = priv->features.conservation_mode;
+	else if (attr == &dev_attr_fn_lock.attr)
+		supported = priv->features.fn_lock;
 
 	return supported ? attr->mode : 0;
 }
@@ -605,7 +604,7 @@ static void ideapad_sync_rfk_state(struct ideapad_private *priv)
 	unsigned long hw_blocked = 0;
 	int i;
 
-	if (priv->has_hw_rfkill_switch) {
+	if (priv->features.hw_rfkill_switch) {
 		if (read_ec_data(priv->adev->handle, VPCCMD_R_RF, &hw_blocked))
 			return;
 		hw_blocked = !hw_blocked;
@@ -905,7 +904,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv)
 {
 	unsigned long value;
 
-	if (!priv->has_touchpad_switch)
+	if (!priv->features.touchpad_ctrl_via_ec)
 		return;
 
 	/* Without reading from EC touchpad LED doesn't switch state */
@@ -1008,6 +1007,26 @@ static const struct dmi_system_id hw_rfkill_list[] = {
 	{}
 };
 
+static void ideapad_check_features(struct ideapad_private *priv)
+{
+	acpi_handle handle = priv->adev->handle;
+	unsigned long val;
+
+	priv->features.hw_rfkill_switch = dmi_check_system(hw_rfkill_list);
+
+	if (!read_ec_data(handle, VPCCMD_R_FAN, &val))
+		priv->features.fan_mode = true;
+
+	/* Most ideapads with ELAN0634 touchpad don't use EC touchpad switch */
+	priv->features.touchpad_ctrl_via_ec = !acpi_dev_present("ELAN0634", NULL, -1);
+
+	if (acpi_has_method(handle, "GBMD") && acpi_has_method(handle, "SBMC"))
+		priv->features.conservation_mode = true;
+
+	if (acpi_has_method(handle, "HALS") && acpi_has_method(handle, "SALS"))
+		priv->features.fn_lock = true;
+}
+
 static int ideapad_acpi_add(struct platform_device *pdev)
 {
 	int ret, i;
@@ -1031,10 +1050,8 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 	priv->cfg = cfg;
 	priv->adev = adev;
 	priv->platform_device = pdev;
-	priv->has_hw_rfkill_switch = dmi_check_system(hw_rfkill_list);
 
-	/* Most ideapads with ELAN0634 touchpad don't use EC touchpad switch */
-	priv->has_touchpad_switch = !acpi_dev_present("ELAN0634", NULL, -1);
+	ideapad_check_features(priv);
 
 	ret = ideapad_sysfs_init(priv);
 	if (ret)
@@ -1050,11 +1067,11 @@ static int ideapad_acpi_add(struct platform_device *pdev)
 	 * On some models without a hw-switch (the yoga 2 13 at least)
 	 * VPCCMD_W_RF must be explicitly set to 1 for the wifi to work.
 	 */
-	if (!priv->has_hw_rfkill_switch)
+	if (!priv->features.hw_rfkill_switch)
 		write_ec_cmd(priv->adev->handle, VPCCMD_W_RF, 1);
 
 	/* The same for Touchpad */
-	if (!priv->has_touchpad_switch)
+	if (!priv->features.touchpad_ctrl_via_ec)
 		write_ec_cmd(priv->adev->handle, VPCCMD_W_TOUCHPAD, 1);
 
 	for (i = 0; i < IDEAPAD_RFKILL_DEV_NUM; i++)
-- 
2.30.0

Re: [PATCH v2 13/24] platform/x86: ideapad-laptop: rework is_visible() logic

[Andy Shevchenko]

On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze <pobrn@protonmail.com> wrote:
>
> Store the supported features in the driver private data, and modify the
> is_visible() callback to use it, and create ideapad_check_features() to
> populate it.

...

> +       struct {
> +               bool hw_rfkill_switch     : 1,

Does it make sense? Not to me.
Why not put all of them (I don't like comma and single occurrence of
the type, it may be problematic in the future) as unsigned int, or
something like that?
Also, is it okay to have bit fields (I mean from synchronization p.o.v.)?

> +                    fan_mode             : 1,
> +                    touchpad_ctrl_via_ec : 1,
> +                    conservation_mode    : 1,
> +                    fn_lock              : 1;
> +       } features;
>  };

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/24] platform/x86: ideapad-laptop: rework is_visible() logic

[Barnabás Pőcze]

Hi


2021. január 16., szombat 20:58 keltezéssel, Andy Shevchenko írta:

> On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze wrote:
> >
> > Store the supported features in the driver private data, and modify the
> > is_visible() callback to use it, and create ideapad_check_features() to
> > populate it.
>
> ...
>
> > +       struct {
> > +               bool hw_rfkill_switch     : 1,
>
> Does it make sense? Not to me.
> Why not put all of them (I don't like comma and single occurrence of
> the type, it may be problematic in the future) as unsigned int, or
> something like that?

I will add the full declaration for each, with type and semicolon in each
line. Would you prefer the type to be `unsigned int` instead of `bool`, or
could you clarify what you mean by the second question?


> Also, is it okay to have bit fields (I mean from synchronization p.o.v.)?

I am fairly certain it is since this bit-field is only ever written in
`ideapad_check_features()` which is called from `ideapad_acpi_add()`. Apart from
two instances they are only read indirectly by `ideapad_acpi_add()`, and even
in those two cases if the values are read as false instead of their real
value, it cannot cause significant issues as far as I see.


>
> > +                    fan_mode             : 1,
> > +                    touchpad_ctrl_via_ec : 1,
> > +                    conservation_mode    : 1,
> > +                    fn_lock              : 1;
> > +       } features;
> >  };
> [...]


Regards,
Barnabás Pőcze

Re: [PATCH v2 13/24] platform/x86: ideapad-laptop: rework is_visible() logic

[Andy Shevchenko]

On Sun, Jan 17, 2021 at 12:21 AM Barnabás Pőcze <pobrn@protonmail.com> wrote:
> 2021. január 16., szombat 20:58 keltezéssel, Andy Shevchenko írta:
> > On Wed, Jan 13, 2021 at 8:23 PM Barnabás Pőcze wrote:

...

> > > +       struct {
> > > +               bool hw_rfkill_switch     : 1,
> >
> > Does it make sense? Not to me.
> > Why not put all of them (I don't like comma and single occurrence of
> > the type, it may be problematic in the future) as unsigned int, or
> > something like that?
>
> I will add the full declaration for each, with type and semicolon in each
> line. Would you prefer the type to be `unsigned int` instead of `bool`, or
> could you clarify what you mean by the second question?

I have no preference and in my code I can do either depending on the
case. But your below answer is fine, so choose what you prefer in this
case.

> > Also, is it okay to have bit fields (I mean from synchronization p.o.v.)?
>
> I am fairly certain it is since this bit-field is only ever written in
> `ideapad_check_features()` which is called from `ideapad_acpi_add()`. Apart from
> two instances they are only read indirectly by `ideapad_acpi_add()`, and even
> in those two cases if the values are read as false instead of their real
> value, it cannot cause significant issues as far as I see.

Okay, then choose what you prefer. Bit fields are beasts when it comes
to synchronization / concurrent access matter, but they will take less
size (when > 4, since bool usually takes 1 byte on some architectures
/ compilers).

> > > +                    fan_mode             : 1,
> > > +                    touchpad_ctrl_via_ec : 1,
> > > +                    conservation_mode    : 1,
> > > +                    fn_lock              : 1;
> > > +       } features;
> > >  };

-- 
With Best Regards,
Andy Shevchenko