Hi, Parvel: Continue the last mail, I'm confused about the rule 80-characters-per-line. I check some information about submitting changes. https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Deprecates-80-Col Could you help to explain the current rule about this limit? still 80 characters per line? or it has been changed to 100. ChiYuan Huang <u0084500@xxxxxxxxx> 於 2020年10月27日 週二 下午5:27寫道： > > Pavel Machek <pavel@xxxxxx> 於 2020年10月27日 週二 下午4:29寫道： > > > > Hi! > > > > > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx> > > > > > > Add support for RT4505 flash led controller. It can support up to 1.5A > > > flash current with hardware timeout and low input voltage > > > protection. > > > > Please use upper-case "LED" everywhere. > > > > This should be 2nd in the series, after DT changes. > Sure, will ack in next series patch. > > > > > Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx> > > > --- > > > drivers/leds/Kconfig | 11 ++ > > > drivers/leds/Makefile | 1 + > > > drivers/leds/leds-rt4505.c | 397 +++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 409 insertions(+) > > > create mode 100644 drivers/leds/leds-rt4505.c > > > > Lets put this into drivers/leds/flash/. (Yes, you'll have to create > > it). > OK > > > > > > > + 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 > > > > > + It can support up to 1.5A flash strobe current with hardware timeout > > > + and low input voltage protection. > > > > This does not / should not be here. > OK > > > + > > > +static int rt4505_torch_brightness_set(struct led_classdev *lcdev, enum led_brightness level) > > > +{ > > > > 80 columns, where easy. > I'll review all source code to meet the 80 column limit. > > > > > + struct rt4505_priv *priv = container_of(lcdev, struct rt4505_priv, flash.led_cdev); > > > + u32 val = 0; > > > + int ret; > > > + > > > + mutex_lock(&priv->lock); > > > + > > > + if (level != LED_OFF) { > > > + ret = regmap_update_bits(priv->regmap, RT4505_REG_ILED, RT4505_ITORCH_MASK, > > > + (level - 1) << RT4505_ITORCH_SHIFT); > > > + if (ret) > > > + goto unlock; > > > + > > > + val = RT4505_TORCH_SET; > > > + } > > > + > > > + 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? > > > > > +static int rt4505_flash_strobe_get(struct led_classdev_flash *fled_cdev, bool *state) > > > +{ > > > + struct rt4505_priv *priv = container_of(fled_cdev, struct rt4505_priv, flash); > > > + u32 val; > > > + int ret; > > > + > > > + mutex_lock(&priv->lock); > > > + > > > + ret = regmap_read(priv->regmap, RT4505_REG_ENABLE, &val); > > > + if (ret) > > > + goto unlock; > > > + > > > + *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. > > > > > +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)) > > > > Otherwise... looks like easy simple driver, thanks. > > > > Best regards, > > Pavel > > -- > > http://www.livejournal.com/~pavelmachek
