linux-kernel - Re: [rtc-linux] [PATCH 2/6] mfd: max77620: add core driver for MAX77620/MAX20024

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <568FB62C.9020109@samsung.com>
Date:	Fri, 8 Jan 2016 22:14:20 +0900
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	rtc-linux@...glegroups.com, ldewangan@...dia.com
Cc:	k.kozlowski.k@...il.com, robh+dt@...nel.org, pawel.moll@....com,
	mark.rutland@....com, ijc+devicetree@...lion.org.uk,
	galak@...eaurora.org, linus.walleij@...aro.org, gnurou@...il.com,
	lee.jones@...aro.org, broonie@...nel.org, a.zummo@...ertech.it,
	alexandre.belloni@...e-electrons.com, lgirdwood@...il.com,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-gpio@...r.kernel.org, swarren@...dia.com, treding@...dia.com,
	Chaitanya Bandi <bandik@...dia.com>,
	Mallikarjun Kasoju <mkasoju@...dia.com>
Subject: Re: [rtc-linux] [PATCH 2/6] mfd: max77620: add core driver for
 MAX77620/MAX20024

W dniu 08.01.2016 o 18:16, Laxman Dewangan pisze:
> Hi Krzysztof,
> Thanks for review.
> I will fix most of your comment on my next patch.
> 
> Answering to some of comment/query.
> 
> On Friday 08 January 2016 07:05 AM, Krzysztof Kozlowski wrote:
>> ()2016-01-07 23:38 GMT+09:00 Laxman Dewangan <ldewangan@...dia.com>:
>> +                               dev_err(dev,
>> +                                   "FPS enable-input %u is not
>> supported\n",
>> +                                       pval);
>> Indentation of arguments does not seem equal here or maybe this is
>> just my email client. Have you run this through checkpatch? And
>> sparse? And coccicheck (that one definitely not because kbuild is
>> complaining)?
> I ran checkpatch before I sent.

Anyway please be sure that indentation is consistent.

> 
>> +               chip->rmap[i] = devm_regmap_init_i2c(chip->clients[i],
>> +               (const struct regmap_config
>> *)&max77620_regmap_config[i]);
>> Indentation looks weird here (or again this is my email client...).
>> The cast is even weirder?!? Why casting?
> There is some parameter difference for MAX77620 and MAX20024. I have
> only one structure for it and changing tun time so I have not define
> this structure as constant.
> Now API needs const type structure and hence casting it.

I don't quite get... usually there is no need of casting pointer to a
writable memory when function accepts pointer to const.

> 
> However, I have  define different structure for MAX77620 and MAX20024
> which are const type and hence no need to explicitly casting here. This
> will be in my next patch.

You mean v2? Okay, let's wait for that...

> 
> +static inline int max77620_reg_update(struct device *dev, int sid,
> +               unsigned int reg, unsigned int mask, unsigned int val)
> +{
> +       struct max77620_chip *chip = dev_get_drvdata(dev);
> +
> +       return regmap_update_bits(chip->rmap[sid], reg, mask, val);
> +}
> 
>> I think all these shouldn't be static inlines in header. Although some
>> of them are one-liners but rest are not. Let the compiler decide what
>> to do with these wrappers.
> 
> If I dont make inline from header then this will complain as unused
> static function on related C compilation if it is not used on C. This
> header included from all sub module driver and they are not using all
> these APIs.
> 
> To avoid compilation warning,  I need to use inline here.

Because this shouldn't be defined in header at the first place. Instead
define it in main MFD driver with EXPORT_SYMBOL() and put in headers
just declaration.

Best regards,
Krzysztof