[PATCH v1 2/2] hwmon: npcm-pwm: add NPCM7xx PWM driver

[Tomer Maimon]

Hi Guenter,

Thanks for the prompt and detailed reply,

On 30 May 2018 at 19:46, Guenter Roeck <linux at roeck-us.net> wrote:

> On Wed, May 30, 2018 at 06:44:58PM +0300, Tomer Maimon wrote:
> > Hi  Guenter,
> >
> > Thanks for your prompt reply!
> >
> >
> > On 29 May 2018 at 19:56, Guenter Roeck <linux at roeck-us.net> wrote:
> >
> > > On Tue, May 29, 2018 at 01:02:21PM +0300, Tomer Maimon wrote:
> > > > 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,
> >
> >
> > > 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
> > 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
> > 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.

>
> >
> > > 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
> > hwmon.
> >
> No problem with that as long as there are no wildcards in the file name.
>
> Guenter
>

Thanks,

Tomer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20180531/89690ee1/attachment.html>