On Wed, 01 May 2024, André Apitzsch wrote:
> Hi Lee Jones,
>
> thanks for the feedback. I will address your comments in the next
> version. I have a few comments/questions though, see below.
>
> Best regards,
> André
>
> Am Donnerstag, dem 11.04.2024 um 13:48 +0100 schrieb Lee Jones:
> > On Mon, 01 Apr 2024, André Apitzsch via B4 Relay wrote:
> > >
> > > [..]
> > > +
> > > +#define SY7802_TIMEOUT_DEFAULT_US 512000U
> > > +#define SY7802_TIMEOUT_MIN_US 32000U
> > > +#define SY7802_TIMEOUT_MAX_US 1024000U
> > > +#define SY7802_TIMEOUT_STEPSIZE_US 32000U
> > > +
> > > +#define SY7802_TORCH_BRIGHTNESS_MAX 8
> > > +
> > > +#define SY7802_FLASH_BRIGHTNESS_DEFAULT 14
> > > +#define SY7802_FLASH_BRIGHTNESS_MIN 0
> > > +#define SY7802_FLASH_BRIGHTNESS_MAX 15
> > > +#define SY7802_FLASH_BRIGHTNESS_STEP 1
> >
> > Much nicer to read if everything was aligned.
>
> Using tab size 8, SY7802_FLASH_BRIGHTNESS_* look aligned to me. Do you
> refer to SY7802_TORCH_BRIGHTNESS_MAX here?
These were not aligned in my mailer. You can ignore.
> > > [..]
> > > +
> > > + /*
> > > + * There is only one set of flash control logic, and this
> > > flag is used to check if 'strobe'
> >
> > The ',' before 'and' is superfluous.
> >
> > > + * is currently being used.
> > > + */
> >
> > Doesn't the variable name kind of imply this?
> >
> > > + if (chip->fled_strobe_used) {
> > > + dev_warn(chip->dev, "Please disable strobe first
> > > [%d]\n", chip->fled_strobe_used);
> >
> > "Cannot set torch brightness whilst strobe is enabled"
>
> The comment and the warn message are taken from 'leds-mt6370-flash.c'.
> But I think using the warn message you suggested the comment can be
> removed.
It's an improvement, right?
> > > + ret = -EBUSY;
> > > + goto unlock;
> > > + }
> > > +
> > > + if (level)
> > > + curr = chip->fled_torch_used | BIT(led->led_no);
> > > + else
> > > + curr = chip->fled_torch_used & ~BIT(led->led_no);
> > > +
> > > + if (curr)
> > > + val |= SY7802_MODE_TORCH;
> > > +
> > > + /* Torch needs to be disabled first to apply new
> > > brightness */
> >
> > "Disable touch to apply brightness"
> >
> > > + ret = regmap_update_bits(chip->regmap, SY7802_REG_ENABLE,
> > > SY7802_MODE_MASK,
> > > + SY7802_MODE_OFF);
> > > + if (ret)
> > > + goto unlock;
> > > +
> > > + mask = led->led_no == SY7802_LED_JOINT ?
> > > SY7802_TORCH_CURRENT_MASK_ALL :
> >
> > Why not just use led->led_no in place of mask?
>
> I might be missing something, but I don't know how to use led->led_no
> in place of mask, when
> led->led_no is in {0,1,2} and
> mask is in {0x07, 0x38, 0x3f}.
This doesn't make much sense.
I guess you mean that led_no is a u8 and mask is a u32.
What happens if you cast led_no to u32 in the call to regmap_update_bits()?
> > Easier to read if you drop SY7802_TORCH_CURRENT_MASK_ALL to its own
> > line.
> >
> > > + SY7802_TORCH_CURRENT_MASK(led->led_no);
> > > +
> > > [..]
> > > +
> > > +static int sy7802_probe(struct i2c_client *client)
> > > +{
> > > + struct device *dev = &client->dev;
> > > + struct sy7802 *chip;
> > > + size_t count;
> > > + int ret;
> > > +
> > > + count = device_get_child_node_count(dev);
> > > + if (!count || count > SY7802_MAX_LEDS)
> > > + return dev_err_probe(dev, -EINVAL,
> > > + "No child node or node count over max led
> > > number %zu\n", count);
> >
> > Split them up and report on them individually or combine the error
> > message:
> >
> > "Invalid amount of LED nodes"
>
> This snippet was also taken from 'leds-mt6370-flash.c'.
Old mistakes should not be repeated. :)
--
Lee Jones [李琼斯]
