[PATCH 1/2] hwmon: add generic GPIO fan driver

Guenter Roeck
2010-10-18T20:50:34Z

On Mon, Oct 18, 2010 at 04:36:10PM -0400, Simon Guinot wrote:
[ ... ]

> > I don't really understand the value of supporting pwm attributes,
> > since you have to convert those to rpm anyway. Why not just stick
> > with fan1_input and fan1_target ? This would simplify the code a lot.
>
> I don't know very well the hwmon API. I have simply been fooled by the
> sysfs-interface document which claim that fan[1-*]_target only make
> sense for a closed-loop fan. Moreover, I was expecting gpio-fan to be
> compliant with the fancontrol shell script...
>
> But anyway, you are right. I just don't want the pwm interface.
>
If the fancontrol script doesn't support fanX_target, and a given fan
doesn't support pwm, it might make sense to update it.

Jean, any comments ?

> > > +
> > > +err_free_gpio:
> > > + for (i = i - 1; i >= 0; i--)
> > > + gpio_free(ctrl[i]);
> > > +
> > This misses the most recently allocated gpio pin if gpio_direction_output() failed.
>
> The gpio is freed above while handling the gpio_direction_output() error.
>
I missed that. Thatks for the clarification.

> > > +
> > > + dev_info(&pdev->dev, "GPIO fan initialized\n");
> > > +
> > Might be a good idea to add a notion of which fan was initialized.
> > After all, there could be more than one.
>
> dev_info() don't do it for me ? anyway, I will check this too.
>
It probably does. Wonder what it actually shows. Can you check ?

Thanks,
Guenter