| Message ID | 20211101185518.306728-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] backlight: lp855x: Move device_config setting out of lp855x_configure() | expand |
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 <hdegoede@redhat.com> > --- > 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.
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 <hdegoede@redhat.com> >> --- >> 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
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 <linux/acpi.h> #include <linux/module.h> #include <linux/slab.h> #include <linux/i2c.h> @@ -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,
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 <hdegoede@redhat.com> --- drivers/video/backlight/lp855x_bl.c | 70 ++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 10 deletions(-)