On Sat, Feb 09, 2019 at 02:01:37PM +0200, Andy Shevchenko wrote: > On Sat, Feb 9, 2019 at 12:00 PM Yauhen Kharuzhy <jekhor@xxxxxxxxx> wrote: > > Thanks for the patch, my comments below. > > > Lenovo Yoga Book has INT33FE definition in the ACPI DSDT but doesn't > > have USB TypeC connector and muxer (it uses micro USB) > > Can you provide a link to acpidump and output of > grep -H . /sys/bus/acpi/devices/*/status Sure: https://github.com/jekhor/yogabook-linux/blob/master/yoga/acpi/yoga.acpidump.log https://github.com/jekhor/yogabook-linux/blob/master/yoga/YB1-X91L-acpi-status.txt > > > Add I2C device definition for BQ27542 battery fuel gauge used in this > > notebook and register only it. > > I would like to hear Hans' opinion about all this. > > > #include <linux/platform_device.h> > > #include <linux/regulator/consumer.h> > > #include <linux/slab.h> > > +#include <linux/dmi.h> > > Please, keep it sorted. OK. > > > struct cht_int33fe_data { > > - struct i2c_client *max17047; > > + struct i2c_client *battery_fg; > > > static int cht_int33fe_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > @@ -97,6 +115,7 @@ static int cht_int33fe_probe(struct platform_device *pdev) > > acpi_status status; > > int fusb302_irq; > > int ret; > > + const char *product_name; > > Keep it in reversed tree order (longer lines earlier). OK. > > > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + /* Lenovo Yoga Book has INT33FE device in DSDT table but this device > > + * doesn't have FUSB302, max17047 and pi3usb30532 definitions but has > > + * a bq27542 battery gauge. > > + */ > > This makes me wondering if the proper solution is to bail out and use > fuel gauge directly. Mmm, can you clarify? > > Btw, comment style should have /* line w/o text. > OK. > > + product_name = dmi_get_system_info(DMI_PRODUCT_NAME); > > + if (dmi_check_system(yogabook_ids)) { > > I guess one call to *_match() should be enough. Hrr, old piece of code. Remove product_name entirely. dmi_match() will do exactly matching, but Yoga Book may be YB1-X91L, YB1-X91F, YB1-X90L, YB1-X90F (L/F – with/without of LTE modem, 91 – Windows 10 is preinstalled, 90 - Android). > > > + dev_info(dev, "Lenovo Yoga Book detected, register only " > > + "BQ27542 battery Fuel Gauge for it\n"); > > Don't split lines like this. Keep it longer than 80 chars? > > > + } > > + > > + > > One blank line is enough. OK > > > - i2c_unregister_device(data->pi3usb30532); > > - i2c_unregister_device(data->fusb302); > > > + if (data->pi3usb30532) > > + i2c_unregister_device(data->pi3usb30532); > > + > > + if (data->fusb302) > > + i2c_unregister_device(data->fusb302); > > Why? What's wrong with the current state? Hm, i2c_unregister_device() correctly handle NULL case now. OK, I will remove this. -- Yauhen Kharuzhy
