Re: [PATCH v2 13/14] spi: bcmbca-hsspi: Add driver for newer HSSPI controller

[William Zhang]

On 01/26/2023 07:16 AM, Jonas Gorski wrote:
> > +static void bcmbca_hsspi_set_cs(struct bcmbca_hsspi *bs, unsigned int cs,
> > +                                bool active)
> > +{
> > +       u32 reg;
> > +
> > +       /* No cs orerriden needed for SS7 internal cs on pcm based voice dev */
> > +       if (cs == 7)
> > +               return;
> > +
> > +       mutex_lock(&bs->bus_mutex);
> > +
> > +       if (active) {
> > +               /* activate cs by setting the override bit and active value bit*/
> > +               reg = __raw_readl(bs->spim_ctrl);
> > +               reg |= BIT(cs+SPIM_CTRL_CS_OVERRIDE_SEL_SHIFT);
>
> Doesn't checkpatch complain about missing spaces around the operator (+)?
Nope...

> > +               reg &= ~BIT(cs+SPIM_CTRL_CS_OVERRIDE_VAL_SHIFT);
> > +               if (bs->cs_polarity & BIT(cs))
> > +                       reg |= BIT(cs+SPIM_CTRL_CS_OVERRIDE_VAL_SHIFT);
>
> Does the CS_OVERRIDE_VAL get reset on clearing the OVERRIDE_SEL bit or
> it is persistent? If the latter, you could initialize its value in
> bcmbca_hsspi_setup() and then this becomes:
>
> reg = _raw_readl(bs->spim_ctrl);
> if (active)
>     reg |= BIT(cs+SPIM_CTRL_CS_OVERRIDE_SEL_SHIFT);
> else
>     reg &= ~BIT(cs + SPIM_CTRL_CS_OVERRIDE_SEL_SHIFT);
>   __raw_writel(reg, bs->spim_ctrl);
>
> and you can drop the cs_polarity field.
No. So I will try this optimization and see if that actually works in 
the controller.

> > +               __raw_writel(reg, bs->spim_ctrl);
> > +       } else {
> > +               /* clear the cs override bit */
> > +               reg = __raw_readl(bs->spim_ctrl);
> > +               reg &= ~BIT(cs+SPIM_CTRL_CS_OVERRIDE_SEL_SHIFT);
> > +               __raw_writel(reg, bs->spim_ctrl);
> > +       }
> > +
> > +       mutex_unlock(&bs->bus_mutex);
> > +}
> > +

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature