Hi Andy,
Thank you for all the reviews.
I don't see anything really problematic in your review,
so I hope that Mauro will honor my pull-request and
then I'll fix the small remarks in some follow-up patches.
One remark regarding your review of this patch below:
On 1/2/24 01:33, Andy Shevchenko wrote:
> On Sun, Dec 31, 2023 at 12:31 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> sysfs attributes preferably should not be manually be registered but
>> instead the driver.groups / driver.dev_groups driver struct members
>> should be used to have the driver core handle this in a race free
>> manner.
>>
>> Using driver.groups would be the most direct replacement for
>> driver_[add|remove]_file, but some of the attributes actually need access
>
> ..._file()
>
>> to the struct atomisp_device (*), so as part of modernizing this part of
>> the atomisp driver this change also makes the sysfs attribute device
>> attributes instead of driver attributes.
>>
>> *) Before this change accessing these attributes without the driver having
>> bound would result in a NULL pointer deref, this commit fixes this.
>
> ...
>
>> + if (dbglvl < 1 || dbglvl > 9)
>
> in_range() ?
ack.
>
>> return -ERANGE;
>
> ...
>
>> +static const struct attribute_group dbg_attr_group = {
>> + .attrs = dbg_attrs,
>> +};
>>
>> +const struct attribute_group *dbg_attr_groups[] = {
>> + &dbg_attr_group,
>> + NULL
>> +};
>
> ATTRIBUTE_GROUPS()
I deliberately wrote this out (had to write this out)
instead of using ATTRIBUTE_GROUPS() because ATTRIBUTE_GROUPS()
makes the groups variable static and here it gets used
in another file then where it is declared.
>
> ...
>
>> +#include <linux/sysfs.h>
>
> But why? You can use forward declaration, no?
True, I'll fix this up in a follow-up patch.
>
>> +extern const struct attribute_group *dbg_attr_groups[];
>
Regards,
Hans
