<div dir="ltr">Hi Guenter,<div><br></div><div>Thanks for the prompt and detailed reply,</div><div class="gmail_extra"><br><div class="gmail_quote">On 30 May 2018 at 19:46, Guenter Roeck <span dir="ltr"><<a href="mailto:linux@roeck-us.net" target="_blank">linux@roeck-us.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed, May 30, 2018 at 06:44:58PM +0300, Tomer Maimon wrote:<br>
> Hi  Guenter,<br>
> <br>
> Thanks for your prompt reply!<br>
> <br>
> <br>
> On 29 May 2018 at 19:56, Guenter Roeck <<a href="mailto:linux@roeck-us.net">linux@roeck-us.net</a>> wrote:<br>
> <br>
> > On Tue, May 29, 2018 at 01:02:21PM +0300, Tomer Maimon wrote:<br>
> > > Add Nuvoton BMC NPCM7xx Pulse Width Modulation (PWM) driver.<br>
> > ><br>
> > > The Nuvoton BMC NPCM7xx has two identical PWM controller modules,<br>
> > > each module has four PWM controller outputs.<br>
> > ><br>
> ><br>
> > I don't see it guaranteed that all future NPCM7xx devices will be PWM<br>
> > controllers, much less that they will be compatible to the chip really<br>
> ><br>
> <br>
> Actually all NPCM7xx family have PWM and FAN modules support,<br>
> <br>
> <br>
> > supported here. NPCM750, I presume ? I would suggest name the driver<br>
> > accordingly.<br>
> ><br>
> <br>
> The compatible name can not be a family name like nuvoton,npcm7xx-pwm, only<br>
> a specific chip name. (in our case the NPCM750 is the full modules SOC)<br>
> still you think i should change the driver name? (note: all of our NPCM7xx<br>
> unique modules drivers are named npcm7xx-*.c or *-npcm7xx.c)<br>
> <br>
<br>
</span>drivers/watchdog/npcm_wdt.c contradicts that statement.<br>
<br>
Please discuss with the pwm maintainer. In hwmon, wildcards in driver file<br>
names are discouraged and will not be accepted. Our recommendation is to pick<br>
one chip from the family and use it as part of the file name.<br></blockquote><div><br></div><div>Thanks for the clarification, we will name the driver according your <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">recommendation.</span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> <br>
> > As a generic pwm driver, not specifically tied to a fan controller,<br>
> > this driver does not belong into hwmon. It should be added to the pwm<br>
> > subsystem. Copying the maintainer and mailing list.<br>
> ><br>
> > In the NPCM7xx we have PWM and FAN controller modules,  usually in the<br>
> aspect of our BMC clients the two module<br>
> are used together to control the fans.<br>
> <br>
> But because in the NPCM7xx the PWM and the FAN controller are separate<br>
> modules we<br>
> thought to do two separate drivers in the hwmon<br>
> is it possible? or you think it is better to do one hwmon driver for the<br>
> PWM and the FAN controller.<br>
> <br>
</span>That depends on how closely the two modules are intertwined. If the pwm module<br>
is generic, it belongs into the pwm subsystem. It only belongs into hwmon<br>
if it can _only_ be used for fan control, eg if the data reported by the fan<br>
module is used by the hardware to control the pwm output based on some<br>
fan speed <-> pw value mapping which is programmed into the chip, and/or<br>
if the pwm output can only connect to a fan and nothing else, and if the<br>
relationship between pwm output and fan input is well defined.<br>
<br></blockquote><div> </div><div>

<div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">Again thank you for your clarification, our PWM module was designed to work with fan devices and for fan control.</div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial">i.e. on all server boards the <span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">pwm output is only connect to a fan</span> device.</div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial"> <br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">If both end up in hwmon, I don't see the benefit of having two separate<br>
drivers. That means two hwmon devices for the same fan or set of fans,<br>
one being all but useless in respect to output from the 'sensors'<br>
command. You'll have to convince me why that would make sense; I just<br>
don't see the point.  In that case, I would also not see the point of<br>
having two separate devicetree nodes; to me, that would be similar<br>
to having two interfaces for a single serial line, one for receive<br>
and one for transmit.</blockquote><div> </div><div>

<div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial;background-color:rgb(255,255,255)">Since our PWM and FAN are made only for handling the fans we will use them in a single hwmon driver as you recommended</div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial;background-color:rgb(255,255,255)"><br></div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"></span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> note that we were going to submit soon also the FAN controller driver under<br>
> hwmon.<br>
> <br>
</span>No problem with that as long as there are no wildcards in the file name.<br>
<span class="HOEnZb"><font color="#888888"><br>
Guenter<br>
</font></span></blockquote></div><br></div><div class="gmail_extra">Thanks,</div><div class="gmail_extra"><br></div><div class="gmail_extra">Tomer</div></div>