| Message ID | 20190213163655.14540-1-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | a043ad87a78f9cfb5ccd80f448e4e76b50546ad2 |
| Headers | show |
| Series | usb: typec: tps6598x: Check mode of operation | expand |
Hello! On 13.02.2019 19:36, Heikki Krogerus wrote: > To prevent loading of the driver when the PD controller is > still in some operational mode that the driver does not > support, checking the mode in driver probe callback > function. > > TI PD controllers may be in undefined mode of operation > for example when the application code (firmware) is > completely missing. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/tps6598x.c | 53 +++++++++++++++++++++++++++++++++--- > 1 file changed, 49 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c > index 1c0033ad8738..9947c87d2a1e 100644 > --- a/drivers/usb/typec/tps6598x.c > +++ b/drivers/usb/typec/tps6598x.c [...] > @@ -384,6 +400,32 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data) > return IRQ_HANDLED; > } > > +static int tps6598x_check_mode(struct tps6598x *tps) > +{ > + char mode[5] = { }; > + int ret; > + > + ret = tps6598x_read32(tps, TPS_REG_MODE, (void *)mode); Casting pointers to 'void *' happens automagically, doesn't i? [...] MBR, Sergei
Hi Sergei, On Thu, Feb 14, 2019 at 11:52:23AM +0300, Sergei Shtylyov wrote: > > +static int tps6598x_check_mode(struct tps6598x *tps) > > +{ > > + char mode[5] = { }; > > + int ret; > > + > > + ret = tps6598x_read32(tps, TPS_REG_MODE, (void *)mode); > > Casting pointers to 'void *' happens automagically, doesn't i? The third parameter in tps6598x_read32() is 'u32 *'. thanks,
On 02/14/2019 03:12 PM, Heikki Krogerus wrote: >>> +static int tps6598x_check_mode(struct tps6598x *tps) >>> +{ >>> + char mode[5] = { }; >>> + int ret; >>> + >>> + ret = tps6598x_read32(tps, TPS_REG_MODE, (void *)mode); >> >> Casting pointers to 'void *' happens automagically, doesn't i? > > The third parameter in tps6598x_read32() is 'u32 *'. Then why cast to 'void *'? > thanks, MBR, Sergei
On Thu, Feb 14, 2019 at 06:22:12PM +0300, Sergei Shtylyov wrote: > On 02/14/2019 03:12 PM, Heikki Krogerus wrote: > > >>> +static int tps6598x_check_mode(struct tps6598x *tps) > >>> +{ > >>> + char mode[5] = { }; > >>> + int ret; > >>> + > >>> + ret = tps6598x_read32(tps, TPS_REG_MODE, (void *)mode); > >> > >> Casting pointers to 'void *' happens automagically, doesn't i? > > > > The third parameter in tps6598x_read32() is 'u32 *'. > > Then why cast to 'void *'? Because if we don't cast: drivers/usb/typec/tps6598x.c:408:43: error: passing argument 3 of ‘tps6598x_read32’ from incompatible pointer type
On 02/14/2019 06:28 PM, Heikki Krogerus wrote: >>>>> +static int tps6598x_check_mode(struct tps6598x *tps) >>>>> +{ >>>>> + char mode[5] = { }; >>>>> + int ret; >>>>> + >>>>> + ret = tps6598x_read32(tps, TPS_REG_MODE, (void *)mode); >>>> >>>> Casting pointers to 'void *' happens automagically, doesn't i? >>> >>> The third parameter in tps6598x_read32() is 'u32 *'. >> >> Then why cast to 'void *'? > > Because if we don't cast: > > drivers/usb/typec/tps6598x.c:408:43: error: passing argument 3 of ‘tps6598x_read32’ from incompatible pointer type Why not cast to 'u32 *', I meant? MBR, Sergei
On Thu, Feb 14, 2019 at 06:29:43PM +0300, Sergei Shtylyov wrote: > On 02/14/2019 06:28 PM, Heikki Krogerus wrote: > > >>>>> +static int tps6598x_check_mode(struct tps6598x *tps) > >>>>> +{ > >>>>> + char mode[5] = { }; > >>>>> + int ret; > >>>>> + > >>>>> + ret = tps6598x_read32(tps, TPS_REG_MODE, (void *)mode); > >>>> > >>>> Casting pointers to 'void *' happens automagically, doesn't i? > >>> > >>> The third parameter in tps6598x_read32() is 'u32 *'. > >> > >> Then why cast to 'void *'? > > > > Because if we don't cast: > > > > drivers/usb/typec/tps6598x.c:408:43: error: passing argument 3 of ‘tps6598x_read32’ from incompatible pointer type > > Why not cast to 'u32 *', I meant? I always prefer (void *) in cases like this, and I believe I'm not the only one. Though in this case, even if we later had to change the tps6598x_read32() parameter to 'void *' from 'u32 *', it would not be an issue to convert callers like this, but as a general rule, we should not need to do that. thanks,
diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c index 1c0033ad8738..9947c87d2a1e 100644 --- a/drivers/usb/typec/tps6598x.c +++ b/drivers/usb/typec/tps6598x.c @@ -14,6 +14,8 @@ #include <linux/usb/typec.h> /* Register offsets */ +#define TPS_REG_VID 0x00 +#define TPS_REG_MODE 0x03 #define TPS_REG_CMD1 0x08 #define TPS_REG_DATA1 0x09 #define TPS_REG_INT_EVENT1 0x14 @@ -66,6 +68,20 @@ struct tps6598x_rx_identity_reg { #define TPS_TASK_TIMEOUT 1 #define TPS_TASK_REJECTED 3 +enum { + TPS_MODE_APP, + TPS_MODE_BOOT, + TPS_MODE_BIST, + TPS_MODE_DISC, +}; + +static const char *const modes[] = { + [TPS_MODE_APP] = "APP ", + [TPS_MODE_BOOT] = "BOOT", + [TPS_MODE_BIST] = "BIST", + [TPS_MODE_DISC] = "DISC", +}; + /* Unrecognized commands will be replaced with "!CMD" */ #define INVALID_CMD(_cmd_) (_cmd_ == 0x444d4321) @@ -384,6 +400,32 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data) return IRQ_HANDLED; } +static int tps6598x_check_mode(struct tps6598x *tps) +{ + char mode[5] = { }; + int ret; + + ret = tps6598x_read32(tps, TPS_REG_MODE, (void *)mode); + if (ret) + return ret; + + switch (match_string(modes, ARRAY_SIZE(modes), mode)) { + case TPS_MODE_APP: + return 0; + case TPS_MODE_BOOT: + dev_warn(tps->dev, "dead-battery condition\n"); + return 0; + case TPS_MODE_BIST: + case TPS_MODE_DISC: + default: + dev_err(tps->dev, "controller in unsupported mode \"%s\"\n", + mode); + break; + } + + return -ENODEV; +} + static const struct regmap_config tps6598x_regmap_config = { .reg_bits = 8, .val_bits = 8, @@ -409,10 +451,8 @@ static int tps6598x_probe(struct i2c_client *client) if (IS_ERR(tps->regmap)) return PTR_ERR(tps->regmap); - ret = tps6598x_read32(tps, 0, &vid); - if (ret < 0) - return ret; - if (!vid) + ret = tps6598x_read32(tps, TPS_REG_VID, &vid); + if (ret < 0 || !vid) return -ENODEV; /* @@ -425,6 +465,11 @@ static int tps6598x_probe(struct i2c_client *client) if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) tps->i2c_protocol = true; + /* Make sure the controller has application firmware running */ + ret = tps6598x_check_mode(tps); + if (ret) + return ret; + ret = tps6598x_read32(tps, TPS_REG_STATUS, &status); if (ret < 0) return ret;
To prevent loading of the driver when the PD controller is still in some operational mode that the driver does not support, checking the mode in driver probe callback function. TI PD controllers may be in undefined mode of operation for example when the application code (firmware) is completely missing. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/usb/typec/tps6598x.c | 53 +++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 4 deletions(-)