Re: [PATCH 3/3] backlight: lp855x: Add support ACPI enumeration
Hi Daniel,
Thank you for the quick review of this series.
On 11/2/21 13:22, Daniel Thompson wrote:
> On Mon, Nov 01, 2021 at 07:55:17PM +0100, Hans de Goede wrote:
>> The Xiaomi Mi Pad 2 tablet uses an ACPI enumerated LP8556 backlight
>> controller for its LCD-panel, with a Xiaomi specific ACPI HID of
>> "XMCC0001", add support for this.
>>
>> Note the new "if (id)" check also fixes a NULL pointer deref when a user
>> tries to manually bind the driver from sysfs.
>>
>> When CONFIG_ACPI is disabled acpi_match_device() will always return NULL,
>> so the lp855x_parse_acpi() call will get optimized away.
>>
>> Signed-off-by: Hans de Goede
>> ---
>> drivers/video/backlight/lp855x_bl.c | 70 -
>> 1 file changed, 60 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/video/backlight/lp855x_bl.c
>> b/drivers/video/backlight/lp855x_bl.c
>> index d1d27d5eb0f2..f075ec84acfb 100644
>> --- a/drivers/video/backlight/lp855x_bl.c
>> +++ b/drivers/video/backlight/lp855x_bl.c
>> @@ -338,10 +339,6 @@ static int lp855x_parse_dt(struct lp855x *lp)
>> return -EINVAL;
>> }
>>
>> -pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> -if (!pdata)
>> -return -ENOMEM;
>> -
>> of_property_read_string(node, "bl-name", &pdata->name);
>> of_property_read_u8(node, "dev-ctrl", &pdata->device_control);
>> of_property_read_u8(node, "init-brt", &pdata->initial_brightness);
>
> Shouldn't there be a removal of an `lp->pdata = pdata` from somewhere in
> this function?
Ack, fixed for v2.
>> @@ -379,8 +376,31 @@ static int lp855x_parse_dt(struct lp855x *lp)
>> }
>> #endif
>>
>> +static int lp855x_parse_acpi(struct lp855x *lp)
>> +{
>> +int ret;
>> +
>> +/*
>> + * On ACPI the device has already been initialized by the firmware
>
> Perhaps nitpicking but ideally I'd like "and is in register mode" here
> since I presume it can also be assumed that everything with this HID
> has adopted that).
Nope not nitpicking, that is a good point, also fixed for v2.
Regards,
Hans
Re: [PATCH 3/3] backlight: lp855x: Add support ACPI enumeration
On Mon, Nov 01, 2021 at 07:55:17PM +0100, Hans de Goede wrote:
> The Xiaomi Mi Pad 2 tablet uses an ACPI enumerated LP8556 backlight
> controller for its LCD-panel, with a Xiaomi specific ACPI HID of
> "XMCC0001", add support for this.
>
> Note the new "if (id)" check also fixes a NULL pointer deref when a user
> tries to manually bind the driver from sysfs.
>
> When CONFIG_ACPI is disabled acpi_match_device() will always return NULL,
> so the lp855x_parse_acpi() call will get optimized away.
>
> Signed-off-by: Hans de Goede
> ---
> drivers/video/backlight/lp855x_bl.c | 70 -
> 1 file changed, 60 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/video/backlight/lp855x_bl.c
> b/drivers/video/backlight/lp855x_bl.c
> index d1d27d5eb0f2..f075ec84acfb 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -338,10 +339,6 @@ static int lp855x_parse_dt(struct lp855x *lp)
> return -EINVAL;
> }
>
> - pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> - if (!pdata)
> - return -ENOMEM;
> -
> of_property_read_string(node, "bl-name", &pdata->name);
> of_property_read_u8(node, "dev-ctrl", &pdata->device_control);
> of_property_read_u8(node, "init-brt", &pdata->initial_brightness);
Shouldn't there be a removal of an `lp->pdata = pdata` from somewhere in
this function?
> @@ -379,8 +376,31 @@ static int lp855x_parse_dt(struct lp855x *lp)
> }
> #endif
>
> +static int lp855x_parse_acpi(struct lp855x *lp)
> +{
> + int ret;
> +
> + /*
> + * On ACPI the device has already been initialized by the firmware
Perhaps nitpicking but ideally I'd like "and is in register mode" here
since I presume it can also be assumed that everything with this HID
has adopted that).
Other than these, LGTM.
Daniel.
[PATCH 3/3] backlight: lp855x: Add support ACPI enumeration
The Xiaomi Mi Pad 2 tablet uses an ACPI enumerated LP8556 backlight
controller for its LCD-panel, with a Xiaomi specific ACPI HID of
"XMCC0001", add support for this.
Note the new "if (id)" check also fixes a NULL pointer deref when a user
tries to manually bind the driver from sysfs.
When CONFIG_ACPI is disabled acpi_match_device() will always return NULL,
so the lp855x_parse_acpi() call will get optimized away.
Signed-off-by: Hans de Goede
---
drivers/video/backlight/lp855x_bl.c | 70 -
1 file changed, 60 insertions(+), 10 deletions(-)
diff --git a/drivers/video/backlight/lp855x_bl.c
b/drivers/video/backlight/lp855x_bl.c
index d1d27d5eb0f2..f075ec84acfb 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -5,6 +5,7 @@
* Copyright (C) 2011 Texas Instruments
*/
+#include
#include
#include
#include
@@ -330,7 +331,7 @@ static int lp855x_parse_dt(struct lp855x *lp)
{
struct device *dev = lp->dev;
struct device_node *node = dev->of_node;
- struct lp855x_platform_data *pdata;
+ struct lp855x_platform_data *pdata = lp->pdata;
int rom_length;
if (!node) {
@@ -338,10 +339,6 @@ static int lp855x_parse_dt(struct lp855x *lp)
return -EINVAL;
}
- pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
- if (!pdata)
- return -ENOMEM;
-
of_property_read_string(node, "bl-name", &pdata->name);
of_property_read_u8(node, "dev-ctrl", &pdata->device_control);
of_property_read_u8(node, "init-brt", &pdata->initial_brightness);
@@ -379,8 +376,31 @@ static int lp855x_parse_dt(struct lp855x *lp)
}
#endif
+static int lp855x_parse_acpi(struct lp855x *lp)
+{
+ int ret;
+
+ /*
+* On ACPI the device has already been initialized by the firmware
+* so we read back the settings from the registers.
+*/
+ ret = i2c_smbus_read_byte_data(lp->client, lp->cfg->reg_brightness);
+ if (ret < 0)
+ return ret;
+
+ lp->pdata->initial_brightness = ret;
+
+ ret = i2c_smbus_read_byte_data(lp->client, lp->cfg->reg_devicectrl);
+ if (ret < 0)
+ return ret;
+
+ lp->pdata->device_control = ret;
+ return 0;
+}
+
static int lp855x_probe(struct i2c_client *cl, const struct i2c_device_id *id)
{
+ const struct acpi_device_id *acpi_id = NULL;
struct device *dev = &cl->dev;
struct lp855x *lp;
int ret;
@@ -394,10 +414,20 @@ static int lp855x_probe(struct i2c_client *cl, const
struct i2c_device_id *id)
lp->client = cl;
lp->dev = dev;
- lp->chipname = id->name;
- lp->chip_id = id->driver_data;
lp->pdata = dev_get_platdata(dev);
+ if (id) {
+ lp->chipname = id->name;
+ lp->chip_id = id->driver_data;
+ } else {
+ acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
+ if (!acpi_id)
+ return -ENODEV;
+
+ lp->chipname = acpi_id->id;
+ lp->chip_id = acpi_id->driver_data;
+ }
+
switch (lp->chip_id) {
case LP8550:
case LP8551:
@@ -415,9 +445,19 @@ static int lp855x_probe(struct i2c_client *cl, const
struct i2c_device_id *id)
}
if (!lp->pdata) {
- ret = lp855x_parse_dt(lp);
- if (ret < 0)
- return ret;
+ lp->pdata = devm_kzalloc(dev, sizeof(*lp->pdata), GFP_KERNEL);
+ if (!lp->pdata)
+ return -ENOMEM;
+
+ if (id) {
+ ret = lp855x_parse_dt(lp);
+ if (ret < 0)
+ return ret;
+ } else {
+ ret = lp855x_parse_acpi(lp);
+ if (ret < 0)
+ return ret;
+ }
}
if (lp->pdata->period_ns > 0)
@@ -537,10 +577,20 @@ static const struct i2c_device_id lp855x_ids[] = {
};
MODULE_DEVICE_TABLE(i2c, lp855x_ids);
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id lp855x_acpi_match[] = {
+ /* Xiaomi specific HID used for the LP8556 on the Mi Pad 2 */
+ { "XMCC0001", LP8556 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, lp855x_acpi_match);
+#endif
+
static struct i2c_driver lp855x_driver = {
.driver = {
.name = "lp855x",
.of_match_table = of_match_ptr(lp855x_dt_ids),
+ .acpi_match_table = ACPI_PTR(lp855x_acpi_match),
},
.probe = lp855x_probe,
.remove = lp855x_remove,
--
2.31.1
