On Tue, May 28, 2024 at 11:27 PM Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > On Tue, May 28, 2024 at 10:41:51PM +0300, Andy Shevchenko wrote: > > Tue, May 28, 2024 at 10:03:14PM +0300, Laurent Pinchart kirjoitti: ... > > > +#define ADP5585_PWM_OSC_FREQ_HZ 1000000U > > > > (1 * HZ_PER_MHZ) ? > > If we had an MHZ macro I would use 1 * MHZ, but I don't think HZ_PER_MHZ > improves readability here. We have MEGA. HZ is already the suffix in this definition. > > > +#define ADP5585_PWM_MIN_PERIOD_NS (2ULL * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) > > > +#define ADP5585_PWM_MAX_PERIOD_NS (2ULL * 0xffff * NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) > > > > Wouldn't be better to use GENMASK() or (BIT(x) - 1) notation to show that > > the limit is due to HW register bits in use? > > I think that would decrease readability to be honest. I think it improves the robustness of the code. I always fail to count 3,4,5,6 f:s in those masks, using BIT()/GENMASK() notation makes it better. ... > > > + ret = regmap_write(regmap, ADP5585_PWM_OFFT_LOW, > > > + off & 0xff); > > > + if (ret) > > > + return ret; > > > + ret = regmap_write(regmap, ADP5585_PWM_OFFT_HIGH, > > > + (off >> 8) & 0xff); > > > + if (ret) > > > + return ret; > > > + ret = regmap_write(regmap, ADP5585_PWM_ONT_LOW, > > > + on & 0xff); > > > + if (ret) > > > + return ret; > > > + ret = regmap_write(regmap, ADP5585_PWM_ONT_HIGH, > > > + (on >> 8) & 0xff); > > > + if (ret) > > > + return ret; > > > > Can be proper __le16/__be16 be used in conjunction with regmap bulk API? > > What I would really like is regmap growing an API similar to > include/media/v4l2-cci.h. Any volunteer ? :-) So, the answer here is yes? ... > > > + regmap_read(regmap, ADP5585_PWM_OFFT_LOW, &off); > > > + regmap_read(regmap, ADP5585_PWM_OFFT_HIGH, &val); > > > + off |= val << 8; > > > + > > > + regmap_read(regmap, ADP5585_PWM_ONT_LOW, &on); > > > + regmap_read(regmap, ADP5585_PWM_ONT_HIGH, &val); > > > + on |= val << 8; > > > > As per above, can it be converted to use proper __le16/__be16 type and > > regmap bulk API? > > As there are only 2 registers, I think that's a bit overkill really. I do not think so. It increases readability (less LoCs) and improves a lot of understanding of the hardware layout from reading the code. Please, consider using it. ... > > > + device_set_of_node_from_dev(dev, dev->parent); > > > > Why this one? What's wrong with device_set_node()? > > See my reply to 3/4. See additional questions there as well. -- With Best Regards, Andy Shevchenko
