| Message ID | 1394576402-2910-4-git-send-email-matthew.garrett@nebula.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
2014-03-12 6:20 GMT+08:00 Matthew Garrett <matthew.garrett@nebula.com>: > The existing SBS code explicitly sets the selected battery in the SBS > manager regardless of whether the battery in question is already selected. > This causes bus timeouts on Apple hardware. Check for this case and avoid > it. > Hi Matthew: This patch is to avoid a redundant battery select operation when the battery is selected. But the symptom "bus timeouts" is a bus transaction issue, right? Will this happen during other SBS write/read operations? Do we need to increase the wait time of SMBUS transaction? > Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> > --- > drivers/acpi/sbs.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c > index dbd4849..c386505 100644 > --- a/drivers/acpi/sbs.c > +++ b/drivers/acpi/sbs.c > @@ -470,17 +470,25 @@ static struct device_attribute alarm_attr = { > static int acpi_battery_read(struct acpi_battery *battery) > { > int result = 0, saved_present = battery->present; > - u16 state; > + u16 state, selected, desired; > > if (battery->sbs->manager_present) { > result = acpi_smbus_read(battery->sbs->hc, SMBUS_READ_WORD, > ACPI_SBS_MANAGER, 0x01, (u8 *)&state); > if (!result) > battery->present = state & (1 << battery->id); > - state &= 0x0fff; > - state |= 1 << (battery->id + 12); > - acpi_smbus_write(battery->sbs->hc, SMBUS_WRITE_WORD, > - ACPI_SBS_MANAGER, 0x01, (u8 *)&state, 2); > + /* > + * Don't switch battery if the correct one is already selected > + */ > + selected = state & 0xf000; > + desired = 1 << (battery->id + 12); > + if (selected != desired) { > + state &= 0x0fff; > + state |= desired; > + acpi_smbus_write(battery->sbs->hc, SMBUS_WRITE_WORD, > + ACPI_SBS_MANAGER, 0x01, > + (u8 *)&state, 2); > + } > } else if (battery->id == 0) > battery->present = 1; > if (result || !battery->present) > -- > 1.8.5.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14/03/2014 06:39, Lan Tianyu wrote: > 2014-03-12 6:20 GMT+08:00 Matthew Garrett <matthew.garrett@nebula.com>: >> The existing SBS code explicitly sets the selected battery in the SBS >> manager regardless of whether the battery in question is already selected. >> This causes bus timeouts on Apple hardware. Check for this case and avoid >> it. >> > Hi Matthew: > This patch is to avoid a redundant battery select operation when > the battery is selected. But the symptom "bus timeouts" is a bus transaction > issue, right? Will this happen during other SBS write/read operations? Do we > need to increase the wait time of SMBUS transaction? Well, the patch does the job and is needed on a macbook air. What should we test to make it potentially better? > >> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> >> --- >> drivers/acpi/sbs.c | 18 +++++++++++++----- >> 1 file changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c >> index dbd4849..c386505 100644 >> --- a/drivers/acpi/sbs.c >> +++ b/drivers/acpi/sbs.c >> @@ -470,17 +470,25 @@ static struct device_attribute alarm_attr = { >> static int acpi_battery_read(struct acpi_battery *battery) >> { >> int result = 0, saved_present = battery->present; >> - u16 state; >> + u16 state, selected, desired; >> >> if (battery->sbs->manager_present) { >> result = acpi_smbus_read(battery->sbs->hc, SMBUS_READ_WORD, >> ACPI_SBS_MANAGER, 0x01, (u8 *)&state); >> if (!result) >> battery->present = state & (1 << battery->id); >> - state &= 0x0fff; >> - state |= 1 << (battery->id + 12); >> - acpi_smbus_write(battery->sbs->hc, SMBUS_WRITE_WORD, >> - ACPI_SBS_MANAGER, 0x01, (u8 *)&state, 2); >> + /* >> + * Don't switch battery if the correct one is already selected >> + */ >> + selected = state & 0xf000; >> + desired = 1 << (battery->id + 12); >> + if (selected != desired) { >> + state &= 0x0fff; >> + state |= desired; >> + acpi_smbus_write(battery->sbs->hc, SMBUS_WRITE_WORD, >> + ACPI_SBS_MANAGER, 0x01, >> + (u8 *)&state, 2); >> + } >> } else if (battery->id == 0) >> battery->present = 1; >> if (result || !battery->present) >> -- >> 1.8.5.3 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c index dbd4849..c386505 100644 --- a/drivers/acpi/sbs.c +++ b/drivers/acpi/sbs.c @@ -470,17 +470,25 @@ static struct device_attribute alarm_attr = { static int acpi_battery_read(struct acpi_battery *battery) { int result = 0, saved_present = battery->present; - u16 state; + u16 state, selected, desired; if (battery->sbs->manager_present) { result = acpi_smbus_read(battery->sbs->hc, SMBUS_READ_WORD, ACPI_SBS_MANAGER, 0x01, (u8 *)&state); if (!result) battery->present = state & (1 << battery->id); - state &= 0x0fff; - state |= 1 << (battery->id + 12); - acpi_smbus_write(battery->sbs->hc, SMBUS_WRITE_WORD, - ACPI_SBS_MANAGER, 0x01, (u8 *)&state, 2); + /* + * Don't switch battery if the correct one is already selected + */ + selected = state & 0xf000; + desired = 1 << (battery->id + 12); + if (selected != desired) { + state &= 0x0fff; + state |= desired; + acpi_smbus_write(battery->sbs->hc, SMBUS_WRITE_WORD, + ACPI_SBS_MANAGER, 0x01, + (u8 *)&state, 2); + } } else if (battery->id == 0) battery->present = 1; if (result || !battery->present)
The existing SBS code explicitly sets the selected battery in the SBS manager regardless of whether the battery in question is already selected. This causes bus timeouts on Apple hardware. Check for this case and avoid it. Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com> --- drivers/acpi/sbs.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)