Re: [PATCH v6 12/17] spi: dw: Add support for AMD Pensando Elba SoC

[Larson, Bradley]

On 9/11/22 11:20 AM, Serge Semin wrote:
> On Wed, Aug 31, 2022 at 06:04:02PM +0000, Larson, Bradley wrote:
>> On 8/21/22 11:18 AM, Serge Semin wrote:
>>> On Sat, Aug 20, 2022 at 12:57:45PM -0700, Brad Larson wrote:
>>>> From: Brad Larson <blarson@xxxxxxx>
>>>>
>>>> The AMD Pensando Elba SoC includes a DW apb_ssi v4 controller
>>>> with device specific chip-select control.  The Elba SoC
>>>> provides four chip-selects where the native DW IP supports
>>>> two chip-selects.  The Elba DW_SPI instance has two native
>>>> CS signals that are always overridden.
>>>>
>>>> Signed-off-by: Brad Larson <blarson@xxxxxxx>
>>>> ---
>>>>    drivers/spi/spi-dw-mmio.c | 77 +++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 77 insertions(+)
>>>>
>>>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
>>>> index 26c40ea6dd12..36b8c5e10bb3 100644
>>>> --- a/drivers/spi/spi-dw-mmio.c
>>>> +++ b/drivers/spi/spi-dw-mmio.c
>>>> @@ -53,6 +53,24 @@ struct dw_spi_mscc {
>>>>         void __iomem        *spi_mst; /* Not sparx5 */
>>>>    };
>>>>
>>>> +struct dw_spi_elba {
>>>> +     struct regmap *syscon;
>>>> +};
>>>> +
>>>> +/*
>>>> + * Elba SoC does not use ssi, pin override is used for cs 0,1 and
>>>> + * gpios for cs 2,3 as defined in the device tree.
>>>> + *
>>>> + * cs:  |       1               0
>>>> + * bit: |---3-------2-------1-------0
>>>> + *      |  cs1   cs1_ovr   cs0   cs0_ovr
>>>> + */
>>>> +#define ELBA_SPICS_REG                       0x2468
>>>> +#define ELBA_SPICS_SHIFT(cs)         (2 * (cs))
>>>> +#define ELBA_SPICS_MASK(cs)          (0x3 << ELBA_SPICS_SHIFT(cs))
>>>> +#define ELBA_SPICS_SET(cs, val)      \
>>>> +                     ((((val) << 1) | 0x1) << ELBA_SPICS_SHIFT(cs))
>>> Please take the @Andy' notes into account:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2FCAHp75Vex0VkECYd%3DkY0m6%3DjXBYSXg2UFu7vn271%2BQ49WZn22GA%40mail.gmail.com%2F&amp;data=05%7C01%7Cbradley.larson%40amd.com%7C9e7cade823344b72f34608da94224dc6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637985172338293739%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=usQHWyOxaoD6iasP2R9VL5i0ZkSzBbdpsPljExHemfE%3D&amp;reserved=0
>> Yes, I had a tested change for this but missed adding to the patch update.
>> This is the change and I'll resend just this patch.
>>
>> --- a/drivers/spi/spi-dw-mmio.c
>> +++ b/drivers/spi/spi-dw-mmio.c
>> @@ -66,10 +66,6 @@ struct dw_spi_elba {
>>     *      |  cs1   cs1_ovr   cs0   cs0_ovr
>>     */
>>    #define ELBA_SPICS_REG 0x2468
>> -#define ELBA_SPICS_SHIFT(cs)           (2 * (cs))
>> -#define ELBA_SPICS_MASK(cs)            (0x3 << ELBA_SPICS_SHIFT(cs))
>> -#define ELBA_SPICS_SET(cs, val)        \
>> -                       ((((val) << 1) | 0x1) << ELBA_SPICS_SHIFT(cs))
> Why do you remove these macros? Just replace 0x3 with GENMASM(1, 0),
> 0x1 with BIT(0), (2 * (cs)) statement with ((cs) << 1) as Andy
> suggested. Using macros for such complex statement is a good practice.
>
> Also please rename ELBA_SPICS_SHIFT() to ELBA_SPICS_OFFSET() so to
> have a more coherent CSR-related macros naming in the driver.


Yes, will add back/rename macros with usage of BIT()/GENMASK() and 
resend just this patch.

Regards,
Brad