ChiYuan Huang <u0084500@xxxxxxxxx> 於 2020年10月27日 週二 下午6:46寫道：
>
> Pavel Machek <pavel@xxxxxx> 於 2020年10月27日 週二 下午6:15寫道：
> >
> > Hi!
> >
> > > > Please use upper-case "LED" everywhere.
> > > >
> > > > This should be 2nd in the series, after DT changes.
> > > Sure, will ack in next series patch.
> >
> > Feel free to wait for dt ACKs before resending.
> >
> Yes, sure.
> > > > > + help
> > > > > + This option enables support for the RT4505 flash led controller.
> > > >
> > > > Information where it is used would be welcome here.
> > > How about to add the below line for the extra information?
> > > Usually used to company with the camera device on smartphone/tablet
> > > products
> >
> > Yes, that would help.
> >
> > "It is commonly used in smartphones, such as Bell Packard T899" would
> > be even better.
>
> Sorry, We don't focus on specific products. It's a general part flash
> led controller.
> I'll change it like as below
> "It's commonly used in smartphones and tablets to assist the builtin camera."
> >
> > > > > + ret = regmap_update_bits(priv->regmap, RT4505_REG_ENABLE, RT4505_ENABLE_MASK, val);
> > > > > +
> > > > > +unlock:
> > > > > + mutex_unlock(&priv->lock);
> > > > > + return ret;
> > > > > +}
> > > >
> > > > Why is the locking needed? What will the /sys/class/leds interface
> > > > look like on system with your flash?
> > >
> > > The original thought is because there's still another way to control
> > > flash like as v4l2.
> > > But after reviewing the source code, led sysfs node will be protected
> > > by led_cdev->led_access.
> > > And V4L2 flash will also be protected by v4l2_fh_is_singular API.
> > > I think the whole locking in the source code code may be removed. Right?
> >
> > Well, maybe you need it, I did not check..
I found all led class is guaranteed by led_cdev->led_access lock.
And v4l2 ctrl handleris guaranteed by ctrl->handler->lock that defined
in v4l2-ctrl.c.
When entering v4l2 control mode, all led sysfs will be disabled.
To remove all lock operation is correct. Due to all operations are guaranteed.
In next series patch, I'll remove it.
How do you think?
> >
> > What will the /sys/class/leds interface look like on system with your flash?
> >
> > > > > + *state = ((val & RT4505_FLASH_GET) == RT4505_FLASH_GET) ? true : false;
> > > >
> > > > No need for ? ... part.
> > > Do you mean this function is not needed? If yes, it can be removed.
> > > But if it removed, led sysfs flash_strobe show will be not supported.
> >
> > I meant "replace line with: *state = (val & RT4505_FLASH_GET) == RT4505_FLASH_GET;"
> Oh, I got it. redundant judgement.
> >
> > > > > +static bool rt4505_is_accessible_reg(struct device *dev, unsigned int reg)
> > > > > +{
> > > > > + if (reg == RT4505_REG_RESET || (reg >= RT4505_REG_CONFIG && reg <= RT4505_REG_FLAGS))
> > > > > + return true;
> > > >
> > > > Make this two stagements.
> > > Like as the below one?? Or separate it into two if case.
> > > if (reg == RT4505_REG_RESET ||
> > > reg >= RT4505_REG_CONFIG && reg <= RT4505_REG_FLAGS))
> >
> > That would be fine, too... if you use just one space before "&&" :-).
> Thx.
> >
> > Best regards,
> > Pavel
> > --
> > http://www.livejournal.com/~pavelmachek
