| Message ID | 20210918120934.28252-4-sven@svenpeter.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: tipd: Add Apple M1 support | expand |
(Asked off list for completeness) use_int2 is always true in this series , so I'm not sure it makes sense to configure it. But for symmetry I think it's good, so
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
On Sat, Sep 18, 2021 at 02:09:28PM +0200, Sven Peter wrote: > TI TPS6598x chips come with two separate i2c buses which each have an > independent interrupt controller. When only a single controller is > connected both can just be used. > On Apple M1 machines the secondary bus is however connected to the system > management controller and we must not modify its configuration. Prepare > for that by allowing to chose which interrupt controller(s) are used. This is good, but... > Signed-off-by: Sven Peter <sven@svenpeter.dev> > --- > drivers/usb/typec/tipd/core.c | 30 +++++++++++++++++++++--------- > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c > index 656020e7f533..c2c399722c37 100644 > --- a/drivers/usb/typec/tipd/core.c > +++ b/drivers/usb/typec/tipd/core.c > @@ -78,6 +78,8 @@ static const char *const modes[] = { > #define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321) > > struct tps6598x_hw { > + bool use_int1; > + bool use_int2; > }; Wouldn't it be better to read that information from a device property/properties? Driver data is OK, but device property would be better. We don't have separate compatible for this on every board that uses it and we have also ACPI platforms. thanks,
On Tue, Sep 21, 2021, at 15:21, Heikki Krogerus wrote: > On Sat, Sep 18, 2021 at 02:09:28PM +0200, Sven Peter wrote: >> TI TPS6598x chips come with two separate i2c buses which each have an >> independent interrupt controller. When only a single controller is >> connected both can just be used. >> On Apple M1 machines the secondary bus is however connected to the system >> management controller and we must not modify its configuration. Prepare >> for that by allowing to chose which interrupt controller(s) are used. > > This is good, but... > >> Signed-off-by: Sven Peter <sven@svenpeter.dev> >> --- >> drivers/usb/typec/tipd/core.c | 30 +++++++++++++++++++++--------- >> 1 file changed, 21 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c >> index 656020e7f533..c2c399722c37 100644 >> --- a/drivers/usb/typec/tipd/core.c >> +++ b/drivers/usb/typec/tipd/core.c >> @@ -78,6 +78,8 @@ static const char *const modes[] = { >> #define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321) >> >> struct tps6598x_hw { >> + bool use_int1; >> + bool use_int2; >> }; > > Wouldn't it be better to read that information from a device > property/properties? > > Driver data is OK, but device property would be better. We don't have > separate compatible for this on every board that uses it and we have > also ACPI platforms. Good point. Taking into account your review for the later patches I think I can drop this completely though: On the Apple hardware the second controller is always connected to the SMC so far. If it's required later on we can always add the additional property with a default value then. Best, Sven
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c index 656020e7f533..c2c399722c37 100644 --- a/drivers/usb/typec/tipd/core.c +++ b/drivers/usb/typec/tipd/core.c @@ -78,6 +78,8 @@ static const char *const modes[] = { #define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321) struct tps6598x_hw { + bool use_int1; + bool use_int2; }; static const struct tps6598x_hw ti_tps6598x_data; @@ -411,22 +413,28 @@ static const struct typec_operations tps6598x_ops = { static irqreturn_t tps6598x_interrupt(int irq, void *data) { struct tps6598x *tps = data; - u64 event1; - u64 event2; + u64 event1 = 0; + u64 event2 = 0; + u64 event = 0; u32 status, data_status; u16 pwr_status; int ret; mutex_lock(&tps->lock); - ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event1); - ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event2); + ret = 0; + if (tps->hw->use_int1) + ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event1); + if (tps->hw->use_int2) + ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event2); if (ret) { dev_err(tps->dev, "%s: failed to read events\n", __func__); goto err_unlock; } trace_tps6598x_irq(event1, event2); + event = event1 | event2; + ret = tps6598x_read32(tps, TPS_REG_STATUS, &status); if (ret) { dev_err(tps->dev, "%s: failed to read status\n", __func__); @@ -434,7 +442,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data) } trace_tps6598x_status(status); - if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE) { + if (event & TPS_REG_INT_POWER_STATUS_UPDATE) { ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status); if (ret < 0) { dev_err(tps->dev, "failed to read power status: %d\n", ret); @@ -443,7 +451,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data) trace_tps6598x_power_status(pwr_status); } - if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE) { + if (event & TPS_REG_INT_DATA_STATUS_UPDATE) { ret = tps6598x_read32(tps, TPS_REG_DATA_STATUS, &data_status); if (ret < 0) { dev_err(tps->dev, "failed to read data status: %d\n", ret); @@ -453,7 +461,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data) } /* Handle plug insert or removal */ - if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT) { + if (event & TPS_REG_INT_PLUG_EVENT) { if (status & TPS_STATUS_PLUG_PRESENT) { ret = tps6598x_connect(tps, status); if (ret) @@ -465,8 +473,10 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data) } err_clear_ints: - tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1); - tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event2); + if (tps->hw->use_int1) + tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1); + if (tps->hw->use_int2) + tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event2); err_unlock: mutex_unlock(&tps->lock); @@ -744,6 +754,8 @@ static int tps6598x_remove(struct i2c_client *client) } static const struct tps6598x_hw ti_tps6598x_data = { + .use_int1 = true, + .use_int2 = true, }; static const struct of_device_id tps6598x_of_match[] = {
TI TPS6598x chips come with two separate i2c buses which each have an independent interrupt controller. When only a single controller is connected both can just be used. On Apple M1 machines the secondary bus is however connected to the system management controller and we must not modify its configuration. Prepare for that by allowing to chose which interrupt controller(s) are used. Signed-off-by: Sven Peter <sven@svenpeter.dev> --- drivers/usb/typec/tipd/core.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)