Re: [PATCH V1 10/15] spmi: pmic_arb: add support for PMIC bus arbiter v3

[kgunda]

On 2017-06-01 03:48, Stephen Boyd wrote:
> On 05/30, Kiran Gunda wrote:
> >  /* PMIC Arbiter channel registers offsets */
> > @@ -96,6 +97,17 @@ enum pmic_arb_cmd_op_code {
> >  /* interrupt enable bit */
> >  #define SPMI_PIC_ACC_ENABLE_BIT		BIT(0)
> >
> > +#define HWIRQ(slave_id, periph_id, irq_id, apid) \
> > +	((((slave_id) & 0xF)   << 28) | \
> > +	(((periph_id) & 0xFF)  << 20) | \
> > +	(((irq_id)    & 0x7)   << 16) | \
> > +	(((apid)      & 0x1FF) << 0))
> > +
> > +#define HWIRQ_SID(hwirq)  (((hwirq) >> 28) & 0xF)
> > +#define HWIRQ_PER(hwirq)  (((hwirq) >> 20) & 0xFF)
> > +#define HWIRQ_IRQ(hwirq)  (((hwirq) >> 16) & 0x7)
> > +#define HWIRQ_APID(hwirq) (((hwirq) >> 0)  & 0x1FF)
>
> How about lowercase and hwirq_to_*() macros? Then it's more
> function like style. And spec_to_hwirq() or something?
Sure. Will change it in the follow up patch.
> > +
> >  struct pmic_arb_ver_ops;
> >
> >  struct apid_data {
> > @@ -151,7 +163,9 @@ struct spmi_pmic_arb {
> >  /**
> >   * pmic_arb_ver: version dependent functionality.
> >   *
> > - * @mode:	access rights to specified pmic peripheral.
> > + * @ver_str:		version string.
> > + * @ppid_to_apid:	finds the apid for a given ppid.
> > + * @mode:		access rights to specified pmic peripheral.
>
> mode didn't need to change tabbing. Not sure why it's appearing
> in the diff.
When adding the new field description added extra tab for mode for 
alignment.
That's why it is showing up.
> >   * @non_data_cmd:	on v1 issues an spmi non-data command.
> >   *			on v2 no HW support, returns -EOPNOTSUPP.
> >   * @offset:		on v1 offset of per-ee channel.
> > @@ -177,10 +192,10 @@ struct pmic_arb_ver_ops {
> >  	u32 (*fmt_cmd)(u8 opc, u8 sid, u16 addr, u8 bc);
> >  	int (*non_data_cmd)(struct spmi_controller *ctrl, u8 opc, u8 sid);
> >  	/* Interrupts controller functionality (offset of PIC registers) */
> > -	u32 (*owner_acc_status)(u8 m, u8 n);
> > -	u32 (*acc_enable)(u8 n);
> > -	u32 (*irq_status)(u8 n);
> > -	u32 (*irq_clear)(u8 n);
> > +	u32 (*owner_acc_status)(u8 m, u16 n);
> > +	u32 (*acc_enable)(u16 n);
> > +	u32 (*irq_status)(u16 n);
> > +	u32 (*irq_clear)(u16 n);
> >  };
> >
> >  static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
> > @@ -776,7 +787,7 @@ static int qpnpint_irq_domain_map(struct 
> > irq_domain *d,
> >  }
> >
> >  static int
> > -pmic_arb_mode_v1(struct spmi_pmic_arb *pa, u8 sid, u16 addr, mode_t 
> > *mode)
> > +pmic_arb_mode_v1_v3(struct spmi_pmic_arb *pa, u8 sid, u16 addr, 
> > mode_t *mode)
>
> Nice to know that the mode became useless in v3 and beyond! Sigh.
>
> >  {
> >  	*mode = S_IRUSR | S_IWUSR;
> >  	return 0;
> > @@ -987,21 +1017,21 @@ static int spmi_pmic_arb_probe(struct 
> > platform_device *pdev)
> >
> >  		/* the apid to ppid table starts at PMIC_ARB_REG_CHNL(0) */
> > -		pa->max_periph =  (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4;
> > +		pa->max_periph = (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4;
>
> This looks unrelated?
Yes. Just removed the extra space after '='.
> >  		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> >  						   "obsrvr");