Re: [PATCH 06/44] staging: spmi: hisi-spmi-controller: use le32 macros where needed

[Mauro Carvalho Chehab]

Em Wed, 12 Aug 2020 09:21:54 -0700
Joe Perches <joe@xxxxxxxxxxx> escreveu:

> On Wed, 2020-08-12 at 17:56 +0200, Mauro Carvalho Chehab wrote:
> > Instead of manually using bswap_32(), just use the
> > le32 macros.  
> 
> Are you certain this code will now work on any endian cpu?
> 
> Maybe just use __swab32 instead

Well, I didn't test, because this driver is for an specific
hardware (arm64). Yet, what happens in practice is that just
one byte is written by the PMIC drivers. If the order is not
LE, the byte written at the buffer will always be zero.

> 
> > diff --git a/drivers/staging/hikey9xx/hisi-spmi-controller.c b/drivers/staging/hikey9xx/hisi-spmi-controller.c  
> []
> > @@ -43,11 +42,6 @@
> >  #define SPMI_APB_SPMI_CMD_TYPE_OFFSET			24
> >  #define SPMI_APB_SPMI_CMD_LENGTH_OFFSET			20
> >  
> > -#define bswap_32(X)   \
> > -    ((((u32)(X) & 0xff000000) >> 24) | \
> > -     (((u32)(X) & 0x00ff0000) >> 8) | \
> > -     (((u32)(X) & 0x0000ff00) << 8) | \
> > -     (((u32)(X) & 0x000000ff) << 24))
> >  #define SPMI_APB_SPMI_CMD_SLAVEID_OFFSET		16
> >  #define SPMI_APB_SPMI_CMD_ADDR_OFFSET			0
> >  
> > @@ -179,14 +173,15 @@ static int spmi_read_cmd(struct spmi_controller *ctrl,
> >  
> >  	writel(cmd, spmi_controller->base + chnl_ofst + SPMI_APB_SPMI_CMD_BASE_ADDR);
> >  
> > -	rc = spmi_controller_wait_for_done(spmi_controller, spmi_controller->base, sid, addr);
> > +	rc = spmi_controller_wait_for_done(spmi_controller,
> > +					   spmi_controller->base, sid, addr);
> >  	if (rc)
> >  		goto done;
> >  
> >  	i = 0;
> >  	do {
> >  		data = readl(spmi_controller->base + chnl_ofst + SPMI_SLAVE_OFFSET * sid + SPMI_APB_SPMI_RDATA0_BASE_ADDR + i * SPMI_PER_DATAREG_BYTE);
> > -		data = bswap_32(data);
> > +		data = be32_to_cpu((__be32)data);
> >  		if ((bc - i * SPMI_PER_DATAREG_BYTE) >> 2) {
> >  			memcpy(buf, &data, sizeof(data));
> >  			buf += sizeof(data);
> > @@ -210,8 +205,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
> >  {
> >  	struct spmi_controller_dev *spmi_controller = dev_get_drvdata(&ctrl->dev);
> >  	unsigned long flags;
> > -	u32 cmd;
> > -	u32 data = 0;
> > +	u32 cmd, data;
> >  	int rc;
> >  	u32 chnl_ofst = SPMI_CHANNEL_OFFSET * spmi_controller->channel;
> >  	u8 op_code, i;
> > @@ -246,7 +240,7 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
> >  
> >  	i = 0;
> >  	do {
> > -		memset(&data, 0, sizeof(data));
> > +		data = 0;
> >  		if ((bc - i * SPMI_PER_DATAREG_BYTE) >> 2) {
> >  			memcpy(&data, buf, sizeof(data));
> >  			buf += sizeof(data);
> > @@ -255,8 +249,8 @@ static int spmi_write_cmd(struct spmi_controller *ctrl,
> >  			buf += (bc % SPMI_PER_DATAREG_BYTE);
> >  		}
> >  
> > -		data = bswap_32(data);
> > -		writel(data, spmi_controller->base + chnl_ofst + SPMI_APB_SPMI_WDATA0_BASE_ADDR + SPMI_PER_DATAREG_BYTE * i);
> > +		writel((u32)cpu_to_be32(data),
> > +		       spmi_controller->base + chnl_ofst + SPMI_APB_SPMI_WDATA0_BASE_ADDR + SPMI_PER_DATAREG_BYTE * i);
> >  		i++;
> >  	} while (bc > i * SPMI_PER_DATAREG_BYTE);
> >    
>