Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 3 Aug 2022 08:56:28 -0700
From:   Colin Foster <colin.foster@in-advantage.com>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Terry Bowman <terry.bowman@amd.com>,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        Wolfram Sang <wsa@kernel.org>,
        Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
        Steen Hegelund <Steen.Hegelund@microchip.com>,
        Lars Povlsen <lars.povlsen@microchip.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Russell King <linux@armlinux.org.uk>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Lee Jones <lee.jones@linaro.org>, katie.morris@in-advantage.com
Subject: Re: [PATCH v15 mfd 9/9] mfd: ocelot: add support for the vsc7512
 chip via spi
Message-ID: <YuqarB067s+rqFKe@euler>
References: <20220803054728.1541104-1-colin.foster@in-advantage.com>
 <20220803054728.1541104-10-colin.foster@in-advantage.com>
 <CAHp75Vc30VW_dYGodyw4mrMwFgTVyDFaMP2ZJXQEB2nFOB2RWw@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHp75Vc30VW_dYGodyw4mrMwFgTVyDFaMP2ZJXQEB2nFOB2RWw@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?sQoy8L/qvWzKZZm8UJXGGHBhmUcqiypMQdn64pfHAi9SGnpREimuqQviHPni?=
 =?us-ascii?Q?eggsi+GpRZA7icvbEZ3HGVv51GXm2qMSwhJiVwWV8WIkK4iEi0m4WTG4SICn?=
 =?us-ascii?Q?i/pmoPXBfAJOtovYE9rSYhp7dJHZ6hh1Z31o5BIEJ7OE0UmMTGz9toTElCMN?=
 =?us-ascii?Q?3jsgGd+wJQ/pStFZj03rtqa4YIY/JJos2+ci9qIludP2O3YdyNteUFmr8gle?=
 =?us-ascii?Q?ZxWARmhW4844kWy9hJVQMGAC4seWLKYf2j7198U1rsgJ8aeyamFQb9xSIDn0?=
 =?us-ascii?Q?VXSl3IdPoSb32+6yn0QtPT7I2+87mqqcaqipTxSuh8NIIcIQ/VcjEHpgNFiu?=
 =?us-ascii?Q?jVgKCSV687xmP6E/JwVFNrUT6111Rs/pqnens8arZeFrSME0TlHpeMmnJ49d?=
 =?us-ascii?Q?f84wuI73cK0JiQ9E8rJKQdmbqXwaQzV+7yk/1UQUwOTTqi4JCjcMJgccGuPQ?=
 =?us-ascii?Q?60Y573V6TqWC1BWWBqbmzRY7Rn9wyb+yE4DQ1lCZCyBDYADPvFnXYhHavY1S?=
 =?us-ascii?Q?A7yzVM1rRh23ptHeFPEVMDZgsE47JM+m5054kAgOhKsZ4XH5xB8n5aGqZJ7W?=
 =?us-ascii?Q?02MsZpSfFodQoIEt4i+VJYnzJgBzvN2yi2h2P92yWv8DjHYpe5O/ydKgf4Jq?=
 =?us-ascii?Q?dXyoDnWzYM0n/N2AChRVVicw09i3aonji08E6bSIwDtMGS1Sunji5RcvStIp?=
 =?us-ascii?Q?V2qdubKPTimN43fpMu9cq+I9zge+oT6GxGmb69gwEyOZZYoRicNodCYzTUyF?=
 =?us-ascii?Q?PKWQhajTpMXuy9xyAfIHVMGZkveGMsWLENZDb0iAg8Roq8uRO6EpEvQjELk1?=
 =?us-ascii?Q?vbTq307eH5WafgFuBfDERpdL/wFx0VJ6IPbA/Labv3sqo8C2laX8fTLzPtY4?=
 =?us-ascii?Q?SXcrRBig4sdFi1G2PM4I2vKOOOpvNYvCV1nEiOoWFl0vFJyN8ube2BJK/xjT?=
 =?us-ascii?Q?594Qmrwx3yrMbzUywj7TMixV4Qau5v9Px6Utvr8jEX/L1w2TkR63xI26TnNP?=
 =?us-ascii?Q?c4Ba5FjhPdNmdoUF54nZGMC4SySYqdyzzv8gwDjoeqWOoTppL3PM3TdeJ2XJ?=
 =?us-ascii?Q?zduxJIgxJjZm3cdGercprRwS7KEGZJr2B+PJMu/SDXQlavSmI7CzmPtK4a3T?=
 =?us-ascii?Q?Doh9js63/GnxauTTT9jtNF4Wm1AEWmz4vHaMBU7GYv45BdopaGnNPa80HiSZ?=
 =?us-ascii?Q?O2oGvEIHQIHufiFCXNiSnpmTRwjV5Ch4wZdyf6TtX+mRxKFFrtw9wWVT56wv?=
 =?us-ascii?Q?RwN0y505fPsmq/hIqk4LYw7yEYWdMU79daTMgjkG+qlB9SZLNyS29CmzI5gJ?=
 =?us-ascii?Q?NOPUl2aN6gc1uyKAIOeWZN0TzlF7inDaojHbqJeYKshgE3SqYJMyJXmTX6RN?=
 =?us-ascii?Q?nsDpedf+lqcLWjZ/HcI/rZ6GSWS4zgOYj40OZ868GLvQX4okV2/7+QmOX454?=
 =?us-ascii?Q?cTr/Zyk6VBPdVCKfCjC814OxAWTAOmdr9yW8jqzLJtNFQ2sLqZE/t2Ezhozw?=
 =?us-ascii?Q?ZKXb+FztnXpnE5BoX3dxi3V86OhEHcSTZR4CZmxeBieN4rx12int8/sGlQLS?=
 =?us-ascii?Q?GPyrTrZOF0m07bgiRo2luPkdXAOKeE49UpYRgti5oN/y2JqXEY50B8GJtS8m?=
 =?us-ascii?Q?mfTcGiJ2VEc/npqwwBPuaE4=3D?=
X-OriginatorOrg: in-advantage.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ebb021eb-11f2-4eb6-6b1f-08da7568bcda
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2351.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Aug 2022 15:56:33.5419
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 48e842ca-fbd8-4633-a79d-0c955a7d3aae
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: t6jL0BvNWSXt0CA80tl7FiqwAddPJ535QBOq/TQJ5PTBh7iHnBFhXuLEsnGWRsgnVldggmpFbFuPxdjHV+OrmJZe+79ujHpcOpbCFZNArYE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB3386
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Aug 03, 2022 at 01:45:04PM +0200, Andy Shevchenko wrote:
> On Wed, Aug 3, 2022 at 7:48 AM Colin Foster
> <colin.foster@in-advantage.com> wrote:
> >
> > The VSC7512 is a networking chip that contains several peripherals. Many of
> > these peripherals are currently supported by the VSC7513 and VSC7514 chips,
> > but those run on an internal CPU. The VSC7512 lacks this CPU, and must be
> > controlled externally.
> >
> > Utilize the existing drivers by referencing the chip as an MFD. Add support
> > for the two MDIO buses, the internal phys, pinctrl, and serial GPIO.
> 
> 
> ...
> 
> > +#include <asm/byteorder.h>
> 
> Not sure I see the user of this header.

Interesting. And I think you uncovered one more issue.

I'd used byteorder at one time to modify the SPI payload (addr is always
big-endian, payload should always be native).

When I migrated to using the spi_bus_read() instead of spi_reg_read(),
this became handled much more elegantly in regmap itself. So this isn't
needed anymore.

I also have byteorder in include/linux/mfd/ocelot.h, where it also isn't
needed. It is checking:

#ifdef __LITTLE_ENDIAN
#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_LE
#else
#define OCELOT_SPI_BYTE_ORDER OCELOT_SPI_BYTE_ORDER_BE
#endif


That file should be replaced with
#include <linux/kconfig.h>

> 
> ...
> 
> > +struct regmap *ocelot_spi_init_regmap(struct device *dev, const struct resource *res)
> > +{
> > +       struct regmap_config regmap_config;
> > +
> > +       memcpy(&regmap_config, &ocelot_spi_regmap_config, sizeof(regmap_config));
> > +
> > +       regmap_config.name = res->name;
> 
> > +       regmap_config.max_register = res->end - res->start;
> 
> Hmm... First of all, resource_size() is for that (with - 1 to the
> result). But don't you need to use stride in the calculations?

DEFINE_RES_NAMED populates the resource .end with (_start) + (_size) - 1
so I don't think resource_size is correct to use here.

reg_stride gets handled at the top of regmap_read(), so I don't think
that's really needed either.


For reference:

#define VSC7512_DEVCPU_ORG_RES_START    0x71000000
#define VSC7512_DEVCPU_ORG_RES_SIZE     0x38


# pwd
/sys/kernel/debug/regmap/spi0.0-devcpu_org
# cat range
0-34
# cat registers
00: 00000000
04: 02000001
08: 00000001
0c: 00000000
10: 00000fff
14: 00000000
18: 00000000
1c: 00000000
20: 00000000
24: 00000000
28: 00000001
2c: 00000004
30: 00000001
34: 00000004


> 
> > +       regmap_config.reg_base = res->start;
> > +
> > +       return devm_regmap_init(dev, &ocelot_spi_regmap_bus, dev, &regmap_config);
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko