diff mbox series

[1/7] usb: typec: ucsi: add get_fw_info function

Message ID 20190118010909.16161-2-ajayg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series Add support for firmware update on Cypres CCGx | expand

Commit Message

Ajay Gupta Jan. 18, 2019, 1:09 a.m. UTC
Function is to get the details of ccg firmware and device version.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
---
 drivers/usb/typec/ucsi/ucsi_ccg.c | 76 ++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 2 deletions(-)

Comments

Greg KH Jan. 18, 2019, 7:06 a.m. UTC | #1
On Thu, Jan 17, 2019 at 05:09:03PM -0800, Ajay Gupta wrote:
> Function is to get the details of ccg firmware and device version.
> 
> Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> ---
>  drivers/usb/typec/ucsi/ucsi_ccg.c | 76 ++++++++++++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index de8a43bdff68..4d35279ab853 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -17,15 +17,46 @@
>  #include <asm/unaligned.h>
>  #include "ucsi.h"
>  
> +struct ccg_dev_info {
> +	u8 fw_mode:2;
> +	u8 two_pd_ports:2;
> +	u8 row_size_256:2;
> +	u8:1; /* reserved */
> +	u8 hpi_v2_mode:1;
> +	u8 bl_mode:1;
> +	u8 cfgtbl_invalid:1;
> +	u8 fw1_invalid:1;
> +	u8 fw2_invalid:1;
> +	u8:4; /* reserved */
> +	u16 silicon_id;
> +	u16 bl_last_row;
> +} __packed;

Doesn't all of this break into tiny pieces when you read the memory into
a big-endian system?  Be very careful when using bitfields as a "raw"
representation of a structure in memory.

> +
> +struct version_format {
> +	u16 build;
> +	u8 patch;
> +	u8 min:4;
> +	u8 maj:4;
> +};

Same here.

And not packed?  That's dangerous :)


> +
> +struct version_info {
> +	struct version_format base;
> +	struct version_format app;
> +};
> +
>  struct ucsi_ccg {
>  	struct device *dev;
>  	struct ucsi *ucsi;
>  	struct ucsi_ppm ppm;
>  	struct i2c_client *client;
> +	struct ccg_dev_info info;
>  };
>  
> -#define CCGX_RAB_INTR_REG			0x06
> -#define CCGX_RAB_UCSI_CONTROL			0x39
> +#define CCGX_RAB_DEVICE_MODE			0x0000
> +#define CCGX_RAB_INTR_REG			0x0006
> +#define CCGX_RAB_READ_ALL_VER			0x0010
> +#define CCGX_RAB_READ_FW2_VER			0x0020
> +#define CCGX_RAB_UCSI_CONTROL			0x0039
>  #define CCGX_RAB_UCSI_CONTROL_START		BIT(0)
>  #define CCGX_RAB_UCSI_CONTROL_STOP		BIT(1)
>  #define CCGX_RAB_UCSI_DATA_BLOCK(offset)	(0xf000 | ((offset) & 0xff))
> @@ -220,6 +251,41 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static int get_fw_info(struct ucsi_ccg *uc)
> +{
> +	struct device *dev = uc->dev;
> +	struct version_info version[3];
> +	struct version_info *v;
> +	int err, i;
> +
> +	err = ccg_read(uc, CCGX_RAB_READ_ALL_VER, (u8 *)(&version),
> +		       sizeof(version));
> +	if (err < 0)
> +		return err;
> +
> +	for (i = 1; i < ARRAY_SIZE(version); i++) {
> +		v = &version[i];
> +		dev_dbg(dev,
> +			"FW%d Version: %c%c v%x.%x%x, [Base %d.%d.%d.%d]\n",
> +			i, (v->app.build >> 8), (v->app.build & 0xFF),
> +			v->app.patch, v->app.maj, v->app.min,
> +			v->base.maj, v->base.min, v->base.patch,
> +			v->base.build);
> +	}
> +
> +	err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
> +		       sizeof(uc->info));
> +	if (err < 0)
> +		return err;
> +
> +	dev_dbg(dev, "fw_mode: %d\n", uc->info.fw_mode);
> +	dev_dbg(dev, "fw1_invalid: %d\n", uc->info.fw1_invalid);
> +	dev_dbg(dev, "fw2_invalid: %d\n", uc->info.fw2_invalid);
> +	dev_dbg(dev, "silicon_id: 0x%04x\n", uc->info.silicon_id);
> +
> +	return 0;
> +}
> +
>  static int ucsi_ccg_probe(struct i2c_client *client,
>  			  const struct i2c_device_id *id)
>  {
> @@ -248,6 +314,12 @@ static int ucsi_ccg_probe(struct i2c_client *client,
>  		return status;
>  	}
>  
> +	status = get_fw_info(uc);
> +	if (status < 0) {
> +		dev_err(uc->dev, "get_fw_info failed - %d\n", status);
> +		return status;

Are all devices required to have this information?  if not, you just
prevented them from working I think :(

thanks,

greg k-h
Heikki Krogerus Jan. 18, 2019, 2:53 p.m. UTC | #2
Hi Ajay,

On Thu, Jan 17, 2019 at 05:09:03PM -0800, Ajay Gupta wrote:
> Function is to get the details of ccg firmware and device version.
> 
> Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> ---
>  drivers/usb/typec/ucsi/ucsi_ccg.c | 76 ++++++++++++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index de8a43bdff68..4d35279ab853 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -17,15 +17,46 @@
>  #include <asm/unaligned.h>
>  #include "ucsi.h"
>  
> +struct ccg_dev_info {
> +	u8 fw_mode:2;
> +	u8 two_pd_ports:2;
> +	u8 row_size_256:2;
> +	u8:1; /* reserved */
> +	u8 hpi_v2_mode:1;
> +	u8 bl_mode:1;
> +	u8 cfgtbl_invalid:1;
> +	u8 fw1_invalid:1;
> +	u8 fw2_invalid:1;
> +	u8:4; /* reserved */
> +	u16 silicon_id;
> +	u16 bl_last_row;
> +} __packed;
> +
> +struct version_format {
> +	u16 build;
> +	u8 patch;
> +	u8 min:4;
> +	u8 maj:4;
> +};

Don't use bit fields. We shouldn't use them even in the core ucsi
driver with the message data structures like we do.

> +struct version_info {
> +	struct version_format base;
> +	struct version_format app;
> +};
> +
>  struct ucsi_ccg {
>  	struct device *dev;
>  	struct ucsi *ucsi;
>  	struct ucsi_ppm ppm;
>  	struct i2c_client *client;
> +	struct ccg_dev_info info;
>  };
>  
> -#define CCGX_RAB_INTR_REG			0x06
> -#define CCGX_RAB_UCSI_CONTROL			0x39
> +#define CCGX_RAB_DEVICE_MODE			0x0000
> +#define CCGX_RAB_INTR_REG			0x0006
> +#define CCGX_RAB_READ_ALL_VER			0x0010
> +#define CCGX_RAB_READ_FW2_VER			0x0020
> +#define CCGX_RAB_UCSI_CONTROL			0x0039
>  #define CCGX_RAB_UCSI_CONTROL_START		BIT(0)
>  #define CCGX_RAB_UCSI_CONTROL_STOP		BIT(1)
>  #define CCGX_RAB_UCSI_DATA_BLOCK(offset)	(0xf000 | ((offset) & 0xff))
> @@ -220,6 +251,41 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static int get_fw_info(struct ucsi_ccg *uc)
> +{
> +	struct device *dev = uc->dev;
> +	struct version_info version[3];
> +	struct version_info *v;
> +	int err, i;
> +
> +	err = ccg_read(uc, CCGX_RAB_READ_ALL_VER, (u8 *)(&version),
> +		       sizeof(version));
> +	if (err < 0)
> +		return err;
> +
> +	for (i = 1; i < ARRAY_SIZE(version); i++) {
> +		v = &version[i];
> +		dev_dbg(dev,
> +			"FW%d Version: %c%c v%x.%x%x, [Base %d.%d.%d.%d]\n",
> +			i, (v->app.build >> 8), (v->app.build & 0xFF),
> +			v->app.patch, v->app.maj, v->app.min,
> +			v->base.maj, v->base.min, v->base.patch,
> +			v->base.build);

How about a sysfs file showing the fw version?

> +	}
> +
> +	err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
> +		       sizeof(uc->info));
> +	if (err < 0)
> +		return err;
> +
> +	dev_dbg(dev, "fw_mode: %d\n", uc->info.fw_mode);
> +	dev_dbg(dev, "fw1_invalid: %d\n", uc->info.fw1_invalid);
> +	dev_dbg(dev, "fw2_invalid: %d\n", uc->info.fw2_invalid);
> +	dev_dbg(dev, "silicon_id: 0x%04x\n", uc->info.silicon_id);

Are those prints really useful for anybody?

If you need the values from that ccg_dev_info for debugging, then I
think debugfs is the appropriate place to expose them.

> +	return 0;
> +}
> +
>  static int ucsi_ccg_probe(struct i2c_client *client,
>  			  const struct i2c_device_id *id)
>  {
> @@ -248,6 +314,12 @@ static int ucsi_ccg_probe(struct i2c_client *client,
>  		return status;
>  	}
>  
> +	status = get_fw_info(uc);
> +	if (status < 0) {
> +		dev_err(uc->dev, "get_fw_info failed - %d\n", status);
> +		return status;
> +	}
> +
>  	status = devm_request_threaded_irq(dev, client->irq, NULL,
>  					   ccg_irq_handler,
>  					   IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> -- 
> 2.17.1

Br,
Ajay Gupta Jan. 24, 2019, 5:41 p.m. UTC | #3
Hi Greg

> -----Original Message-----
> From: Greg KH <greg@kroah.com>
> Sent: Thursday, January 17, 2019 11:06 PM
> To: Ajay Gupta <ajayg@nvidia.com>
> Cc: heikki.krogerus@linux.intel.com; linux-usb@vger.kernel.org
> Subject: Re: [PATCH 1/7] usb: typec: ucsi: add get_fw_info function
> 
> On Thu, Jan 17, 2019 at 05:09:03PM -0800, Ajay Gupta wrote:
> > Function is to get the details of ccg firmware and device version.
> >
> > Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> > ---
> >  drivers/usb/typec/ucsi/ucsi_ccg.c | 76
> > ++++++++++++++++++++++++++++++-
> >  1 file changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > index de8a43bdff68..4d35279ab853 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > @@ -17,15 +17,46 @@
> >  #include <asm/unaligned.h>
> >  #include "ucsi.h"
> >
> > +struct ccg_dev_info {
> > +	u8 fw_mode:2;
> > +	u8 two_pd_ports:2;
> > +	u8 row_size_256:2;
> > +	u8:1; /* reserved */
> > +	u8 hpi_v2_mode:1;
> > +	u8 bl_mode:1;
> > +	u8 cfgtbl_invalid:1;
> > +	u8 fw1_invalid:1;
> > +	u8 fw2_invalid:1;
> > +	u8:4; /* reserved */
> > +	u16 silicon_id;
> > +	u16 bl_last_row;
> > +} __packed;
> 
> Doesn't all of this break into tiny pieces when you read the memory into a big-
> endian system?  Be very careful when using bitfields as a "raw"
> representation of a structure in memory.
I am planning to drop bit fields itself.

> > +
> > +struct version_format {
> > +	u16 build;
> > +	u8 patch;
> > +	u8 min:4;
> > +	u8 maj:4;
> > +};
> 
> Same here.
> 
> And not packed?  That's dangerous :)
Same here.

> 
> 
> > +
> > +struct version_info {
> > +	struct version_format base;
> > +	struct version_format app;
> > +};
> > +
> >  struct ucsi_ccg {
> >  	struct device *dev;
> >  	struct ucsi *ucsi;
> >  	struct ucsi_ppm ppm;
> >  	struct i2c_client *client;
> > +	struct ccg_dev_info info;
> >  };
> >
> > -#define CCGX_RAB_INTR_REG			0x06
> > -#define CCGX_RAB_UCSI_CONTROL			0x39
> > +#define CCGX_RAB_DEVICE_MODE			0x0000
> > +#define CCGX_RAB_INTR_REG			0x0006
> > +#define CCGX_RAB_READ_ALL_VER			0x0010
> > +#define CCGX_RAB_READ_FW2_VER			0x0020
> > +#define CCGX_RAB_UCSI_CONTROL			0x0039
> >  #define CCGX_RAB_UCSI_CONTROL_START		BIT(0)
> >  #define CCGX_RAB_UCSI_CONTROL_STOP		BIT(1)
> >  #define CCGX_RAB_UCSI_DATA_BLOCK(offset)	(0xf000 | ((offset) &
> 0xff))
> > @@ -220,6 +251,41 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
> >  	return IRQ_HANDLED;
> >  }
> >
> > +static int get_fw_info(struct ucsi_ccg *uc) {
> > +	struct device *dev = uc->dev;
> > +	struct version_info version[3];
> > +	struct version_info *v;
> > +	int err, i;
> > +
> > +	err = ccg_read(uc, CCGX_RAB_READ_ALL_VER, (u8 *)(&version),
> > +		       sizeof(version));
> > +	if (err < 0)
> > +		return err;
> > +
> > +	for (i = 1; i < ARRAY_SIZE(version); i++) {
> > +		v = &version[i];
> > +		dev_dbg(dev,
> > +			"FW%d Version: %c%c v%x.%x%x, [Base
> %d.%d.%d.%d]\n",
> > +			i, (v->app.build >> 8), (v->app.build & 0xFF),
> > +			v->app.patch, v->app.maj, v->app.min,
> > +			v->base.maj, v->base.min, v->base.patch,
> > +			v->base.build);
> > +	}
> > +
> > +	err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
> > +		       sizeof(uc->info));
> > +	if (err < 0)
> > +		return err;
> > +
> > +	dev_dbg(dev, "fw_mode: %d\n", uc->info.fw_mode);
> > +	dev_dbg(dev, "fw1_invalid: %d\n", uc->info.fw1_invalid);
> > +	dev_dbg(dev, "fw2_invalid: %d\n", uc->info.fw2_invalid);
> > +	dev_dbg(dev, "silicon_id: 0x%04x\n", uc->info.silicon_id);
> > +
> > +	return 0;
> > +}
> > +
> >  static int ucsi_ccg_probe(struct i2c_client *client,
> >  			  const struct i2c_device_id *id)
> >  {
> > @@ -248,6 +314,12 @@ static int ucsi_ccg_probe(struct i2c_client *client,
> >  		return status;
> >  	}
> >
> > +	status = get_fw_info(uc);
> > +	if (status < 0) {
> > +		dev_err(uc->dev, "get_fw_info failed - %d\n", status);
> > +		return status;
> 
> Are all devices required to have this information?  if not, you just prevented
> them from working I think :(
The error is due to ccg_read() failure and in that case we want to stop.

Thanks
> nvpublic 
> thanks,
> 
> greg k-h
Ajay Gupta Jan. 24, 2019, 5:45 p.m. UTC | #4
Hi Heikki,

> > Function is to get the details of ccg firmware and device version.
> >
> > Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> > ---
> >  drivers/usb/typec/ucsi/ucsi_ccg.c | 76
> > ++++++++++++++++++++++++++++++-
> >  1 file changed, 74 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > index de8a43bdff68..4d35279ab853 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > @@ -17,15 +17,46 @@
> >  #include <asm/unaligned.h>
> >  #include "ucsi.h"
> >
> > +struct ccg_dev_info {
> > +	u8 fw_mode:2;
> > +	u8 two_pd_ports:2;
> > +	u8 row_size_256:2;
> > +	u8:1; /* reserved */
> > +	u8 hpi_v2_mode:1;
> > +	u8 bl_mode:1;
> > +	u8 cfgtbl_invalid:1;
> > +	u8 fw1_invalid:1;
> > +	u8 fw2_invalid:1;
> > +	u8:4; /* reserved */
> > +	u16 silicon_id;
> > +	u16 bl_last_row;
> > +} __packed;
> > +
> > +struct version_format {
> > +	u16 build;
> > +	u8 patch;
> > +	u8 min:4;
> > +	u8 maj:4;
> > +};
> 
> Don't use bit fields. We shouldn't use them even in the core ucsi driver with the
> message data structures like we do.

So are you saying to remove bit fields from structure and instead use shift and mask
on u8 variable to get bit fields?
 
> > +struct version_info {
> > +	struct version_format base;
> > +	struct version_format app;
> > +};
> > +
> >  struct ucsi_ccg {
> >  	struct device *dev;
> >  	struct ucsi *ucsi;
> >  	struct ucsi_ppm ppm;
> >  	struct i2c_client *client;
> > +	struct ccg_dev_info info;
> >  };
> >
> > -#define CCGX_RAB_INTR_REG			0x06
> > -#define CCGX_RAB_UCSI_CONTROL			0x39
> > +#define CCGX_RAB_DEVICE_MODE			0x0000
> > +#define CCGX_RAB_INTR_REG			0x0006
> > +#define CCGX_RAB_READ_ALL_VER			0x0010
> > +#define CCGX_RAB_READ_FW2_VER			0x0020
> > +#define CCGX_RAB_UCSI_CONTROL			0x0039
> >  #define CCGX_RAB_UCSI_CONTROL_START		BIT(0)
> >  #define CCGX_RAB_UCSI_CONTROL_STOP		BIT(1)
> >  #define CCGX_RAB_UCSI_DATA_BLOCK(offset)	(0xf000 | ((offset) &
> 0xff))
> > @@ -220,6 +251,41 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
> >  	return IRQ_HANDLED;
> >  }
> >
> > +static int get_fw_info(struct ucsi_ccg *uc) {
> > +	struct device *dev = uc->dev;
> > +	struct version_info version[3];
> > +	struct version_info *v;
> > +	int err, i;
> > +
> > +	err = ccg_read(uc, CCGX_RAB_READ_ALL_VER, (u8 *)(&version),
> > +		       sizeof(version));
> > +	if (err < 0)
> > +		return err;
> > +
> > +	for (i = 1; i < ARRAY_SIZE(version); i++) {
> > +		v = &version[i];
> > +		dev_dbg(dev,
> > +			"FW%d Version: %c%c v%x.%x%x, [Base
> %d.%d.%d.%d]\n",
> > +			i, (v->app.build >> 8), (v->app.build & 0xFF),
> > +			v->app.patch, v->app.maj, v->app.min,
> > +			v->base.maj, v->base.min, v->base.patch,
> > +			v->base.build);
> 
> How about a sysfs file showing the fw version?
I can add that.

> 
> > +	}
> > +
> > +	err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
> > +		       sizeof(uc->info));
> > +	if (err < 0)
> > +		return err;
> > +
> > +	dev_dbg(dev, "fw_mode: %d\n", uc->info.fw_mode);
> > +	dev_dbg(dev, "fw1_invalid: %d\n", uc->info.fw1_invalid);
> > +	dev_dbg(dev, "fw2_invalid: %d\n", uc->info.fw2_invalid);
> > +	dev_dbg(dev, "silicon_id: 0x%04x\n", uc->info.silicon_id);
> 
> Are those prints really useful for anybody?
They are mostly informational purpose.
> 
> If you need the values from that ccg_dev_info for debugging, then I think
> debugfs is the appropriate place to expose them.
Will remove them.

Thanks
> nvpublic
> > +	return 0;
> > +}
> > +
> >  static int ucsi_ccg_probe(struct i2c_client *client,
> >  			  const struct i2c_device_id *id)
> >  {
> > @@ -248,6 +314,12 @@ static int ucsi_ccg_probe(struct i2c_client *client,
> >  		return status;
> >  	}
> >
> > +	status = get_fw_info(uc);
> > +	if (status < 0) {
> > +		dev_err(uc->dev, "get_fw_info failed - %d\n", status);
> > +		return status;
> > +	}
> > +
> >  	status = devm_request_threaded_irq(dev, client->irq, NULL,
> >  					   ccg_irq_handler,
> >  					   IRQF_ONESHOT |
> IRQF_TRIGGER_HIGH,
> > --
> > 2.17.1
> 
> Br,
> 
> --
> heikki
Heikki Krogerus Jan. 25, 2019, 1:55 p.m. UTC | #5
On Thu, Jan 24, 2019 at 05:45:48PM +0000, Ajay Gupta wrote:
> Hi Heikki,
> 
> > > Function is to get the details of ccg firmware and device version.
> > >
> > > Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> > > ---
> > >  drivers/usb/typec/ucsi/ucsi_ccg.c | 76
> > > ++++++++++++++++++++++++++++++-
> > >  1 file changed, 74 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > index de8a43bdff68..4d35279ab853 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> > > @@ -17,15 +17,46 @@
> > >  #include <asm/unaligned.h>
> > >  #include "ucsi.h"
> > >
> > > +struct ccg_dev_info {
> > > +	u8 fw_mode:2;
> > > +	u8 two_pd_ports:2;
> > > +	u8 row_size_256:2;
> > > +	u8:1; /* reserved */
> > > +	u8 hpi_v2_mode:1;
> > > +	u8 bl_mode:1;
> > > +	u8 cfgtbl_invalid:1;
> > > +	u8 fw1_invalid:1;
> > > +	u8 fw2_invalid:1;
> > > +	u8:4; /* reserved */
> > > +	u16 silicon_id;
> > > +	u16 bl_last_row;
> > > +} __packed;
> > > +
> > > +struct version_format {
> > > +	u16 build;
> > > +	u8 patch;
> > > +	u8 min:4;
> > > +	u8 maj:4;
> > > +};
> > 
> > Don't use bit fields. We shouldn't use them even in the core ucsi driver with the
> > message data structures like we do.
> 
> So are you saying to remove bit fields from structure and instead use shift and mask
> on u8 variable to get bit fields?

Yes, exactly.

thanks,
diff mbox series

Patch

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index de8a43bdff68..4d35279ab853 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -17,15 +17,46 @@ 
 #include <asm/unaligned.h>
 #include "ucsi.h"
 
+struct ccg_dev_info {
+	u8 fw_mode:2;
+	u8 two_pd_ports:2;
+	u8 row_size_256:2;
+	u8:1; /* reserved */
+	u8 hpi_v2_mode:1;
+	u8 bl_mode:1;
+	u8 cfgtbl_invalid:1;
+	u8 fw1_invalid:1;
+	u8 fw2_invalid:1;
+	u8:4; /* reserved */
+	u16 silicon_id;
+	u16 bl_last_row;
+} __packed;
+
+struct version_format {
+	u16 build;
+	u8 patch;
+	u8 min:4;
+	u8 maj:4;
+};
+
+struct version_info {
+	struct version_format base;
+	struct version_format app;
+};
+
 struct ucsi_ccg {
 	struct device *dev;
 	struct ucsi *ucsi;
 	struct ucsi_ppm ppm;
 	struct i2c_client *client;
+	struct ccg_dev_info info;
 };
 
-#define CCGX_RAB_INTR_REG			0x06
-#define CCGX_RAB_UCSI_CONTROL			0x39
+#define CCGX_RAB_DEVICE_MODE			0x0000
+#define CCGX_RAB_INTR_REG			0x0006
+#define CCGX_RAB_READ_ALL_VER			0x0010
+#define CCGX_RAB_READ_FW2_VER			0x0020
+#define CCGX_RAB_UCSI_CONTROL			0x0039
 #define CCGX_RAB_UCSI_CONTROL_START		BIT(0)
 #define CCGX_RAB_UCSI_CONTROL_STOP		BIT(1)
 #define CCGX_RAB_UCSI_DATA_BLOCK(offset)	(0xf000 | ((offset) & 0xff))
@@ -220,6 +251,41 @@  static irqreturn_t ccg_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static int get_fw_info(struct ucsi_ccg *uc)
+{
+	struct device *dev = uc->dev;
+	struct version_info version[3];
+	struct version_info *v;
+	int err, i;
+
+	err = ccg_read(uc, CCGX_RAB_READ_ALL_VER, (u8 *)(&version),
+		       sizeof(version));
+	if (err < 0)
+		return err;
+
+	for (i = 1; i < ARRAY_SIZE(version); i++) {
+		v = &version[i];
+		dev_dbg(dev,
+			"FW%d Version: %c%c v%x.%x%x, [Base %d.%d.%d.%d]\n",
+			i, (v->app.build >> 8), (v->app.build & 0xFF),
+			v->app.patch, v->app.maj, v->app.min,
+			v->base.maj, v->base.min, v->base.patch,
+			v->base.build);
+	}
+
+	err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
+		       sizeof(uc->info));
+	if (err < 0)
+		return err;
+
+	dev_dbg(dev, "fw_mode: %d\n", uc->info.fw_mode);
+	dev_dbg(dev, "fw1_invalid: %d\n", uc->info.fw1_invalid);
+	dev_dbg(dev, "fw2_invalid: %d\n", uc->info.fw2_invalid);
+	dev_dbg(dev, "silicon_id: 0x%04x\n", uc->info.silicon_id);
+
+	return 0;
+}
+
 static int ucsi_ccg_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
@@ -248,6 +314,12 @@  static int ucsi_ccg_probe(struct i2c_client *client,
 		return status;
 	}
 
+	status = get_fw_info(uc);
+	if (status < 0) {
+		dev_err(uc->dev, "get_fw_info failed - %d\n", status);
+		return status;
+	}
+
 	status = devm_request_threaded_irq(dev, client->irq, NULL,
 					   ccg_irq_handler,
 					   IRQF_ONESHOT | IRQF_TRIGGER_HIGH,