Date:   Tue, 14 Mar 2023 17:22:02 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Markus Schneider-Pargmann <msp@baylibre.com>
Cc:     Marc Kleine-Budde <mkl@pengutronix.de>,
        Chandrasekar Ramakrishnan <rcsekar@samsung.com>,
        Wolfgang Grandegger <wg@grandegger.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Vincent MAILHOL <mailhol.vincent@wanadoo.fr>,
        linux-can@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/5] can: tcan4x5x: Add support for tcan4552/4553
Message-ID: <ZBCfKhPZrIMqvmbO@corigine.com>
References: <20230314151201.2317134-1-msp@baylibre.com>
 <20230314151201.2317134-6-msp@baylibre.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230314151201.2317134-6-msp@baylibre.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?053tMdapkSLor23vzhGwFV7N4TTmjtmqAwr7MzTNHCRbYnqUDOwUFyQN4L06?=
 =?us-ascii?Q?USXXQvlF26ONFuufP9ip347hgoFRMAsaVTCHU8F1Y5WXIqTJT2eFCNgxIy85?=
 =?us-ascii?Q?K5jD4nQdg28XnP6FknuJtHNklyNMpKKkVEUQJ0BXe2h4ZjHdcl1BhUWpEfcE?=
 =?us-ascii?Q?UGQaijNfBNTo1cNglBBGP9qB+xUcMoIDcC2i9fpkidF0pZ6vTxGgUpxpgdxN?=
 =?us-ascii?Q?PVlXbg1/hDTPmzCcxY2ICouNJtCrkp5qnkMeQtiopWFck4p+emMALCCX3Hea?=
 =?us-ascii?Q?yI3GRlvzYht2LMLWki1JXpDGldmby2X5GNM7HOvk5gskp76iawBGfVkbd3/c?=
 =?us-ascii?Q?EbQO2Oa28kV0fZLnNAOMp8gNsaV3vrRASWbA0tbGyiM7WajqZBpT7ngCjQ8u?=
 =?us-ascii?Q?io/0xEKDJ+Z07W/DoUi9ysytzWj5AHNWB1nCE1Vjx9+BeaTrJRd1b3eDJkCn?=
 =?us-ascii?Q?TkZ4R5O5HACDr/TvZAClta4FteV2HEvBjTaqi38wJ34BVh3Nnb0VkBLs8B+/?=
 =?us-ascii?Q?y5Ox95Qb6LRljTz9YTl+99FZbGuTY4SWc2CFn221JW0wOV/QjWkM5+ByGmlm?=
 =?us-ascii?Q?flQYjjBsb4JY41XRstbanVxv9Cw+MwEzWyyymPAvglzUHn4qNClImyv1jvw6?=
 =?us-ascii?Q?Ps1zasBx3HuwyYzug03etlW8EoeMwmDjDtIbkZPjtJMyjj5LKv0euM1VikCY?=
 =?us-ascii?Q?hzFB0FSGJ4Wj+KiB5EgoucXh9CP4qNDn9Rg6uGlcuGcyWJFfl+sdq0+SF9nj?=
 =?us-ascii?Q?QqT9FpfKMwDyp8bCEumWv0ztsGqnY7nvo8Abt/L3aaPspEismSeKykcsTX5z?=
 =?us-ascii?Q?f2nRjT9UB6g+CD3o7fhqmwmDsbJFYl7xwiplTVYXlh7HbbcDmnA88dOM8SIm?=
 =?us-ascii?Q?nfl5w9U/hYtNFA03O3FsXurKTpllimCK2y+fp+unU+frrsoU5fMaTOwayhUH?=
 =?us-ascii?Q?8qjrEAGmQYdavXjZ76+lmUJcUVPGhkRV5Ul07v9zNaRui7i+PdVXzyAkbI97?=
 =?us-ascii?Q?07oD3Ixgiy82opanFx4fO6Y0aGFXPkw0NHOzusPf2Ed6xp4awtjlCS03LvHw?=
 =?us-ascii?Q?bf26Q5W4hVZp3ZSsCGioTtGwecPqw/ZtZqDzSPUH36zsN4zj+gWRRD+Y6434?=
 =?us-ascii?Q?0ONMCSBEREjgwKMqP83hW7y7ZOaFC2gX4KKVZq/AtU0TJ6YTL+GhkA56Im6K?=
 =?us-ascii?Q?ZmS2adm8c4uLtFtSTw7lbNcUAAauoMyegFXdssuEeWlBio6nEgefm/HH34Wh?=
 =?us-ascii?Q?0cClU87HlpfX/TUJHDuCVVfUT9gkGR5eOQXBLRCgoxRhJI5Ee4yZitWGEWNF?=
 =?us-ascii?Q?qlp/LsvboNa0TUY73ABg026bnmPg3fsvRjdc9hxxW7Ba6/EXIZgubkF50sLu?=
 =?us-ascii?Q?gC/dWeBkHP4y8EKfM8sI2rle8GOw79uf6OqVCeGpeKf/WZBZYD+3gkudnBod?=
 =?us-ascii?Q?6QjY8/WvGwZ8Y2lt1txp5vY8lAw+bSzxKBhTrLcnilz8Kdea+xF1zCoehbbM?=
 =?us-ascii?Q?NbqTZQnaHI8RAMuP52TVAvBPOCcZMIhPoVIJnAAdmeG9g279owDJHffeWZdZ?=
 =?us-ascii?Q?Mx9SlUDDit6YVKu2FFgvttQiv3xwDA+1VJhmNwo+vC5IdzIRWJYOi4MpIVId?=
 =?us-ascii?Q?GjOHWagZhucHewIjjn0aLEjDxAKVUmlY9wHg7+Udegk7F5ysVEj//CbtVjk5?=
 =?us-ascii?Q?lNiUQQ=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a6b4624-6dd8-40f6-a039-08db24a841d9
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Mar 2023 16:22:08.2193
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Dv/gZ22lV5wYJoIr6s9VXsA3U0JHItS2ZasUm8IetnZfuwERBEXKEwoJh3Ax3gfpXtGyGUCFoHufiNwO7rUMwq6mSP/mjgpmrUJole8wtH4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR13MB6122
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Mar 14, 2023 at 04:12:01PM +0100, Markus Schneider-Pargmann wrote:
> tcan4552 and tcan4553 do not have wake or state pins, so they are
> currently not compatible with the generic driver. The generic driver
> uses tcan4x5x_disable_state() and tcan4x5x_disable_wake() if the gpios
> are not defined. These functions use register bits that are not
> available in tcan4552/4553.
> 
> This patch adds support by introducing version information to reflect if
> the chip has wake and state pins. Also the version is now checked.
> 
> Signed-off-by: Markus Schneider-Pargmann

Hi Markus,

you forgot your email address in the signed-off-by line.

> ---
>  drivers/net/can/m_can/tcan4x5x-core.c | 113 ++++++++++++++++++++------
>  1 file changed, 89 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index fb9375fa20ec..e7fa509dacc9 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c

...

> @@ -254,18 +262,53 @@ static int tcan4x5x_disable_state(struct m_can_classdev *cdev)
>  				  TCAN4X5X_DISABLE_INH_MSK, 0x01);
>  }
>  
> -static int tcan4x5x_get_gpios(struct m_can_classdev *cdev)
> +static int tcan4x5x_verify_version(
> +		struct tcan4x5x_priv *priv,
> +		const struct tcan4x5x_version_info *version_info)

nit:

static int
tcan4x5x_verify_version(struct tcan4x5x_priv *priv,                                                     const struct tcan4x5x_version_info *version_info)

or:

static int tcan4x5x_verify_version(struct tcan4x5x_priv *priv,                                                     const struct tcan4x5x_version_info *version_info)

Your could make the line shorter by renaming the 'version_info' parameter,
say to 'info'.

...

> @@ -394,21 +448,32 @@ static void tcan4x5x_can_remove(struct spi_device *spi)
>  	m_can_class_free_dev(priv->cdev.net);
>  }
>  
> +static const struct tcan4x5x_version_info tcan4x5x_generic = {
> +	.has_state_pin = true,
> +	.has_wake_pin = true,
> +};
> +
> +static const struct tcan4x5x_version_info tcan4x5x_tcan4552 = {
> +	.id2_register = 0x32353534, /* ASCII = 4552 */
> +};
> +
> +static const struct tcan4x5x_version_info tcan4x5x_tcan4553 = {
> +	.id2_register = 0x33353534, /* ASCII = 4553 */
> +};
> +
>  static const struct of_device_id tcan4x5x_of_match[] = {
> -	{
> -		.compatible = "ti,tcan4x5x",
> -	}, {
> -		/* sentinel */
> -	},
> +	{ .compatible = "ti,tcan4x5x", .data = &tcan4x5x_generic },
> +	{ .compatible = "ti,tcan4552", .data = &tcan4x5x_tcan4552 },
> +	{ .compatible = "ti,tcan4553", .data = &tcan4x5x_tcan4553 },
> +	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, tcan4x5x_of_match);
>  
>  static const struct spi_device_id tcan4x5x_id_table[] = {
> -	{
> -		.name = "tcan4x5x",
> -	}, {
> -		/* sentinel */
> -	},
> +	{ .name = "tcan4x5x", .driver_data = (unsigned long) &tcan4x5x_generic, },
> +	{ .name = "tcan4552", .driver_data = (unsigned long) &tcan4x5x_tcan4552, },
> +	{ .name = "tcan4553", .driver_data = (unsigned long) &tcan4x5x_tcan4553, },

nit: checkpatch tells me that no space is necessary after a cast.

> +	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(spi, tcan4x5x_id_table);
>  
> -- 
> 2.39.2
>