On Thu, Jan 12, 2023, at 14:16, Clay Chang wrote: > On Tue, Jan 10, 2023 at 10:49:36AM +0100, Krzysztof Kozlowski wrote: >> On 10/01/2023 05:25, clayc@xxxxxxx wrote: >> > @@ -0,0 +1,36 @@ >> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> > +%YAML 1.2 >> > +--- >> > +$id: http://devicetree.org/schemas/soc/hpe/hpe,gxp-srom.yaml# >> > +$schema: http://devicetree.org/meta-schemas/core.yaml# >> > + >> > +title: HPE GXP SoC SROM Control Register >> > + >> > +maintainers: >> > + - Clay Chang <clayc@xxxxxxx> >> > + >> > +description: |+ >> > + The SROM control register can be used to configure LPC related legacy >> > + I/O registers. >> >> And why this is a hardware? No, you now add fake devices to be able to >> write some stuff from user-space... Otherwise this needs proper hardware >> description. > > Thank you for commenting on this. You are right, this is not a real > hardware device, but simply exposes MMIO regions to the user-space. > Maybe we should rewrite this as a syscon driver. Is writing a syscon > driver a right direction? There are two completely separate questions about the DT binding and about the user-visible interface. The binding needs to properly identify what this device is. I don't think anyone without the datasheet can tell you the right answer here, it really depends what the other registers do. If there are lots of unrelated registers in a small area, a syscon might be the right answer, but if they are all related to an external memory bus, then categorizing it as a memory controller may be more appropriate. For the user interface side, I don't really like the idea of having a hardware register directly exposed as driver in drivers/soc, this generally makes it impossible to have portable userspace that works across implementations of multiple SoC vendors, and it makes it too easy to come up with an ad-hoc interface to make a chip work for a particular use case when a more general solution would be better. Again, it's hard for me to tell why this even needs to be runtime configurable, please try to describe what type of application would access the sysfs interface here, and why this can't just be set to a fixed value by bootloader or kernel without user interaction. Arnd
