linux-doc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
       [not found] <CAGUgbhCqOJaEPjS96o2au21uW4NhqFScm4Ayd8PzOQvqxQ94SQ@mail.gmail.com>
@ 2023-07-14  7:13 ` Krzysztof Kozlowski
  2023-07-14  9:59   ` Guenter Roeck
  0 siblings, 1 reply; 20+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-14  7:13 UTC (permalink / raw)
  To: 蔡承達,
	Guenter Roeck, jdelvare, robh+dt, krzysztof.kozlowski+dt, joel,
	andrew, thierry.reding, u.kleine-koenig, corbet, p.zabel,
	linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick, Billy Tsai

On 14/07/2023 09:04, 蔡承達 wrote:

>         > This is because our register layout for PWM and Tach is not
> continuous.
> 
>         > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
> 
>         > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
> 
>         > ...
> 
>         > Each PWM/Tach instance has its own controller register and is not
> dependent on others.

Your email reply quoting style is very difficult to read.

> 
> 
> 
> Hi Guenter,
> 
> 
> 
> Did you receive a response to my previous email?
> 
> I would like to inquire if you have any further concerns regarding the PWM
> and Tach with 16 instances.

But isn't like this in all PWMs in all SoCs?

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-14  7:13 ` [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation Krzysztof Kozlowski
@ 2023-07-14  9:59   ` Guenter Roeck
  2023-07-14 10:18     ` 蔡承達
  0 siblings, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2023-07-14  9:59 UTC (permalink / raw)
  To: Krzysztof Kozlowski, 蔡承達,
	jdelvare, robh+dt, krzysztof.kozlowski+dt, joel, andrew,
	thierry.reding, u.kleine-koenig, corbet, p.zabel, linux-hwmon,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel,
	linux-pwm, linux-doc, patrick, Billy Tsai

On 7/14/23 00:13, Krzysztof Kozlowski wrote:
> On 14/07/2023 09:04, 蔡承達 wrote:
> 
>>          > This is because our register layout for PWM and Tach is not
>> continuous.
>>
>>          > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
>>
>>          > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
>>
>>          > ...
>>
>>          > Each PWM/Tach instance has its own controller register and is not
>> dependent on others.
> 
> Your email reply quoting style is very difficult to read.
> 
>>
>>
>>
>> Hi Guenter,
>>
>>
>>
>> Did you receive a response to my previous email?
>>
>> I would like to inquire if you have any further concerns regarding the PWM
>> and Tach with 16 instances.
> 
> But isn't like this in all PWMs in all SoCs?
> 

Correct, pretty much every fan controller is implemented that way.
I don't understand the logic.

Guenter



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-14  9:59   ` Guenter Roeck
@ 2023-07-14 10:18     ` 蔡承達
  2023-07-14 10:26       ` Guenter Roeck
  0 siblings, 1 reply; 20+ messages in thread
From: 蔡承達 @ 2023-07-14 10:18 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Krzysztof Kozlowski, jdelvare, robh+dt, krzysztof.kozlowski+dt,
	joel, andrew, thierry.reding, u.kleine-koenig, corbet, p.zabel,
	linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick, Billy Tsai

Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午5:59寫道:
>
> On 7/14/23 00:13, Krzysztof Kozlowski wrote:
> > On 14/07/2023 09:04, 蔡承達 wrote:
> >
> >>          > This is because our register layout for PWM and Tach is not
> >> continuous.
> >>
> >>          > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
> >>
> >>          > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
> >>
> >>          > ...
> >>
> >>          > Each PWM/Tach instance has its own controller register and is not
> >> dependent on others.
> >
> > Your email reply quoting style is very difficult to read.
> >
> >>
> >>
> >>
> >> Hi Guenter,
> >>
> >>
> >>
> >> Did you receive a response to my previous email?
> >>
> >> I would like to inquire if you have any further concerns regarding the PWM
> >> and Tach with 16 instances.
> >
> > But isn't like this in all PWMs in all SoCs?
> >
>
> Correct, pretty much every fan controller is implemented that way.
> I don't understand the logic.
>

Hi Krzysztof and Guenter,

Apologies for any confusion earlier.
So, you think that the implementation with 16 instances of TACH/PWM
device nodes in dts instead of one is ok to you, right?

Thanks

Best regards,
Billy Tsai

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-14 10:18     ` 蔡承達
@ 2023-07-14 10:26       ` Guenter Roeck
  2023-07-14 11:17         ` 蔡承達
  0 siblings, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2023-07-14 10:26 UTC (permalink / raw)
  To: 蔡承達
  Cc: Krzysztof Kozlowski, jdelvare, robh+dt, krzysztof.kozlowski+dt,
	joel, andrew, thierry.reding, u.kleine-koenig, corbet, p.zabel,
	linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick, Billy Tsai

On 7/14/23 03:18, 蔡承達 wrote:
> Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午5:59寫道:
>>
>> On 7/14/23 00:13, Krzysztof Kozlowski wrote:
>>> On 14/07/2023 09:04, 蔡承達 wrote:
>>>
>>>>           > This is because our register layout for PWM and Tach is not
>>>> continuous.
>>>>
>>>>           > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
>>>>
>>>>           > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
>>>>
>>>>           > ...
>>>>
>>>>           > Each PWM/Tach instance has its own controller register and is not
>>>> dependent on others.
>>>
>>> Your email reply quoting style is very difficult to read.
>>>
>>>>
>>>>
>>>>
>>>> Hi Guenter,
>>>>
>>>>
>>>>
>>>> Did you receive a response to my previous email?
>>>>
>>>> I would like to inquire if you have any further concerns regarding the PWM
>>>> and Tach with 16 instances.
>>>
>>> But isn't like this in all PWMs in all SoCs?
>>>
>>
>> Correct, pretty much every fan controller is implemented that way.
>> I don't understand the logic.
>>
> 
> Hi Krzysztof and Guenter,
> 
> Apologies for any confusion earlier.
> So, you think that the implementation with 16 instances of TACH/PWM
> device nodes in dts instead of one is ok to you, right?
> 

Did I say that ? No, it is not ok with me. It is confusing and doesn't make
sense to me. This is one fan controller with 16 channels, not 16 separate
controllers.

Guenter


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-14 10:26       ` Guenter Roeck
@ 2023-07-14 11:17         ` 蔡承達
  2023-07-16 16:08           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 20+ messages in thread
From: 蔡承達 @ 2023-07-14 11:17 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Krzysztof Kozlowski, jdelvare, robh+dt, krzysztof.kozlowski+dt,
	joel, andrew, thierry.reding, u.kleine-koenig, corbet, p.zabel,
	linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick, Billy Tsai

Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午6:26寫道:
>
> On 7/14/23 03:18, 蔡承達 wrote:
> > Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午5:59寫道:
> >>
> >> On 7/14/23 00:13, Krzysztof Kozlowski wrote:
> >>> On 14/07/2023 09:04, 蔡承達 wrote:
> >>>
> >>>>           > This is because our register layout for PWM and Tach is not
> >>>> continuous.
> >>>>
> >>>>           > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
> >>>>
> >>>>           > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
> >>>>
> >>>>           > ...
> >>>>
> >>>>           > Each PWM/Tach instance has its own controller register and is not
> >>>> dependent on others.
> >>>
> >>> Your email reply quoting style is very difficult to read.
> >>>
> >>>>
> >>>>
> >>>>
> >>>> Hi Guenter,
> >>>>
> >>>>
> >>>>
> >>>> Did you receive a response to my previous email?
> >>>>
> >>>> I would like to inquire if you have any further concerns regarding the PWM
> >>>> and Tach with 16 instances.
> >>>
> >>> But isn't like this in all PWMs in all SoCs?
> >>>
> >>
> >> Correct, pretty much every fan controller is implemented that way.
> >> I don't understand the logic.
> >>
> >
> > Hi Krzysztof and Guenter,
> >
> > Apologies for any confusion earlier.
> > So, you think that the implementation with 16 instances of TACH/PWM
> > device nodes in dts instead of one is ok to you, right?
> >
>
> Did I say that ? No, it is not ok with me. It is confusing and doesn't make
> sense to me. This is one fan controller with 16 channels, not 16 separate
> controllers.
>

This patch serial doesn't use to binding the fan control h/w. It is
used to binding the two independent h/w blocks.
One is used to provide pwm output and another is used to monitor the
speed of the input.
My patch is used to point out that the pwm and the tach is the
different function and don't need to
bind together. You can not only combine them as the fan usage but also
treat them as the individual module for
use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
tach can be used to monitor the heart beat signal.

Thanks

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-14 11:17         ` 蔡承達
@ 2023-07-16 16:08           ` Krzysztof Kozlowski
  2023-07-16 17:00             ` Guenter Roeck
  0 siblings, 1 reply; 20+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-16 16:08 UTC (permalink / raw)
  To: 蔡承達, Guenter Roeck
  Cc: jdelvare, robh+dt, krzysztof.kozlowski+dt, joel, andrew,
	thierry.reding, u.kleine-koenig, corbet, p.zabel, linux-hwmon,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel,
	linux-pwm, linux-doc, patrick, Billy Tsai

On 14/07/2023 13:17, 蔡承達 wrote:
> Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午6:26寫道:
>>
>> On 7/14/23 03:18, 蔡承達 wrote:
>>> Guenter Roeck <linux@roeck-us.net> 於 2023年7月14日 週五 下午5:59寫道:
>>>>
>>>> On 7/14/23 00:13, Krzysztof Kozlowski wrote:
>>>>> On 14/07/2023 09:04, 蔡承達 wrote:
>>>>>
>>>>>>           > This is because our register layout for PWM and Tach is not
>>>>>> continuous.
>>>>>>
>>>>>>           > PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
>>>>>>
>>>>>>           > PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
>>>>>>
>>>>>>           > ...
>>>>>>
>>>>>>           > Each PWM/Tach instance has its own controller register and is not
>>>>>> dependent on others.
>>>>>
>>>>> Your email reply quoting style is very difficult to read.
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi Guenter,
>>>>>>
>>>>>>
>>>>>>
>>>>>> Did you receive a response to my previous email?
>>>>>>
>>>>>> I would like to inquire if you have any further concerns regarding the PWM
>>>>>> and Tach with 16 instances.
>>>>>
>>>>> But isn't like this in all PWMs in all SoCs?
>>>>>
>>>>
>>>> Correct, pretty much every fan controller is implemented that way.
>>>> I don't understand the logic.
>>>>
>>>
>>> Hi Krzysztof and Guenter,
>>>
>>> Apologies for any confusion earlier.
>>> So, you think that the implementation with 16 instances of TACH/PWM
>>> device nodes in dts instead of one is ok to you, right?
>>>
>>
>> Did I say that ? No, it is not ok with me. It is confusing and doesn't make
>> sense to me. This is one fan controller with 16 channels, not 16 separate
>> controllers.
>>
> 
> This patch serial doesn't use to binding the fan control h/w. It is
> used to binding the two independent h/w blocks.
> One is used to provide pwm output and another is used to monitor the
> speed of the input.
> My patch is used to point out that the pwm and the tach is the
> different function and don't need to
> bind together. You can not only combine them as the fan usage but also
> treat them as the individual module for
> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> tach can be used to monitor the heart beat signal.

Isn't this exactly the same as in every other SoC? PWMs can be used in
different ways?

Anyway, it is tricky to keep the discussion since you avoid posting
entire DTS. I already said:

"I will start NAKing such patches without DTS user. It's like reviewing
fake code for some unknown solution and trying to get from you piece of
answers one by one, because you do not want to share entire part."



Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-16 16:08           ` Krzysztof Kozlowski
@ 2023-07-16 17:00             ` Guenter Roeck
  2023-07-17  9:01               ` 蔡承達
  0 siblings, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2023-07-16 17:00 UTC (permalink / raw)
  To: Krzysztof Kozlowski, 蔡承達
  Cc: jdelvare, robh+dt, krzysztof.kozlowski+dt, joel, andrew,
	thierry.reding, u.kleine-koenig, corbet, p.zabel, linux-hwmon,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel,
	linux-pwm, linux-doc, patrick, Billy Tsai

On 7/16/23 09:08, Krzysztof Kozlowski wrote:

[ ... ]

>>
>> This patch serial doesn't use to binding the fan control h/w. It is
>> used to binding the two independent h/w blocks.
>> One is used to provide pwm output and another is used to monitor the
>> speed of the input.
>> My patch is used to point out that the pwm and the tach is the
>> different function and don't need to
>> bind together. You can not only combine them as the fan usage but also
>> treat them as the individual module for
>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
>> tach can be used to monitor the heart beat signal.
> 
> Isn't this exactly the same as in every other SoC? PWMs can be used in
> different ways?
> 

... and in every fan controller. Not that it really makes sense because
normally the pwm controller part of such chips is tied to the fan input,
to enable automatic fan control, but it is technically possible.
In many cases this is also the case in SoCs, for example, in ast2500.
Apparently this was redesigned in ast2600 where they two blocks are
only lightly coupled (there are two pwm status bits in the fan status
register, but I have no idea what those mean). If the blocks are tightly
coupled, separate drivers don't really make sense.

There are multiple ways to separate the pwm controller part from the
fan inputs if that is really necessary. One would be to provide a
sequence of address mappings, the other would be to pass the memory
region from an mfd driver. It is not necessary to have N instances
of the fan controller, even if the address space is not continuous.

Guenter

> Anyway, it is tricky to keep the discussion since you avoid posting
> entire DTS. I already said:
> 
> "I will start NAKing such patches without DTS user. It's like reviewing
> fake code for some unknown solution and trying to get from you piece of
> answers one by one, because you do not want to share entire part."
> 
> 
> 
> Best regards,
> Krzysztof
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-16 17:00             ` Guenter Roeck
@ 2023-07-17  9:01               ` 蔡承達
  2023-07-17  9:39                 ` Krzysztof Kozlowski
  0 siblings, 1 reply; 20+ messages in thread
From: 蔡承達 @ 2023-07-17  9:01 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Krzysztof Kozlowski, jdelvare, robh+dt, krzysztof.kozlowski+dt,
	joel, andrew, thierry.reding, u.kleine-koenig, corbet, p.zabel,
	linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick, Billy Tsai

Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道:
>
> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
>
> [ ... ]
>
> >>
> >> This patch serial doesn't use to binding the fan control h/w. It is
> >> used to binding the two independent h/w blocks.
> >> One is used to provide pwm output and another is used to monitor the
> >> speed of the input.
> >> My patch is used to point out that the pwm and the tach is the
> >> different function and don't need to
> >> bind together. You can not only combine them as the fan usage but also
> >> treat them as the individual module for
> >> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> >> tach can be used to monitor the heart beat signal.
> >
> > Isn't this exactly the same as in every other SoC? PWMs can be used in
> > different ways?
> >
>
> ... and in every fan controller. Not that it really makes sense because
> normally the pwm controller part of such chips is tied to the fan input,
> to enable automatic fan control, but it is technically possible.
> In many cases this is also the case in SoCs, for example, in ast2500.
> Apparently this was redesigned in ast2600 where they two blocks are
> only lightly coupled (there are two pwm status bits in the fan status
> register, but I have no idea what those mean). If the blocks are tightly
> coupled, separate drivers don't really make sense.
>
> There are multiple ways to separate the pwm controller part from the
> fan inputs if that is really necessary. One would be to provide a
> sequence of address mappings, the other would be to pass the memory
> region from an mfd driver. It is not necessary to have N instances
> of the fan controller, even if the address space is not continuous.
>

Hi Guenter,

May I ask about the meaning of the sequence of address mappings? It appears
to consist of multiple tuples within the 'reg' property, indicating
the usage of PWM/Tach
registers within a single instance. After that I can use the dts like following:

pwm: pwm@1e610000 {
...
reg = <0x1e610000 0x8
0x1e610010 0x8
0x1e610020 0x8
0x1e610030 0x8
0x1e610040 0x8
0x1e610050 0x8
0x1e610060 0x8
0x1e610070 0x8
0x1e610080 0x8
0x1e610090 0x8
0x1e6100A0 0x8
0x1e6100B0 0x8
0x1e6100C0 0x8
0x1e6100D0 0x8
0x1e6100E0 0x8
0x1e6100F0 0x8>;
...
};

tach: tach@
...
reg = <0x1e610008 0x8
0x1e610018 0x8
0x1e610028 0x8
0x1e610038 0x8
0x1e610048 0x8
0x1e610058 0x8
0x1e610068 0x8
0x1e610078 0x8
0x1e610088 0x8
0x1e610098 0x8
0x1e6100A8 0x8
0x1e6100B8 0x8
0x1e6100C8 0x8
0x1e6100D8 0x8
0x1e6100E8 0x8
0x1e6100F8 0x8>;
...
};

correct?

Thanks

> Guenter
>
> > Anyway, it is tricky to keep the discussion since you avoid posting
> > entire DTS. I already said:
> >
> > "I will start NAKing such patches without DTS user. It's like reviewing
> > fake code for some unknown solution and trying to get from you piece of
> > answers one by one, because you do not want to share entire part."
> >

Hi Krzysztof,

Do you mean the DTS example of the usage in the binding, like the
following right?
PWM:
pwm0: pwm0@1e610000 {
compatible = "aspeed,ast2600-pwm";
reg = <0x1e610000 0x8>;
#pwm-cells = <3>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pwm0_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
};

TACH:
examples:
- |
tach0: tach0@1e610008 {
        compatible = "aspeed,ast2600-tach";
        reg = <0x1e610008 0x8>;
        #address-cells = <1>;
        #size-cells = <0>;
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_tach0_default>;
        clocks = <&syscon ASPEED_CLK_AHB>;
        resets = <&syscon ASPEED_RESET_PWM>;
};

Thanks

> >
> >
> > Best regards,
> > Krzysztof
> >
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-17  9:01               ` 蔡承達
@ 2023-07-17  9:39                 ` Krzysztof Kozlowski
  2023-07-18  4:01                   ` 蔡承達
  0 siblings, 1 reply; 20+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-17  9:39 UTC (permalink / raw)
  To: 蔡承達, Guenter Roeck
  Cc: jdelvare, robh+dt, krzysztof.kozlowski+dt, joel, andrew,
	thierry.reding, u.kleine-koenig, corbet, p.zabel, linux-hwmon,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel,
	linux-pwm, linux-doc, patrick, Billy Tsai

On 17/07/2023 11:01, 蔡承達 wrote:
> Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道:
>>
>> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
>>
>> [ ... ]
>>
>>>>
>>>> This patch serial doesn't use to binding the fan control h/w. It is
>>>> used to binding the two independent h/w blocks.
>>>> One is used to provide pwm output and another is used to monitor the
>>>> speed of the input.
>>>> My patch is used to point out that the pwm and the tach is the
>>>> different function and don't need to
>>>> bind together. You can not only combine them as the fan usage but also
>>>> treat them as the individual module for
>>>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
>>>> tach can be used to monitor the heart beat signal.
>>>
>>> Isn't this exactly the same as in every other SoC? PWMs can be used in
>>> different ways?
>>>
>>
>> ... and in every fan controller. Not that it really makes sense because
>> normally the pwm controller part of such chips is tied to the fan input,
>> to enable automatic fan control, but it is technically possible.
>> In many cases this is also the case in SoCs, for example, in ast2500.
>> Apparently this was redesigned in ast2600 where they two blocks are
>> only lightly coupled (there are two pwm status bits in the fan status
>> register, but I have no idea what those mean). If the blocks are tightly
>> coupled, separate drivers don't really make sense.
>>
>> There are multiple ways to separate the pwm controller part from the
>> fan inputs if that is really necessary. One would be to provide a
>> sequence of address mappings, the other would be to pass the memory
>> region from an mfd driver. It is not necessary to have N instances
>> of the fan controller, even if the address space is not continuous.
>>
> 
> Hi Guenter,
> 
> May I ask about the meaning of the sequence of address mappings? It appears
> to consist of multiple tuples within the 'reg' property, indicating
> the usage of PWM/Tach
> registers within a single instance. After that I can use the dts like following:
> 
> pwm: pwm@1e610000 {
> ...
> reg = <0x1e610000 0x8
> 0x1e610010 0x8
> 0x1e610020 0x8
> 0x1e610030 0x8
> 0x1e610040 0x8
> 0x1e610050 0x8
> 0x1e610060 0x8
> 0x1e610070 0x8
> 0x1e610080 0x8
> 0x1e610090 0x8
> 0x1e6100A0 0x8
> 0x1e6100B0 0x8
> 0x1e6100C0 0x8
> 0x1e6100D0 0x8
> 0x1e6100E0 0x8
> 0x1e6100F0 0x8>;


Uh, no... I mean, why? We keep pointing out that this should not be done
differently than any other SoC. Open any other SoC PWM controller and
tell me why this is different? Why this cannot be one address space?

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-17  9:39                 ` Krzysztof Kozlowski
@ 2023-07-18  4:01                   ` 蔡承達
  2023-07-18  6:04                     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 20+ messages in thread
From: 蔡承達 @ 2023-07-18  4:01 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Guenter Roeck, jdelvare, robh+dt, krzysztof.kozlowski+dt, joel,
	andrew, thierry.reding, u.kleine-koenig, corbet, p.zabel,
	linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick, Billy Tsai

>
> On 17/07/2023 11:01, 蔡承達 wrote:
> > Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道:
> >>
> >> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
> >>
> >> [ ... ]
> >>
> >>>>
> >>>> This patch serial doesn't use to binding the fan control h/w. It is
> >>>> used to binding the two independent h/w blocks.
> >>>> One is used to provide pwm output and another is used to monitor the
> >>>> speed of the input.
> >>>> My patch is used to point out that the pwm and the tach is the
> >>>> different function and don't need to
> >>>> bind together. You can not only combine them as the fan usage but also
> >>>> treat them as the individual module for
> >>>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> >>>> tach can be used to monitor the heart beat signal.
> >>>
> >>> Isn't this exactly the same as in every other SoC? PWMs can be used in
> >>> different ways?
> >>>
> >>
> >> ... and in every fan controller. Not that it really makes sense because
> >> normally the pwm controller part of such chips is tied to the fan input,
> >> to enable automatic fan control, but it is technically possible.
> >> In many cases this is also the case in SoCs, for example, in ast2500.
> >> Apparently this was redesigned in ast2600 where they two blocks are
> >> only lightly coupled (there are two pwm status bits in the fan status
> >> register, but I have no idea what those mean). If the blocks are tightly
> >> coupled, separate drivers don't really make sense.
> >>
> >> There are multiple ways to separate the pwm controller part from the
> >> fan inputs if that is really necessary. One would be to provide a
> >> sequence of address mappings, the other would be to pass the memory
> >> region from an mfd driver. It is not necessary to have N instances
> >> of the fan controller, even if the address space is not continuous.
> >>
> >
> > Hi Guenter,
> >
> > May I ask about the meaning of the sequence of address mappings? It appears
> > to consist of multiple tuples within the 'reg' property, indicating
> > the usage of PWM/Tach
> > registers within a single instance. After that I can use the dts like following:
> >
> > pwm: pwm@1e610000 {
> > ...
> > reg = <0x1e610000 0x8
> > 0x1e610010 0x8
> > 0x1e610020 0x8
> > 0x1e610030 0x8
> > 0x1e610040 0x8
> > 0x1e610050 0x8
> > 0x1e610060 0x8
> > 0x1e610070 0x8
> > 0x1e610080 0x8
> > 0x1e610090 0x8
> > 0x1e6100A0 0x8
> > 0x1e6100B0 0x8
> > 0x1e6100C0 0x8
> > 0x1e6100D0 0x8
> > 0x1e6100E0 0x8
> > 0x1e6100F0 0x8>;
>
>
> Uh, no... I mean, why? We keep pointing out that this should not be done
> differently than any other SoC. Open any other SoC PWM controller and
> tell me why this is different? Why this cannot be one address space?

Hi Krzysztof,

This is because the register layout for PWM and Tach is not continuous.
Each PWM/Tach instance has its own set of controller registers, and they
are independent of each other.

For example:
PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
...

To separate the PWM controller part from the fan inputs, Guenter has
provided two methods.
The first method involves passing the memory region from an MFD
driver, which was the
initial method I intended to use. However, it seems that this method
does not make sense to you.

Therefore, I would like to explore the second method suggested by
Guenter, which involves providing
a sequence of address mappings.

Thanks

Best Regards,
Billy Tsai

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-18  4:01                   ` 蔡承達
@ 2023-07-18  6:04                     ` Krzysztof Kozlowski
  2023-07-18  6:39                       ` Thierry Reding
  2023-07-18  6:45                       ` Guenter Roeck
  0 siblings, 2 replies; 20+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-18  6:04 UTC (permalink / raw)
  To: 蔡承達
  Cc: Guenter Roeck, jdelvare, robh+dt, krzysztof.kozlowski+dt, joel,
	andrew, thierry.reding, u.kleine-koenig, corbet, p.zabel,
	linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick, Billy Tsai

On 18/07/2023 06:01, 蔡承達 wrote:
>>
>> On 17/07/2023 11:01, 蔡承達 wrote:
>>> Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道:
>>>>
>>>> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
>>>>
>>>> [ ... ]
>>>>
>>>>>>
>>>>>> This patch serial doesn't use to binding the fan control h/w. It is
>>>>>> used to binding the two independent h/w blocks.
>>>>>> One is used to provide pwm output and another is used to monitor the
>>>>>> speed of the input.
>>>>>> My patch is used to point out that the pwm and the tach is the
>>>>>> different function and don't need to
>>>>>> bind together. You can not only combine them as the fan usage but also
>>>>>> treat them as the individual module for
>>>>>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
>>>>>> tach can be used to monitor the heart beat signal.
>>>>>
>>>>> Isn't this exactly the same as in every other SoC? PWMs can be used in
>>>>> different ways?
>>>>>
>>>>
>>>> ... and in every fan controller. Not that it really makes sense because
>>>> normally the pwm controller part of such chips is tied to the fan input,
>>>> to enable automatic fan control, but it is technically possible.
>>>> In many cases this is also the case in SoCs, for example, in ast2500.
>>>> Apparently this was redesigned in ast2600 where they two blocks are
>>>> only lightly coupled (there are two pwm status bits in the fan status
>>>> register, but I have no idea what those mean). If the blocks are tightly
>>>> coupled, separate drivers don't really make sense.
>>>>
>>>> There are multiple ways to separate the pwm controller part from the
>>>> fan inputs if that is really necessary. One would be to provide a
>>>> sequence of address mappings, the other would be to pass the memory
>>>> region from an mfd driver. It is not necessary to have N instances
>>>> of the fan controller, even if the address space is not continuous.
>>>>
>>>
>>> Hi Guenter,
>>>
>>> May I ask about the meaning of the sequence of address mappings? It appears
>>> to consist of multiple tuples within the 'reg' property, indicating
>>> the usage of PWM/Tach
>>> registers within a single instance. After that I can use the dts like following:
>>>
>>> pwm: pwm@1e610000 {
>>> ...
>>> reg = <0x1e610000 0x8
>>> 0x1e610010 0x8
>>> 0x1e610020 0x8
>>> 0x1e610030 0x8
>>> 0x1e610040 0x8
>>> 0x1e610050 0x8
>>> 0x1e610060 0x8
>>> 0x1e610070 0x8
>>> 0x1e610080 0x8
>>> 0x1e610090 0x8
>>> 0x1e6100A0 0x8
>>> 0x1e6100B0 0x8
>>> 0x1e6100C0 0x8
>>> 0x1e6100D0 0x8
>>> 0x1e6100E0 0x8
>>> 0x1e6100F0 0x8>;
>>
>>
>> Uh, no... I mean, why? We keep pointing out that this should not be done
>> differently than any other SoC. Open any other SoC PWM controller and
>> tell me why this is different? Why this cannot be one address space?
> 
> Hi Krzysztof,
> 
> This is because the register layout for PWM and Tach is not continuous.
> Each PWM/Tach instance has its own set of controller registers, and they
> are independent of each other.

Register layout is not continuous in many other devices, so again - why
this must be different?

> 
> For example:
> PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
> PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
> ...
> 
> To separate the PWM controller part from the fan inputs, Guenter has
> provided two methods.
> The first method involves passing the memory region from an MFD
> driver, which was the

I have no clue how can you pass memory region
(Documentation/devicetree/bindings/reserved-memory/) from MFD and why
does it make sense here.

> initial method I intended to use. However, it seems that this method
> does not make sense to you.
> 
> Therefore, I would like to explore the second method suggested by
> Guenter, which involves providing
> a sequence of address mappings.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-18  6:04                     ` Krzysztof Kozlowski
@ 2023-07-18  6:39                       ` Thierry Reding
  2023-07-18  6:54                         ` Guenter Roeck
  2023-07-18  6:45                       ` Guenter Roeck
  1 sibling, 1 reply; 20+ messages in thread
From: Thierry Reding @ 2023-07-18  6:39 UTC (permalink / raw)
  To: 蔡承達
  Cc: Krzysztof Kozlowski, Guenter Roeck, jdelvare, robh+dt,
	krzysztof.kozlowski+dt, joel, andrew, u.kleine-koenig, corbet,
	p.zabel, linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick, Billy Tsai

[-- Attachment #1: Type: text/plain, Size: 5866 bytes --]

On Tue, Jul 18, 2023 at 08:04:24AM +0200, Krzysztof Kozlowski wrote:
> On 18/07/2023 06:01, 蔡承達 wrote:
> >>
> >> On 17/07/2023 11:01, 蔡承達 wrote:
> >>> Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道:
> >>>>
> >>>> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
> >>>>
> >>>> [ ... ]
> >>>>
> >>>>>>
> >>>>>> This patch serial doesn't use to binding the fan control h/w. It is
> >>>>>> used to binding the two independent h/w blocks.
> >>>>>> One is used to provide pwm output and another is used to monitor the
> >>>>>> speed of the input.
> >>>>>> My patch is used to point out that the pwm and the tach is the
> >>>>>> different function and don't need to
> >>>>>> bind together. You can not only combine them as the fan usage but also
> >>>>>> treat them as the individual module for
> >>>>>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> >>>>>> tach can be used to monitor the heart beat signal.
> >>>>>
> >>>>> Isn't this exactly the same as in every other SoC? PWMs can be used in
> >>>>> different ways?
> >>>>>
> >>>>
> >>>> ... and in every fan controller. Not that it really makes sense because
> >>>> normally the pwm controller part of such chips is tied to the fan input,
> >>>> to enable automatic fan control, but it is technically possible.
> >>>> In many cases this is also the case in SoCs, for example, in ast2500.
> >>>> Apparently this was redesigned in ast2600 where they two blocks are
> >>>> only lightly coupled (there are two pwm status bits in the fan status
> >>>> register, but I have no idea what those mean). If the blocks are tightly
> >>>> coupled, separate drivers don't really make sense.
> >>>>
> >>>> There are multiple ways to separate the pwm controller part from the
> >>>> fan inputs if that is really necessary. One would be to provide a
> >>>> sequence of address mappings, the other would be to pass the memory
> >>>> region from an mfd driver. It is not necessary to have N instances
> >>>> of the fan controller, even if the address space is not continuous.
> >>>>
> >>>
> >>> Hi Guenter,
> >>>
> >>> May I ask about the meaning of the sequence of address mappings? It appears
> >>> to consist of multiple tuples within the 'reg' property, indicating
> >>> the usage of PWM/Tach
> >>> registers within a single instance. After that I can use the dts like following:
> >>>
> >>> pwm: pwm@1e610000 {
> >>> ...
> >>> reg = <0x1e610000 0x8
> >>> 0x1e610010 0x8
> >>> 0x1e610020 0x8
> >>> 0x1e610030 0x8
> >>> 0x1e610040 0x8
> >>> 0x1e610050 0x8
> >>> 0x1e610060 0x8
> >>> 0x1e610070 0x8
> >>> 0x1e610080 0x8
> >>> 0x1e610090 0x8
> >>> 0x1e6100A0 0x8
> >>> 0x1e6100B0 0x8
> >>> 0x1e6100C0 0x8
> >>> 0x1e6100D0 0x8
> >>> 0x1e6100E0 0x8
> >>> 0x1e6100F0 0x8>;
> >>
> >>
> >> Uh, no... I mean, why? We keep pointing out that this should not be done
> >> differently than any other SoC. Open any other SoC PWM controller and
> >> tell me why this is different? Why this cannot be one address space?
> > 
> > Hi Krzysztof,
> > 
> > This is because the register layout for PWM and Tach is not continuous.
> > Each PWM/Tach instance has its own set of controller registers, and they
> > are independent of each other.
> 
> Register layout is not continuous in many other devices, so again - why
> this must be different?
> 
> > 
> > For example:
> > PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
> > PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
> > ...
> > 
> > To separate the PWM controller part from the fan inputs, Guenter has
> > provided two methods.
> > The first method involves passing the memory region from an MFD
> > driver, which was the
> 
> I have no clue how can you pass memory region
> (Documentation/devicetree/bindings/reserved-memory/) from MFD and why
> does it make sense here.
> 
> > initial method I intended to use. However, it seems that this method
> > does not make sense to you.
> > 
> > Therefore, I would like to explore the second method suggested by
> > Guenter, which involves providing
> > a sequence of address mappings.

At the risk of saying what others have said: given that there's a single
reset line and a single clock line controlling all of these channels and
given what I recall of how address demuxers work in chips, everything
indicates that this is a single hardware block/device.

So the way that this should be described in DT is:

	pwm@1e610000 {
		reg = <0x1e610000 0x100>;
		clocks = ...;
		resets = ...
	};

That'd be the most accurate representation of this hardware in DT. It is
then up to the driver to expose this in any way you see fit. For Linux
it may make sense to expose this as 16 PWM channels and 16 hardware
monitoring devices. Other operating systems using the same DT may choose
to expose this differently, depending on their frameworks, etc. A simple
operating system may not expose this as separate resources at all but
instead directly program individual registers from this block.

I'd also like to add that I think trying to split this up into multiple
drivers in Linux is a bit overkill. In my opinion, though I know not
everyone shares this view, it's perfectly fine for one driver to expose
multiple types of resources. There's plenty of use-cases across the
kernel where tightly coupled devices like this have a single driver that
registers with multiple subsystems. Going through MFD only because this
particular hardware doesn't split registers nicely along Linux subsystem
boundaries.

So FWIW, I'm fine carrying hwmon code in a PWM driver and I'm equally
fine if PWM code ends up in a hwmon driver (or any other subsystem
really) if that makes sense for a given hardware.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-18  6:04                     ` Krzysztof Kozlowski
  2023-07-18  6:39                       ` Thierry Reding
@ 2023-07-18  6:45                       ` Guenter Roeck
  1 sibling, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2023-07-18  6:45 UTC (permalink / raw)
  To: Krzysztof Kozlowski, 蔡承達
  Cc: jdelvare, robh+dt, krzysztof.kozlowski+dt, joel, andrew,
	thierry.reding, u.kleine-koenig, corbet, p.zabel, linux-hwmon,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel,
	linux-pwm, linux-doc, patrick, Billy Tsai

On 7/17/23 23:04, Krzysztof Kozlowski wrote:

[ ... ]

>> The first method involves passing the memory region from an MFD
>> driver, which was the
> 
> I have no clue how can you pass memory region
> (Documentation/devicetree/bindings/reserved-memory/) from MFD and why
> does it make sense here.
> 

It is quite common for mfd drivers to pass regmap information to
child drivers. Child drivers typically call dev_get_regmap() with
the parent device as parameter. Granted, that is typically used
for spi or i2c devices, but I don't see why it would not work
here as well.

Then there s syscon, but I don't know if that works with overlapping
memory regions.

Guenter


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-18  6:39                       ` Thierry Reding
@ 2023-07-18  6:54                         ` Guenter Roeck
  2023-07-18  7:14                           ` Thierry Reding
  0 siblings, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2023-07-18  6:54 UTC (permalink / raw)
  To: Thierry Reding, 蔡承達
  Cc: Krzysztof Kozlowski, jdelvare, robh+dt, krzysztof.kozlowski+dt,
	joel, andrew, u.kleine-koenig, corbet, p.zabel, linux-hwmon,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel,
	linux-pwm, linux-doc, patrick, Billy Tsai

On 7/17/23 23:39, Thierry Reding wrote:
> On Tue, Jul 18, 2023 at 08:04:24AM +0200, Krzysztof Kozlowski wrote:
>> On 18/07/2023 06:01, 蔡承達 wrote:
>>>>
>>>> On 17/07/2023 11:01, 蔡承達 wrote:
>>>>> Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道:
>>>>>>
>>>>>> On 7/16/23 09:08, Krzysztof Kozlowski wrote:
>>>>>>
>>>>>> [ ... ]
>>>>>>
>>>>>>>>
>>>>>>>> This patch serial doesn't use to binding the fan control h/w. It is
>>>>>>>> used to binding the two independent h/w blocks.
>>>>>>>> One is used to provide pwm output and another is used to monitor the
>>>>>>>> speed of the input.
>>>>>>>> My patch is used to point out that the pwm and the tach is the
>>>>>>>> different function and don't need to
>>>>>>>> bind together. You can not only combine them as the fan usage but also
>>>>>>>> treat them as the individual module for
>>>>>>>> use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
>>>>>>>> tach can be used to monitor the heart beat signal.
>>>>>>>
>>>>>>> Isn't this exactly the same as in every other SoC? PWMs can be used in
>>>>>>> different ways?
>>>>>>>
>>>>>>
>>>>>> ... and in every fan controller. Not that it really makes sense because
>>>>>> normally the pwm controller part of such chips is tied to the fan input,
>>>>>> to enable automatic fan control, but it is technically possible.
>>>>>> In many cases this is also the case in SoCs, for example, in ast2500.
>>>>>> Apparently this was redesigned in ast2600 where they two blocks are
>>>>>> only lightly coupled (there are two pwm status bits in the fan status
>>>>>> register, but I have no idea what those mean). If the blocks are tightly
>>>>>> coupled, separate drivers don't really make sense.
>>>>>>
>>>>>> There are multiple ways to separate the pwm controller part from the
>>>>>> fan inputs if that is really necessary. One would be to provide a
>>>>>> sequence of address mappings, the other would be to pass the memory
>>>>>> region from an mfd driver. It is not necessary to have N instances
>>>>>> of the fan controller, even if the address space is not continuous.
>>>>>>
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> May I ask about the meaning of the sequence of address mappings? It appears
>>>>> to consist of multiple tuples within the 'reg' property, indicating
>>>>> the usage of PWM/Tach
>>>>> registers within a single instance. After that I can use the dts like following:
>>>>>
>>>>> pwm: pwm@1e610000 {
>>>>> ...
>>>>> reg = <0x1e610000 0x8
>>>>> 0x1e610010 0x8
>>>>> 0x1e610020 0x8
>>>>> 0x1e610030 0x8
>>>>> 0x1e610040 0x8
>>>>> 0x1e610050 0x8
>>>>> 0x1e610060 0x8
>>>>> 0x1e610070 0x8
>>>>> 0x1e610080 0x8
>>>>> 0x1e610090 0x8
>>>>> 0x1e6100A0 0x8
>>>>> 0x1e6100B0 0x8
>>>>> 0x1e6100C0 0x8
>>>>> 0x1e6100D0 0x8
>>>>> 0x1e6100E0 0x8
>>>>> 0x1e6100F0 0x8>;
>>>>
>>>>
>>>> Uh, no... I mean, why? We keep pointing out that this should not be done
>>>> differently than any other SoC. Open any other SoC PWM controller and
>>>> tell me why this is different? Why this cannot be one address space?
>>>
>>> Hi Krzysztof,
>>>
>>> This is because the register layout for PWM and Tach is not continuous.
>>> Each PWM/Tach instance has its own set of controller registers, and they
>>> are independent of each other.
>>
>> Register layout is not continuous in many other devices, so again - why
>> this must be different?
>>
>>>
>>> For example:
>>> PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
>>> PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
>>> ...
>>>
>>> To separate the PWM controller part from the fan inputs, Guenter has
>>> provided two methods.
>>> The first method involves passing the memory region from an MFD
>>> driver, which was the
>>
>> I have no clue how can you pass memory region
>> (Documentation/devicetree/bindings/reserved-memory/) from MFD and why
>> does it make sense here.
>>
>>> initial method I intended to use. However, it seems that this method
>>> does not make sense to you.
>>>
>>> Therefore, I would like to explore the second method suggested by
>>> Guenter, which involves providing
>>> a sequence of address mappings.
> 
> At the risk of saying what others have said: given that there's a single
> reset line and a single clock line controlling all of these channels and
> given what I recall of how address demuxers work in chips, everything
> indicates that this is a single hardware block/device.
> 
> So the way that this should be described in DT is:
> 
> 	pwm@1e610000 {
> 		reg = <0x1e610000 0x100>;
> 		clocks = ...;
> 		resets = ...
> 	};
> 
> That'd be the most accurate representation of this hardware in DT. It is
> then up to the driver to expose this in any way you see fit. For Linux
> it may make sense to expose this as 16 PWM channels and 16 hardware
> monitoring devices. Other operating systems using the same DT may choose

It is single chip. It should be a single hardware monitoring device with
16 channels. I don't even want to think about the mess we'd get if people
start modeling a single chip as N hardware monitoring devices, one for
each monitoring channel supported by that chip. It would be even more messy
if the driver supporting those N devices would be marked for asynchronous
probe, which would result in random hwmon device assignments.

> to expose this differently, depending on their frameworks, etc. A simple
> operating system may not expose this as separate resources at all but
> instead directly program individual registers from this block.
> 
> I'd also like to add that I think trying to split this up into multiple
> drivers in Linux is a bit overkill. In my opinion, though I know not
> everyone shares this view, it's perfectly fine for one driver to expose
> multiple types of resources. There's plenty of use-cases across the
> kernel where tightly coupled devices like this have a single driver that
> registers with multiple subsystems. Going through MFD only because this
> particular hardware doesn't split registers nicely along Linux subsystem
> boundaries.
> 
> So FWIW, I'm fine carrying hwmon code in a PWM driver and I'm equally
> fine if PWM code ends up in a hwmon driver (or any other subsystem
> really) if that makes sense for a given hardware.
> 

I am fine either way as well, as long as we are talking about a single
hwmon device and not 16 of them.

Guenter


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-18  6:54                         ` Guenter Roeck
@ 2023-07-18  7:14                           ` Thierry Reding
  2023-08-15  6:32                             ` 蔡承達
  0 siblings, 1 reply; 20+ messages in thread
From: Thierry Reding @ 2023-07-18  7:14 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: 蔡承達,
	Krzysztof Kozlowski, jdelvare, robh+dt, krzysztof.kozlowski+dt,
	joel, andrew, u.kleine-koenig, corbet, p.zabel, linux-hwmon,
	devicetree, linux-arm-kernel, linux-aspeed, linux-kernel,
	linux-pwm, linux-doc, patrick, Billy Tsai

[-- Attachment #1: Type: text/plain, Size: 7487 bytes --]

On Mon, Jul 17, 2023 at 11:54:26PM -0700, Guenter Roeck wrote:
> On 7/17/23 23:39, Thierry Reding wrote:
> > On Tue, Jul 18, 2023 at 08:04:24AM +0200, Krzysztof Kozlowski wrote:
> > > On 18/07/2023 06:01, 蔡承達 wrote:
> > > > > 
> > > > > On 17/07/2023 11:01, 蔡承達 wrote:
> > > > > > Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道:
> > > > > > > 
> > > > > > > On 7/16/23 09:08, Krzysztof Kozlowski wrote:
> > > > > > > 
> > > > > > > [ ... ]
> > > > > > > 
> > > > > > > > > 
> > > > > > > > > This patch serial doesn't use to binding the fan control h/w. It is
> > > > > > > > > used to binding the two independent h/w blocks.
> > > > > > > > > One is used to provide pwm output and another is used to monitor the
> > > > > > > > > speed of the input.
> > > > > > > > > My patch is used to point out that the pwm and the tach is the
> > > > > > > > > different function and don't need to
> > > > > > > > > bind together. You can not only combine them as the fan usage but also
> > > > > > > > > treat them as the individual module for
> > > > > > > > > use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> > > > > > > > > tach can be used to monitor the heart beat signal.
> > > > > > > > 
> > > > > > > > Isn't this exactly the same as in every other SoC? PWMs can be used in
> > > > > > > > different ways?
> > > > > > > > 
> > > > > > > 
> > > > > > > ... and in every fan controller. Not that it really makes sense because
> > > > > > > normally the pwm controller part of such chips is tied to the fan input,
> > > > > > > to enable automatic fan control, but it is technically possible.
> > > > > > > In many cases this is also the case in SoCs, for example, in ast2500.
> > > > > > > Apparently this was redesigned in ast2600 where they two blocks are
> > > > > > > only lightly coupled (there are two pwm status bits in the fan status
> > > > > > > register, but I have no idea what those mean). If the blocks are tightly
> > > > > > > coupled, separate drivers don't really make sense.
> > > > > > > 
> > > > > > > There are multiple ways to separate the pwm controller part from the
> > > > > > > fan inputs if that is really necessary. One would be to provide a
> > > > > > > sequence of address mappings, the other would be to pass the memory
> > > > > > > region from an mfd driver. It is not necessary to have N instances
> > > > > > > of the fan controller, even if the address space is not continuous.
> > > > > > > 
> > > > > > 
> > > > > > Hi Guenter,
> > > > > > 
> > > > > > May I ask about the meaning of the sequence of address mappings? It appears
> > > > > > to consist of multiple tuples within the 'reg' property, indicating
> > > > > > the usage of PWM/Tach
> > > > > > registers within a single instance. After that I can use the dts like following:
> > > > > > 
> > > > > > pwm: pwm@1e610000 {
> > > > > > ...
> > > > > > reg = <0x1e610000 0x8
> > > > > > 0x1e610010 0x8
> > > > > > 0x1e610020 0x8
> > > > > > 0x1e610030 0x8
> > > > > > 0x1e610040 0x8
> > > > > > 0x1e610050 0x8
> > > > > > 0x1e610060 0x8
> > > > > > 0x1e610070 0x8
> > > > > > 0x1e610080 0x8
> > > > > > 0x1e610090 0x8
> > > > > > 0x1e6100A0 0x8
> > > > > > 0x1e6100B0 0x8
> > > > > > 0x1e6100C0 0x8
> > > > > > 0x1e6100D0 0x8
> > > > > > 0x1e6100E0 0x8
> > > > > > 0x1e6100F0 0x8>;
> > > > > 
> > > > > 
> > > > > Uh, no... I mean, why? We keep pointing out that this should not be done
> > > > > differently than any other SoC. Open any other SoC PWM controller and
> > > > > tell me why this is different? Why this cannot be one address space?
> > > > 
> > > > Hi Krzysztof,
> > > > 
> > > > This is because the register layout for PWM and Tach is not continuous.
> > > > Each PWM/Tach instance has its own set of controller registers, and they
> > > > are independent of each other.
> > > 
> > > Register layout is not continuous in many other devices, so again - why
> > > this must be different?
> > > 
> > > > 
> > > > For example:
> > > > PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
> > > > PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
> > > > ...
> > > > 
> > > > To separate the PWM controller part from the fan inputs, Guenter has
> > > > provided two methods.
> > > > The first method involves passing the memory region from an MFD
> > > > driver, which was the
> > > 
> > > I have no clue how can you pass memory region
> > > (Documentation/devicetree/bindings/reserved-memory/) from MFD and why
> > > does it make sense here.
> > > 
> > > > initial method I intended to use. However, it seems that this method
> > > > does not make sense to you.
> > > > 
> > > > Therefore, I would like to explore the second method suggested by
> > > > Guenter, which involves providing
> > > > a sequence of address mappings.
> > 
> > At the risk of saying what others have said: given that there's a single
> > reset line and a single clock line controlling all of these channels and
> > given what I recall of how address demuxers work in chips, everything
> > indicates that this is a single hardware block/device.
> > 
> > So the way that this should be described in DT is:
> > 
> > 	pwm@1e610000 {
> > 		reg = <0x1e610000 0x100>;
> > 		clocks = ...;
> > 		resets = ...
> > 	};
> > 
> > That'd be the most accurate representation of this hardware in DT. It is
> > then up to the driver to expose this in any way you see fit. For Linux
> > it may make sense to expose this as 16 PWM channels and 16 hardware
> > monitoring devices. Other operating systems using the same DT may choose
> 
> It is single chip. It should be a single hardware monitoring device with
> 16 channels. I don't even want to think about the mess we'd get if people
> start modeling a single chip as N hardware monitoring devices, one for
> each monitoring channel supported by that chip. It would be even more messy
> if the driver supporting those N devices would be marked for asynchronous
> probe, which would result in random hwmon device assignments.

Sorry, I badly worded it. What I meant to say was: one hardware
monitoring device with 16 channels.

> > to expose this differently, depending on their frameworks, etc. A simple
> > operating system may not expose this as separate resources at all but
> > instead directly program individual registers from this block.
> > 
> > I'd also like to add that I think trying to split this up into multiple
> > drivers in Linux is a bit overkill. In my opinion, though I know not
> > everyone shares this view, it's perfectly fine for one driver to expose
> > multiple types of resources. There's plenty of use-cases across the
> > kernel where tightly coupled devices like this have a single driver that
> > registers with multiple subsystems. Going through MFD only because this
> > particular hardware doesn't split registers nicely along Linux subsystem
> > boundaries.
> > 
> > So FWIW, I'm fine carrying hwmon code in a PWM driver and I'm equally
> > fine if PWM code ends up in a hwmon driver (or any other subsystem
> > really) if that makes sense for a given hardware.
> > 
> 
> I am fine either way as well, as long as we are talking about a single
> hwmon device and not 16 of them.

Excellent. Should make it pretty clear in which direction this should
go.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-07-18  7:14                           ` Thierry Reding
@ 2023-08-15  6:32                             ` 蔡承達
  0 siblings, 0 replies; 20+ messages in thread
From: 蔡承達 @ 2023-08-15  6:32 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Guenter Roeck, Krzysztof Kozlowski, jdelvare, robh+dt,
	krzysztof.kozlowski+dt, joel, andrew, u.kleine-koenig, corbet,
	p.zabel, linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick, Billy Tsai

Thierry Reding <thierry.reding@gmail.com> 於 2023年7月18日 週二 下午3:14寫道:
>
> On Mon, Jul 17, 2023 at 11:54:26PM -0700, Guenter Roeck wrote:
> > On 7/17/23 23:39, Thierry Reding wrote:
> > > On Tue, Jul 18, 2023 at 08:04:24AM +0200, Krzysztof Kozlowski wrote:
> > > > On 18/07/2023 06:01, 蔡承達 wrote:
> > > > > >
> > > > > > On 17/07/2023 11:01, 蔡承達 wrote:
> > > > > > > Guenter Roeck <linux@roeck-us.net> 於 2023年7月17日 週一 上午1:00寫道:
> > > > > > > >
> > > > > > > > On 7/16/23 09:08, Krzysztof Kozlowski wrote:
> > > > > > > >
> > > > > > > > [ ... ]
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > This patch serial doesn't use to binding the fan control h/w. It is
> > > > > > > > > > used to binding the two independent h/w blocks.
> > > > > > > > > > One is used to provide pwm output and another is used to monitor the
> > > > > > > > > > speed of the input.
> > > > > > > > > > My patch is used to point out that the pwm and the tach is the
> > > > > > > > > > different function and don't need to
> > > > > > > > > > bind together. You can not only combine them as the fan usage but also
> > > > > > > > > > treat them as the individual module for
> > > > > > > > > > use. For example: the pwm can use to be the beeper (pwm-beeper.c), the
> > > > > > > > > > tach can be used to monitor the heart beat signal.
> > > > > > > > >
> > > > > > > > > Isn't this exactly the same as in every other SoC? PWMs can be used in
> > > > > > > > > different ways?
> > > > > > > > >
> > > > > > > >
> > > > > > > > ... and in every fan controller. Not that it really makes sense because
> > > > > > > > normally the pwm controller part of such chips is tied to the fan input,
> > > > > > > > to enable automatic fan control, but it is technically possible.
> > > > > > > > In many cases this is also the case in SoCs, for example, in ast2500.
> > > > > > > > Apparently this was redesigned in ast2600 where they two blocks are
> > > > > > > > only lightly coupled (there are two pwm status bits in the fan status
> > > > > > > > register, but I have no idea what those mean). If the blocks are tightly
> > > > > > > > coupled, separate drivers don't really make sense.
> > > > > > > >
> > > > > > > > There are multiple ways to separate the pwm controller part from the
> > > > > > > > fan inputs if that is really necessary. One would be to provide a
> > > > > > > > sequence of address mappings, the other would be to pass the memory
> > > > > > > > region from an mfd driver. It is not necessary to have N instances
> > > > > > > > of the fan controller, even if the address space is not continuous.
> > > > > > > >
> > > > > > >
> > > > > > > Hi Guenter,
> > > > > > >
> > > > > > > May I ask about the meaning of the sequence of address mappings? It appears
> > > > > > > to consist of multiple tuples within the 'reg' property, indicating
> > > > > > > the usage of PWM/Tach
> > > > > > > registers within a single instance. After that I can use the dts like following:
> > > > > > >
> > > > > > > pwm: pwm@1e610000 {
> > > > > > > ...
> > > > > > > reg = <0x1e610000 0x8
> > > > > > > 0x1e610010 0x8
> > > > > > > 0x1e610020 0x8
> > > > > > > 0x1e610030 0x8
> > > > > > > 0x1e610040 0x8
> > > > > > > 0x1e610050 0x8
> > > > > > > 0x1e610060 0x8
> > > > > > > 0x1e610070 0x8
> > > > > > > 0x1e610080 0x8
> > > > > > > 0x1e610090 0x8
> > > > > > > 0x1e6100A0 0x8
> > > > > > > 0x1e6100B0 0x8
> > > > > > > 0x1e6100C0 0x8
> > > > > > > 0x1e6100D0 0x8
> > > > > > > 0x1e6100E0 0x8
> > > > > > > 0x1e6100F0 0x8>;
> > > > > >
> > > > > >
> > > > > > Uh, no... I mean, why? We keep pointing out that this should not be done
> > > > > > differently than any other SoC. Open any other SoC PWM controller and
> > > > > > tell me why this is different? Why this cannot be one address space?
> > > > >
> > > > > Hi Krzysztof,
> > > > >
> > > > > This is because the register layout for PWM and Tach is not continuous.
> > > > > Each PWM/Tach instance has its own set of controller registers, and they
> > > > > are independent of each other.
> > > >
> > > > Register layout is not continuous in many other devices, so again - why
> > > > this must be different?
> > > >
> > > > >
> > > > > For example:
> > > > > PWM0 uses registers 0x0 and 0x4, while Tach0 uses registers 0x8 and 0xc.
> > > > > PWM1 uses registers 0x10 and 0x14, while Tach1 uses registers 0x18 and 0x1c.
> > > > > ...
> > > > >
> > > > > To separate the PWM controller part from the fan inputs, Guenter has
> > > > > provided two methods.
> > > > > The first method involves passing the memory region from an MFD
> > > > > driver, which was the
> > > >
> > > > I have no clue how can you pass memory region
> > > > (Documentation/devicetree/bindings/reserved-memory/) from MFD and why
> > > > does it make sense here.
> > > >
> > > > > initial method I intended to use. However, it seems that this method
> > > > > does not make sense to you.
> > > > >
> > > > > Therefore, I would like to explore the second method suggested by
> > > > > Guenter, which involves providing
> > > > > a sequence of address mappings.
> > >
> > > At the risk of saying what others have said: given that there's a single
> > > reset line and a single clock line controlling all of these channels and
> > > given what I recall of how address demuxers work in chips, everything
> > > indicates that this is a single hardware block/device.
> > >
> > > So the way that this should be described in DT is:
> > >
> > >     pwm@1e610000 {
> > >             reg = <0x1e610000 0x100>;
> > >             clocks = ...;
> > >             resets = ...
> > >     };
> > >
> > > That'd be the most accurate representation of this hardware in DT. It is
> > > then up to the driver to expose this in any way you see fit. For Linux
> > > it may make sense to expose this as 16 PWM channels and 16 hardware
> > > monitoring devices. Other operating systems using the same DT may choose
> >
> > It is single chip. It should be a single hardware monitoring device with
> > 16 channels. I don't even want to think about the mess we'd get if people
> > start modeling a single chip as N hardware monitoring devices, one for
> > each monitoring channel supported by that chip. It would be even more messy
> > if the driver supporting those N devices would be marked for asynchronous
> > probe, which would result in random hwmon device assignments.
>
> Sorry, I badly worded it. What I meant to say was: one hardware
> monitoring device with 16 channels.
>
> > > to expose this differently, depending on their frameworks, etc. A simple
> > > operating system may not expose this as separate resources at all but
> > > instead directly program individual registers from this block.
> > >
> > > I'd also like to add that I think trying to split this up into multiple
> > > drivers in Linux is a bit overkill. In my opinion, though I know not
> > > everyone shares this view, it's perfectly fine for one driver to expose
> > > multiple types of resources. There's plenty of use-cases across the
> > > kernel where tightly coupled devices like this have a single driver that
> > > registers with multiple subsystems. Going through MFD only because this
> > > particular hardware doesn't split registers nicely along Linux subsystem
> > > boundaries.
> > >
> > > So FWIW, I'm fine carrying hwmon code in a PWM driver and I'm equally
> > > fine if PWM code ends up in a hwmon driver (or any other subsystem
> > > really) if that makes sense for a given hardware.
> > >
> >
> > I am fine either way as well, as long as we are talking about a single
> > hwmon device and not 16 of them.
>
> Excellent. Should make it pretty clear in which direction this should
> go.
>
Hi Thierry,

Thank you for organizing and providing guidance on this patch series.
I'd like to
confirm my understanding of the next steps. Based on the previous discussions,
I should proceed by combining the PWM and TACH drivers into a single driver.
This consolidated driver would then be exposed to both the hwmon and
pwm subsystems.
Base on this driver the dts node of this would like following:

pwm_tach: pwm-tach-controller@1e610000 {
compatible = "aspeed,ast2600-pwm-tach"
reg = <0x1e610000 0x100>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
#pwm-cells = <3>;
#address-cells = <1>;
#size-cells = <0>;
fan@0 {
aspeed,fan-tach-ch = /bits/ 8 <0x00>;
};
fan@1 {
aspeed,fan-tach-ch = /bits/ 8 <0x01, 0x02>;
};
};

The subnode of the fan is utilized to create the hwmon interface,
which facilitates monitoring
the speed of the input tach pin.
If users wish to employ PWM for fan or beeper control, they can create
the necessary configuration
through the following DTS declaration:
fan0: pwm-fan0 {
compatible = "pwm-fan";
pwms = <&pwm_tach 0 40000 0>; /* Target freq:25 kHz */
cooling-min-state = <0>;
cooling-max-state = <3>;
#cooling-cells = <2>;
cooling-levels = <0 15 128 255>;
};

buzzer {
compatible = "pwm-beeper";
pwms = <&pwm_tach 1 500000 0>;
};

Thanks
Best Regards,
Billy Tsai

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
       [not found]     ` <SG2PR06MB3365E360F3FCDE639F3D2D1E8B50A@SG2PR06MB3365.apcprd06.prod.outlook.com>
@ 2023-06-08 13:18       ` Guenter Roeck
  0 siblings, 0 replies; 20+ messages in thread
From: Guenter Roeck @ 2023-06-08 13:18 UTC (permalink / raw)
  To: Billy Tsai, jdelvare, robh+dt, krzysztof.kozlowski+dt, joel,
	andrew, thierry.reding, u.kleine-koenig, corbet, p.zabel,
	linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick

On 6/7/23 23:21, Billy Tsai wrote:
>          > The code says:
> 
>          > In Aspeed AST2600 SoC features 16 TACH controllers, with each
> 
>          > controller capable of supporting up to 1 input.
> 
>          > which is a bit different. I guess there are no examples anymore,
> 
>          > but I'd really like to see how this looks like in the devicetree file,
> 
>          > and how the driver is supposed to distinguish/select the 16 inputs.
> 
> Hi Roeck,
> 
> The node in the devicetree file will looks like following:
> 
> tach0: tach0@1e610008 {
> 
>          compatible = "aspeed,ast2600-tach";
> 
>          reg = <0x1e610008 0x8>;
> 
>          #address-cells = <1>;
> 
>          #size-cells = <0>;
> 
>          pinctrl-names = "default";
> 
>          pinctrl-0 = <&pinctrl_tach0_default>;
> 
>          clocks = <&syscon ASPEED_CLK_AHB>;
> 
>          resets = <&syscon ASPEED_RESET_PWM>;
> 
>          status = "disabled";
> 
> };
> 

Neither reg nor pinctrl is mentioned in the bindings. Maybe that is not needed nowadays,
but I find it confusing.

Either case, it is highly unusual that there would be 16 instances of this device
instead of one. Why is this done ? It doesn't really make sense to me.

Guenter


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-06-08  2:18 ` [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation Billy Tsai
  2023-06-08  4:58   ` Guenter Roeck
@ 2023-06-08  6:40   ` Krzysztof Kozlowski
  1 sibling, 0 replies; 20+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-08  6:40 UTC (permalink / raw)
  To: Billy Tsai, jdelvare, linux, robh+dt, krzysztof.kozlowski+dt,
	joel, andrew, thierry.reding, u.kleine-koenig, corbet, p.zabel,
	linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick

On 08/06/2023 04:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-tach device.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  .../bindings/hwmon/aspeed,ast2600-tach.yaml   | 32 +++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> new file mode 100644
> index 000000000000..627aa00f2e92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 Aspeed, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed Ast2600 Tach controller
> +
> +maintainers:
> +  - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description: |
> +  The Aspeed Tach controller can support upto 1 fan input.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2600-tach
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1

NAK, not true based on previous discussions. Device does not come with
resets and clocks.


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-06-08  2:18 ` [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation Billy Tsai
@ 2023-06-08  4:58   ` Guenter Roeck
       [not found]     ` <SG2PR06MB3365E360F3FCDE639F3D2D1E8B50A@SG2PR06MB3365.apcprd06.prod.outlook.com>
  2023-06-08  6:40   ` Krzysztof Kozlowski
  1 sibling, 1 reply; 20+ messages in thread
From: Guenter Roeck @ 2023-06-08  4:58 UTC (permalink / raw)
  To: Billy Tsai, jdelvare, robh+dt, krzysztof.kozlowski+dt, joel,
	andrew, thierry.reding, u.kleine-koenig, corbet, p.zabel,
	linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick

On 6/7/23 19:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-tach device.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>   .../bindings/hwmon/aspeed,ast2600-tach.yaml   | 32 +++++++++++++++++++
>   1 file changed, 32 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> new file mode 100644
> index 000000000000..627aa00f2e92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 Aspeed, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed Ast2600 Tach controller
> +
> +maintainers:
> +  - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description: |
> +  The Aspeed Tach controller can support upto 1 fan input.
> +

The code says:

In Aspeed AST2600 SoC features 16 TACH controllers, with each
controller capable of supporting up to 1 input.

which is a bit different. I guess there are no examples anymore,
but I'd really like to see how this looks like in the devicetree file,
and how the driver is supposed to distinguish/select the 16 inputs.

> +properties:
> +  compatible:
> +    enum:
> +      - aspeed,ast2600-tach
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - clocks
> +  - resets
> +
> +additionalProperties: false


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
  2023-06-08  2:18 [v6 0/4] Support pwm/tach driver for aspeed ast26xx Billy Tsai
@ 2023-06-08  2:18 ` Billy Tsai
  2023-06-08  4:58   ` Guenter Roeck
  2023-06-08  6:40   ` Krzysztof Kozlowski
  0 siblings, 2 replies; 20+ messages in thread
From: Billy Tsai @ 2023-06-08  2:18 UTC (permalink / raw)
  To: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, joel, andrew,
	thierry.reding, u.kleine-koenig, corbet, p.zabel, billy_tsai,
	linux-hwmon, devicetree, linux-arm-kernel, linux-aspeed,
	linux-kernel, linux-pwm, linux-doc, patrick

Document the compatible for aspeed,ast2600-tach device.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 .../bindings/hwmon/aspeed,ast2600-tach.yaml   | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
new file mode 100644
index 000000000000..627aa00f2e92
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
@@ -0,0 +1,32 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 Aspeed, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed Ast2600 Tach controller
+
+maintainers:
+  - Billy Tsai <billy_tsai@aspeedtech.com>
+
+description: |
+  The Aspeed Tach controller can support upto 1 fan input.
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-tach
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+  - resets
+
+additionalProperties: false
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2023-08-15  6:33 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CAGUgbhCqOJaEPjS96o2au21uW4NhqFScm4Ayd8PzOQvqxQ94SQ@mail.gmail.com>
2023-07-14  7:13 ` [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation Krzysztof Kozlowski
2023-07-14  9:59   ` Guenter Roeck
2023-07-14 10:18     ` 蔡承達
2023-07-14 10:26       ` Guenter Roeck
2023-07-14 11:17         ` 蔡承達
2023-07-16 16:08           ` Krzysztof Kozlowski
2023-07-16 17:00             ` Guenter Roeck
2023-07-17  9:01               ` 蔡承達
2023-07-17  9:39                 ` Krzysztof Kozlowski
2023-07-18  4:01                   ` 蔡承達
2023-07-18  6:04                     ` Krzysztof Kozlowski
2023-07-18  6:39                       ` Thierry Reding
2023-07-18  6:54                         ` Guenter Roeck
2023-07-18  7:14                           ` Thierry Reding
2023-08-15  6:32                             ` 蔡承達
2023-07-18  6:45                       ` Guenter Roeck
2023-06-08  2:18 [v6 0/4] Support pwm/tach driver for aspeed ast26xx Billy Tsai
2023-06-08  2:18 ` [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation Billy Tsai
2023-06-08  4:58   ` Guenter Roeck
     [not found]     ` <SG2PR06MB3365E360F3FCDE639F3D2D1E8B50A@SG2PR06MB3365.apcprd06.prod.outlook.com>
2023-06-08 13:18       ` Guenter Roeck
2023-06-08  6:40   ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).