Re: [PATCH 2/5] dt-bindings: soc: hpe: hpe,gxp-srom.yaml

["Arnd Bergmann" <arnd@arndb.de>]

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@hpe.com 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@hpe.com>
>> > +
>> > +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