Hi Guenter,

Thanks for the prompt and detailed reply,

On 30 May 2018 at 19:46, Guenter Roeck [off-list ref] wrote:

On Wed, May 30, 2018 at 06:44:58PM +0300, Tomer Maimon wrote:

quoted

Hi  Guenter,

Thanks for your prompt reply!


On 29 May 2018 at 19:56, Guenter Roeck [off-list ref] wrote:

quoted

On Tue, May 29, 2018 at 01:02:21PM +0300, Tomer Maimon wrote:

quoted

Add Nuvoton BMC NPCM7xx Pulse Width Modulation (PWM) driver.

The Nuvoton BMC NPCM7xx has two identical PWM controller modules,
each module has four PWM controller outputs.

I don't see it guaranteed that all future NPCM7xx devices will be PWM
controllers, much less that they will be compatible to the chip really

Actually all NPCM7xx family have PWM and FAN modules support,

quoted

supported here. NPCM750, I presume ? I would suggest name the driver
accordingly.

The compatible name can not be a family name like nuvoton,npcm7xx-pwm,

only

quoted

a specific chip name. (in our case the NPCM750 is the full modules SOC)
still you think i should change the driver name? (note: all of our

NPCM7xx

quoted

unique modules drivers are named npcm7xx-*.c or *-npcm7xx.c)

drivers/watchdog/npcm_wdt.c contradicts that statement.

Please discuss with the pwm maintainer. In hwmon, wildcards in driver file
names are discouraged and will not be accepted. Our recommendation is to
pick
one chip from the family and use it as part of the file name.

Thanks for the clarification, we will name the driver according your
recommendation.

quoted

quoted

As a generic pwm driver, not specifically tied to a fan controller,
this driver does not belong into hwmon. It should be added to the pwm
subsystem. Copying the maintainer and mailing list.

In the NPCM7xx we have PWM and FAN controller modules,  usually in the

aspect of our BMC clients the two module
are used together to control the fans.

But because in the NPCM7xx the PWM and the FAN controller are separate
modules we
thought to do two separate drivers in the hwmon
is it possible? or you think it is better to do one hwmon driver for the
PWM and the FAN controller.

That depends on how closely the two modules are intertwined. If the pwm
module
is generic, it belongs into the pwm subsystem. It only belongs into hwmon
if it can _only_ be used for fan control, eg if the data reported by the
fan
module is used by the hardware to control the pwm output based on some
fan speed <-> pw value mapping which is programmed into the chip, and/or
if the pwm output can only connect to a fan and nothing else, and if the
relationship between pwm output and fan input is well defined.

Again thank you for your clarification, our PWM module was designed to work
with fan devices and for fan control.
i.e. on all server boards the pwm output is only connect to a fan device.

If both end up in hwmon, I don't see the benefit of having two separate
drivers. That means two hwmon devices for the same fan or set of fans,
one being all but useless in respect to output from the 'sensors'
command. You'll have to convince me why that would make sense; I just
don't see the point.  In that case, I would also not see the point of
having two separate devicetree nodes; to me, that would be similar
to having two interfaces for a single serial line, one for receive
and one for transmit.

Since our PWM and FAN are made only for handling the fans we will use them
in a single hwmon driver as you recommended

note that we were going to submit soon also the FAN controller driver
under

quoted

hwmon.

No problem with that as long as there are no wildcards in the file name.

Guenter

Thanks,

Tomer