lkml.org 
[lkml]   [2010]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRE: [PATCHv2 1/7] pwm: Add pwm core driver
    > On Tue, 5 Oct 2010 17:29:56 +0530
    > Arun Murthy <arun.murthy@stericsson.com> wrote:
    >
    > > The existing pwm based led and backlight driver makes use of the
    > > pwm(include/linux/pwm.h). So all the board specific pwm drivers will
    > > be exposing the same set of function name as in include/linux/pwm.h.
    > > Consder a platform with multi Soc or having more than one pwm module,
    > in
    > > such a case, there exists more than one pwm driver for a platform.
    > Each
    > > of these pwm drivers export the same set of function and hence leads
    > to
    > > re-declaration build error.
    > >
    > > In order to overcome this issue all the pwm drivers must register to
    > > some core pwm driver with function pointers for pwm operations (i.e
    > > pwm_config, pwm_enable, pwm_disable).
    > >
    > > The clients of pwm device will have to call pwm_request, wherein
    > > they will get the pointer to struct pwm_ops. This structure include
    > > function pointers for pwm_config, pwm_enable and pwm_disable.
    > >
    >
    > Have we worked out who will be merging this work, if it gets merged?
    I request Samuel to merge this through MFD tree.

    >
    > >
    > > ...
    > >
    > > +struct pwm_dev_info {
    > > + struct pwm_device *pwm_dev;
    > > + struct list_head list;
    > > +};
    > > +static struct pwm_dev_info *di;
    >
    > We could just do
    >
    > static struct pwm_dev_info {
    > ...
    > } *di;
    >
    > > +DECLARE_RWSEM(pwm_list_lock);
    >
    > This can/should be static.
    >
    > > +void __deprecated pwm_free(struct pwm_device *pwm)
    > > +{
    > > +}
    >
    > Why are we adding a new function and already deprecating it?
    >
    > Probably this was already addressed in earlier review, but I'm asking
    > again, because there's no comment explaining the reasons. Lesson
    > learned, please add a comment.
    >
    > Oh, I see that pwm_free() already exists. This patch adds a new copy
    > and doesn't remove the old function. Does this all actually work?
    >
    > It still needs a comment explaining why it's deprecated.
    The existing pwm drivers make use of this function and now I am in the process
    of developing a new pwm core driver and align the existing pwm drivers with
    this core driver. I was able to align all the existing pwm drivers except the
    jz4740 pwm driver in mips. So in order to retain the support for this mips, I
    have deprecated this function. This will be removed once jz4740 pwm driver is
    aligned with pwm core driver.
    Will add the same comments in code.

    > > + struct pwm_dev_info *pwm;
    > > +
    > > + down_write(&pwm_list_lock);
    > > + pwm = kzalloc(sizeof(struct pwm_dev_info), GFP_KERNEL);
    > > + if (!pwm) {
    > > + up_write(&pwm_list_lock);
    > > + return -ENOMEM;
    > > + }
    >
    > The allocation attempt can be moved outside the lock, making the code
    > faster, cleaner and shorter.
    Will correct this in v3 patch.

    > > + up_write(&pwm_list_lock);
    > > + return -ENOENT;
    > > +}
    > > +EXPORT_SYMBOL(pwm_device_unregister);
    > > +
    > > +struct pwm_device *pwm_request(int pwm_id, const char *name)
    > > +{
    > > + struct pwm_dev_info *pwm;
    > > + struct list_head *pos;
    > > +
    > > + down_read(&pwm_list_lock);
    > > + list_for_each(pos, &di->list) {
    > > + pwm = list_entry(pos, struct pwm_dev_info, list);
    > > + if ((!strcmp(pwm->pwm_dev->pops->name, name)) &&
    > > + (pwm->pwm_dev->pwm_id == pwm_id)) {
    > > + up_read(&pwm_list_lock);
    > > + return pwm->pwm_dev;
    > > + }
    > > + }
    > > + up_read(&pwm_list_lock);
    > > + return ERR_PTR(-ENOENT);
    > > +}
    > > +EXPORT_SYMBOL(pwm_request);
    >
    > We have a new kernel-wide exported-to-modules formal API. We prefer
    > that such things be fully documented, please. kerneldoc is a suitable
    > way but please avoid falling into the kerneldoc trap of filling out
    > fields with obvious boilerplate and not actually telling people
    > anything interesting or useful.
    Sure, Will document this as part of v3 patch.

    >
    > > +static int __init pwm_init(void)
    > > +{
    > > + struct pwm_dev_info *pwm;
    > > +
    > > + pwm = kzalloc(sizeof(struct pwm_dev_info), GFP_KERNEL);
    > > + if (!pwm)
    > > + return -ENOMEM;
    > > + INIT_LIST_HEAD(&pwm->list);
    > > + di = pwm;
    > > + return 0;
    > > +}
    >
    > OK, this looks wrong.
    >
    > AFACIT you've created a dummy pwm_dev_info as a singleton, kernel-wide
    > anchor for a list of all pwm_dev_info's. So this "anchor" pwm_dev_info
    > never actually gets used for anything.
    >
    > The way to do this is to remove `di' altogether and instead use a
    > singleton, kernel-wide list_head as the anchor for all the
    > dynamically-allocated pwm_dev_info's.
    OK, will implement this in v3 patch.

    >
    > > +subsys_initcall(pwm_init);
    > > +
    > > +static void __exit pwm_exit(void)
    > > +{
    > > + kfree(di);
    > > +}
    > > +
    > > +module_exit(pwm_exit);
    > > +
    > > +MODULE_LICENSE("GPL v2");
    > > +MODULE_AUTHOR("Arun R Murthy");
    > > +MODULE_ALIAS("core:pwm");
    > > +MODULE_DESCRIPTION("Core pwm driver");
    > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
    > > index 7c77575..6e7da1f 100644
    > > --- a/include/linux/pwm.h
    > > +++ b/include/linux/pwm.h
    > > @@ -3,6 +3,13 @@
    > >
    > > struct pwm_device;
    > >
    > > +struct pwm_ops {
    > > + int (*pwm_config)(struct pwm_device *pwm, int duty_ns, int
    > period_ns);
    > > + int (*pwm_enable)(struct pwm_device *pwm);
    > > + int (*pwm_disable)(struct pwm_device *pwm);
    > > + char *name;
    > > +};
    >
    > This also should be documented.
    Sure, will take up this in v3 patch.

    >
    > >
    > > ...
    > >
    >
    > I suggest that you work on Kevin's comments before making any code
    > changes though.
    This pwm driver also supports the Davinci pwm driver as suggested by Kelvin.

    Thanks and Regards,
    Arun R Murthy
    ------------


    \
     
     \ /
      Last update: 2010-10-06 06:07    [W:0.057 / U:0.196 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site