Re: [PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

2017-06-05 Thread kgunda 

On 2017-06-02 23:59, Stephen Boyd wrote:

On 06/01, kgu...@codeaurora.org wrote:

>>@@ -209,23 +210,24 @@ static void pa_read_data(struct
>>spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc)
>>  * @buf:   buffer to write. length must be bc + 1.
>>  */
>> static void
>>-pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32
>>reg, u8 bc)
>>+pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg,
>>u8 bc)
>> {
>>u32 data = 0;
>>+
>>memcpy(, buf, (bc & 3) + 1);
>>-   __raw_writel(data, dev->wr_base + reg);
>>+   pmic_arb_base_write(pa, reg, data);
>
>This is an unrelated change. Not sure what's going on with this
>diff but we most likely want to keep the __raw_writel() here. See
>how renames introduce bugs and why we don't value them?
>
Actually pmic_arb_base_write has the writel_relaxed inside it.
that's why we removed the __raw_writel to use the common function.
Anyways, we drop the renaming patch from this patch series.


__raw_writel() is there on purpose because we're reading bytes at
a time and the CPU could be big-endian or little-endian.
readl_relaxed() would do a byte swap which we don't want.

ok. Thanks for clarifying it. We do not remove the __raw_writel.

Re: [PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

2017-06-05 Thread kgunda 

On 2017-06-02 23:59, Stephen Boyd wrote:

On 06/01, kgu...@codeaurora.org wrote:

>>@@ -209,23 +210,24 @@ static void pa_read_data(struct
>>spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc)
>>  * @buf:   buffer to write. length must be bc + 1.
>>  */
>> static void
>>-pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32
>>reg, u8 bc)
>>+pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg,
>>u8 bc)
>> {
>>u32 data = 0;
>>+
>>memcpy(, buf, (bc & 3) + 1);
>>-   __raw_writel(data, dev->wr_base + reg);
>>+   pmic_arb_base_write(pa, reg, data);
>
>This is an unrelated change. Not sure what's going on with this
>diff but we most likely want to keep the __raw_writel() here. See
>how renames introduce bugs and why we don't value them?
>
Actually pmic_arb_base_write has the writel_relaxed inside it.
that's why we removed the __raw_writel to use the common function.
Anyways, we drop the renaming patch from this patch series.


__raw_writel() is there on purpose because we're reading bytes at
a time and the CPU could be big-endian or little-endian.
readl_relaxed() would do a byte swap which we don't want.

ok. Thanks for clarifying it. We do not remove the __raw_writel.

Re: [PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

2017-06-02 Thread Stephen Boyd 
On 06/01, kgu...@codeaurora.org wrote:
> >>@@ -209,23 +210,24 @@ static void pa_read_data(struct
> >>spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc)
> >>  * @buf:   buffer to write. length must be bc + 1.
> >>  */
> >> static void
> >>-pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32
> >>reg, u8 bc)
> >>+pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg,
> >>u8 bc)
> >> {
> >>u32 data = 0;
> >>+
> >>memcpy(, buf, (bc & 3) + 1);
> >>-   __raw_writel(data, dev->wr_base + reg);
> >>+   pmic_arb_base_write(pa, reg, data);
> >
> >This is an unrelated change. Not sure what's going on with this
> >diff but we most likely want to keep the __raw_writel() here. See
> >how renames introduce bugs and why we don't value them?
> >
> Actually pmic_arb_base_write has the writel_relaxed inside it.
> that's why we removed the __raw_writel to use the common function.
> Anyways, we drop the renaming patch from this patch series.

__raw_writel() is there on purpose because we're reading bytes at
a time and the CPU could be big-endian or little-endian.
readl_relaxed() would do a byte swap which we don't want.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

2017-06-02 Thread Stephen Boyd 
On 06/01, kgu...@codeaurora.org wrote:
> >>@@ -209,23 +210,24 @@ static void pa_read_data(struct
> >>spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc)
> >>  * @buf:   buffer to write. length must be bc + 1.
> >>  */
> >> static void
> >>-pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32
> >>reg, u8 bc)
> >>+pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg,
> >>u8 bc)
> >> {
> >>u32 data = 0;
> >>+
> >>memcpy(, buf, (bc & 3) + 1);
> >>-   __raw_writel(data, dev->wr_base + reg);
> >>+   pmic_arb_base_write(pa, reg, data);
> >
> >This is an unrelated change. Not sure what's going on with this
> >diff but we most likely want to keep the __raw_writel() here. See
> >how renames introduce bugs and why we don't value them?
> >
> Actually pmic_arb_base_write has the writel_relaxed inside it.
> that's why we removed the __raw_writel to use the common function.
> Anyways, we drop the renaming patch from this patch series.

__raw_writel() is there on purpose because we're reading bytes at
a time and the CPU could be big-endian or little-endian.
readl_relaxed() would do a byte swap which we don't want.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

2017-06-01 Thread kgunda 

Thanks Stephen for reviewing the patches. Responses inline.

Thanks,
Kiran

On 2017-05-31 06:16, Stephen Boyd wrote:

On 05/30, Kiran Gunda wrote:

From: Abhijeet Dharmapurikar 

Usually *_dev best used for structures that embed a struct device in
them. spmi_pmic_arb_dev doesn't embed one. It is simply a driver data
structure. Use an appropriate name for it.

Also there are many places in the driver that left shift the bit to
generate a bit mask. Replace it with the BIT() macro.


That would be a different patch because the subject doesn't even
mention this.


Sure. Agree. Will split this in to different patch.


Signed-off-by: Abhijeet Dharmapurikar 
Signed-off-by: Kiran Gunda 
---
 drivers/spmi/spmi-pmic-arb.c | 164 
+--


Would also be nice if you ran scripts/objdiff on this so we can
be confident the code didn't change.


Sure. Will do that in the next patch.


 1 file changed, 82 insertions(+), 82 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c 
b/drivers/spmi/spmi-pmic-arb.c

index df463d4..7f918ea 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -58,10 +58,10 @@

 /* Channel Status fields */
 enum pmic_arb_chnl_status {
-   PMIC_ARB_STATUS_DONE= (1 << 0),
-   PMIC_ARB_STATUS_FAILURE = (1 << 1),
-   PMIC_ARB_STATUS_DENIED  = (1 << 2),
-   PMIC_ARB_STATUS_DROPPED = (1 << 3),
+   PMIC_ARB_STATUS_DONE= BIT(0),
+   PMIC_ARB_STATUS_FAILURE = BIT(1),
+   PMIC_ARB_STATUS_DENIED  = BIT(2),
+   PMIC_ARB_STATUS_DROPPED = BIT(3),
 };

 /* Command register fields */
@@ -99,7 +99,7 @@ enum pmic_arb_cmd_op_code {
 struct pmic_arb_ver_ops;

 /**
- * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
+ * spmi_pmic_arb - SPMI PMIC Arbiter object
  *
  * @rd_base:   on v1 "core", on v2 "observer" register base off DT.
  * @wr_base:   on v1 "core", on v2 "chnls"register base off DT.
@@ -120,7 +120,7 @@ enum pmic_arb_cmd_op_code {
  * @ppid_to_chan	in-memory copy of PPID -> channel (APID) mapping 
table.

  * v2 only.
  */
-struct spmi_pmic_arb_dev {
+struct spmi_pmic_arb {
void __iomem*rd_base;
void __iomem*wr_base;
void __iomem*intr;
@@ -164,10 +164,10 @@ struct spmi_pmic_arb_dev {
  * on v2 offset of SPMI_PIC_IRQ_CLEARn.
  */
 struct pmic_arb_ver_ops {
-   int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+   int (*mode)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,


But we leave dev here? I'm losing faith that this patch is
worthwhile.

Ok. As per your suggestion we will drop this renaming patch.




mode_t *mode);
/* spmi commands (read_cmd, write_cmd, cmd) functionality */
-   int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+   int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
  u32 *offset);
u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
@@ -178,16 +178,16 @@ struct pmic_arb_ver_ops {
u32 (*irq_clear)(u8 n);
 };

-static inline void pmic_arb_base_write(struct spmi_pmic_arb_dev *dev,
+static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
   u32 offset, u32 val)
 {
-   writel_relaxed(val, dev->wr_base + offset);
+   writel_relaxed(val, pa->wr_base + offset);


"pa" is a little confusing with things like physical address and
such. I would have gone for "arb", but the code is written
already, so why change it now?


Ok. As per your suggestion we will drop this renaming patch.

 }

-static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb_dev *dev,
+static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pa,
   u32 offset, u32 val)
 {
-   writel_relaxed(val, dev->rd_base + offset);
+   writel_relaxed(val, pa->rd_base + offset);
 }

 /**
@@ -196,9 +196,10 @@ static inline void pmic_arb_set_rd_cmd(struct 
spmi_pmic_arb_dev *dev,

  * @reg:   register's address
  * @buf:   output parameter, length must be bc + 1
  */
-static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 
reg, u8 bc)
+static void pa_read_data(struct spmi_pmic_arb *pa, u8 *buf, u32 reg, 
u8 bc)


In fact, I would rename these pa_{read,write}_data() functions as
pmic_arb_{read,write}_data() to be consistent. These are the only
places "pa_" is used right now.

Sure. Agree. Will rename this to pmic_arb_{read,write}_data in the next 
patch.

 {
-   u32 data = __raw_readl(dev->rd_base + reg);
+   u32 data = __raw_readl(pa->rd_base + reg);
+
memcpy(buf, , (bc & 3) + 1);
 }

@@ -209,23 +210,24 @@ static void pa_read_data(struct 
spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc)

  * @buf:   buffer to write. length must be

Re: [PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

2017-06-01 Thread kgunda 

Thanks Stephen for reviewing the patches. Responses inline.

Thanks,
Kiran

On 2017-05-31 06:16, Stephen Boyd wrote:

On 05/30, Kiran Gunda wrote:

From: Abhijeet Dharmapurikar 

Usually *_dev best used for structures that embed a struct device in
them. spmi_pmic_arb_dev doesn't embed one. It is simply a driver data
structure. Use an appropriate name for it.

Also there are many places in the driver that left shift the bit to
generate a bit mask. Replace it with the BIT() macro.


That would be a different patch because the subject doesn't even
mention this.


Sure. Agree. Will split this in to different patch.


Signed-off-by: Abhijeet Dharmapurikar 
Signed-off-by: Kiran Gunda 
---
 drivers/spmi/spmi-pmic-arb.c | 164 
+--


Would also be nice if you ran scripts/objdiff on this so we can
be confident the code didn't change.


Sure. Will do that in the next patch.


 1 file changed, 82 insertions(+), 82 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c 
b/drivers/spmi/spmi-pmic-arb.c

index df463d4..7f918ea 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -58,10 +58,10 @@

 /* Channel Status fields */
 enum pmic_arb_chnl_status {
-   PMIC_ARB_STATUS_DONE= (1 << 0),
-   PMIC_ARB_STATUS_FAILURE = (1 << 1),
-   PMIC_ARB_STATUS_DENIED  = (1 << 2),
-   PMIC_ARB_STATUS_DROPPED = (1 << 3),
+   PMIC_ARB_STATUS_DONE= BIT(0),
+   PMIC_ARB_STATUS_FAILURE = BIT(1),
+   PMIC_ARB_STATUS_DENIED  = BIT(2),
+   PMIC_ARB_STATUS_DROPPED = BIT(3),
 };

 /* Command register fields */
@@ -99,7 +99,7 @@ enum pmic_arb_cmd_op_code {
 struct pmic_arb_ver_ops;

 /**
- * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
+ * spmi_pmic_arb - SPMI PMIC Arbiter object
  *
  * @rd_base:   on v1 "core", on v2 "observer" register base off DT.
  * @wr_base:   on v1 "core", on v2 "chnls"register base off DT.
@@ -120,7 +120,7 @@ enum pmic_arb_cmd_op_code {
  * @ppid_to_chan	in-memory copy of PPID -> channel (APID) mapping 
table.

  * v2 only.
  */
-struct spmi_pmic_arb_dev {
+struct spmi_pmic_arb {
void __iomem*rd_base;
void __iomem*wr_base;
void __iomem*intr;
@@ -164,10 +164,10 @@ struct spmi_pmic_arb_dev {
  * on v2 offset of SPMI_PIC_IRQ_CLEARn.
  */
 struct pmic_arb_ver_ops {
-   int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+   int (*mode)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,


But we leave dev here? I'm losing faith that this patch is
worthwhile.

Ok. As per your suggestion we will drop this renaming patch.




mode_t *mode);
/* spmi commands (read_cmd, write_cmd, cmd) functionality */
-   int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+   int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
  u32 *offset);
u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
@@ -178,16 +178,16 @@ struct pmic_arb_ver_ops {
u32 (*irq_clear)(u8 n);
 };

-static inline void pmic_arb_base_write(struct spmi_pmic_arb_dev *dev,
+static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
   u32 offset, u32 val)
 {
-   writel_relaxed(val, dev->wr_base + offset);
+   writel_relaxed(val, pa->wr_base + offset);


"pa" is a little confusing with things like physical address and
such. I would have gone for "arb", but the code is written
already, so why change it now?


Ok. As per your suggestion we will drop this renaming patch.

 }

-static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb_dev *dev,
+static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pa,
   u32 offset, u32 val)
 {
-   writel_relaxed(val, dev->rd_base + offset);
+   writel_relaxed(val, pa->rd_base + offset);
 }

 /**
@@ -196,9 +196,10 @@ static inline void pmic_arb_set_rd_cmd(struct 
spmi_pmic_arb_dev *dev,

  * @reg:   register's address
  * @buf:   output parameter, length must be bc + 1
  */
-static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 
reg, u8 bc)
+static void pa_read_data(struct spmi_pmic_arb *pa, u8 *buf, u32 reg, 
u8 bc)


In fact, I would rename these pa_{read,write}_data() functions as
pmic_arb_{read,write}_data() to be consistent. These are the only
places "pa_" is used right now.

Sure. Agree. Will rename this to pmic_arb_{read,write}_data in the next 
patch.

 {
-   u32 data = __raw_readl(dev->rd_base + reg);
+   u32 data = __raw_readl(pa->rd_base + reg);
+
memcpy(buf, , (bc & 3) + 1);
 }

@@ -209,23 +210,24 @@ static void pa_read_data(struct 
spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc)

  * @buf:   buffer to write. length must be bc + 1.
  */
 static void
-pa_write_data(struct spmi_pmic_arb_dev *dev,

Re: [PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

2017-05-30 Thread Stephen Boyd 
On 05/30, Kiran Gunda wrote:
> From: Abhijeet Dharmapurikar 
> 
> Usually *_dev best used for structures that embed a struct device in
> them. spmi_pmic_arb_dev doesn't embed one. It is simply a driver data
> structure. Use an appropriate name for it.
> 
> Also there are many places in the driver that left shift the bit to
> generate a bit mask. Replace it with the BIT() macro.

That would be a different patch because the subject doesn't even
mention this.

> 
> Signed-off-by: Abhijeet Dharmapurikar 
> Signed-off-by: Kiran Gunda 
> ---
>  drivers/spmi/spmi-pmic-arb.c | 164 
> +--

Would also be nice if you ran scripts/objdiff on this so we can
be confident the code didn't change.

>  1 file changed, 82 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index df463d4..7f918ea 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -58,10 +58,10 @@
>  
>  /* Channel Status fields */
>  enum pmic_arb_chnl_status {
> - PMIC_ARB_STATUS_DONE= (1 << 0),
> - PMIC_ARB_STATUS_FAILURE = (1 << 1),
> - PMIC_ARB_STATUS_DENIED  = (1 << 2),
> - PMIC_ARB_STATUS_DROPPED = (1 << 3),
> + PMIC_ARB_STATUS_DONE= BIT(0),
> + PMIC_ARB_STATUS_FAILURE = BIT(1),
> + PMIC_ARB_STATUS_DENIED  = BIT(2),
> + PMIC_ARB_STATUS_DROPPED = BIT(3),
>  };
>  
>  /* Command register fields */
> @@ -99,7 +99,7 @@ enum pmic_arb_cmd_op_code {
>  struct pmic_arb_ver_ops;
>  
>  /**
> - * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
> + * spmi_pmic_arb - SPMI PMIC Arbiter object
>   *
>   * @rd_base: on v1 "core", on v2 "observer" register base off DT.
>   * @wr_base: on v1 "core", on v2 "chnls"register base off DT.
> @@ -120,7 +120,7 @@ enum pmic_arb_cmd_op_code {
>   * @ppid_to_chan in-memory copy of PPID -> channel (APID) mapping table.
>   *   v2 only.
>   */
> -struct spmi_pmic_arb_dev {
> +struct spmi_pmic_arb {
>   void __iomem*rd_base;
>   void __iomem*wr_base;
>   void __iomem*intr;
> @@ -164,10 +164,10 @@ struct spmi_pmic_arb_dev {
>   *   on v2 offset of SPMI_PIC_IRQ_CLEARn.
>   */
>  struct pmic_arb_ver_ops {
> - int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> + int (*mode)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,

But we leave dev here? I'm losing faith that this patch is
worthwhile.

>   mode_t *mode);
>   /* spmi commands (read_cmd, write_cmd, cmd) functionality */
> - int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> + int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
> u32 *offset);
>   u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
>   int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
> @@ -178,16 +178,16 @@ struct pmic_arb_ver_ops {
>   u32 (*irq_clear)(u8 n);
>  };
>  
> -static inline void pmic_arb_base_write(struct spmi_pmic_arb_dev *dev,
> +static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
>  u32 offset, u32 val)
>  {
> - writel_relaxed(val, dev->wr_base + offset);
> + writel_relaxed(val, pa->wr_base + offset);

"pa" is a little confusing with things like physical address and
such. I would have gone for "arb", but the code is written
already, so why change it now?

>  }
>  
> -static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb_dev *dev,
> +static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pa,
>  u32 offset, u32 val)
>  {
> - writel_relaxed(val, dev->rd_base + offset);
> + writel_relaxed(val, pa->rd_base + offset);
>  }
>  
>  /**
> @@ -196,9 +196,10 @@ static inline void pmic_arb_set_rd_cmd(struct 
> spmi_pmic_arb_dev *dev,
>   * @reg: register's address
>   * @buf: output parameter, length must be bc + 1
>   */
> -static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 
> bc)
> +static void pa_read_data(struct spmi_pmic_arb *pa, u8 *buf, u32 reg, u8 bc)

In fact, I would rename these pa_{read,write}_data() functions as
pmic_arb_{read,write}_data() to be consistent. These are the only
places "pa_" is used right now.

>  {
> - u32 data = __raw_readl(dev->rd_base + reg);
> + u32 data = __raw_readl(pa->rd_base + reg);
> +
>   memcpy(buf, , (bc & 3) + 1);
>  }
>  
> @@ -209,23 +210,24 @@ static void pa_read_data(struct spmi_pmic_arb_dev *dev, 
> u8 *buf, u32 reg, u8 bc)
>   * @buf: buffer to write. length must be bc + 1.
>   */
>  static void
> -pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, u8 bc)
> +pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg, u8 bc)
>  {
>   u32 data = 0;
> +
>   memcpy(, buf, (bc & 3) + 1);
> - __raw_writel(data,

Re: [PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

2017-05-30 Thread Stephen Boyd 
On 05/30, Kiran Gunda wrote:
> From: Abhijeet Dharmapurikar 
> 
> Usually *_dev best used for structures that embed a struct device in
> them. spmi_pmic_arb_dev doesn't embed one. It is simply a driver data
> structure. Use an appropriate name for it.
> 
> Also there are many places in the driver that left shift the bit to
> generate a bit mask. Replace it with the BIT() macro.

That would be a different patch because the subject doesn't even
mention this.

> 
> Signed-off-by: Abhijeet Dharmapurikar 
> Signed-off-by: Kiran Gunda 
> ---
>  drivers/spmi/spmi-pmic-arb.c | 164 
> +--

Would also be nice if you ran scripts/objdiff on this so we can
be confident the code didn't change.

>  1 file changed, 82 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index df463d4..7f918ea 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -58,10 +58,10 @@
>  
>  /* Channel Status fields */
>  enum pmic_arb_chnl_status {
> - PMIC_ARB_STATUS_DONE= (1 << 0),
> - PMIC_ARB_STATUS_FAILURE = (1 << 1),
> - PMIC_ARB_STATUS_DENIED  = (1 << 2),
> - PMIC_ARB_STATUS_DROPPED = (1 << 3),
> + PMIC_ARB_STATUS_DONE= BIT(0),
> + PMIC_ARB_STATUS_FAILURE = BIT(1),
> + PMIC_ARB_STATUS_DENIED  = BIT(2),
> + PMIC_ARB_STATUS_DROPPED = BIT(3),
>  };
>  
>  /* Command register fields */
> @@ -99,7 +99,7 @@ enum pmic_arb_cmd_op_code {
>  struct pmic_arb_ver_ops;
>  
>  /**
> - * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
> + * spmi_pmic_arb - SPMI PMIC Arbiter object
>   *
>   * @rd_base: on v1 "core", on v2 "observer" register base off DT.
>   * @wr_base: on v1 "core", on v2 "chnls"register base off DT.
> @@ -120,7 +120,7 @@ enum pmic_arb_cmd_op_code {
>   * @ppid_to_chan in-memory copy of PPID -> channel (APID) mapping table.
>   *   v2 only.
>   */
> -struct spmi_pmic_arb_dev {
> +struct spmi_pmic_arb {
>   void __iomem*rd_base;
>   void __iomem*wr_base;
>   void __iomem*intr;
> @@ -164,10 +164,10 @@ struct spmi_pmic_arb_dev {
>   *   on v2 offset of SPMI_PIC_IRQ_CLEARn.
>   */
>  struct pmic_arb_ver_ops {
> - int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> + int (*mode)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,

But we leave dev here? I'm losing faith that this patch is
worthwhile.

>   mode_t *mode);
>   /* spmi commands (read_cmd, write_cmd, cmd) functionality */
> - int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
> + int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
> u32 *offset);
>   u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
>   int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
> @@ -178,16 +178,16 @@ struct pmic_arb_ver_ops {
>   u32 (*irq_clear)(u8 n);
>  };
>  
> -static inline void pmic_arb_base_write(struct spmi_pmic_arb_dev *dev,
> +static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
>  u32 offset, u32 val)
>  {
> - writel_relaxed(val, dev->wr_base + offset);
> + writel_relaxed(val, pa->wr_base + offset);

"pa" is a little confusing with things like physical address and
such. I would have gone for "arb", but the code is written
already, so why change it now?

>  }
>  
> -static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb_dev *dev,
> +static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pa,
>  u32 offset, u32 val)
>  {
> - writel_relaxed(val, dev->rd_base + offset);
> + writel_relaxed(val, pa->rd_base + offset);
>  }
>  
>  /**
> @@ -196,9 +196,10 @@ static inline void pmic_arb_set_rd_cmd(struct 
> spmi_pmic_arb_dev *dev,
>   * @reg: register's address
>   * @buf: output parameter, length must be bc + 1
>   */
> -static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 
> bc)
> +static void pa_read_data(struct spmi_pmic_arb *pa, u8 *buf, u32 reg, u8 bc)

In fact, I would rename these pa_{read,write}_data() functions as
pmic_arb_{read,write}_data() to be consistent. These are the only
places "pa_" is used right now.

>  {
> - u32 data = __raw_readl(dev->rd_base + reg);
> + u32 data = __raw_readl(pa->rd_base + reg);
> +
>   memcpy(buf, , (bc & 3) + 1);
>  }
>  
> @@ -209,23 +210,24 @@ static void pa_read_data(struct spmi_pmic_arb_dev *dev, 
> u8 *buf, u32 reg, u8 bc)
>   * @buf: buffer to write. length must be bc + 1.
>   */
>  static void
> -pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, u8 bc)
> +pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg, u8 bc)
>  {
>   u32 data = 0;
> +
>   memcpy(, buf, (bc & 3) + 1);
> - __raw_writel(data, dev->wr_base + reg);
> + pmic_arb_base_write(pa, reg, data);

This is an

[PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

2017-05-30 Thread Kiran Gunda 
From: Abhijeet Dharmapurikar 

Usually *_dev best used for structures that embed a struct device in
them. spmi_pmic_arb_dev doesn't embed one. It is simply a driver data
structure. Use an appropriate name for it.

Also there are many places in the driver that left shift the bit to
generate a bit mask. Replace it with the BIT() macro.

Signed-off-by: Abhijeet Dharmapurikar 
Signed-off-by: Kiran Gunda 
---
 drivers/spmi/spmi-pmic-arb.c | 164 +--
 1 file changed, 82 insertions(+), 82 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index df463d4..7f918ea 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -58,10 +58,10 @@
 
 /* Channel Status fields */
 enum pmic_arb_chnl_status {
-   PMIC_ARB_STATUS_DONE= (1 << 0),
-   PMIC_ARB_STATUS_FAILURE = (1 << 1),
-   PMIC_ARB_STATUS_DENIED  = (1 << 2),
-   PMIC_ARB_STATUS_DROPPED = (1 << 3),
+   PMIC_ARB_STATUS_DONE= BIT(0),
+   PMIC_ARB_STATUS_FAILURE = BIT(1),
+   PMIC_ARB_STATUS_DENIED  = BIT(2),
+   PMIC_ARB_STATUS_DROPPED = BIT(3),
 };
 
 /* Command register fields */
@@ -99,7 +99,7 @@ enum pmic_arb_cmd_op_code {
 struct pmic_arb_ver_ops;
 
 /**
- * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
+ * spmi_pmic_arb - SPMI PMIC Arbiter object
  *
  * @rd_base:   on v1 "core", on v2 "observer" register base off DT.
  * @wr_base:   on v1 "core", on v2 "chnls"register base off DT.
@@ -120,7 +120,7 @@ enum pmic_arb_cmd_op_code {
  * @ppid_to_chan   in-memory copy of PPID -> channel (APID) mapping table.
  * v2 only.
  */
-struct spmi_pmic_arb_dev {
+struct spmi_pmic_arb {
void __iomem*rd_base;
void __iomem*wr_base;
void __iomem*intr;
@@ -164,10 +164,10 @@ struct spmi_pmic_arb_dev {
  * on v2 offset of SPMI_PIC_IRQ_CLEARn.
  */
 struct pmic_arb_ver_ops {
-   int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+   int (*mode)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
mode_t *mode);
/* spmi commands (read_cmd, write_cmd, cmd) functionality */
-   int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+   int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
  u32 *offset);
u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
@@ -178,16 +178,16 @@ struct pmic_arb_ver_ops {
u32 (*irq_clear)(u8 n);
 };
 
-static inline void pmic_arb_base_write(struct spmi_pmic_arb_dev *dev,
+static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
   u32 offset, u32 val)
 {
-   writel_relaxed(val, dev->wr_base + offset);
+   writel_relaxed(val, pa->wr_base + offset);
 }
 
-static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb_dev *dev,
+static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pa,
   u32 offset, u32 val)
 {
-   writel_relaxed(val, dev->rd_base + offset);
+   writel_relaxed(val, pa->rd_base + offset);
 }
 
 /**
@@ -196,9 +196,10 @@ static inline void pmic_arb_set_rd_cmd(struct 
spmi_pmic_arb_dev *dev,
  * @reg:   register's address
  * @buf:   output parameter, length must be bc + 1
  */
-static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 
bc)
+static void pa_read_data(struct spmi_pmic_arb *pa, u8 *buf, u32 reg, u8 bc)
 {
-   u32 data = __raw_readl(dev->rd_base + reg);
+   u32 data = __raw_readl(pa->rd_base + reg);
+
memcpy(buf, , (bc & 3) + 1);
 }
 
@@ -209,23 +210,24 @@ static void pa_read_data(struct spmi_pmic_arb_dev *dev, 
u8 *buf, u32 reg, u8 bc)
  * @buf:   buffer to write. length must be bc + 1.
  */
 static void
-pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, u8 bc)
+pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg, u8 bc)
 {
u32 data = 0;
+
memcpy(, buf, (bc & 3) + 1);
-   __raw_writel(data, dev->wr_base + reg);
+   pmic_arb_base_write(pa, reg, data);
 }
 
 static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
  void __iomem *base, u8 sid, u16 addr)
 {
-   struct spmi_pmic_arb_dev *dev = spmi_controller_get_drvdata(ctrl);
+   struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);
u32 status = 0;
u32 timeout = PMIC_ARB_TIMEOUT_US;
u32 offset;
int rc;
 
-   rc = dev->ver_ops->offset(dev, sid, addr, );
+   rc = pa->ver_ops->offset(pa, sid, addr, );
if (rc)
return rc;
 
@@ -270,22 +272,22 @@ static int pmic_arb_wait_for_done(struct spmi_controller 
*ctrl,
 static int
 pmic_arb_non_data_cmd_v1(struct spmi_controller *ctrl, u8

[PATCH V1 02/15] spmi: pmic-arb: rename spmi_pmic_arb_dev to spmi_pmic_arb

2017-05-30 Thread Kiran Gunda 
From: Abhijeet Dharmapurikar 

Usually *_dev best used for structures that embed a struct device in
them. spmi_pmic_arb_dev doesn't embed one. It is simply a driver data
structure. Use an appropriate name for it.

Also there are many places in the driver that left shift the bit to
generate a bit mask. Replace it with the BIT() macro.

Signed-off-by: Abhijeet Dharmapurikar 
Signed-off-by: Kiran Gunda 
---
 drivers/spmi/spmi-pmic-arb.c | 164 +--
 1 file changed, 82 insertions(+), 82 deletions(-)

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index df463d4..7f918ea 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -58,10 +58,10 @@
 
 /* Channel Status fields */
 enum pmic_arb_chnl_status {
-   PMIC_ARB_STATUS_DONE= (1 << 0),
-   PMIC_ARB_STATUS_FAILURE = (1 << 1),
-   PMIC_ARB_STATUS_DENIED  = (1 << 2),
-   PMIC_ARB_STATUS_DROPPED = (1 << 3),
+   PMIC_ARB_STATUS_DONE= BIT(0),
+   PMIC_ARB_STATUS_FAILURE = BIT(1),
+   PMIC_ARB_STATUS_DENIED  = BIT(2),
+   PMIC_ARB_STATUS_DROPPED = BIT(3),
 };
 
 /* Command register fields */
@@ -99,7 +99,7 @@ enum pmic_arb_cmd_op_code {
 struct pmic_arb_ver_ops;
 
 /**
- * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
+ * spmi_pmic_arb - SPMI PMIC Arbiter object
  *
  * @rd_base:   on v1 "core", on v2 "observer" register base off DT.
  * @wr_base:   on v1 "core", on v2 "chnls"register base off DT.
@@ -120,7 +120,7 @@ enum pmic_arb_cmd_op_code {
  * @ppid_to_chan   in-memory copy of PPID -> channel (APID) mapping table.
  * v2 only.
  */
-struct spmi_pmic_arb_dev {
+struct spmi_pmic_arb {
void __iomem*rd_base;
void __iomem*wr_base;
void __iomem*intr;
@@ -164,10 +164,10 @@ struct spmi_pmic_arb_dev {
  * on v2 offset of SPMI_PIC_IRQ_CLEARn.
  */
 struct pmic_arb_ver_ops {
-   int (*mode)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+   int (*mode)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
mode_t *mode);
/* spmi commands (read_cmd, write_cmd, cmd) functionality */
-   int (*offset)(struct spmi_pmic_arb_dev *dev, u8 sid, u16 addr,
+   int (*offset)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
  u32 *offset);
u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
@@ -178,16 +178,16 @@ struct pmic_arb_ver_ops {
u32 (*irq_clear)(u8 n);
 };
 
-static inline void pmic_arb_base_write(struct spmi_pmic_arb_dev *dev,
+static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
   u32 offset, u32 val)
 {
-   writel_relaxed(val, dev->wr_base + offset);
+   writel_relaxed(val, pa->wr_base + offset);
 }
 
-static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb_dev *dev,
+static inline void pmic_arb_set_rd_cmd(struct spmi_pmic_arb *pa,
   u32 offset, u32 val)
 {
-   writel_relaxed(val, dev->rd_base + offset);
+   writel_relaxed(val, pa->rd_base + offset);
 }
 
 /**
@@ -196,9 +196,10 @@ static inline void pmic_arb_set_rd_cmd(struct 
spmi_pmic_arb_dev *dev,
  * @reg:   register's address
  * @buf:   output parameter, length must be bc + 1
  */
-static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 
bc)
+static void pa_read_data(struct spmi_pmic_arb *pa, u8 *buf, u32 reg, u8 bc)
 {
-   u32 data = __raw_readl(dev->rd_base + reg);
+   u32 data = __raw_readl(pa->rd_base + reg);
+
memcpy(buf, , (bc & 3) + 1);
 }
 
@@ -209,23 +210,24 @@ static void pa_read_data(struct spmi_pmic_arb_dev *dev, 
u8 *buf, u32 reg, u8 bc)
  * @buf:   buffer to write. length must be bc + 1.
  */
 static void
-pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, u8 bc)
+pa_write_data(struct spmi_pmic_arb *pa, const u8 *buf, u32 reg, u8 bc)
 {
u32 data = 0;
+
memcpy(, buf, (bc & 3) + 1);
-   __raw_writel(data, dev->wr_base + reg);
+   pmic_arb_base_write(pa, reg, data);
 }
 
 static int pmic_arb_wait_for_done(struct spmi_controller *ctrl,
  void __iomem *base, u8 sid, u16 addr)
 {
-   struct spmi_pmic_arb_dev *dev = spmi_controller_get_drvdata(ctrl);
+   struct spmi_pmic_arb *pa = spmi_controller_get_drvdata(ctrl);
u32 status = 0;
u32 timeout = PMIC_ARB_TIMEOUT_US;
u32 offset;
int rc;
 
-   rc = dev->ver_ops->offset(dev, sid, addr, );
+   rc = pa->ver_ops->offset(pa, sid, addr, );
if (rc)
return rc;
 
@@ -270,22 +272,22 @@ static int pmic_arb_wait_for_done(struct spmi_controller 
*ctrl,
 static int
 pmic_arb_non_data_cmd_v1(struct spmi_controller *ctrl, u8 opc, u8 sid)
 {
-   struct spmi_pmic_arb_dev *pmic_arb =