| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 24 Sep 2021 16:58:52 +0200
From: "Sven Peter" <sven@...npeter.dev>
To: "Heikki Krogerus" <heikki.krogerus@...ux.intel.com>
Cc: "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
Guido Günther <agx@...xcpu.org>,
"Bryan O'Donoghue" <bryan.odonoghue@...aro.org>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
"Hector Martin" <marcan@...can.st>,
"Mohamed Mediouni" <mohamed.mediouni@...amail.com>,
"Stan Skowronek" <stan@...ellium.com>,
"Mark Kettenis" <mark.kettenis@...all.nl>,
"Alexander Graf" <graf@...zon.com>,
"Alyssa Rosenzweig" <alyssa@...enzweig.io>
Subject: Re: [PATCH v2 4/6] usb: typec: tipd: Add support for Apple CD321X
Hi,
On Fri, Sep 24, 2021, at 16:41, Heikki Krogerus wrote:
> Hi,
>
> One more question below.
>
> On Thu, Sep 23, 2021 at 08:13:19PM +0200, Sven Peter wrote:
>> Apple CD321x chips are a variant of the TI TPS 6598x chips.
>> The major differences are the changed interrupt numbers and
>> the concurrent connection to the SMC which we must not disturb.
>>
>> Signed-off-by: Sven Peter <sven@...npeter.dev>
>> ---
>> changes since v1:
>> - new commit since Heikki suggested to just add a separate irq handler
>>
>> drivers/usb/typec/tipd/core.c | 86 ++++++++++++++++++++++++++++++-
>> drivers/usb/typec/tipd/tps6598x.h | 6 +++
>> drivers/usb/typec/tipd/trace.h | 23 +++++++++
>> 3 files changed, 113 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index cd1e37eb8a0c..6c9c8f19a1cf 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -9,6 +9,7 @@
>> #include <linux/i2c.h>
>> #include <linux/acpi.h>
>> #include <linux/module.h>
>> +#include <linux/of_device.h>
>> #include <linux/power_supply.h>
>> #include <linux/regmap.h>
>> #include <linux/interrupt.h>
>> @@ -76,6 +77,16 @@ static const char *const modes[] = {
>> /* Unrecognized commands will be replaced with "!CMD" */
>> #define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321)
>>
>> +enum tipd_hw_type {
>> + HW_TPS6598X,
>> + HW_CD321X
>> +};
>> +
>> +struct tipd_hw {
>> + enum tipd_hw_type type;
>> + irq_handler_t irq_handler;
>> +};
>> +
>> struct tps6598x {
>> struct device *dev;
>> struct regmap *regmap;
>> @@ -458,6 +469,51 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
>> }
>> }
>>
>> +static irqreturn_t cd321x_interrupt(int irq, void *data)
>> +{
>> + struct tps6598x *tps = data;
>> + u64 event = 0;
>> + u32 status;
>> + int ret;
>> +
>> + mutex_lock(&tps->lock);
>> +
>> + ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event);
>> + if (ret) {
>> + dev_err(tps->dev, "%s: failed to read events\n", __func__);
>> + goto err_unlock;
>> + }
>> + trace_cd321x_irq(event);
>> +
>> + if (!event)
>> + goto err_unlock;
>> +
>> + if (!tps6598x_read_status(tps, &status))
>> + goto err_clear_ints;
>> +
>> + if (event & APPLE_CD_REG_INT_POWER_STATUS_UPDATE)
>> + if (!tps6598x_read_power_status(tps))
>> + goto err_clear_ints;
>> +
>> + if (event & APPLE_CD_REG_INT_DATA_STATUS_UPDATE)
>> + if (!tps6598x_read_data_status(tps))
>> + goto err_clear_ints;
>> +
>> + /* Handle plug insert or removal */
>> + if (event & APPLE_CD_REG_INT_PLUG_EVENT)
>> + tps6598x_handle_plug_event(tps, status);
>> +
>> +err_clear_ints:
>> + tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event);
>> +
>> +err_unlock:
>> + mutex_unlock(&tps->lock);
>> +
>> + if (event)
>> + return IRQ_HANDLED;
>> + return IRQ_NONE;
>> +}
>> +
>> static irqreturn_t tps6598x_interrupt(int irq, void *data)
>> {
>> struct tps6598x *tps = data;
>> @@ -615,8 +671,19 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
>> return PTR_ERR_OR_ZERO(tps->psy);
>> }
>>
>> +static const struct tipd_hw ti_tps6598x_data = {
>> + .type = HW_TPS6598X,
>> + .irq_handler = tps6598x_interrupt,
>> +};
>> +
>> +static const struct tipd_hw apple_cd321x_data = {
>> + .type = HW_CD321X,
>> + .irq_handler = cd321x_interrupt,
>> +};
>> +
>> static int tps6598x_probe(struct i2c_client *client)
>> {
>> + const struct tipd_hw *hw;
>> struct typec_capability typec_cap = { };
>> struct tps6598x *tps;
>> struct fwnode_handle *fwnode;
>> @@ -629,6 +696,10 @@ static int tps6598x_probe(struct i2c_client *client)
>> if (!tps)
>> return -ENOMEM;
>>
>> + hw = of_device_get_match_data(&client->dev);
>> + if (!hw)
>> + hw = &ti_tps6598x_data;
>> +
>> mutex_init(&tps->lock);
>> tps->dev = &client->dev;
>>
>> @@ -655,6 +726,16 @@ static int tps6598x_probe(struct i2c_client *client)
>> if (ret)
>> return ret;
>>
>> + if (hw->type == HW_CD321X) {
>> + /* CD321X chips have all interrupts masked initially */
>> + ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
>> + APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
>> + APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
>> + APPLE_CD_REG_INT_PLUG_EVENT);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
>> if (ret < 0)
>> return ret;
>> @@ -736,7 +817,7 @@ static int tps6598x_probe(struct i2c_client *client)
>> }
>>
>> ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> - tps6598x_interrupt,
>> + hw->irq_handler,
>> IRQF_SHARED | IRQF_ONESHOT,
>> dev_name(&client->dev), tps);
>
> Couldn't you just use the compatible property and local variable here?
>
> irq_handler_t irq_handler = tps6598x_interrupt;
> struct device_node *np = client->dev.of_node;
>
> if (np && of_device_is_compatible(np, "apple,cd321x")) {
> /* CD321X chips have all interrupts masked initially */
> ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
> APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
> APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
> APPLE_CD_REG_INT_PLUG_EVENT);
> if (ret)
> return ret;
>
> irq_handler = cd321x_interrupt;
> }
>
> ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> irq_handler,
> IRQF_SHARED | IRQF_ONESHOT,
> dev_name(&client->dev), tps);
>
> I did not go over the whole series yet, so I may have missed
> something.
Sure, that will work and get rid of the slightly awkward and redundant interrupt/enum
combination. I'll wait for your comments for the rest of the series and do that for v3 then :)
Thanks,
Sven
Powered by blists - more mailing lists