Re: [PATCH v1 1/1] drivers: Modify some parts

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2643 bytes --]

On Sun, Aug 07, 2022 at 11:40:52AM +0800, Kevin Lu wrote:

> Add a new kcontrol for phase calib, remove unnecessary header file,
> make code more comply with linux coding style
> 
> Signed-off-by: Kevin Lu <luminlong@139.com>
> ---
>  tlv320adcx140.c | 114 ++++++++++++++++++++++++++++++++++++------------
>  tlv320adcx140.h |   7 +--
>  2 files changed, 90 insertions(+), 31 deletions(-)

As covered in submitting-patches.rst your diff should be against the
root of the kernel tree rather than files in a subdirectory, look at how
other patches on the list are being formatted for examples of how things
look.  If you use git format-patch to generate patches it should do the
right thing for you.

The fact that you're describing three different changes changes here
suggests that this should be a patch series rather than a single patch,
as covered in submitting-patches.rst this is the general style for Linux
as it makes things much easier to review.  Each logical change should be
a separate patch.

> -// Copyright (C) 2020 Texas Instruments Incorporated - https://www.ti.com/
> +// Copyright (C) 2020 - 2022 Texas Instruments Incorporated
> +// - https://www.ti.com/
> +/*
> + * Author: Kevin Lu <kevin-lu@ti.com>
> + *
> + *  Features:-

Please add the new bits to the header as C++ comments as well, this
makes things look more intentional.

> +static const char * const phase_calib_text[] = {
> +	"Disable",
> +	"Enable"
> +};
> +
> +static const struct soc_enum phase_calib_enum[] = {
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(phase_calib_text), phase_calib_text),
> +};

This is an on/off switch, it should be a normal boolean control ending
in Switch and taking 0 and 1 as valid values.

> +static int adcx140_phase_calib_put(struct snd_kcontrol *pKcontrol,
> +	struct snd_ctl_elem_value *pValue)
> +{
> +	struct snd_soc_component *codec
> +		= snd_soc_kcontrol_component(pKcontrol);
> +	struct adcx140_priv *adcx140 = snd_soc_component_get_drvdata(codec);
> +
> +	adcx140->phase_calib_on = pValue->value.integer.value[0];
> +
> +	return 0;

The issues with use of hungarian notation and not generating events that
were present previously remain.

Please don't ignore review comments, people are generally making them
for a reason and are likely to have the same concerns if issues remain
unaddressed.  Having to repeat the same comments can get repetitive and
make people question the value of time spent reviewing.  If you disagree
with the review comments that's fine but you need to reply and discuss
your concerns so that the reviewer can understand your decisions.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]