tinnedkarma commented on code in PR #16999: URL: https://github.com/apache/nuttx/pull/16999#discussion_r3413810688
########## arch/arm/src/stm32h7/stm32_mdio.c: ########## @@ -0,0 +1,232 @@ +/**************************************************************************** + * arch/arm/src/stm32h7/stm32_mdio.c + * + * SPDX-License-Identifier: Apache-2.0 + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. The + * ASF licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the + * License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + * + ****************************************************************************/ + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <arm_internal.h> + +/**************************************************************************** + * Pre-processor Definitions + ****************************************************************************/ + +#ifdef CONFIG_STM32H7_ETHMAC_REGDEBUG +static uint32_t stm32_getreg(uint32_t addr); +static void stm32_putreg(uint32_t val, uint32_t addr); +static void stm32_checksetup(void); +#else +# define stm32_getreg(addr) getreg32(addr) +# define stm32_putreg(val,addr) putreg32(val,addr) +# define stm32_checksetup() +#endif + +/**************************************************************************** + * Included Files + ****************************************************************************/ + +#include <nuttx/mutex.h> +#include <nuttx/config.h> + +#include <debug.h> +#include <errno.h> +#include <inttypes.h> + +#include "stm32_mdio.h" +#include "hardware/stm32_ethernet.h" + +/**************************************************************************** + * Private Types + ****************************************************************************/ + +struct stm32_mdio_bus_s +{ + struct mdio_lowerhalf_s *lower; + + /* MDIO bus timeout in milliseconds */ + + int timeout; +}; + +/**************************************************************************** + * Private Function Prototypes + ****************************************************************************/ + +static int stm32_c22_read(struct mdio_lowerhalf_s *dev, uint8_t phydev, + uint8_t regaddr, uint16_t *value); + +static int stm32_c22_write(struct mdio_lowerhalf_s *dev, uint8_t phydev, + uint8_t regaddr, uint16_t value); + +/**************************************************************************** + * Private Data + ****************************************************************************/ + +const struct mdio_ops_s g_stm32_mdio_ops = +{ + .read = stm32_c22_read, + .write = stm32_c22_write, + .reset = NULL, +}; + +struct mdio_lowerhalf_s g_stm32_mdio_lowerhalf = +{ + .ops = &g_stm32_mdio_ops +}; + +struct stm32_mdio_bus_s g_stm32_mdio_bus = +{ + .lower = &g_stm32_mdio_lowerhalf, + .timeout = 10 +}; + +/**************************************************************************** + * Private Functions + ****************************************************************************/ + +static int stm32_c22_read(struct mdio_lowerhalf_s *dev, uint8_t phydev, + uint8_t regaddr, uint16_t *value) +{ + int to; + uint32_t regval; + + int retval = -ETIMEDOUT; + struct stm32_mdio_bus_s *priv = (struct stm32_mdio_bus_s *)dev; Review Comment: Hi @zkkkk12 . You are correct. I'll explain my thoughts behind my decision, and we can agree upon an solution. * the ```struct stm32_mdio_bus_s``` should be an "upcast" of ```struct mdio_lowerhalf_s```. This way stm32 *(or any)* arch can add additional data if needed. I saw this as a common practice in nuttx codebase. * for this "upcast" to work, my understanding is that the member should be embedded not referenced *(so no pointer in ```struct stm32_mdio_bus_s```)* * if we embed the ```struct mdio_lowerhalf_s``` we increase the heap allocation as the ```struct mdio_bus_s```, which I tried to about (useless choice if you ask me as the ```g_stm32_mdio_bus``` should be in ram anyway, the only advantage is that the current choice is compile time checked) * when I was working on the mdio bus I was not aware of the existence of ```container_of``` macro, at least I did not see it used in the monolithic/full ```netdev``` drivers, so I did not use it, but we definitely need to use it. I see the following fixes, not sure what is better: * Drop the ```struct stm32_mdio_bus_s``` completely, I do not like this kind of variance in nuttx. the ```stm32_mdio.c``` **IS** the implementation of the ```struct mdio_lowerhalf_s```. All the arch should implement the exact same ```lowerhalf```. Move the timeout to ```lowerhalf```. * Embed the ```struct mdio_lowerhalf_s lower``` into ```struct stm32_mdio_bus_s```, not reference it. I would rather go with the first one, I think nuttx is overly complicated as it is, I would rather simplify as much as I can, but I think the second choice is the one that will be agreed by the community. Anyway, what is your take on this? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
